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

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

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

On Thu, Aug 03, 2023 at 02:29:28PM +1000, David Gibson wrote:
> 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.

Uhhh... then I realized I don't really see what that entails either.

The basic problem is this, given a structure:
	struct req {
		struct nlmsghdr nlh;
		a_t a;
		b_t b;
		c_t c;
	} req;

then, what's the correct req.nlh.nlmsg_length?

sizeof(req) will probably work in practice, but as you say could be an
overestimate if struct req has end padding.

So try:
	payload_len = sizeof(req) - offsetof(struct req, a);
	NLMSG_LENGTH(payload_len)
But.. that still relies on sizeof(req) so will overestimate in exactly
the same circumstances.

So try:
	NLMSG_LENGTH(sizeof(a_t) + sizeof(b_t) + sizeof(c_t))
For one thing that's bulky and annoying... but also it will
*under*estimate the length if struct req has any mid-structure
padding.

Ok, so try:
	struct req {
		struct nlmsghdr nlg;
		struct payload {
			a_t a;
			b_t b;
			c_t c;
		}
	} req;

	NLMSG_LENGTH(sizeof(struct payload))

Well, if struct req has end padding, but struct payload doesn't, then
this will be correct, but struct payload isn't guaranteed to lack end
padding any more than struct req.

At that point I'm out of ideas.

So, I'm inclined to stick with sizeof(req) for its simplicity.  This
has the implicit requirement that we're always careful and ABI aware
about the construction of our structures so that they don't have
unexpected end padding.  But.. then we're also relying on the
structures having mid-padding only at the places that netlink expects
it, which already requires some awareness of the ABI, so I'm not sure
that avoiding the NLMSG_LENGTH() really loses us anything.

-- 
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 --]