From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id E465A5A026F for ; Thu, 3 Aug 2023 07:39:48 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1691041183; bh=PgD6eYvfUPxCjuAkXGdjJIQlTYGNHjS3uC/oi1sJ6ok=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=XJGtICbmeIhh2QTcf312jPxNlrGyBZIxCabrJmdnkoLbJajE5ZzuIL5a6HOj6YbQr 12wpgFlsCuxxkZpJKTNnX214NzrPT4Dppt/blOriCKWBMFakSAUgizPq5sFUFxLn7B c4zr53iNKWqcAr/kVrdRbRjNBRNyfT+OsG7+kInQ= Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4RGd2l1b3Nz4wxR; Thu, 3 Aug 2023 15:39:43 +1000 (AEST) Date: Thu, 3 Aug 2023 15:39:38 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 01/17] netlink: Split up functionality if nl_link() Message-ID: References: <20230724060936.952659-1-david@gibson.dropbear.id.au> <20230724060936.952659-2-david@gibson.dropbear.id.au> <20230803004729.03ca0e36@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="EdxX9PrHrhLQpcDR" Content-Disposition: inline In-Reply-To: Message-ID-Hash: ZTRIG5ONKWT7QSBOJHQZMJ5ZMJOWNDHB X-Message-ID-Hash: ZTRIG5ONKWT7QSBOJHQZMJ5ZMJOWNDHB X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --EdxX9PrHrhLQpcDR Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 =3D !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 =3D { > > > > - .nlh.nlmsg_type =3D change ? RTM_NEWLINK : RTM_GETLINK, > > > > - .nlh.nlmsg_len =3D NLMSG_LENGTH(sizeof(struct ifinfomsg)), > > > > - .nlh.nlmsg_flags =3D NLM_F_REQUEST | (change ? NLM_F_ACK : 0), > > > > + .nlh.nlmsg_type =3D RTM_GETLINK, > > > > + .nlh.nlmsg_len =3D sizeof(req), > > >=20 > > > 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: > > >=20 > > > - NLMSG_LENGTH() aligns to 4 bytes, not to whatever > > > architecture-dependent alignment we might have: the message might > > > actually be smaller > >=20 > > 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. >=20 > 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 =3D 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. --=20 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 --EdxX9PrHrhLQpcDR Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmTLPYAACgkQzQJF27ox 2GdtXBAAhkRKKruCdl90RHxd0b3+cR7ItVlwZOmtZUMVKfhVmwD99hM7+hLxx/dQ M27oHXEMO9icX57eI7E8PrjIDZS6o7u2rs0SRfsrjWgHsn6e3YIndD+z2MOHN2qF XxeLdrAdzEU8ApFJJUyifpFk/k75fgQNtbTHMBv/ECXqSB30+XmqVtdvKjFaFGkU tkiDj36XLLtWrWnWJMIozVearpxc8ijwin3yaOb100oVzul3ZM4nsQSSCbmkeMJ+ MWUqjIvWJ4EPcjLRK4CUKyBzz2IcH9FGM3hrP16rYgs51EK4fLoSB7c6VVf1p9tb Ial+GDj5hngEd95z5IeaWjk/ohfsPSzjDhIsZEtkz/g5sfMAj3CrBN9HAYph8JHx DgAG9cl8SAbJZe52aNW+zk/TuIuDhAhR79ayyx3Ttwce6KN2eiJbALwQTupJ+ykS h8p8tlHfcP/SEJsn5nTDNsbwHKYGSFoL4NRehXAN+aLyeDrwYrnISlCN8LcRNTSj IJXGaE4Zzd0TuDxX/daNHzv94DwVRJnKiAB6Dig9yFKsPCLeQ2T+6rVFg4LcH4TY FqeP6fR2btk1I0Nz+6/fyaWGObYoKXu/iqa0CTzSMIcrquia0vGIvE8qhRsvipOG cm9tnAtGHvsYw86Hdcus0q9FYIbxdSxW1adDL2CiKqiKF/7/56I= =3EV/ -----END PGP SIGNATURE----- --EdxX9PrHrhLQpcDR--