UAPI: net/sched: Open-code __struct_group() in flex struct tc_u32_sel

PATCH In review Kees Cook 3 participants 9 messages Dec 17, 2024 1 message not in the archive

Subsystems (4)

Networking Network drivers Traffic control (tc) CXGB4 Ethernet driver (CXGB4)

lore

Share & tools

lore

Kees Cook

author

2024-12-17 03:00 UTC #0 lore raw

PATCH 1/1 UAPI: net/sched: Open-code __struct_group() in flex struct tc_u32_sel

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 [email protected]

Closes https://lore.kernel.org/linux-hardening/[email protected]/ open here

Fixes 216203bdc228 ("UAPI: net/sched: Use __struct_group() in flex struct tc_u32_sel")

Link https://lore.kernel.org/r/202412120927.943DFEDD@keescook open here

Signed-off-by Kees Cook <[email protected]>

---
Cc: Jakub Kicinski <[email protected]>
Cc: Jamal Hadi Salim <[email protected]>
Cc: Cong Wang <[email protected]>
Cc: Jiri Pirko <[email protected]>
Cc: [email protected]

---
 include/uapi/linux/pkt_cls.h | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

include/uapi/linux/pkt_cls.h +21 ~~-13~~

index 2c32080416b5..02aee6ed6bf0 100644
@@ -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[];
 };

Signature · 1 line

2.34.1

Alexander Lobakin

2024-12-17 14:56 UTC #1 lore raw

From: Kees Cook <[email protected]>
Date: Mon, 16 Dec 2024 18:59:55 -0800

Quoted 5 lines

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?

Quoted 54 lines

To avoid having multiple struct member lists, use a define to declare them.
Reported-by [email protected]
Closes https://lore.kernel.org/linux-hardening/[email protected]/ open here
Fixes 216203bdc228 ("UAPI: net/sched: Use __struct_group() in flex struct tc_u32_sel")
Link https://lore.kernel.org/r/202412120927.943DFEDD@keescook open here
Signed-off-by Kees Cook <[email protected]>
---
Cc: Jakub Kicinski <[email protected]>
Cc: Jamal Hadi Salim <[email protected]>
Cc: Cong Wang <[email protected]>
Cc: Jiri Pirko <[email protected]>
Cc: [email protected]
---
 include/uapi/linux/pkt_cls.h | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)
include/uapi/linux/pkt_cls.h +14 ~~-12~~
index 2c32080416b5..02aee6ed6bf0 100644
@@ -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;

