Messages in this thread | | | From | Marco Elver <> | Date | Thu, 11 Nov 2021 16:52:46 +0100 | Subject | Re: [PATCH] skbuff: suppress clang object-size-mismatch error |
| |
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.
| |