netdev - Re: [PATCH net-next] net: dst: fix missing initialization of rt

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]

Message-ID: <CANn89i+qdrgifdqBTc2sZMtHG66B6qojzPRLgWcy-97gDiie+A@mail.gmail.com>
Date:   Wed, 19 Apr 2023 11:36:31 +0200
From:   Eric Dumazet <edumazet@...gle.com>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     Maxime Bizon <mbizon@...ebox.fr>, davem@...emloft.net,
        tglx@...utronix.de, wangyang.guo@...el.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: dst: fix missing initialization of rt_uncached

On Wed, Apr 19, 2023 at 11:03 AM Leon Romanovsky <leon@...nel.org> wrote:
>
> On Wed, Apr 19, 2023 at 11:58:02AM +0300, Leon Romanovsky wrote:
> > On Tue, Apr 18, 2023 at 06:54:26PM +0200, Maxime Bizon wrote:
> > > xfrm_alloc_dst() followed by xfrm4_dst_destroy(), without a
> > > xfrm4_fill_dst() call in between, causes the following BUG:
> > >
> > >  BUG: spinlock bad magic on CPU#0, fbxhostapd/732
> > >   lock: 0x890b7668, .magic: 890b7668, .owner: <none>/-1, .owner_cpu: 0
> > >  CPU: 0 PID: 732 Comm: fbxhostapd Not tainted 6.3.0-rc6-next-20230414-00613-ge8de66369925-dirty #9
> > >  Hardware name: Marvell Kirkwood (Flattened Device Tree)
> > >   unwind_backtrace from show_stack+0x10/0x14
> > >   show_stack from dump_stack_lvl+0x28/0x30
> > >   dump_stack_lvl from do_raw_spin_lock+0x20/0x80
> > >   do_raw_spin_lock from rt_del_uncached_list+0x30/0x64
> > >   rt_del_uncached_list from xfrm4_dst_destroy+0x3c/0xbc
> > >   xfrm4_dst_destroy from dst_destroy+0x5c/0xb0
> > >   dst_destroy from rcu_process_callbacks+0xc4/0xec
> > >   rcu_process_callbacks from __do_softirq+0xb4/0x22c
> > >   __do_softirq from call_with_stack+0x1c/0x24
> > >   call_with_stack from do_softirq+0x60/0x6c
> > >   do_softirq from __local_bh_enable_ip+0xa0/0xcc
> > >
> > > Patch "net: dst: Prevent false sharing vs. dst_entry:: __refcnt" moved
> > > rt_uncached and rt_uncached_list fields from rtable struct to dst
> > > struct, so they are more zeroed by memset_after(xdst, 0, u.dst) in
> > > xfrm_alloc_dst().
> > >
> > > Note that rt_uncached (list_head) was never properly initialized at
> > > alloc time, but xfrm[46]_dst_destroy() is written in such a way that
> > > it was not an issue thanks to the memset:
> > >
> > >     if (xdst->u.rt.dst.rt_uncached_list)
> > >             rt_del_uncached_list(&xdst->u.rt);
> > >
> > > The route code does it the other way around: rt_uncached_list is
> > > assumed to be valid IIF rt_uncached list_head is not empty:
> > >
> > > void rt_del_uncached_list(struct rtable *rt)
> > > {
> > >         if (!list_empty(&rt->dst.rt_uncached)) {
> > >                 struct uncached_list *ul = rt->dst.rt_uncached_list;
> > >
> > >                 spin_lock_bh(&ul->lock);
> > >                 list_del_init(&rt->dst.rt_uncached);
> > >                 spin_unlock_bh(&ul->lock);
> > >         }
> > > }
> > >
> > > This patch adds mandatory rt_uncached list_head initialization in
> > > generic dst_init(), and adapt xfrm[46]_dst_destroy logic to match the
> > > rest of the code.
> > >
> > > Fixes: d288a162dd1c ("net: dst: Prevent false sharing vs. dst_entry:: __refcnt")
> > > Signed-off-by: Maxime Bizon <mbizon@...ebox.fr>
> > > ---
> > >  net/core/dst.c          | 1 +
> > >  net/ipv4/xfrm4_policy.c | 4 +---
> > >  net/ipv6/route.c        | 1 -
> > >  net/ipv6/xfrm6_policy.c | 4 +---
> > >  4 files changed, 3 insertions(+), 7 deletions(-)
> >
> > It should go to net. Right now -rc7 is broken.
> >
> > Also the change is not complete, you need to delete INIT_LIST_HEAD(..rt_uncached)
> > from rt_dst_alloc and rt_dst_clone too.
>
> It will be nice to give a credit to kbuild.
> https://lore.kernel.org/all/202304162125.18b7bcdd-oliver.sang@intel.com
>

It seems Maxime found the issue before kbuild.

But feel free to add additional tags, sure.