include/uapi/linux/pkt_cls.h struct tc_u32_key { · line 271

Quoted 10 lines

include/uapi/linux/pkt_cls.h +7 -1

-	);
+	/* 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

Gustavo A. R. Silva

2024-12-17 15:58 UTC #2 lore raw

Quoted 7 lines · Alexander Lobakin

From: Kees Cook <[email protected]>
Date: Mon, 16 Dec 2024 18:59:55 -0800
Quoted 4 lines
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"':

Quoted 6 lines

Quoted 1 line
../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:

drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h +1 -1

index 9050568a034c..64663112cad8 100644
@@ -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/[email protected]/

Thanks
--
Gustavo

Alexander Lobakin

2024-12-17 16:05 UTC #3 lore raw

From: Gustavo A. R. Silva <[email protected]>
Date: Tue, 17 Dec 2024 09:58:28 -0600

Quoted 14 lines · Alexander Lobakin

Quoted 11 lines
From: Kees Cook <[email protected]>
Date: Mon, 16 Dec 2024 18:59:55 -0800
Quoted 7 lines
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]

Quoted 7 lines

Quoted 4 lines
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++.

Quoted 16 lines

(quoted diff) renamed from drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h +0 -0
drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h index 9050568a034c..64663112cad8 100644
drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32_parse.h +1 -1
@@ -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;;

Quoted 8 lines

You can also take a look at the original series:
https://lore.kernel.org/linux-hardening/
[email protected]/
Thanks
Signature · 1 line
Gustavo

Thanks,
Olek

Gustavo A. R. Silva

2024-12-17 16:25 UTC #4 lore raw

Quoted 22 lines · Alexander Lobakin

From: Gustavo A. R. Silva <[email protected]>
Date: Tue, 17 Dec 2024 09:58:28 -0600
Quoted 19 lines · Alexander Lobakin
Quoted 15 lines
From: Kees Cook <[email protected]>
Date: Mon, 16 Dec 2024 18:59:55 -0800
Quoted 7 lines
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?

Quoted 5 lines

Quoted 2 lines
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

Alexander Lobakin

2024-12-17 16:55 UTC #5 lore raw

From: Gustavo A. R. Silva <[email protected]>
Date: Tue, 17 Dec 2024 10:25:29 -0600

Quoted 29 lines · Alexander Lobakin

Quoted 26 lines
From: Gustavo A. R. Silva <[email protected]>
Date: Tue, 17 Dec 2024 09:58:28 -0600
Quoted 22 lines · Alexander Lobakin
Quoted 7 lines
From: Kees Cook <[email protected]>
Date: Mon, 16 Dec 2024 18:59:55 -0800
Quoted 4 lines
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"':
Quoted 8 lines
Quoted 3 lines
../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:

Quoted 7 lines

Quoted 2 lines
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?

Quoted 4 lines

Thanks
Signature · 1 line
Gustavo

Thanks,
Olek

Gustavo A. R. Silva

2024-12-17 18:31 UTC #6 lore raw

Quoted 36 lines · Alexander Lobakin

From: Gustavo A. R. Silva <[email protected]>
Date: Tue, 17 Dec 2024 10:25:29 -0600
Quoted 33 lines · Alexander Lobakin
Quoted 28 lines
From: Gustavo A. R. Silva <[email protected]>
Date: Tue, 17 Dec 2024 09:58:28 -0600
Quoted 14 lines · Alexander Lobakin
Quoted 11 lines
From: Kees Cook <[email protected]>
Date: Mon, 16 Dec 2024 18:59:55 -0800
Quoted 7 lines
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]
Quoted 8 lines
Quoted 4 lines
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++.

Quoted 7 lines

Quoted 3 lines
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.

Quoted 5 lines

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

A message that is not in the archive (sent off-list or not yet received): CANtHk4nhH9XJi5+9BAu3kFoL14+4YAZTH7t6QApEvEAeMxdXgw@mail.gmail.com

Gustavo A. R. Silva

2024-12-17 19:19 UTC #8 lore raw

Quoted 2 lines · Christopher Ferris

I verified that this does fix the compilation problem on Android. Thanks for working on this.

Awesome! :)

Thanks for confirming.

-Gustavo

Quoted 74 lines

Christopher
Quoted 67 lines · Gustavo A. R. Silva
Quoted 25 lines · Alexander Lobakin
From: Gustavo A. R. Silva <[email protected]>
Date: Tue, 17 Dec 2024 10:25:29 -0600
Quoted 22 lines · Alexander Lobakin
Quoted 19 lines
From: Gustavo A. R. Silva <[email protected]>
Date: Tue, 17 Dec 2024 09:58:28 -0600
Quoted 16 lines · Alexander Lobakin
Quoted 13 lines
From: Kees Cook <[email protected]>
Date: Mon, 16 Dec 2024 18:59:55 -0800
Quoted 7 lines
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
Quoted 11 lines
Quoted 11 lines
Quoted 9 lines
Quoted 6 lines
Quoted 2 lines
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++.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 5 lines
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

Kees Cook

author

2024-12-17 20:25 UTC #9 lore raw

In reply to a message not in the archive

Quoted 2 lines · Christopher Ferris

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.

Quoted 2 lines

Quoted 2 lines
[0]
https://github.com/alobakin/linux/commit/2a065c7bae821f5fa85fff6f97fbbd460f4aa0f3

-Kees

Signature · 1 line

Kees Cook