| Message ID | Yw5H3E3a6mmpuTeT@work (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [next] uapi: net/ipv4: Use __DECLARE_FLEX_ARRAY() helper | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Tue, Aug 30, 2022 at 12:24:44PM -0500, Gustavo A. R. Silva wrote: > We now have a cleaner way to keep compatibility with user-space > (a.k.a. not breaking it) when we need to keep in place a one-element > array (for its use in user-space) together with a flexible-array > member (for its use in kernel-space) without making it hard to read > at the source level. This is through the use of the new > __DECLARE_FLEX_ARRAY() helper macro. > > The size and memory layout of the structure is preserved after the > changes. See below. > > Before changes: > > $ pahole -C ip_msfilter net/ipv4/igmp.o > struct ip_msfilter { > union { > struct { > __be32 imsf_multiaddr_aux; /* 0 4 */ > __be32 imsf_interface_aux; /* 4 4 */ > __u32 imsf_fmode_aux; /* 8 4 */ > __u32 imsf_numsrc_aux; /* 12 4 */ > __be32 imsf_slist[1]; /* 16 4 */ > }; /* 0 20 */ > struct { > __be32 imsf_multiaddr; /* 0 4 */ > __be32 imsf_interface; /* 4 4 */ > __u32 imsf_fmode; /* 8 4 */ > __u32 imsf_numsrc; /* 12 4 */ > __be32 imsf_slist_flex[0]; /* 16 0 */ > }; /* 0 16 */ > }; /* 0 20 */ > > /* size: 20, cachelines: 1, members: 1 */ > /* last cacheline: 20 bytes */ > }; > > After changes: > > $ pahole -C ip_msfilter net/ipv4/igmp.o > struct ip_msfilter { > struct { > __be32 imsf_multiaddr; /* 0 4 */ > __be32 imsf_interface; /* 4 4 */ > __u32 imsf_fmode; /* 8 4 */ > __u32 imsf_numsrc; /* 12 4 */ > union { > __be32 imsf_slist[1]; /* 16 4 */ > struct { > struct { > } __empty_imsf_slist_flex; /* 16 0 */ > __be32 imsf_slist_flex[0]; /* 16 0 */ > }; /* 16 0 */ > }; /* 16 4 */ > }; /* 0 20 */ > > /* size: 20, cachelines: 1, members: 1 */ > /* last cacheline: 20 bytes */ > }; > > In the past, we had to duplicate the whole original structure within > a union, and update the names of all the members. Now, we just need to > declare the flexible-array member to be used in kernel-space through > the __DECLARE_FLEX_ARRAY() helper together with the one-element array, > within a union. This makes the source code more clean and easier to read. > > Link: https://github.com/KSPP/linux/issues/193 > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > include/uapi/linux/in.h | 20 +++++++------------- > 1 file changed, 7 insertions(+), 13 deletions(-) > > diff --git a/include/uapi/linux/in.h b/include/uapi/linux/in.h > index 14168225cecd..fa4dc8f8f081 100644 > --- a/include/uapi/linux/in.h > +++ b/include/uapi/linux/in.h > @@ -188,20 +188,14 @@ struct ip_mreq_source { > }; > > struct ip_msfilter { > - union { > - struct { > - __be32 imsf_multiaddr_aux; > - __be32 imsf_interface_aux; > - __u32 imsf_fmode_aux; > - __u32 imsf_numsrc_aux; > + struct { I don't think this internal anonymous struct is needed any more? > + __be32 imsf_multiaddr; > + __be32 imsf_interface; > + __u32 imsf_fmode; > + __u32 imsf_numsrc; > + union { > __be32 imsf_slist[1]; > - }; > - struct { > - __be32 imsf_multiaddr; > - __be32 imsf_interface; > - __u32 imsf_fmode; > - __u32 imsf_numsrc; > - __be32 imsf_slist_flex[]; > + __DECLARE_FLEX_ARRAY(__be32, imsf_slist_flex); > }; > }; > }; I.e. can't this now just be: struct ip_msfilter { __be32 imsf_multiaddr; __be32 imsf_interface; __u32 imsf_fmode; __u32 imsf_numsrc; union { __be32 imsf_slist[1]; __DECLARE_FLEX_ARRAY(__be32, imsf_slist_flex); }; }; > -- > 2.34.1 >
On Wed, Aug 31, 2022 at 11:17:31AM -0700, Kees Cook wrote: > > struct ip_msfilter { > > - union { > > - struct { > > - __be32 imsf_multiaddr_aux; > > - __be32 imsf_interface_aux; > > - __u32 imsf_fmode_aux; > > - __u32 imsf_numsrc_aux; > > + struct { > > I don't think this internal anonymous struct is needed any more? yes, aaargh... copy/paste error D: I'll respin right away. Thanks! -- Gustavo
diff --git a/include/uapi/linux/in.h b/include/uapi/linux/in.h index 14168225cecd..fa4dc8f8f081 100644 --- a/include/uapi/linux/in.h +++ b/include/uapi/linux/in.h @@ -188,20 +188,14 @@ struct ip_mreq_source { }; struct ip_msfilter { - union { - struct { - __be32 imsf_multiaddr_aux; - __be32 imsf_interface_aux; - __u32 imsf_fmode_aux; - __u32 imsf_numsrc_aux; + struct { + __be32 imsf_multiaddr; + __be32 imsf_interface; + __u32 imsf_fmode; + __u32 imsf_numsrc; + union { __be32 imsf_slist[1]; - }; - struct { - __be32 imsf_multiaddr; - __be32 imsf_interface; - __u32 imsf_fmode; - __u32 imsf_numsrc; - __be32 imsf_slist_flex[]; + __DECLARE_FLEX_ARRAY(__be32, imsf_slist_flex); }; }; };
We now have a cleaner way to keep compatibility with user-space (a.k.a. not breaking it) when we need to keep in place a one-element array (for its use in user-space) together with a flexible-array member (for its use in kernel-space) without making it hard to read at the source level. This is through the use of the new __DECLARE_FLEX_ARRAY() helper macro. The size and memory layout of the structure is preserved after the changes. See below. Before changes: $ pahole -C ip_msfilter net/ipv4/igmp.o struct ip_msfilter { union { struct { __be32 imsf_multiaddr_aux; /* 0 4 */ __be32 imsf_interface_aux; /* 4 4 */ __u32 imsf_fmode_aux; /* 8 4 */ __u32 imsf_numsrc_aux; /* 12 4 */ __be32 imsf_slist[1]; /* 16 4 */ }; /* 0 20 */ struct { __be32 imsf_multiaddr; /* 0 4 */ __be32 imsf_interface; /* 4 4 */ __u32 imsf_fmode; /* 8 4 */ __u32 imsf_numsrc; /* 12 4 */ __be32 imsf_slist_flex[0]; /* 16 0 */ }; /* 0 16 */ }; /* 0 20 */ /* size: 20, cachelines: 1, members: 1 */ /* last cacheline: 20 bytes */ }; After changes: $ pahole -C ip_msfilter net/ipv4/igmp.o struct ip_msfilter { struct { __be32 imsf_multiaddr; /* 0 4 */ __be32 imsf_interface; /* 4 4 */ __u32 imsf_fmode; /* 8 4 */ __u32 imsf_numsrc; /* 12 4 */ union { __be32 imsf_slist[1]; /* 16 4 */ struct { struct { } __empty_imsf_slist_flex; /* 16 0 */ __be32 imsf_slist_flex[0]; /* 16 0 */ }; /* 16 0 */ }; /* 16 4 */ }; /* 0 20 */ /* size: 20, cachelines: 1, members: 1 */ /* last cacheline: 20 bytes */ }; In the past, we had to duplicate the whole original structure within a union, and update the names of all the members. Now, we just need to declare the flexible-array member to be used in kernel-space through the __DECLARE_FLEX_ARRAY() helper together with the one-element array, within a union. This makes the source code more clean and easier to read. Link: https://github.com/KSPP/linux/issues/193 Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- include/uapi/linux/in.h | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)