netdev - Re: [PATCH net-next V2 2/2] net: kfree_skb_list use kmem_cache_free

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 19 Jan 2023 12:46:26 +0100
From:   Eric Dumazet <edumazet@...gle.com>
To:     Jesper Dangaard Brouer <jbrouer@...hat.com>
Cc:     brouer@...hat.com, Jakub Kicinski <kuba@...nel.org>,
        netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        pabeni@...hat.com
Subject: Re: [PATCH net-next V2 2/2] net: kfree_skb_list use kmem_cache_free_bulk

On Thu, Jan 19, 2023 at 12:22 PM Jesper Dangaard Brouer
<jbrouer@...hat.com> wrote:
>
>
>
> On 19/01/2023 11.28, Eric Dumazet wrote:
> > On Thu, Jan 19, 2023 at 11:18 AM Jesper Dangaard Brouer
> > <jbrouer@...hat.com> wrote:
> >>
> >>
> >>
> >> On 19/01/2023 03.26, Jakub Kicinski wrote:
> >>> On Wed, 18 Jan 2023 22:37:47 +0100 Jesper Dangaard Brouer wrote:
> >>>>> +           skb_mark_not_on_list(segs);
> >>>>
> >>>> The syzbot[1] bug goes way if I remove this skb_mark_not_on_list().
> >>>>
> >>>> I don't understand why I cannot clear skb->next here?
> >>>
> >>> Some of the skbs on the list are not private?
> >>> IOW we should only unlink them if skb_unref().
> >>
> >> Yes, you are right.
> >>
> >> The skb_mark_not_on_list() should only be called if __kfree_skb_reason()
> >> returns true, meaning the SKB is ready to be free'ed (as it calls/check
> >> skb_unref()).
> >
> >
> > This was the case already before your changes.
> >
> > skb->next/prev can not be shared by multiple users.
> >
> > One skb can be put on a single list by definition.
> >
> > Whoever calls kfree_skb_list(list) owns all the skbs->next|prev found
> > in the list
> >
> > So you can mangle skb->next as you like, even if the unref() is
> > telling that someone
> > else has a reference on skb.
>
> Then why does the bug go way if I remove the skb_mark_not_on_list() call
> then?
>

Some side effects.

This _particular_ repro uses a specific pattern that might be defeated
by a small change.
(just working around another bug)

Instead of setting skb->next to NULL, try to set it to

skb->next = (struct sk_buff *)0x800;

This might show a different pattern.

> >>
> >> I will send a proper fix patch shortly... after syzbot do a test on it.
> >>
>
> I've send a patch for syzbot that only calls skb_mark_not_on_list() when
> unref() and __kfree_skb_reason() "permits" this.
> I tested it locally with reproducer and it also fixes/"removes" the bug.

This does not mean we will accept a patch with no clear explanation
other than "this removes a syzbot bug, so this must be good"

Make sure to give precise details on _why_ this is needed or not.

Again, the user of kfree_skb_list(list) _owns_ skb->next for sure.
If you think this assertion is not true, we are in big trouble.