Re: [PATCH 01/17] netlink: Split up functionality if nl_link()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 9171 bytes --]

On Thu, Aug 03, 2023 at 12:09:16PM +1000, David Gibson wrote:
> On Thu, Aug 03, 2023 at 12:47:29AM +0200, Stefano Brivio wrote:
[snip]
> > > -void nl_link(int ns, unsigned int ifi, void *mac, int up, int mtu)
> > > +void nl_link_get_mac(int ns, unsigned int ifi, void *mac)
> > >  {
> > > -	int change = !MAC_IS_ZERO(mac) || up || mtu;
> > >  	struct req_t {
> > >  		struct nlmsghdr nlh;
> > >  		struct ifinfomsg ifm;
> > > -		struct rtattr rta;
> > > -		union {
> > > -			unsigned char mac[ETH_ALEN];
> > > -			struct {
> > > -				unsigned int mtu;
> > > -			} mtu;
> > > -		} set;
> > >  	} req = {
> > > -		.nlh.nlmsg_type   = change ? RTM_NEWLINK : RTM_GETLINK,
> > > -		.nlh.nlmsg_len    = NLMSG_LENGTH(sizeof(struct ifinfomsg)),
> > > -		.nlh.nlmsg_flags  = NLM_F_REQUEST | (change ? NLM_F_ACK : 0),
> > > +		.nlh.nlmsg_type	  = RTM_GETLINK,
> > > +		.nlh.nlmsg_len	  = sizeof(req),
> > 
> > I don't think there's a practical issue with this, but there were two
> > reasons why I used NLMSG_LENGTH(sizeof(struct ifinfomsg)) instead:
> > 
> > - NLMSG_LENGTH() aligns to 4 bytes, not to whatever
> >   architecture-dependent alignment we might have: the message might
> >   actually be smaller
> 
> Oof... so.  On the one hand, I see the issue; if these are different,
> I'm not sure what the effect will be.  On the other hand, if we use
> NLMSG_LENGTH and it *is* longer than the structure size, we'll be
> saying that this message is longer than the datagram containing it.
> I'm not sure what the effect of that will be either.

Duh, sorry, I realized I had this backwards.  NLSMSG_LENGTH() is the
non-aligned length, sizeof() may include alignment.  I'll rework based
on that understanding.

> Not really sure what to do about this.
> 
> > - I see that this works with gcc and clang, but, strictly
> >   speaking, is the size of the struct known "before"
> >   (sequence-point-wise) we're done initialising it? I have a very vague
> >   memory of this not working with gcc 2.9 or suchlike -- which is not a
> >   problem, as long as our new friend C11 actually supports this (but
> >   I'm not entirely sure).
> 
> I'm pretty sure it's ok, regardless of C11 state.  It's not really a
> question of sequence points: those are about the ordering of run time
> operations.  Even though the structure is being defined inline,
> determining it's size and layout will still happen at compile time,
> whereas the initialization is obviously a runtime event.
> 
> > Then, in 9/17, NLMSG_LENGTH() could be conveniently used by nl_req().
> > 
> > > +		.nlh.nlmsg_flags  = NLM_F_REQUEST | NLM_F_ACK,
> > >  		.nlh.nlmsg_seq	  = nl_seq++,
> > >  		.ifm.ifi_family	  = AF_UNSPEC,
> > >  		.ifm.ifi_index	  = ifi,
> > > -		.ifm.ifi_flags	  = up ? IFF_UP : 0,
> > > -		.ifm.ifi_change	  = up ? IFF_UP : 0,
> > >  	};
> > > -	struct ifinfomsg *ifm;
> > >  	struct nlmsghdr *nh;
> > > -	struct rtattr *rta;
> > >  	char buf[NLBUFSIZ];
> > >  	ssize_t n;
> > > -	size_t na;
> > > -
> > > -	if (!MAC_IS_ZERO(mac)) {
> > > -		req.nlh.nlmsg_len = sizeof(req);
> > > -		memcpy(req.set.mac, mac, ETH_ALEN);
> > > -		req.rta.rta_type = IFLA_ADDRESS;
> > > -		req.rta.rta_len = RTA_LENGTH(ETH_ALEN);
> > > -		if (nl_req(ns, buf, &req, req.nlh.nlmsg_len) < 0)
> > > -			return;
> > > -
> > > -		up = 0;
> > > -	}
> > > -
> > > -	if (mtu) {
> > > -		req.nlh.nlmsg_len = offsetof(struct req_t, set.mtu)
> > > -			+ sizeof(req.set.mtu);
> > > -		req.set.mtu.mtu = mtu;
> > > -		req.rta.rta_type = IFLA_MTU;
> > > -		req.rta.rta_len = RTA_LENGTH(sizeof(unsigned int));
> > > -		if (nl_req(ns, buf, &req, req.nlh.nlmsg_len) < 0)
> > > -			return;
> > > -
> > > -		up = 0;
> > > -	}
> > > -
> > > -	if (up && nl_req(ns, buf, &req, req.nlh.nlmsg_len) < 0)
> > > -		return;
> > > -
> > > -	if (change)
> > > -		return;
> > >  
> > > -	if ((n = nl_req(ns, buf, &req, req.nlh.nlmsg_len)) < 0)
> > > +	n = nl_req(ns, buf, &req, sizeof(req));
> > > +	if (n < 0)
> > >  		return;
> > > +	
> > > +	for (nh = (struct nlmsghdr *)buf;
> > > +	     NLMSG_OK(nh, n) && nh->nlmsg_type != NLMSG_DONE;
> > > +	     nh = NLMSG_NEXT(nh, n)) {
> > > +		struct ifinfomsg *ifm = (struct ifinfomsg *)NLMSG_DATA(nh);
> > > +		struct rtattr *rta;
> > > +		size_t na;
> > >  
> > > -	nh = (struct nlmsghdr *)buf;
> > > -	for ( ; NLMSG_OK(nh, n); nh = NLMSG_NEXT(nh, n)) {
> > >  		if (nh->nlmsg_type != RTM_NEWLINK)
> > > -			goto next;
> > > -
> > > -		ifm = (struct ifinfomsg *)NLMSG_DATA(nh);
> > > +			continue;
> > >  
> > > -		for (rta = IFLA_RTA(ifm), na = RTM_PAYLOAD(nh); RTA_OK(rta, na);
> > > +		for (rta = IFLA_RTA(ifm), na = RTM_PAYLOAD(nh);
> > > +		     RTA_OK(rta, na);
> > >  		     rta = RTA_NEXT(rta, na)) {
> > >  			if (rta->rta_type != IFLA_ADDRESS)
> > >  				continue;
> > > @@ -570,8 +531,70 @@ void nl_link(int ns, unsigned int ifi, void *mac, int up, int mtu)
> > >  			memcpy(mac, RTA_DATA(rta), ETH_ALEN);
> > >  			break;
> > >  		}
> > > -next:
> > > -		if (nh->nlmsg_type == NLMSG_DONE)
> > > -			break;
> > >  	}
> > >  }
> > > +
> > > +/**
> > > + * nl_link_set_mac() - Set link MAC address
> > > + * @ns:		Use netlink socket in namespace
> > > + * @ifi:	Interface index
> > > + * @mac:	MAC address to set
> > > + */
> > > +void nl_link_set_mac(int ns, unsigned int ifi, void *mac)
> > > +{
> > > +	struct req_t {
> > > +		struct nlmsghdr nlh;
> > > +		struct ifinfomsg ifm;
> > > +		struct rtattr rta;
> > > +		unsigned char mac[ETH_ALEN];
> > > +	} req = {
> > > +		.nlh.nlmsg_type	  = RTM_NEWLINK,
> > > +		.nlh.nlmsg_len	  = sizeof(req),
> > 
> > Same here.
> > 
> > > +		.nlh.nlmsg_flags  = NLM_F_REQUEST | NLM_F_ACK,
> > > +		.nlh.nlmsg_seq	  = nl_seq++,
> > > +		.ifm.ifi_family	  = AF_UNSPEC,
> > > +		.ifm.ifi_index	  = ifi,
> > > +		.rta.rta_type	  = IFLA_ADDRESS,
> > > +		.rta.rta_len	  = RTA_LENGTH(ETH_ALEN),
> > > +	};
> > > +	char buf[NLBUFSIZ];
> > > +
> > > +	memcpy(req.mac, mac, ETH_ALEN);
> > > +
> > > +	nl_req(ns, buf, &req, sizeof(req));
> > > +}
> > > +
> > > +/**
> > > + * nl_link_up() - Bring link up
> > > + * @ns:		Use netlink socket in namespace
> > > + * @ifi:	Interface index
> > > + * @mtu:	If non-zero, set interface MTU
> > > + */
> > > +void nl_link_up(int ns, unsigned int ifi, int mtu)
> > > +{
> > > +	struct req_t {
> > > +		struct nlmsghdr nlh;
> > > +		struct ifinfomsg ifm;
> > > +		struct rtattr rta;
> > > +		unsigned int mtu;
> > > +	} req = {
> > > +		.nlh.nlmsg_type   = RTM_NEWLINK,
> > > +		.nlh.nlmsg_len    = sizeof(req),
> > 
> > And here.
> > 
> > > +		.nlh.nlmsg_flags  = NLM_F_REQUEST | NLM_F_ACK,
> > > +		.nlh.nlmsg_seq	  = nl_seq++,
> > > +		.ifm.ifi_family	  = AF_UNSPEC,
> > > +		.ifm.ifi_index	  = ifi,
> > > +		.ifm.ifi_flags	  = IFF_UP,
> > > +		.ifm.ifi_change	  = IFF_UP,
> > > +		.rta.rta_type	  = IFLA_MTU,
> > > +		.rta.rta_len	  = RTA_LENGTH(sizeof(unsigned int)),
> > > +		.mtu		  = mtu,
> > > +	};
> > > +	char buf[NLBUFSIZ];
> > > +
> > > +	if (!mtu)
> > > +		/* Shorten request to drop MTU attribute */
> > > +		req.nlh.nlmsg_len = offsetof(struct req_t, rta);
> > 
> > Pre-existing issue I see now: we should probably use NLMSG_LENGTH()
> > here, in any case.
> 
> Well.. if NLMSG_LENGTH() really is different here, we're (by
> definition) including some of req.rta in the message, which isn't our
> intention.  So.. if we trust the rta member to be aligned properly for
> the case where we *do* include it, can't we also trust it for the case
> where we don't?
> 
> > > +
> > > +	nl_req(ns, buf, &req, req.nlh.nlmsg_len);
> > > +}
> > > diff --git a/netlink.h b/netlink.h
> > > index cd0e666..980ac44 100644
> > > --- a/netlink.h
> > > +++ b/netlink.h
> > > @@ -18,6 +18,8 @@ void nl_route(enum nl_op op, unsigned int ifi, unsigned int ifi_ns,
> > >  	      sa_family_t af, void *gw);
> > >  void nl_addr(enum nl_op op, unsigned int ifi, unsigned int ifi_ns,
> > >  	     sa_family_t af, void *addr, int *prefix_len, void *addr_l);
> > > -void nl_link(int ns, unsigned int ifi, void *mac, int up, int mtu);
> > > +void nl_link_get_mac(int ns, unsigned int ifi, void *mac);
> > > +void nl_link_set_mac(int ns, unsigned int ifi, void *mac);
> > > +void nl_link_up(int ns, unsigned int ifi, int mtu);
> > >  
> > >  #endif /* NETLINK_H */
> > > diff --git a/pasta.c b/pasta.c
> > > index 8c85546..3b5537d 100644
> > > --- a/pasta.c
> > > +++ b/pasta.c
> > > @@ -272,13 +272,19 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
> > >   */
> > >  void pasta_ns_conf(struct ctx *c)
> > >  {
> > > -	nl_link(1, 1 /* lo */, MAC_ZERO, 1, 0);
> > > +	nl_link_up(1, 1 /* lo */, 0);
> > > +
> > > +	/* Get or set guest MAC */
> > 
> > I know it's called mac_guest, my bad, but what about "MAC address in
> > the target namespace"?
> 
> Good idea, changed.
> 



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]