| Message ID | 1c12601bea3e9c18da6adc106bfcf5b7569e5dfb.1729037131.git.gustavoars@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | net: Avoid thousands of -Wflex-array-member-not-at-end warnings | expand |
From: "Gustavo A. R. Silva" <gustavoars@kernel.org> Date: Tue, 15 Oct 2024 18:27:16 -0600 > We are currently working on enabling the -Wflex-array-member-not-at-end > compiler option. This option has helped us detect several objects of > the type `struct sockaddr` that appear in the middle of composite > structures like `struct rtentry`, `struct compat_rtentry`, and others: > > include/uapi/linux/wireless.h:751:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > include/uapi/linux/wireless.h:776:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > include/uapi/linux/wireless.h:833:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > include/uapi/linux/wireless.h:857:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > include/uapi/linux/wireless.h:864:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > include/uapi/linux/route.h:33:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > include/uapi/linux/route.h:34:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > include/uapi/linux/route.h:35:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > include/uapi/linux/if_arp.h:118:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > include/uapi/linux/if_arp.h:119:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > include/uapi/linux/if_arp.h:121:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > include/uapi/linux/if_arp.h:126:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > include/uapi/linux/if_arp.h:127:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > include/net/compat.h:34:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > include/net/compat.h:35:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > fs/nfsd/nfsd.h:74:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > fs/nfsd/nfsd.h:75:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > > In order to fix the warnings above, we introduce `struct sockaddr_legacy`. > The intention is to use it to replace the type of several struct members > in the middle of composite structures, currently of type `struct sockaddr`. > > These middle struct members are currently causing thousands of warnings > because `struct sockaddr` contains a flexible-array member, introduced > by commit b5f0de6df6dce ("net: dev: Convert sa_data to flexible array in > struct sockaddr"). > > The new `struct sockaddr_legacy` doesn't include a flexible-array > member, making it suitable for use as the type of middle members > in composite structs that don't really require the flexible-array > member in `struct sockaddr`, thus avoiding -Wflex-array-member-not-at-end > warnings. > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > include/linux/socket.h | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/include/linux/socket.h b/include/linux/socket.h > index d18cc47e89bd..f370ae0e6c82 100644 > --- a/include/linux/socket.h > +++ b/include/linux/socket.h > @@ -40,6 +40,25 @@ struct sockaddr { > }; > }; > > +/* > + * This is the legacy form of `struct sockaddr`. The original `struct sockaddr` > + * was modified in commit b5f0de6df6dce ("net: dev: Convert sa_data to flexible > + * array in struct sockaddR") due to the fact that "One of the worst offenders s/sockaddR/sockaddr/ The same typo? exists in the cover letter. With it fixed, Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com> > + * of "fake flexible arrays" is struct sockaddr". This means that the original > + * `char sa_data[14]` behaved as a flexible array at runtime, so a proper > + * flexible-array member was introduced. > + * > + * This caused several flexible-array-in-the-middle issues: > + * https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wflex-array-member-not-at-end > + * > + * `struct sockaddr_legacy` replaces `struct sockaddr` in all instances where > + * objects of this type do not appear at the end of composite structures. > + */ > +struct sockaddr_legacy { > + sa_family_t sa_family; /* address family, AF_xxx */ > + char sa_data[14]; /* 14 bytes of protocol address */ > +}; > + > struct linger { > int l_onoff; /* Linger active */ > int l_linger; /* How long to linger for */ > -- > 2.34.1
From: Gustavo A. R. Silva > Sent: 16 October 2024 01:27 > > We are currently working on enabling the -Wflex-array-member-not-at-end > compiler option. This option has helped us detect several objects of > the type `struct sockaddr` that appear in the middle of composite > structures like `struct rtentry`, `struct compat_rtentry`, and others: > ... > > In order to fix the warnings above, we introduce `struct sockaddr_legacy`. > The intention is to use it to replace the type of several struct members > in the middle of composite structures, currently of type `struct sockaddr`. > > These middle struct members are currently causing thousands of warnings > because `struct sockaddr` contains a flexible-array member, introduced > by commit b5f0de6df6dce ("net: dev: Convert sa_data to flexible array in > struct sockaddr"). > > The new `struct sockaddr_legacy` doesn't include a flexible-array > member, making it suitable for use as the type of middle members > in composite structs that don't really require the flexible-array > member in `struct sockaddr`, thus avoiding -Wflex-array-member-not-at-end > warnings. > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > include/linux/socket.h | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/include/linux/socket.h b/include/linux/socket.h > index d18cc47e89bd..f370ae0e6c82 100644 > --- a/include/linux/socket.h > +++ b/include/linux/socket.h > @@ -40,6 +40,25 @@ struct sockaddr { > }; > }; > > +/* > + * This is the legacy form of `struct sockaddr`. The original `struct sockaddr` > + * was modified in commit b5f0de6df6dce ("net: dev: Convert sa_data to flexible > + * array in struct sockaddR") due to the fact that "One of the worst offenders > + * of "fake flexible arrays" is struct sockaddr". This means that the original > + * `char sa_data[14]` behaved as a flexible array at runtime, so a proper > + * flexible-array member was introduced. > + * > + * This caused several flexible-array-in-the-middle issues: > + * https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wflex-array-member-not-at-end I'd bet that the code even indexed the array? So it is all worse that just a compiler warning/ > + * > + * `struct sockaddr_legacy` replaces `struct sockaddr` in all instances where > + * objects of this type do not appear at the end of composite structures. > + */ > +struct sockaddr_legacy { > + sa_family_t sa_family; /* address family, AF_xxx */ > + char sa_data[14]; /* 14 bytes of protocol address */ > +}; > + I'm not sure that is a very good name. Reading it you don't know when it is 'legacy' from. It's size is clearly that of the original IPv4 sockaddr. (I'm not sure there was ever an earlier one.) Perhaps 'strict sockaddr_16' would be better? Or, looking at the actual failures, sockaddr_ipv4? Alternatively revert b5f0de6df6dce and add a new type that has the char[] field?? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
>> >> +/* >> + * This is the legacy form of `struct sockaddr`. The original `struct sockaddr` >> + * was modified in commit b5f0de6df6dce ("net: dev: Convert sa_data to flexible >> + * array in struct sockaddR") due to the fact that "One of the worst offenders > > s/sockaddR/sockaddr/ > > The same typo? exists in the cover letter. Thanks for catching this! :) -- Gustavo > > With it fixed, > > Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com> > > >> + * of "fake flexible arrays" is struct sockaddr". This means that the original >> + * `char sa_data[14]` behaved as a flexible array at runtime, so a proper >> + * flexible-array member was introduced. >> + * >> + * This caused several flexible-array-in-the-middle issues: >> + * https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wflex-array-member-not-at-end >> + * >> + * `struct sockaddr_legacy` replaces `struct sockaddr` in all instances where >> + * objects of this type do not appear at the end of composite structures. >> + */ >> +struct sockaddr_legacy { >> + sa_family_t sa_family; /* address family, AF_xxx */ >> + char sa_data[14]; /* 14 bytes of protocol address */ >> +}; >> + >> struct linger { >> int l_onoff; /* Linger active */ >> int l_linger; /* How long to linger for */ >> -- >> 2.34.1 >
On 22/10/24 06:13, David Laight wrote: > From: Gustavo A. R. Silva >> Sent: 16 October 2024 01:27 >> >> We are currently working on enabling the -Wflex-array-member-not-at-end >> compiler option. This option has helped us detect several objects of >> the type `struct sockaddr` that appear in the middle of composite >> structures like `struct rtentry`, `struct compat_rtentry`, and others: >> > ... >> >> In order to fix the warnings above, we introduce `struct sockaddr_legacy`. >> The intention is to use it to replace the type of several struct members >> in the middle of composite structures, currently of type `struct sockaddr`. >> >> These middle struct members are currently causing thousands of warnings >> because `struct sockaddr` contains a flexible-array member, introduced >> by commit b5f0de6df6dce ("net: dev: Convert sa_data to flexible array in >> struct sockaddr"). >> >> The new `struct sockaddr_legacy` doesn't include a flexible-array >> member, making it suitable for use as the type of middle members >> in composite structs that don't really require the flexible-array >> member in `struct sockaddr`, thus avoiding -Wflex-array-member-not-at-end >> warnings. >> >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> >> --- >> include/linux/socket.h | 19 +++++++++++++++++++ >> 1 file changed, 19 insertions(+) >> >> diff --git a/include/linux/socket.h b/include/linux/socket.h >> index d18cc47e89bd..f370ae0e6c82 100644 >> --- a/include/linux/socket.h >> +++ b/include/linux/socket.h >> @@ -40,6 +40,25 @@ struct sockaddr { >> }; >> }; >> >> +/* >> + * This is the legacy form of `struct sockaddr`. The original `struct sockaddr` >> + * was modified in commit b5f0de6df6dce ("net: dev: Convert sa_data to flexible >> + * array in struct sockaddR") due to the fact that "One of the worst offenders >> + * of "fake flexible arrays" is struct sockaddr". This means that the original >> + * `char sa_data[14]` behaved as a flexible array at runtime, so a proper >> + * flexible-array member was introduced. >> + * >> + * This caused several flexible-array-in-the-middle issues: >> + * https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wflex-array-member-not-at-end > > I'd bet that the code even indexed the array? > So it is all worse that just a compiler warning/ I haven't found evidence of that, but this is precisely what we want to prevent from happening. :) > >> + * >> + * `struct sockaddr_legacy` replaces `struct sockaddr` in all instances where >> + * objects of this type do not appear at the end of composite structures. >> + */ >> +struct sockaddr_legacy { >> + sa_family_t sa_family; /* address family, AF_xxx */ >> + char sa_data[14]; /* 14 bytes of protocol address */ >> +}; >> + > > I'm not sure that is a very good name. > Reading it you don't know when it is 'legacy' from. Yep, naming is hard sometimes. This is why I added that long comment above the struct. :) However, this is changing and now it looks like this: diff --git a/include/uapi/linux/socket.h b/include/uapi/linux/socket.h index d3fcd3b5ec53d2..2e179706bec4d8 100644 --- a/include/uapi/linux/socket.h +++ b/include/uapi/linux/socket.h @@ -35,4 +35,32 @@ struct __kernel_sockaddr_storage { #define SOCK_TXREHASH_DISABLED 0 #define SOCK_TXREHASH_ENABLED 1 +typedef __kernel_sa_family_t sa_family_t; + +/* + * This is the legacy form of `struct sockaddr`. The original `struct sockaddr` + * was modified in commit b5f0de6df6dce ("net: dev: Convert sa_data to flexible + * array in struct sockaddr") due to the fact that "One of the worst offenders + * of "fake flexible arrays" is struct sockaddr". This means that the original + * `char sa_data[14]` behaved as a flexible array at runtime, so a proper + * flexible-array member was introduced. + * + * This caused several flexible-array-in-the-middle issues: + * https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wflex-array-member-not-at-end + * + * `struct sockaddr_legacy` replaces `struct sockaddr` in all instances where + * objects of this type do not appear at the end of composite structures. + */ +struct sockaddr_legacy { + sa_family_t sa_family; /* address family, AF_xxx */ + char sa_data[14]; /* 14 bytes of protocol address */ +}; + +#ifdef __KERNEL__ +# define __kernel_sockaddr_legacy sockaddr_legacy +#else +# define __kernel_sockaddr_legacy sockaddr +#endif + + #endif /* _UAPI_LINUX_SOCKET_H */ https://lore.kernel.org/linux-hardening/202410160942.000495E@keescook/ -- Gustavo > It's size is clearly that of the original IPv4 sockaddr. > (I'm not sure there was ever an earlier one.) > > Perhaps 'strict sockaddr_16' would be better? > Or, looking at the actual failures, sockaddr_ipv4? > > Alternatively revert b5f0de6df6dce and add a new type that has the char[] > field?? > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) >
diff --git a/include/linux/socket.h b/include/linux/socket.h index d18cc47e89bd..f370ae0e6c82 100644 --- a/include/linux/socket.h +++ b/include/linux/socket.h @@ -40,6 +40,25 @@ struct sockaddr { }; }; +/* + * This is the legacy form of `struct sockaddr`. The original `struct sockaddr` + * was modified in commit b5f0de6df6dce ("net: dev: Convert sa_data to flexible + * array in struct sockaddR") due to the fact that "One of the worst offenders + * of "fake flexible arrays" is struct sockaddr". This means that the original + * `char sa_data[14]` behaved as a flexible array at runtime, so a proper + * flexible-array member was introduced. + * + * This caused several flexible-array-in-the-middle issues: + * https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wflex-array-member-not-at-end + * + * `struct sockaddr_legacy` replaces `struct sockaddr` in all instances where + * objects of this type do not appear at the end of composite structures. + */ +struct sockaddr_legacy { + sa_family_t sa_family; /* address family, AF_xxx */ + char sa_data[14]; /* 14 bytes of protocol address */ +}; + struct linger { int l_onoff; /* Linger active */ int l_linger; /* How long to linger for */
We are currently working on enabling the -Wflex-array-member-not-at-end compiler option. This option has helped us detect several objects of the type `struct sockaddr` that appear in the middle of composite structures like `struct rtentry`, `struct compat_rtentry`, and others: include/uapi/linux/wireless.h:751:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] include/uapi/linux/wireless.h:776:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] include/uapi/linux/wireless.h:833:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] include/uapi/linux/wireless.h:857:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] include/uapi/linux/wireless.h:864:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] include/uapi/linux/route.h:33:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] include/uapi/linux/route.h:34:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] include/uapi/linux/route.h:35:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] include/uapi/linux/if_arp.h:118:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] include/uapi/linux/if_arp.h:119:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] include/uapi/linux/if_arp.h:121:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] include/uapi/linux/if_arp.h:126:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] include/uapi/linux/if_arp.h:127:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] include/net/compat.h:34:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] include/net/compat.h:35:25: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] fs/nfsd/nfsd.h:74:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] fs/nfsd/nfsd.h:75:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] In order to fix the warnings above, we introduce `struct sockaddr_legacy`. The intention is to use it to replace the type of several struct members in the middle of composite structures, currently of type `struct sockaddr`. These middle struct members are currently causing thousands of warnings because `struct sockaddr` contains a flexible-array member, introduced by commit b5f0de6df6dce ("net: dev: Convert sa_data to flexible array in struct sockaddr"). The new `struct sockaddr_legacy` doesn't include a flexible-array member, making it suitable for use as the type of middle members in composite structs that don't really require the flexible-array member in `struct sockaddr`, thus avoiding -Wflex-array-member-not-at-end warnings. Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- include/linux/socket.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)