| Message ID | 20211111003519.1050494-1-tadeusz.struk@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | skbuff: suppress clang object-size-mismatch error | expand |
On Thu, 11 Nov 2021 at 01:36, Tadeusz Struk <tadeusz.struk@linaro.org> wrote: > Kernel throws a runtime object-size-mismatch error in skbuff queue > helpers like in [1]. This happens every time there is a pattern > like the below: > > int skbuf_xmit(struct sk_buff *skb) > { > struct sk_buff_head list; > > __skb_queue_head_init(&list); > __skb_queue_tail(&list, skb); <-- offending call > > return do_xmit(net, &list); > } > > and the kernel is build with clang and -fsanitize=undefined flag set. > The reason is that the functions __skb_queue_[tail|head]() access the > struct sk_buff_head object via a pointer to struct sk_buff, which is > much bigger in size than the sk_buff_head. This could cause undefined > behavior and clang is complaining: > > 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' The config includes CONFIG_UBSAN_OBJECT_SIZE, right? Normally that's disabled by default, probably why nobody has noticed these much. > Suppress the error with __attribute__((no_sanitize("undefined"))) > in the skb helpers. Isn't there a better way, because doing this might also suppress other issues wholesale. __no_sanitize_undefined should be the last resort. > [1] https://syzkaller.appspot.com/bug?id=5d9f0bca58cea80f272b73500df67dcd9e35c886 > > Cc: "Nathan Chancellor" <nathan@kernel.org> > Cc: "Nick Desaulniers" <ndesaulniers@google.com> > Cc: "Jakub Kicinski" <kuba@kernel.org> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: "Jonathan Lemon" <jonathan.lemon@gmail.com> > Cc: "Alexander Lobakin" <alobakin@pm.me> > Cc: "Willem de Bruijn" <willemb@google.com> > Cc: "Paolo Abeni" <pabeni@redhat.com> > Cc: "Cong Wang" <cong.wang@bytedance.com> > Cc: "Kevin Hao" <haokexin@gmail.com> > Cc: "Ilias Apalodimas" <ilias.apalodimas@linaro.org> > Cc: "Marco Elver" <elver@google.com> > Cc: <netdev@vger.kernel.org> > Cc: <linux-kernel@vger.kernel.org> > Cc: <llvm@lists.linux.dev> > > Signed-off-by: Tadeusz Struk <tadeusz.struk@linaro.org> > --- > include/linux/skbuff.h | 49 ++++++++++++++++++++++++------------------ > 1 file changed, 28 insertions(+), 21 deletions(-) > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index 0bd6520329f6..8ec46e3a503d 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -1933,9 +1933,10 @@ static inline void skb_queue_head_init_class(struct sk_buff_head *list, > * The "__skb_xxxx()" functions are the non-atomic ones that > * can only be called with interrupts disabled. > */ > -static inline void __skb_insert(struct sk_buff *newsk, > - struct sk_buff *prev, struct sk_buff *next, > - struct sk_buff_head *list) > +static inline void __no_sanitize_undefined > +__skb_insert(struct sk_buff *newsk, > + struct sk_buff *prev, struct sk_buff *next, > + struct sk_buff_head *list) > { > /* See skb_queue_empty_lockless() and skb_peek_tail() > * for the opposite READ_ONCE() > @@ -1966,8 +1967,9 @@ static inline void __skb_queue_splice(const struct sk_buff_head *list, > * @list: the new list to add > * @head: the place to add it in the first list > */ > -static inline void skb_queue_splice(const struct sk_buff_head *list, > - struct sk_buff_head *head) > +static inline void __no_sanitize_undefined > +skb_queue_splice(const struct sk_buff_head *list, > + struct sk_buff_head *head) > { > if (!skb_queue_empty(list)) { > __skb_queue_splice(list, (struct sk_buff *) head, head->next); > @@ -1982,8 +1984,9 @@ static inline void skb_queue_splice(const struct sk_buff_head *list, > * > * The list at @list is reinitialised > */ > -static inline void skb_queue_splice_init(struct sk_buff_head *list, > - struct sk_buff_head *head) > +static inline void __no_sanitize_undefined > +skb_queue_splice_init(struct sk_buff_head *list, > + struct sk_buff_head *head) > { > if (!skb_queue_empty(list)) { > __skb_queue_splice(list, (struct sk_buff *) head, head->next); > @@ -1997,8 +2000,9 @@ static inline void skb_queue_splice_init(struct sk_buff_head *list, > * @list: the new list to add > * @head: the place to add it in the first list > */ > -static inline void skb_queue_splice_tail(const struct sk_buff_head *list, > - struct sk_buff_head *head) > +static inline void __no_sanitize_undefined > +skb_queue_splice_tail(const struct sk_buff_head *list, > + struct sk_buff_head *head) > { > if (!skb_queue_empty(list)) { > __skb_queue_splice(list, head->prev, (struct sk_buff *) head); > @@ -2014,8 +2018,9 @@ static inline void skb_queue_splice_tail(const struct sk_buff_head *list, > * Each of the lists is a queue. > * The list at @list is reinitialised > */ > -static inline void skb_queue_splice_tail_init(struct sk_buff_head *list, > - struct sk_buff_head *head) > +static inline void __no_sanitize_undefined > +skb_queue_splice_tail_init(struct sk_buff_head *list, > + struct sk_buff_head *head) > { > if (!skb_queue_empty(list)) { > __skb_queue_splice(list, head->prev, (struct sk_buff *) head); > @@ -2035,9 +2040,10 @@ static inline void skb_queue_splice_tail_init(struct sk_buff_head *list, > * > * A buffer cannot be placed on two lists at the same time. > */ > -static inline void __skb_queue_after(struct sk_buff_head *list, > - struct sk_buff *prev, > - struct sk_buff *newsk) > +static inline void __no_sanitize_undefined > +__skb_queue_after(struct sk_buff_head *list, > + struct sk_buff *prev, > + struct sk_buff *newsk) > { > __skb_insert(newsk, prev, prev->next, list); > } > @@ -2045,9 +2051,10 @@ static inline void __skb_queue_after(struct sk_buff_head *list, > void skb_append(struct sk_buff *old, struct sk_buff *newsk, > struct sk_buff_head *list); > > -static inline void __skb_queue_before(struct sk_buff_head *list, > - struct sk_buff *next, > - struct sk_buff *newsk) > +static inline void __no_sanitize_undefined > +__skb_queue_before(struct sk_buff_head *list, > + struct sk_buff *next, > + struct sk_buff *newsk) > { > __skb_insert(newsk, next->prev, next, list); > } > @@ -2062,8 +2069,8 @@ static inline void __skb_queue_before(struct sk_buff_head *list, > * > * A buffer cannot be placed on two lists at the same time. > */ > -static inline void __skb_queue_head(struct sk_buff_head *list, > - struct sk_buff *newsk) > +static inline void __no_sanitize_undefined > +__skb_queue_head(struct sk_buff_head *list, struct sk_buff *newsk) > { > __skb_queue_after(list, (struct sk_buff *)list, newsk); > } > @@ -2079,8 +2086,8 @@ void skb_queue_head(struct sk_buff_head *list, struct sk_buff *newsk); > * > * A buffer cannot be placed on two lists at the same time. > */ > -static inline void __skb_queue_tail(struct sk_buff_head *list, > - struct sk_buff *newsk) > +static inline void __no_sanitize_undefined > +__skb_queue_tail(struct sk_buff_head *list, struct sk_buff *newsk) > { > __skb_queue_before(list, (struct sk_buff *)list, newsk); > } > -- > 2.33.1 >
Hi Marco, On 11/11/21 01:51, Marco Elver wrote: > On Thu, 11 Nov 2021 at 01:36, Tadeusz Struk<tadeusz.struk@linaro.org> wrote: >> Kernel throws a runtime object-size-mismatch error in skbuff queue >> helpers like in [1]. This happens every time there is a pattern >> like the below: >> >> int skbuf_xmit(struct sk_buff *skb) >> { >> struct sk_buff_head list; >> >> __skb_queue_head_init(&list); >> __skb_queue_tail(&list, skb); <-- offending call >> >> return do_xmit(net, &list); >> } >> >> and the kernel is build with clang and -fsanitize=undefined flag set. >> The reason is that the functions __skb_queue_[tail|head]() access the >> struct sk_buff_head object via a pointer to struct sk_buff, which is >> much bigger in size than the sk_buff_head. This could cause undefined >> behavior and clang is complaining: >> >> 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' > The config includes CONFIG_UBSAN_OBJECT_SIZE, right? Normally that's > disabled by default, probably why nobody has noticed these much. Right, in all the defconfigs CONFIG_UBSAN_OBJECT_SIZE is not set. > >> Suppress the error with __attribute__((no_sanitize("undefined"))) >> in the skb helpers. > Isn't there a better way, because doing this might also suppress other > issues wholesale. __no_sanitize_undefined should be the last resort. > The other way to fix it would be to make the struct sk_buff_head equal in size with struct sk_buff: struct sk_buff_head { - /* These two members must be first. */ - struct sk_buff *next; - struct sk_buff *prev; + union { + struct { + /* These two members must be first. */ + struct sk_buff *next; + struct sk_buff *prev; - __u32 qlen; - spinlock_t lock; + __u32 qlen; + spinlock_t lock; + }; + struct sk_buff __prv; + }; }; but that's much more invasive, and I don't even have means to quantify this in terms of final binary size and performance impact. I think that would be a flat out no go. From the other hand if you look at the __skb_queue functions they don't do much and at all so there is no much room for other issues really. I followed the suggestion in [1]: "if your function deliberately contains possible ..., you can use __attribute__((no_sanitize... " [1] https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
On Thu, 11 Nov 2021 at 16:46, Tadeusz Struk <tadeusz.struk@linaro.org> wrote: > > Hi Marco, > On 11/11/21 01:51, Marco Elver wrote: > > On Thu, 11 Nov 2021 at 01:36, Tadeusz Struk<tadeusz.struk@linaro.org> wrote: > >> Kernel throws a runtime object-size-mismatch error in skbuff queue > >> helpers like in [1]. This happens every time there is a pattern > >> like the below: > >> > >> int skbuf_xmit(struct sk_buff *skb) > >> { > >> struct sk_buff_head list; > >> > >> __skb_queue_head_init(&list); > >> __skb_queue_tail(&list, skb); <-- offending call > >> > >> return do_xmit(net, &list); > >> } > >> > >> and the kernel is build with clang and -fsanitize=undefined flag set. > >> The reason is that the functions __skb_queue_[tail|head]() access the > >> struct sk_buff_head object via a pointer to struct sk_buff, which is > >> much bigger in size than the sk_buff_head. This could cause undefined > >> behavior and clang is complaining: > >> > >> 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' > > The config includes CONFIG_UBSAN_OBJECT_SIZE, right? Normally that's > > disabled by default, probably why nobody has noticed these much. > > Right, in all the defconfigs CONFIG_UBSAN_OBJECT_SIZE is not set. > > > > >> Suppress the error with __attribute__((no_sanitize("undefined"))) > >> in the skb helpers. > > Isn't there a better way, because doing this might also suppress other > > issues wholesale. __no_sanitize_undefined should be the last resort. > > > > The other way to fix it would be to make the struct sk_buff_head > equal in size with struct sk_buff: > > struct sk_buff_head { > - /* These two members must be first. */ > - struct sk_buff *next; > - struct sk_buff *prev; > + union { > + struct { > + /* These two members must be first. */ > + struct sk_buff *next; > + struct sk_buff *prev; > > - __u32 qlen; > - spinlock_t lock; > + __u32 qlen; > + spinlock_t lock; > + }; > + struct sk_buff __prv; > + }; > }; > > but that's much more invasive, and I don't even have means to > quantify this in terms of final binary size and performance > impact. I think that would be a flat out no go. > > From the other hand if you look at the __skb_queue functions > they don't do much and at all so there is no much room for > other issues really. I followed the suggestion in [1]: > > "if your function deliberately contains possible ..., you can > use __attribute__((no_sanitize... " That general advice might not be compatible with what the kernel wants, especially since UBSAN_OBJECT_SIZE is normally disabled and I think known to cause these issues in the kernel. I'll defer to maintainers to decide what would be the preferred way of handling this.
On 11/11/21 07:52, Marco Elver wrote: >> The other way to fix it would be to make the struct sk_buff_head >> equal in size with struct sk_buff: >> >> struct sk_buff_head { >> - /* These two members must be first. */ >> - struct sk_buff *next; >> - struct sk_buff *prev; >> + union { >> + struct { >> + /* These two members must be first. */ >> + struct sk_buff *next; >> + struct sk_buff *prev; >> >> - __u32 qlen; >> - spinlock_t lock; >> + __u32 qlen; >> + spinlock_t lock; >> + }; >> + struct sk_buff __prv; >> + }; >> }; >> >> but that's much more invasive, and I don't even have means to >> quantify this in terms of final binary size and performance >> impact. I think that would be a flat out no go. >> >> From the other hand if you look at the __skb_queue functions >> they don't do much and at all so there is no much room for >> other issues really. I followed the suggestion in [1]: >> >> "if your function deliberately contains possible ..., you can >> use __attribute__((no_sanitize... " > That general advice might not be compatible with what the kernel > wants, especially since UBSAN_OBJECT_SIZE is normally disabled and I > think known to cause these issues in the kernel. > > I'll defer to maintainers to decide what would be the preferred way of > handling this. Sure, I would also like to know if there is a better way of fixing this. Thanks for your feedback.
On Thu, 11 Nov 2021 08:01:26 -0800 Tadeusz Struk wrote: > > That general advice might not be compatible with what the kernel > > wants, especially since UBSAN_OBJECT_SIZE is normally disabled and I > > think known to cause these issues in the kernel. > > > > I'll defer to maintainers to decide what would be the preferred way of > > handling this. > > Sure, I would also like to know if there is a better way of fixing this. > Thanks for your feedback. I remember Dave was working thru the tree at some point to clean up all skb->next/skb->prev accesses so that we can switch over to using normal list helpers. I'm not sure if that stalled due to lack of time or some fundamental problems. Seems like finishing that would let us clean up such misuses?
From: Jakub Kicinski <kuba@kernel.org> Date: Thu, 11 Nov 2021 09:54:44 -0800 > I'm not sure if that stalled due to lack of time or some fundamental > problems. ran out of time, then had a stroke... > Seems like finishing that would let us clean up such misuses? yes it would
On 11/12/21 07:42, David Miller wrote: > From: Jakub Kicinski <kuba@kernel.org> > Date: Thu, 11 Nov 2021 09:54:44 -0800 > >> I'm not sure if that stalled due to lack of time or some fundamental >> problems. > > ran out of time, then had a stroke... > >> Seems like finishing that would let us clean up such misuses? > > yes it would > so since there is not better way of suppressing the issue atm are you ok with taking this fix for now?
On Thu, 18 Nov 2021 08:05:01 -0800 Tadeusz Struk wrote: > On 11/12/21 07:42, David Miller wrote: > > From: Jakub Kicinski <kuba@kernel.org> > > Date: Thu, 11 Nov 2021 09:54:44 -0800 > > > >> I'm not sure if that stalled due to lack of time or some fundamental > >> problems. > > > > ran out of time, then had a stroke... > > > >> Seems like finishing that would let us clean up such misuses? > > > > yes it would > > so since there is not better way of suppressing the issue atm are > you ok with taking this fix for now? I vote no on sprinkling ugly tags around to silence some random checkers warning. We already have too many of them. They are meaningless and confusing to people reading the code. This is not a fundamental problem, the solution is clear.
On 11/18/21 08:38, Jakub Kicinski wrote: > On Thu, 18 Nov 2021 08:05:01 -0800 Tadeusz Struk wrote: >> On 11/12/21 07:42, David Miller wrote: >>> From: Jakub Kicinski <kuba@kernel.org> >>> Date: Thu, 11 Nov 2021 09:54:44 -0800 >>> >>>> I'm not sure if that stalled due to lack of time or some fundamental >>>> problems. >>> >>> ran out of time, then had a stroke... >>> >>>> Seems like finishing that would let us clean up such misuses? >>> >>> yes it would >> >> so since there is not better way of suppressing the issue atm are >> you ok with taking this fix for now? > > I vote no on sprinkling ugly tags around to silence some random > checkers warning. We already have too many of them. They are > meaningless and confusing to people reading the code. > > This is not a fundamental problem, the solution is clear. > Fair enough. David, did you post your work somewhere if someone would like to pick it up and finish it?
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 0bd6520329f6..8ec46e3a503d 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -1933,9 +1933,10 @@ static inline void skb_queue_head_init_class(struct sk_buff_head *list, * The "__skb_xxxx()" functions are the non-atomic ones that * can only be called with interrupts disabled. */ -static inline void __skb_insert(struct sk_buff *newsk, - struct sk_buff *prev, struct sk_buff *next, - struct sk_buff_head *list) +static inline void __no_sanitize_undefined +__skb_insert(struct sk_buff *newsk, + struct sk_buff *prev, struct sk_buff *next, + struct sk_buff_head *list) { /* See skb_queue_empty_lockless() and skb_peek_tail() * for the opposite READ_ONCE() @@ -1966,8 +1967,9 @@ static inline void __skb_queue_splice(const struct sk_buff_head *list, * @list: the new list to add * @head: the place to add it in the first list */ -static inline void skb_queue_splice(const struct sk_buff_head *list, - struct sk_buff_head *head) +static inline void __no_sanitize_undefined +skb_queue_splice(const struct sk_buff_head *list, + struct sk_buff_head *head) { if (!skb_queue_empty(list)) { __skb_queue_splice(list, (struct sk_buff *) head, head->next); @@ -1982,8 +1984,9 @@ static inline void skb_queue_splice(const struct sk_buff_head *list, * * The list at @list is reinitialised */ -static inline void skb_queue_splice_init(struct sk_buff_head *list, - struct sk_buff_head *head) +static inline void __no_sanitize_undefined +skb_queue_splice_init(struct sk_buff_head *list, + struct sk_buff_head *head) { if (!skb_queue_empty(list)) { __skb_queue_splice(list, (struct sk_buff *) head, head->next); @@ -1997,8 +2000,9 @@ static inline void skb_queue_splice_init(struct sk_buff_head *list, * @list: the new list to add * @head: the place to add it in the first list */ -static inline void skb_queue_splice_tail(const struct sk_buff_head *list, - struct sk_buff_head *head) +static inline void __no_sanitize_undefined +skb_queue_splice_tail(const struct sk_buff_head *list, + struct sk_buff_head *head) { if (!skb_queue_empty(list)) { __skb_queue_splice(list, head->prev, (struct sk_buff *) head); @@ -2014,8 +2018,9 @@ static inline void skb_queue_splice_tail(const struct sk_buff_head *list, * Each of the lists is a queue. * The list at @list is reinitialised */ -static inline void skb_queue_splice_tail_init(struct sk_buff_head *list, - struct sk_buff_head *head) +static inline void __no_sanitize_undefined +skb_queue_splice_tail_init(struct sk_buff_head *list, + struct sk_buff_head *head) { if (!skb_queue_empty(list)) { __skb_queue_splice(list, head->prev, (struct sk_buff *) head); @@ -2035,9 +2040,10 @@ static inline void skb_queue_splice_tail_init(struct sk_buff_head *list, * * A buffer cannot be placed on two lists at the same time. */ -static inline void __skb_queue_after(struct sk_buff_head *list, - struct sk_buff *prev, - struct sk_buff *newsk) +static inline void __no_sanitize_undefined +__skb_queue_after(struct sk_buff_head *list, + struct sk_buff *prev, + struct sk_buff *newsk) { __skb_insert(newsk, prev, prev->next, list); } @@ -2045,9 +2051,10 @@ static inline void __skb_queue_after(struct sk_buff_head *list, void skb_append(struct sk_buff *old, struct sk_buff *newsk, struct sk_buff_head *list); -static inline void __skb_queue_before(struct sk_buff_head *list, - struct sk_buff *next, - struct sk_buff *newsk) +static inline void __no_sanitize_undefined +__skb_queue_before(struct sk_buff_head *list, + struct sk_buff *next, + struct sk_buff *newsk) { __skb_insert(newsk, next->prev, next, list); } @@ -2062,8 +2069,8 @@ static inline void __skb_queue_before(struct sk_buff_head *list, * * A buffer cannot be placed on two lists at the same time. */ -static inline void __skb_queue_head(struct sk_buff_head *list, - struct sk_buff *newsk) +static inline void __no_sanitize_undefined +__skb_queue_head(struct sk_buff_head *list, struct sk_buff *newsk) { __skb_queue_after(list, (struct sk_buff *)list, newsk); } @@ -2079,8 +2086,8 @@ void skb_queue_head(struct sk_buff_head *list, struct sk_buff *newsk); * * A buffer cannot be placed on two lists at the same time. */ -static inline void __skb_queue_tail(struct sk_buff_head *list, - struct sk_buff *newsk) +static inline void __no_sanitize_undefined +__skb_queue_tail(struct sk_buff_head *list, struct sk_buff *newsk) { __skb_queue_before(list, (struct sk_buff *)list, newsk); }
Kernel throws a runtime object-size-mismatch error in skbuff queue helpers like in [1]. This happens every time there is a pattern like the below: int skbuf_xmit(struct sk_buff *skb) { struct sk_buff_head list; __skb_queue_head_init(&list); __skb_queue_tail(&list, skb); <-- offending call return do_xmit(net, &list); } and the kernel is build with clang and -fsanitize=undefined flag set. The reason is that the functions __skb_queue_[tail|head]() access the struct sk_buff_head object via a pointer to struct sk_buff, which is much bigger in size than the sk_buff_head. This could cause undefined behavior and clang is complaining: 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' Suppress the error with __attribute__((no_sanitize("undefined"))) in the skb helpers. [1] https://syzkaller.appspot.com/bug?id=5d9f0bca58cea80f272b73500df67dcd9e35c886 Cc: "Nathan Chancellor" <nathan@kernel.org> Cc: "Nick Desaulniers" <ndesaulniers@google.com> Cc: "Jakub Kicinski" <kuba@kernel.org> Cc: "David S. Miller" <davem@davemloft.net> Cc: "Jonathan Lemon" <jonathan.lemon@gmail.com> Cc: "Alexander Lobakin" <alobakin@pm.me> Cc: "Willem de Bruijn" <willemb@google.com> Cc: "Paolo Abeni" <pabeni@redhat.com> Cc: "Cong Wang" <cong.wang@bytedance.com> Cc: "Kevin Hao" <haokexin@gmail.com> Cc: "Ilias Apalodimas" <ilias.apalodimas@linaro.org> Cc: "Marco Elver" <elver@google.com> Cc: <netdev@vger.kernel.org> Cc: <linux-kernel@vger.kernel.org> Cc: <llvm@lists.linux.dev> Signed-off-by: Tadeusz Struk <tadeusz.struk@linaro.org> --- include/linux/skbuff.h | 49 ++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 21 deletions(-)