Please apply this to stable 5.10.y, 5.15.y ---8<---
From: Kees Cook keescook@chromium.org
Upstream commit: 50d7bd38c3aa ("stddef: Introduce struct_group() helper macro")
Kernel code has a regular need to describe groups of members within a structure usually when they need to be copied or initialized separately from the rest of the surrounding structure. The generally accepted design pattern in C is to use a named sub-struct:
struct foo { int one; struct { int two; int three, four; } thing; int five; };
This would allow for traditional references and sizing:
memcpy(&dst.thing, &src.thing, sizeof(dst.thing));
However, doing this would mean that referencing struct members enclosed by such named structs would always require including the sub-struct name in identifiers:
do_something(dst.thing.three);
This has tended to be quite inflexible, especially when such groupings need to be added to established code which causes huge naming churn. Three workarounds exist in the kernel for this problem, and each have other negative properties.
To avoid the naming churn, there is a design pattern of adding macro aliases for the named struct:
#define f_three thing.three
This ends up polluting the global namespace, and makes it difficult to search for identifiers.
Another common work-around in kernel code avoids the pollution by avoiding the named struct entirely, instead identifying the group's boundaries using either a pair of empty anonymous structs of a pair of zero-element arrays:
struct foo { int one; struct { } start; int two; int three, four; struct { } finish; int five; };
struct foo { int one; int start[0]; int two; int three, four; int finish[0]; int five; };
This allows code to avoid needing to use a sub-struct named for member references within the surrounding structure, but loses the benefits of being able to actually use such a struct, making it rather fragile. Using these requires open-coded calculation of sizes and offsets. The efforts made to avoid common mistakes include lots of comments, or adding various BUILD_BUG_ON()s. Such code is left with no way for the compiler to reason about the boundaries (e.g. the "start" object looks like it's 0 bytes in length), making bounds checking depend on open-coded calculations:
if (length > offsetof(struct foo, finish) - offsetof(struct foo, start)) return -EINVAL; memcpy(&dst.start, &src.start, offsetof(struct foo, finish) - offsetof(struct foo, start));
However, the vast majority of places in the kernel that operate on groups of members do so without any identification of the grouping, relying either on comments or implicit knowledge of the struct contents, which is even harder for the compiler to reason about, and results in even more fragile manual sizing, usually depending on member locations outside of the region (e.g. to copy "two" and "three", use the start of "four" to find the size):
BUILD_BUG_ON((offsetof(struct foo, four) < offsetof(struct foo, two)) || (offsetof(struct foo, four) < offsetof(struct foo, three)); if (length > offsetof(struct foo, four) - offsetof(struct foo, two)) return -EINVAL; memcpy(&dst.two, &src.two, length);
In order to have a regular programmatic way to describe a struct region that can be used for references and sizing, can be examined for bounds checking, avoids forcing the use of intermediate identifiers, and avoids polluting the global namespace, introduce the struct_group() macro. This macro wraps the member declarations to create an anonymous union of an anonymous struct (no intermediate name) and a named struct (for references and sizing):
struct foo { int one; struct_group(thing, int two; int three, four; ); int five; };
if (length > sizeof(src.thing)) return -EINVAL; memcpy(&dst.thing, &src.thing, length); do_something(dst.three);
There are some rare cases where the resulting struct_group() needs attributes added, so struct_group_attr() is also introduced to allow for specifying struct attributes (e.g. __align(x) or __packed). Additionally, there are places where such declarations would like to have the struct be tagged, so struct_group_tagged() is added.
Given there is a need for a handful of UAPI uses too, the underlying __struct_group() macro has been defined in UAPI so it can be used there too.
To avoid confusing scripts/kernel-doc, hide the macro from its struct parsing.
Co-developed-by: Keith Packard keithp@keithp.com Signed-off-by: Keith Packard keithp@keithp.com Acked-by: Gustavo A. R. Silva gustavoars@kernel.org Link: https://lore.kernel.org/lkml/20210728023217.GC35706@embeddedor Enhanced-by: Rasmus Villemoes linux@rasmusvillemoes.dk Link: https://lore.kernel.org/lkml/41183a98-bdb9-4ad6-7eab-5a7292a6df84@rasmusvill... Enhanced-by: Dan Williams dan.j.williams@intel.com Link: https://lore.kernel.org/lkml/1d9a2e6df2a9a35b2cdd50a9a68cac5991e7e5f0.camel@... Enhanced-by: Daniel Vetter daniel.vetter@ffwll.ch Link: https://lore.kernel.org/lkml/YQKa76A6XuFqgM03@phenom.ffwll.local Acked-by: Dan Williams dan.j.williams@intel.com Signed-off-by: Kees Cook keescook@chromium.org Signed-off-by: Tadeusz Struk tadeusz.struk@linaro.org --- include/linux/stddef.h | 48 +++++++++++++++++++++++++++++++++++++ include/uapi/linux/stddef.h | 24 +++++++++++++++++++ 2 files changed, 72 insertions(+)
diff --git a/include/linux/stddef.h b/include/linux/stddef.h index 998a4ba28eba..938216f8ab7e 100644 --- a/include/linux/stddef.h +++ b/include/linux/stddef.h @@ -36,4 +36,52 @@ enum { #define offsetofend(TYPE, MEMBER) \ (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
+/** + * struct_group() - Wrap a set of declarations in a mirrored struct + * + * @NAME: The identifier name of the mirrored sub-struct + * @MEMBERS: The member declarations for the mirrored structs + * + * Used to create an anonymous union of two structs with identical + * layout and size: one anonymous and one named. The former can be + * used normally without sub-struct naming, and the latter can be + * used to reason about the start, end, and size of the group of + * struct members. + */ +#define struct_group(NAME, MEMBERS...) \ + __struct_group(/* no tag */, NAME, /* no attrs */, MEMBERS) + +/** + * struct_group_attr() - Create a struct_group() with trailing attributes + * + * @NAME: The identifier name of the mirrored sub-struct + * @ATTRS: Any struct attributes to apply + * @MEMBERS: The member declarations for the mirrored structs + * + * Used to create an anonymous union of two structs with identical + * layout and size: one anonymous and one named. The former can be + * used normally without sub-struct naming, and the latter can be + * used to reason about the start, end, and size of the group of + * struct members. Includes structure attributes argument. + */ +#define struct_group_attr(NAME, ATTRS, MEMBERS...) \ + __struct_group(/* no tag */, NAME, ATTRS, MEMBERS) + +/** + * struct_group_tagged() - Create a struct_group with a reusable tag + * + * @TAG: The tag name for the named sub-struct + * @NAME: The identifier name of the mirrored sub-struct + * @MEMBERS: The member declarations for the mirrored structs + * + * Used to create an anonymous union of two structs with identical + * layout and size: one anonymous and one named. The former can be + * used normally without sub-struct naming, and the latter can be + * used to reason about the start, end, and size of the group of + * struct members. Includes struct tag argument for the named copy, + * so the specified layout can be reused later. + */ +#define struct_group_tagged(TAG, NAME, MEMBERS...) \ + __struct_group(TAG, NAME, /* no attrs */, MEMBERS) + #endif diff --git a/include/uapi/linux/stddef.h b/include/uapi/linux/stddef.h index ee8220f8dcf5..9f5da295ff1c 100644 --- a/include/uapi/linux/stddef.h +++ b/include/uapi/linux/stddef.h @@ -1,6 +1,30 @@ /* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ #include <linux/compiler_types.h> +#ifndef _UAPI_LINUX_STDDEF_H +#define _UAPI_LINUX_STDDEF_H
#ifndef __always_inline #define __always_inline inline #endif + +/** + * __struct_group() - Create a mirrored named and anonyomous struct + * + * @TAG: The tag name for the named sub-struct (usually empty) + * @NAME: The identifier name of the mirrored sub-struct + * @ATTRS: Any struct attributes (usually empty) + * @MEMBERS: The member declarations for the mirrored structs + * + * Used to create an anonymous union of two structs with identical layout + * and size: one anonymous and one named. The former's members can be used + * normally without sub-struct naming, and the latter can be used to + * reason about the start, end, and size of the group of struct members. + * The named struct can also be explicitly tagged for layer reuse, as well + * as both having struct attributes appended. + */ +#define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \ + union { \ + struct { MEMBERS } ATTRS; \ + struct TAG { MEMBERS } ATTRS NAME; \ + } +#endif
Please apply this to stable 5.10.y, and 5.15.y ---8<---
From: Kees Cook keescook@chromium.org
Upstream commit: 1a2fb220edca ("skbuff: Extract list pointers to silence compiler warnings")
Under both -Warray-bounds and the object_size sanitizer, the compiler is upset about accessing prev/next of sk_buff when the object it thinks it is coming from is sk_buff_head. The warning is a false positive due to the compiler taking a conservative approach, opting to warn at casting time rather than access time.
However, in support of enabling -Warray-bounds globally (which has found many real bugs), arrange things for sk_buff so that the compiler can unambiguously see that there is no intention to access anything except prev/next. Introduce and cast to a separate struct sk_buff_list, which contains _only_ the first two fields, silencing the warnings:
In file included from ./include/net/net_namespace.h:39, from ./include/linux/netdevice.h:37, from net/core/netpoll.c:17: net/core/netpoll.c: In function 'refill_skbs': ./include/linux/skbuff.h:2086:9: warning: array subscript 'struct sk_buff[0]' is partly outside array bounds of 'struct sk_buff_head[1]' [-Warray-bounds] 2086 | __skb_insert(newsk, next->prev, next, list); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ net/core/netpoll.c:49:28: note: while referencing 'skb_pool' 49 | static struct sk_buff_head skb_pool; | ^~~~~~~~
This also upsets UBSAN, which throws a runtime object-size-mismatch error complaining about skbuff queue helpers, as below, when the kernel is built with clang and -fsanitize=undefined flag set:
UBSAN: object-size-mismatch in ./include/linux/skbuff.h:2023:28 member access within address ffffc90000cb71c0 with insufficient space for an object of type 'struct sk_buff'
This change results in no executable instruction differences.
Signed-off-by: Kees Cook keescook@chromium.org Link: https://lore.kernel.org/r/20211207062758.2324338-1-keescook@chromium.org Signed-off-by: Jakub Kicinski kuba@kernel.org Signed-off-by: Tadeusz Struk tadeusz.struk@linaro.org --- include/linux/skbuff.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index acbf1875ad50..b7de22193ec8 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -289,9 +289,11 @@ struct tc_skb_ext { #endif
struct sk_buff_head { + struct_group_tagged(sk_buff_list, list, /* These two members must be first. */ struct sk_buff *next; struct sk_buff *prev; + );
__u32 qlen; spinlock_t lock; @@ -1906,8 +1908,8 @@ static inline void __skb_insert(struct sk_buff *newsk, */ WRITE_ONCE(newsk->next, next); WRITE_ONCE(newsk->prev, prev); - WRITE_ONCE(next->prev, newsk); - WRITE_ONCE(prev->next, newsk); + WRITE_ONCE(((struct sk_buff_list *)next)->prev, newsk); + WRITE_ONCE(((struct sk_buff_list *)prev)->next, newsk); WRITE_ONCE(list->qlen, list->qlen + 1); }
@@ -2003,7 +2005,7 @@ static inline void __skb_queue_after(struct sk_buff_head *list, struct sk_buff *prev, struct sk_buff *newsk) { - __skb_insert(newsk, prev, prev->next, list); + __skb_insert(newsk, prev, ((struct sk_buff_list *)prev)->next, list); }
void skb_append(struct sk_buff *old, struct sk_buff *newsk, @@ -2013,7 +2015,7 @@ static inline void __skb_queue_before(struct sk_buff_head *list, struct sk_buff *next, struct sk_buff *newsk) { - __skb_insert(newsk, next->prev, next, list); + __skb_insert(newsk, ((struct sk_buff_list *)next)->prev, next, list); }
/**
On Tue, Mar 29, 2022 at 03:02:56PM -0700, Tadeusz Struk wrote:
Please apply this to stable 5.10.y, and 5.15.y ---8<---
From: Kees Cook keescook@chromium.org
Upstream commit: 1a2fb220edca ("skbuff: Extract list pointers to silence compiler warnings")
Under both -Warray-bounds and the object_size sanitizer, the compiler is upset about accessing prev/next of sk_buff when the object it thinks it is coming from is sk_buff_head. The warning is a false positive due to the compiler taking a conservative approach, opting to warn at casting time rather than access time.
However, in support of enabling -Warray-bounds globally (which has found many real bugs), arrange things for sk_buff so that the compiler can unambiguously see that there is no intention to access anything except prev/next. Introduce and cast to a separate struct sk_buff_list, which contains _only_ the first two fields, silencing the warnings:
We don't have -Warray-bounds enabled on any stable kernel tree, so why is this needed?
Where is this showing up as a problem?
thanks,
greg k-h
On 3/30/22 07:46, Greg KH wrote:
On Tue, Mar 29, 2022 at 03:02:56PM -0700, Tadeusz Struk wrote:
Please apply this to stable 5.10.y, and 5.15.y ---8<---
From: Kees Cookkeescook@chromium.org
Upstream commit: 1a2fb220edca ("skbuff: Extract list pointers to silence compiler warnings")
Under both -Warray-bounds and the object_size sanitizer, the compiler is upset about accessing prev/next of sk_buff when the object it thinks it is coming from is sk_buff_head. The warning is a false positive due to the compiler taking a conservative approach, opting to warn at casting time rather than access time.
However, in support of enabling -Warray-bounds globally (which has found many real bugs), arrange things for sk_buff so that the compiler can unambiguously see that there is no intention to access anything except prev/next. Introduce and cast to a separate struct sk_buff_list, which contains_only_ the first two fields, silencing the warnings:
We don't have -Warray-bounds enabled on any stable kernel tree, so why is this needed?
Where is this showing up as a problem?
The issue shows up and hinders testing stable kernels in test automations like syzkaller:
https://syzkaller.appspot.com/text?tag=Error&x=12b3aac3700000
Applying it to stable would enable more test coverage.
On Wed, Mar 30, 2022 at 07:59:57AM -0700, Tadeusz Struk wrote:
On 3/30/22 07:46, Greg KH wrote:
On Tue, Mar 29, 2022 at 03:02:56PM -0700, Tadeusz Struk wrote:
Please apply this to stable 5.10.y, and 5.15.y ---8<---
From: Kees Cookkeescook@chromium.org
Upstream commit: 1a2fb220edca ("skbuff: Extract list pointers to silence compiler warnings")
Under both -Warray-bounds and the object_size sanitizer, the compiler is upset about accessing prev/next of sk_buff when the object it thinks it is coming from is sk_buff_head. The warning is a false positive due to the compiler taking a conservative approach, opting to warn at casting time rather than access time.
However, in support of enabling -Warray-bounds globally (which has found many real bugs), arrange things for sk_buff so that the compiler can unambiguously see that there is no intention to access anything except prev/next. Introduce and cast to a separate struct sk_buff_list, which contains_only_ the first two fields, silencing the warnings:
We don't have -Warray-bounds enabled on any stable kernel tree, so why is this needed?
Where is this showing up as a problem?
The issue shows up and hinders testing stable kernels in test automations like syzkaller:
https://syzkaller.appspot.com/text?tag=Error&x=12b3aac3700000
Applying it to stable would enable more test coverage.
Ok, again, that was not obvious, it seemed like you only needed this for build warnings.
thanks,
greg k-h
On 3/30/22 08:29, Greg KH wrote:
On Wed, Mar 30, 2022 at 07:59:57AM -0700, Tadeusz Struk wrote:
On 3/30/22 07:46, Greg KH wrote:
On Tue, Mar 29, 2022 at 03:02:56PM -0700, Tadeusz Struk wrote:
Please apply this to stable 5.10.y, and 5.15.y ---8<---
From: Kees Cookkeescook@chromium.org
Upstream commit: 1a2fb220edca ("skbuff: Extract list pointers to silence compiler warnings")
Under both -Warray-bounds and the object_size sanitizer, the compiler is upset about accessing prev/next of sk_buff when the object it thinks it is coming from is sk_buff_head. The warning is a false positive due to the compiler taking a conservative approach, opting to warn at casting time rather than access time.
However, in support of enabling -Warray-bounds globally (which has found many real bugs), arrange things for sk_buff so that the compiler can unambiguously see that there is no intention to access anything except prev/next. Introduce and cast to a separate struct sk_buff_list, which contains_only_ the first two fields, silencing the warnings:
We don't have -Warray-bounds enabled on any stable kernel tree, so why is this needed?
Where is this showing up as a problem?
The issue shows up and hinders testing stable kernels in test automations like syzkaller:
https://syzkaller.appspot.com/text?tag=Error&x=12b3aac3700000
Applying it to stable would enable more test coverage.
Ok, again, that was not obvious, it seemed like you only needed this for build warnings.
The original commit message was already long so I only added short statement about UBSAN. I was afraid that if I add more details nobody would ready it ;)
Thanks!
On Wed, Mar 30, 2022 at 07:59:57AM -0700, Tadeusz Struk wrote:
On 3/30/22 07:46, Greg KH wrote:
On Tue, Mar 29, 2022 at 03:02:56PM -0700, Tadeusz Struk wrote:
Please apply this to stable 5.10.y, and 5.15.y ---8<---
From: Kees Cookkeescook@chromium.org
Upstream commit: 1a2fb220edca ("skbuff: Extract list pointers to silence compiler warnings")
Under both -Warray-bounds and the object_size sanitizer, the compiler is upset about accessing prev/next of sk_buff when the object it thinks it is coming from is sk_buff_head. The warning is a false positive due to the compiler taking a conservative approach, opting to warn at casting time rather than access time.
However, in support of enabling -Warray-bounds globally (which has found many real bugs), arrange things for sk_buff so that the compiler can unambiguously see that there is no intention to access anything except prev/next. Introduce and cast to a separate struct sk_buff_list, which contains_only_ the first two fields, silencing the warnings:
We don't have -Warray-bounds enabled on any stable kernel tree, so why is this needed?
Where is this showing up as a problem?
The issue shows up and hinders testing stable kernels in test automations like syzkaller:
https://syzkaller.appspot.com/text?tag=Error&x=12b3aac3700000
Applying it to stable would enable more test coverage.
Hi! I think a better solution may be to backport this change instead:
69d0db01e210 ("ubsan: remove CONFIG_UBSAN_OBJECT_SIZE")
i.e. remove CONFIG_UBSAN_OBJECT_SIZE entirely, which is the cause of these syzkaller splats.
-Kees
On 3/30/22 14:46, Kees Cook wrote:
On Wed, Mar 30, 2022 at 07:59:57AM -0700, Tadeusz Struk wrote:
On 3/30/22 07:46, Greg KH wrote:
On Tue, Mar 29, 2022 at 03:02:56PM -0700, Tadeusz Struk wrote:
Please apply this to stable 5.10.y, and 5.15.y ---8<---
From: Kees Cookkeescook@chromium.org
Upstream commit: 1a2fb220edca ("skbuff: Extract list pointers to silence compiler warnings")
Under both -Warray-bounds and the object_size sanitizer, the compiler is upset about accessing prev/next of sk_buff when the object it thinks it is coming from is sk_buff_head. The warning is a false positive due to the compiler taking a conservative approach, opting to warn at casting time rather than access time.
However, in support of enabling -Warray-bounds globally (which has found many real bugs), arrange things for sk_buff so that the compiler can unambiguously see that there is no intention to access anything except prev/next. Introduce and cast to a separate struct sk_buff_list, which contains_only_ the first two fields, silencing the warnings:
We don't have -Warray-bounds enabled on any stable kernel tree, so why is this needed?
Where is this showing up as a problem?
The issue shows up and hinders testing stable kernels in test automations like syzkaller:
https://syzkaller.appspot.com/text?tag=Error&x=12b3aac3700000
Applying it to stable would enable more test coverage.
Hi! I think a better solution may be to backport this change instead:
69d0db01e210 ("ubsan: remove CONFIG_UBSAN_OBJECT_SIZE")
i.e. remove CONFIG_UBSAN_OBJECT_SIZE entirely, which is the cause of these syzkaller splats.
That works for me. I will test it and send a request or a backport soon.
On Tue, Mar 29, 2022 at 03:02:56PM -0700, Tadeusz Struk wrote:
Please apply this to stable 5.10.y, and 5.15.y ---8<---
From: Kees Cook keescook@chromium.org
Upstream commit: 1a2fb220edca ("skbuff: Extract list pointers to silence compiler warnings")
What about 5.16?
On 3/30/22 09:37, Greg KH wrote:
On Tue, Mar 29, 2022 at 03:02:56PM -0700, Tadeusz Struk wrote:
Please apply this to stable 5.10.y, and 5.15.y ---8<---
From: Kees Cook keescook@chromium.org
Upstream commit: 1a2fb220edca ("skbuff: Extract list pointers to silence compiler warnings")
What about 5.16?
The first one is already in 5.16. The second one applies cleanly, and the build looks ok.
On Tue, Mar 29, 2022 at 03:02:56PM -0700, Tadeusz Struk wrote:
Please apply this to stable 5.10.y, and 5.15.y ---8<---
From: Kees Cook keescook@chromium.org
Upstream commit: 1a2fb220edca ("skbuff: Extract list pointers to silence compiler warnings")
Under both -Warray-bounds and the object_size sanitizer, the compiler is upset about accessing prev/next of sk_buff when the object it thinks it is coming from is sk_buff_head. The warning is a false positive due to the compiler taking a conservative approach, opting to warn at casting time rather than access time.
However, in support of enabling -Warray-bounds globally (which has found many real bugs), arrange things for sk_buff so that the compiler can unambiguously see that there is no intention to access anything except prev/next. Introduce and cast to a separate struct sk_buff_list, which contains _only_ the first two fields, silencing the warnings:
In file included from ./include/net/net_namespace.h:39, from ./include/linux/netdevice.h:37, from net/core/netpoll.c:17: net/core/netpoll.c: In function 'refill_skbs': ./include/linux/skbuff.h:2086:9: warning: array subscript 'struct sk_buff[0]' is partly outside array bounds of 'struct sk_buff_head[1]' [-Warray-bounds] 2086 | __skb_insert(newsk, next->prev, next, list); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ net/core/netpoll.c:49:28: note: while referencing 'skb_pool' 49 | static struct sk_buff_head skb_pool; | ^~~~~~~~
This also upsets UBSAN, which throws a runtime object-size-mismatch error complaining about skbuff queue helpers, as below, when the kernel is built with clang and -fsanitize=undefined flag set:
UBSAN: object-size-mismatch in ./include/linux/skbuff.h:2023:28 member access within address ffffc90000cb71c0 with insufficient space for an object of type 'struct sk_buff'
This change results in no executable instruction differences.
Signed-off-by: Kees Cook keescook@chromium.org Link: https://lore.kernel.org/r/20211207062758.2324338-1-keescook@chromium.org Signed-off-by: Jakub Kicinski kuba@kernel.org Signed-off-by: Tadeusz Struk tadeusz.struk@linaro.org
include/linux/skbuff.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
Again, this diff is modified from the original.
I'm not going to take either of these now, sorry,
greg k-h
On 3/29/22 21:44, Greg KH wrote:
On Tue, Mar 29, 2022 at 03:02:55PM -0700, Tadeusz Struk wrote:
Please apply this to stable 5.10.y, 5.15.y ---8<---
Why? What problem does this infrastructure solve?
It is required to enable the PATCH 2/2 ("skbuff: Extract list pointers to silence compiler warnings.")
On Wed, Mar 30, 2022 at 07:38:12AM -0700, Tadeusz Struk wrote:
On 3/29/22 21:44, Greg KH wrote:
On Tue, Mar 29, 2022 at 03:02:55PM -0700, Tadeusz Struk wrote:
Please apply this to stable 5.10.y, 5.15.y ---8<---
Why? What problem does this infrastructure solve?
It is required to enable the PATCH 2/2 ("skbuff: Extract list pointers to silence compiler warnings.")
That wasn't obvious :(
On 3/30/22 07:45, Greg KH wrote:
On Wed, Mar 30, 2022 at 07:38:12AM -0700, Tadeusz Struk wrote:
On 3/29/22 21:44, Greg KH wrote:
On Tue, Mar 29, 2022 at 03:02:55PM -0700, Tadeusz Struk wrote:
Please apply this to stable 5.10.y, 5.15.y ---8<---
Why? What problem does this infrastructure solve?
It is required to enable the PATCH 2/2 ("skbuff: Extract list pointers to silence compiler warnings.")
That wasn't obvious :(
Yes, sorry, my bad.
On Tue, Mar 29, 2022 at 03:02:55PM -0700, Tadeusz Struk wrote:
Please apply this to stable 5.10.y, 5.15.y ---8<---
From: Kees Cook keescook@chromium.org
Upstream commit: 50d7bd38c3aa ("stddef: Introduce struct_group() helper macro")
Kernel code has a regular need to describe groups of members within a structure usually when they need to be copied or initialized separately from the rest of the surrounding structure. The generally accepted design pattern in C is to use a named sub-struct:
struct foo { int one; struct { int two; int three, four; } thing; int five; };
This would allow for traditional references and sizing:
memcpy(&dst.thing, &src.thing, sizeof(dst.thing));
However, doing this would mean that referencing struct members enclosed by such named structs would always require including the sub-struct name in identifiers:
do_something(dst.thing.three);
This has tended to be quite inflexible, especially when such groupings need to be added to established code which causes huge naming churn. Three workarounds exist in the kernel for this problem, and each have other negative properties.
To avoid the naming churn, there is a design pattern of adding macro aliases for the named struct:
#define f_three thing.three
This ends up polluting the global namespace, and makes it difficult to search for identifiers.
Another common work-around in kernel code avoids the pollution by avoiding the named struct entirely, instead identifying the group's boundaries using either a pair of empty anonymous structs of a pair of zero-element arrays:
struct foo { int one; struct { } start; int two; int three, four; struct { } finish; int five; }; struct foo { int one; int start[0]; int two; int three, four; int finish[0]; int five; };
This allows code to avoid needing to use a sub-struct named for member references within the surrounding structure, but loses the benefits of being able to actually use such a struct, making it rather fragile. Using these requires open-coded calculation of sizes and offsets. The efforts made to avoid common mistakes include lots of comments, or adding various BUILD_BUG_ON()s. Such code is left with no way for the compiler to reason about the boundaries (e.g. the "start" object looks like it's 0 bytes in length), making bounds checking depend on open-coded calculations:
if (length > offsetof(struct foo, finish) - offsetof(struct foo, start)) return -EINVAL; memcpy(&dst.start, &src.start, offsetof(struct foo, finish) - offsetof(struct foo, start));
However, the vast majority of places in the kernel that operate on groups of members do so without any identification of the grouping, relying either on comments or implicit knowledge of the struct contents, which is even harder for the compiler to reason about, and results in even more fragile manual sizing, usually depending on member locations outside of the region (e.g. to copy "two" and "three", use the start of "four" to find the size):
BUILD_BUG_ON((offsetof(struct foo, four) < offsetof(struct foo, two)) || (offsetof(struct foo, four) < offsetof(struct foo, three)); if (length > offsetof(struct foo, four) - offsetof(struct foo, two)) return -EINVAL; memcpy(&dst.two, &src.two, length);
In order to have a regular programmatic way to describe a struct region that can be used for references and sizing, can be examined for bounds checking, avoids forcing the use of intermediate identifiers, and avoids polluting the global namespace, introduce the struct_group() macro. This macro wraps the member declarations to create an anonymous union of an anonymous struct (no intermediate name) and a named struct (for references and sizing):
struct foo { int one; struct_group(thing, int two; int three, four; ); int five; }; if (length > sizeof(src.thing)) return -EINVAL; memcpy(&dst.thing, &src.thing, length); do_something(dst.three);
There are some rare cases where the resulting struct_group() needs attributes added, so struct_group_attr() is also introduced to allow for specifying struct attributes (e.g. __align(x) or __packed). Additionally, there are places where such declarations would like to have the struct be tagged, so struct_group_tagged() is added.
Given there is a need for a handful of UAPI uses too, the underlying __struct_group() macro has been defined in UAPI so it can be used there too.
To avoid confusing scripts/kernel-doc, hide the macro from its struct parsing.
Co-developed-by: Keith Packard keithp@keithp.com Signed-off-by: Keith Packard keithp@keithp.com Acked-by: Gustavo A. R. Silva gustavoars@kernel.org Link: https://lore.kernel.org/lkml/20210728023217.GC35706@embeddedor Enhanced-by: Rasmus Villemoes linux@rasmusvillemoes.dk Link: https://lore.kernel.org/lkml/41183a98-bdb9-4ad6-7eab-5a7292a6df84@rasmusvill... Enhanced-by: Dan Williams dan.j.williams@intel.com Link: https://lore.kernel.org/lkml/1d9a2e6df2a9a35b2cdd50a9a68cac5991e7e5f0.camel@... Enhanced-by: Daniel Vetter daniel.vetter@ffwll.ch Link: https://lore.kernel.org/lkml/YQKa76A6XuFqgM03@phenom.ffwll.local Acked-by: Dan Williams dan.j.williams@intel.com Signed-off-by: Kees Cook keescook@chromium.org Signed-off-by: Tadeusz Struk tadeusz.struk@linaro.org
include/linux/stddef.h | 48 +++++++++++++++++++++++++++++++++++++ include/uapi/linux/stddef.h | 24 +++++++++++++++++++ 2 files changed, 72 insertions(+)
Any specific reason this backport dropped a whole file from the original commit?
You can't send me modified patches without mentioning it, otherwise I assume you are doing something wrong :(
greg k-h
On 3/30/22 09:37, Greg KH wrote:
include/linux/stddef.h | 48 +++++++++++++++++++++++++++++++++++++ include/uapi/linux/stddef.h | 24 +++++++++++++++++++ 2 files changed, 72 insertions(+)
Any specific reason this backport dropped a whole file from the original commit?
You can't send me modified patches without mentioning it, otherwise I assume you are doing something wrong:(
I dropped the updates to scripts/kernel-doc. I didn't think it was relevant for stable. Here are the original stats compared to the backport stats:
orig: include/linux/stddef.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/stddef.h | 21 +++++++++++++++++++++ scripts/kernel-doc | 7 +++++++ 3 files changed, 76 insertions(+)
5.10 backport: include/linux/stddef.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/stddef.h | 24 ++++++++++++++++++++++++ 2 files changed, 72 insertions(+)
Do you want me to generate a new version with all three files?
linux-stable-mirror@lists.linaro.org