Re: HARDENED_USERCOPY will BUG on multiple slub objects coalesced into an sk_buff fragment

[Anton Eidelman <anton@lightbitslabs.com>]

[-- Attachment #1: Type: text/plain, Size: 3343 bytes --]

Hi Christopher,

This eliminates the failure as expected (at the source).
I do not think such a solution is required, and it probably affect
performance.

As Matthew said, slab objects should not be used in sk_buff fragments.
The source of these is my kernel TCP sockets, where kernel_sendpage() is
used with slab payload.
I eliminated this, and the failure disappeared, even though with this kind
of fine timing issues, no failure does not mean anything
Moreover, I tried triggering on slab in sk_buff fragments and nothing came
up.

So far:
1) Use of slab payload in kernel_sendpage() is not polite, even though we
do not BUG on this and documentation does not tell it was just wrong.
2) RX path cannot bring sk_buffs in slab: drivers use alloc_pagexxx or
page_frag_alloc().

What I am still wondering about (and investigating), is how kernel_sendpage()
with slab payload results in slab payload on another socket RX.
Do you see how page ref-counting can be broken with extra references taken
on a slab page containing the fragments, and dropped when networking is
done with them?

Thanks,
Anton



On Tue, Jun 5, 2018 at 8:27 AM, Christopher Lameter <cl@linux.com> wrote:

> On Fri, 1 Jun 2018, Anton Eidelman wrote:
>
> > I do not have a way of reproducing this decent enough to recommend: I'll
> > keep digging.
>
> If you can reproduce it: Could you try the following patch?
>
>
>
> Subject: [NET] Fix false positives of skb_can_coalesce
>
> Skb fragments may be slab objects. Two slab objects may reside
> in the same slab page. In that case skb_can_coalesce() may return
> true althought the skb cannot be expanded because it would
> cross a slab boundary.
>
> Enabling slab debugging will avoid the issue since red zones will
> be inserted and thus the skb_can_coalesce() check will not detect
> neighboring objects and return false.
>
> Signed-off-by: Christoph Lameter <cl@linux.com>
>
> Index: linux/include/linux/skbuff.h
> ===================================================================
> --- linux.orig/include/linux/skbuff.h
> +++ linux/include/linux/skbuff.h
> @@ -3010,8 +3010,29 @@ static inline bool skb_can_coalesce(stru
>         if (i) {
>                 const struct skb_frag_struct *frag =
> &skb_shinfo(skb)->frags[i - 1];
>
> -               return page == skb_frag_page(frag) &&
> -                      off == frag->page_offset + skb_frag_size(frag);
> +               if (page != skb_frag_page(frag))
> +                       return false;
> +
> +               if (off != frag->page_offset + skb_frag_size(frag))
> +                       return false;
> +
> +               /*
> +                * This may be a slab page and we may have pointers
> +                * to different slab objects in the same page
> +                */
> +               if (!PageSlab(skb_frag_page(frag)))
> +                       return true;
> +
> +               /*
> +                * We could still return true if we would check here
> +                * if the two fragments are within the same
> +                * slab object. But that is complicated and
> +                * I guess we would need a new slab function
> +                * to check if two pointers are within the same
> +                * object.
> +                */
> +               return false;
> +
>         }
>         return false;
>  }
>

[-- Attachment #2: Type: text/html, Size: 5611 bytes --]