| Message ID | 20241217025950.work.601-kees@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | UAPI: net/sched: Open-code __struct_group() in flex struct tc_u32_sel | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
From: Kees Cook <kees@kernel.org> Date: Mon, 16 Dec 2024 18:59:55 -0800 > This switches to using a manually constructed form of struct tagging > to avoid issues with C++ being unable to parse tagged structs within > anonymous unions, even under 'extern "C"': > > ../linux/include/uapi/linux/pkt_cls.h:25124: error: ‘struct tc_u32_sel::<unnamed union>::tc_u32_sel_hdr,’ invalid; an anonymous union may only have public non-static data members [-fpermissive] I worked around that like this in the past: [0] As I'm not sure it would be fine to fix every such occurrence manually by open-coding. What do you think? > > To avoid having multiple struct member lists, use a define to declare > them. > > Reported-by: cferris@google.com > Closes: https://lore.kernel.org/linux-hardening/Z1HZpe3WE5As8UAz@google.com/ > Fixes: 216203bdc228 ("UAPI: net/sched: Use __struct_group() in flex struct tc_u32_sel") > Link: https://lore.kernel.org/r/202412120927.943DFEDD@keescook > Signed-off-by: Kees Cook <kees@kernel.org> > --- > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Jamal Hadi Salim <jhs@mojatatu.com> > Cc: Cong Wang <xiyou.wangcong@gmail.com> > Cc: Jiri Pirko <jiri@resnulli.us> > Cc: netdev@vger.kernel.org > --- > include/uapi/linux/pkt_cls.h | 34 +++++++++++++++++++++------------- > 1 file changed, 21 insertions(+), 13 deletions(-) > > diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h > index 2c32080416b5..02aee6ed6bf0 100644 > --- a/include/uapi/linux/pkt_cls.h > +++ b/include/uapi/linux/pkt_cls.h > @@ -245,20 +245,28 @@ struct tc_u32_key { > int offmask; > }; > > +#define tc_u32_sel_hdr_members \ > + unsigned char flags; \ > + unsigned char offshift; \ > + unsigned char nkeys; \ > + __be16 offmask; \ > + __u16 off; \ > + short offoff; \ > + short hoff; \ > + __be32 hmask > + > +struct tc_u32_sel_hdr { > + tc_u32_sel_hdr_members; > +}; > + > struct tc_u32_sel { > - /* New members MUST be added within the __struct_group() macro below. */ > - __struct_group(tc_u32_sel_hdr, hdr, /* no attrs */, > - unsigned char flags; > - unsigned char offshift; > - unsigned char nkeys; > - > - __be16 offmask; > - __u16 off; > - short offoff; > - > - short hoff; > - __be32 hmask; > - ); > + /* Open-coded struct_group() to avoid C++ errors. */ > + union { > + struct tc_u32_sel_hdr hdr; > + struct { > + tc_u32_sel_hdr_members; > + }; > + }; > struct tc_u32_key keys[]; > }; [0] https://github.com/alobakin/linux/commit/2a065c7bae821f5fa85fff6f97fbbd460f4aa0f3 Thanks, Olek
On 17/12/24 08:55, Alexander Lobakin wrote: > From: Kees Cook <kees@kernel.org> > Date: Mon, 16 Dec 2024 18:59:55 -0800 > >> This switches to using a manually constructed form of struct tagging >> to avoid issues with C++ being unable to parse tagged structs within >> anonymous unions, even under 'extern "C"': >> >> ../linux/include/uapi/linux/pkt_cls.h:25124: error: ‘struct tc_u32_sel::<unnamed union>::tc_u32_sel_hdr,’ invalid; an anonymous union may only have public non-static data members [-fpermissive] > > I worked around that like this in the past: [0] > As I'm not sure it would be fine to fix every such occurrence manually > by open-coding. > What do you think? The thing is that, in this particular case, we need a struct tag to change the type of an object in another struct. See: diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h index 9050568a034c..64663112cad8 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h @@ -242,7 +242,7 @@ struct cxgb4_next_header { * field's value to jump to next header such as IHL field * in IPv4 header. */ - struct tc_u32_sel sel; + struct tc_u32_sel_hdr sel; struct tc_u32_key key; /* location of jump to make */ const struct cxgb4_match_field *jump;; You can also take a look at the original series: https://lore.kernel.org/linux-hardening/cover.1723586870.git.gustavoars@kernel.org/ Thanks -- Gustavo
From: Gustavo A. R. Silva <gustavo@embeddedor.com> Date: Tue, 17 Dec 2024 09:58:28 -0600 > > > On 17/12/24 08:55, Alexander Lobakin wrote: >> From: Kees Cook <kees@kernel.org> >> Date: Mon, 16 Dec 2024 18:59:55 -0800 >> >>> This switches to using a manually constructed form of struct tagging >>> to avoid issues with C++ being unable to parse tagged structs within >>> anonymous unions, even under 'extern "C"': >>> >>> ../linux/include/uapi/linux/pkt_cls.h:25124: error: ‘struct >>> tc_u32_sel::<unnamed union>::tc_u32_sel_hdr,’ invalid; an anonymous >>> union may only have public non-static data members [-fpermissive] >> >> I worked around that like this in the past: [0] >> As I'm not sure it would be fine to fix every such occurrence manually >> by open-coding. >> What do you think? > > The thing is that, in this particular case, we need a struct tag to change > the type of an object in another struct. See: But the fix I mentioned still allows you to specify a tag in C code... cxgb4 is for sure not C++. > > diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h b/ > drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h > index 9050568a034c..64663112cad8 100644 > --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h > +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h > @@ -242,7 +242,7 @@ struct cxgb4_next_header { > * field's value to jump to next header such as IHL field > * in IPv4 header. > */ > - struct tc_u32_sel sel; > + struct tc_u32_sel_hdr sel; > struct tc_u32_key key; > /* location of jump to make */ > const struct cxgb4_match_field *jump;; > > You can also take a look at the original series: > > https://lore.kernel.org/linux-hardening/ > cover.1723586870.git.gustavoars@kernel.org/ > > Thanks > -- > Gustavo Thanks, Olek
On 17/12/24 10:04, Alexander Lobakin wrote: > From: Gustavo A. R. Silva <gustavo@embeddedor.com> > Date: Tue, 17 Dec 2024 09:58:28 -0600 > >> >> >> On 17/12/24 08:55, Alexander Lobakin wrote: >>> From: Kees Cook <kees@kernel.org> >>> Date: Mon, 16 Dec 2024 18:59:55 -0800 >>> >>>> This switches to using a manually constructed form of struct tagging >>>> to avoid issues with C++ being unable to parse tagged structs within >>>> anonymous unions, even under 'extern "C"': >>>> >>>> ../linux/include/uapi/linux/pkt_cls.h:25124: error: ‘struct >>>> tc_u32_sel::<unnamed union>::tc_u32_sel_hdr,’ invalid; an anonymous >>>> union may only have public non-static data members [-fpermissive] >>> >>> I worked around that like this in the past: [0] >>> As I'm not sure it would be fine to fix every such occurrence manually >>> by open-coding. >>> What do you think? >> >> The thing is that, in this particular case, we need a struct tag to change >> the type of an object in another struct. See: > > But the fix I mentioned still allows you to specify a tag in C code... > cxgb4 is for sure not C++. Oh yes, I see what you mean. If it works, then you should probably submit that patch upstream. :) Thanks -- Gustavo
From: Gustavo A. R. Silva <gustavo@embeddedor.com> Date: Tue, 17 Dec 2024 10:25:29 -0600 > > > On 17/12/24 10:04, Alexander Lobakin wrote: >> From: Gustavo A. R. Silva <gustavo@embeddedor.com> >> Date: Tue, 17 Dec 2024 09:58:28 -0600 >> >>> >>> >>> On 17/12/24 08:55, Alexander Lobakin wrote: >>>> From: Kees Cook <kees@kernel.org> >>>> Date: Mon, 16 Dec 2024 18:59:55 -0800 >>>> >>>>> This switches to using a manually constructed form of struct tagging >>>>> to avoid issues with C++ being unable to parse tagged structs within >>>>> anonymous unions, even under 'extern "C"': >>>>> >>>>> ../linux/include/uapi/linux/pkt_cls.h:25124: error: ‘struct >>>>> tc_u32_sel::<unnamed union>::tc_u32_sel_hdr,’ invalid; an anonymous >>>>> union may only have public non-static data members [-fpermissive] >>>> >>>> I worked around that like this in the past: [0] >>>> As I'm not sure it would be fine to fix every such occurrence manually >>>> by open-coding. >>>> What do you think? >>> >>> The thing is that, in this particular case, we need a struct tag to >>> change >>> the type of an object in another struct. See: >> >> But the fix I mentioned still allows you to specify a tag in C code... >> cxgb4 is for sure not C++. > > > Oh yes, I see what you mean. If it works, then you should probably > submit that > patch upstream. :) I added it to my CI tree and will wait for a report (24-36 hrs) before sending. In the meantime, feel free to test whether it solves your issue and give a Tested-by (or an error report :)). BTW, I mentioned in the commit message back in 2022 that some C++ standards support tagged structs with anonymous unions (I don't remember that already). Would it make sense to use a separate #define not for the whole __cplusplus, but only for certain standards? > > Thanks > -- > Gustavo Thanks, Olek
On 17/12/24 10:54, Alexander Lobakin wrote: > From: Gustavo A. R. Silva <gustavo@embeddedor.com> > Date: Tue, 17 Dec 2024 10:25:29 -0600 > >> >> >> On 17/12/24 10:04, Alexander Lobakin wrote: >>> From: Gustavo A. R. Silva <gustavo@embeddedor.com> >>> Date: Tue, 17 Dec 2024 09:58:28 -0600 >>> >>>> >>>> >>>> On 17/12/24 08:55, Alexander Lobakin wrote: >>>>> From: Kees Cook <kees@kernel.org> >>>>> Date: Mon, 16 Dec 2024 18:59:55 -0800 >>>>> >>>>>> This switches to using a manually constructed form of struct tagging >>>>>> to avoid issues with C++ being unable to parse tagged structs within >>>>>> anonymous unions, even under 'extern "C"': >>>>>> >>>>>> ../linux/include/uapi/linux/pkt_cls.h:25124: error: ‘struct >>>>>> tc_u32_sel::<unnamed union>::tc_u32_sel_hdr,’ invalid; an anonymous >>>>>> union may only have public non-static data members [-fpermissive] >>>>> >>>>> I worked around that like this in the past: [0] >>>>> As I'm not sure it would be fine to fix every such occurrence manually >>>>> by open-coding. >>>>> What do you think? >>>> >>>> The thing is that, in this particular case, we need a struct tag to >>>> change >>>> the type of an object in another struct. See: >>> >>> But the fix I mentioned still allows you to specify a tag in C code... >>> cxgb4 is for sure not C++. >> >> >> Oh yes, I see what you mean. If it works, then you should probably >> submit that >> patch upstream. :) > > I added it to my CI tree and will wait for a report (24-36 hrs) before > sending. In the meantime, feel free to test whether it solves your issue > and give a Tested-by (or an error report :)). Hopefully, Christopher can confirm whether this[0] resolves the issue he's seeing. > > BTW, I mentioned in the commit message back in 2022 that some C++ > standards support tagged structs with anonymous unions (I don't remember > that already). Would it make sense to use a separate #define not for the > whole __cplusplus, but only for certain standards? I'd say entirely preventing C++ from seeing the tag is cleaner and safer for now. Thanks -Gustavo [0] https://github.com/alobakin/linux/commit/2a065c7bae821f5fa85fff6f97fbbd460f4aa0f3
On 17/12/24 13:10, Christopher Ferris wrote: > I verified that this does fix the compilation problem on Android. Thanks > for working on this. Awesome! :) Thanks for confirming. -Gustavo > > Christopher > > On Tue, Dec 17, 2024 at 10:31 AM Gustavo A. R. Silva <gustavo@embeddedor.com> > wrote: > >> >> >> On 17/12/24 10:54, Alexander Lobakin wrote: >>> From: Gustavo A. R. Silva <gustavo@embeddedor.com> >>> Date: Tue, 17 Dec 2024 10:25:29 -0600 >>> >>>> >>>> >>>> On 17/12/24 10:04, Alexander Lobakin wrote: >>>>> From: Gustavo A. R. Silva <gustavo@embeddedor.com> >>>>> Date: Tue, 17 Dec 2024 09:58:28 -0600 >>>>> >>>>>> >>>>>> >>>>>> On 17/12/24 08:55, Alexander Lobakin wrote: >>>>>>> From: Kees Cook <kees@kernel.org> >>>>>>> Date: Mon, 16 Dec 2024 18:59:55 -0800 >>>>>>> >>>>>>>> This switches to using a manually constructed form of struct tagging >>>>>>>> to avoid issues with C++ being unable to parse tagged structs within >>>>>>>> anonymous unions, even under 'extern "C"': >>>>>>>> >>>>>>>> ../linux/include/uapi/linux/pkt_cls.h:25124: error: ‘struct >>>>>>>> tc_u32_sel::<unnamed union>::tc_u32_sel_hdr,’ invalid; an anonymous >>>>>>>> union may only have public non-static data members [-fpermissive] >>>>>>> >>>>>>> I worked around that like this in the past: [0] >>>>>>> As I'm not sure it would be fine to fix every such occurrence >> manually >>>>>>> by open-coding. >>>>>>> What do you think? >>>>>> >>>>>> The thing is that, in this particular case, we need a struct tag to >>>>>> change >>>>>> the type of an object in another struct. See: >>>>> >>>>> But the fix I mentioned still allows you to specify a tag in C code... >>>>> cxgb4 is for sure not C++. >>>> >>>> >>>> Oh yes, I see what you mean. If it works, then you should probably >>>> submit that >>>> patch upstream. :) >>> >>> I added it to my CI tree and will wait for a report (24-36 hrs) before >>> sending. In the meantime, feel free to test whether it solves your issue >>> and give a Tested-by (or an error report :)). >> >> Hopefully, Christopher can confirm whether this[0] resolves the issue he's >> seeing. >> >>> >>> BTW, I mentioned in the commit message back in 2022 that some C++ >>> standards support tagged structs with anonymous unions (I don't remember >>> that already). Would it make sense to use a separate #define not for the >>> whole __cplusplus, but only for certain standards? >> >> I'd say entirely preventing C++ from seeing the tag is cleaner and safer >> for >> now. >> >> Thanks >> -Gustavo >> >> [0] >> https://github.com/alobakin/linux/commit/2a065c7bae821f5fa85fff6f97fbbd460f4aa0f3 >> >
On Tue, Dec 17, 2024 at 11:10:41AM -0800, Christopher Ferris wrote: > I verified that this does fix the compilation problem on Android. Thanks > for working on this. Thanks! Yeah, let's use Alexander's solution instead of my proposed patch. > > [0] > > https://github.com/alobakin/linux/commit/2a065c7bae821f5fa85fff6f97fbbd460f4aa0f3 -Kees
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h index 2c32080416b5..02aee6ed6bf0 100644 --- a/include/uapi/linux/pkt_cls.h +++ b/include/uapi/linux/pkt_cls.h @@ -245,20 +245,28 @@ struct tc_u32_key { int offmask; }; +#define tc_u32_sel_hdr_members \ + unsigned char flags; \ + unsigned char offshift; \ + unsigned char nkeys; \ + __be16 offmask; \ + __u16 off; \ + short offoff; \ + short hoff; \ + __be32 hmask + +struct tc_u32_sel_hdr { + tc_u32_sel_hdr_members; +}; + struct tc_u32_sel { - /* New members MUST be added within the __struct_group() macro below. */ - __struct_group(tc_u32_sel_hdr, hdr, /* no attrs */, - unsigned char flags; - unsigned char offshift; - unsigned char nkeys; - - __be16 offmask; - __u16 off; - short offoff; - - short hoff; - __be32 hmask; - ); + /* Open-coded struct_group() to avoid C++ errors. */ + union { + struct tc_u32_sel_hdr hdr; + struct { + tc_u32_sel_hdr_members; + }; + }; struct tc_u32_key keys[]; };
This switches to using a manually constructed form of struct tagging to avoid issues with C++ being unable to parse tagged structs within anonymous unions, even under 'extern "C"': ../linux/include/uapi/linux/pkt_cls.h:25124: error: ‘struct tc_u32_sel::<unnamed union>::tc_u32_sel_hdr,’ invalid; an anonymous union may only have public non-static data members [-fpermissive] To avoid having multiple struct member lists, use a define to declare them. Reported-by: cferris@google.com Closes: https://lore.kernel.org/linux-hardening/Z1HZpe3WE5As8UAz@google.com/ Fixes: 216203bdc228 ("UAPI: net/sched: Use __struct_group() in flex struct tc_u32_sel") Link: https://lore.kernel.org/r/202412120927.943DFEDD@keescook Signed-off-by: Kees Cook <kees@kernel.org> --- Cc: Jakub Kicinski <kuba@kernel.org> Cc: Jamal Hadi Salim <jhs@mojatatu.com> Cc: Cong Wang <xiyou.wangcong@gmail.com> Cc: Jiri Pirko <jiri@resnulli.us> Cc: netdev@vger.kernel.org --- include/uapi/linux/pkt_cls.h | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-)