Message-ID: <CANn89iJJefUheeur5E=bziiqxjqmKXEk3NCO=8em4XVJThExMQ@mail.gmail.com>
Date: Tue, 30 Apr 2024 19:58:14 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Davide Caratti <dcaratti@...hat.com>
Cc: Jamal Hadi Salim <jhs@...atatu.com>, Cong Wang <xiyou.wangcong@...il.com>, 
	Jiri Pirko <jiri@...nulli.us>, "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Naresh Kamboju <naresh.kamboju@...aro.org>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net/sched: unregister lockdep keys in
 qdisc_create/qdisc_alloc error path

On Tue, Apr 30, 2024 at 7:11 PM Davide Caratti <dcaratti@...hat.com> wrote:
>
> Naresh and Eric report several errors (corrupted elements in the dynamic
> key hash list), when running tdc.py or syzbot. The error path of
> qdisc_alloc() and qdisc_create() frees the qdisc memory, but it forgets
> to unregister the lockdep key, thus causing use-after-free like the
> following one:
>
>  ==================================================================
>  BUG: KASAN: slab-use-after-free in lockdep_register_key+0x5f2/0x700
>  Read of size 8 at addr ffff88811236f2a8 by task ip/7925
>
>  CPU: 26 PID: 7925 Comm: ip Kdump: loaded Not tainted 6.9.0-rc2+ #648
>  Hardware name: Supermicro SYS-6027R-72RF/X9DRH-7TF/7F/iTF/iF, BIOS 3.0  07/26/2013
>  Call Trace:
>   <TASK>
>   dump_stack_lvl+0x7c/0xc0
>   print_report+0xc9/0x610
>   kasan_report+0x89/0xc0
>   lockdep_register_key+0x5f2/0x700
>   qdisc_alloc+0x21d/0xb60
>   qdisc_create_dflt+0x63/0x3c0
>   attach_one_default_qdisc.constprop.37+0x8e/0x170
>   dev_activate+0x4bd/0xc30
>   __dev_open+0x275/0x380
>   __dev_change_flags+0x3f1/0x570
>   dev_change_flags+0x7c/0x160
>   do_setlink+0x1ea1/0x34b0
>   __rtnl_newlink+0x8c9/0x1510
>   rtnl_newlink+0x61/0x90
>   rtnetlink_rcv_msg+0x2f0/0xbc0
>   netlink_rcv_skb+0x120/0x380
>   netlink_unicast+0x420/0x630
>   netlink_sendmsg+0x732/0xbc0
>   __sock_sendmsg+0x1ea/0x280
>   ____sys_sendmsg+0x5a9/0x990
>   ___sys_sendmsg+0xf1/0x180
>   __sys_sendmsg+0xd3/0x180
>   do_syscall_64+0x96/0x180
>   entry_SYSCALL_64_after_hwframe+0x71/0x79
>  RIP: 0033:0x7f9503f4fa07
>  Code: 0a 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b9 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 89 54 24 1c 48 89 74 24 10
>  RSP: 002b:00007fff6c729068 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
>  RAX: ffffffffffffffda RBX: 000000006630c681 RCX: 00007f9503f4fa07
>  RDX: 0000000000000000 RSI: 00007fff6c7290d0 RDI: 0000000000000003
>  RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000078
>  R10: 000000000000009b R11: 0000000000000246 R12: 0000000000000001
>  R13: 00007fff6c729180 R14: 0000000000000000 R15: 000055bf67dd9040
>   </TASK>
>
>  Allocated by task 7745:
>   kasan_save_stack+0x1c/0x40
>   kasan_save_track+0x10/0x30
>   __kasan_kmalloc+0x7b/0x90
>   __kmalloc_node+0x1ff/0x460
>   qdisc_alloc+0xae/0xb60
>   qdisc_create+0xdd/0xfb0
>   tc_modify_qdisc+0x37e/0x1960
>   rtnetlink_rcv_msg+0x2f0/0xbc0
>   netlink_rcv_skb+0x120/0x380
>   netlink_unicast+0x420/0x630
>   netlink_sendmsg+0x732/0xbc0
>   __sock_sendmsg+0x1ea/0x280
>   ____sys_sendmsg+0x5a9/0x990
>   ___sys_sendmsg+0xf1/0x180
>   __sys_sendmsg+0xd3/0x180
>   do_syscall_64+0x96/0x180
>   entry_SYSCALL_64_after_hwframe+0x71/0x79
>
>  Freed by task 7745:
>   kasan_save_stack+0x1c/0x40
>   kasan_save_track+0x10/0x30
>   kasan_save_free_info+0x36/0x60
>   __kasan_slab_free+0xfe/0x180
>   kfree+0x113/0x380
>   qdisc_create+0xafb/0xfb0
>   tc_modify_qdisc+0x37e/0x1960
>   rtnetlink_rcv_msg+0x2f0/0xbc0
>   netlink_rcv_skb+0x120/0x380
>   netlink_unicast+0x420/0x630
>   netlink_sendmsg+0x732/0xbc0
>   __sock_sendmsg+0x1ea/0x280
>   ____sys_sendmsg+0x5a9/0x990
>   ___sys_sendmsg+0xf1/0x180
>   __sys_sendmsg+0xd3/0x180
>   do_syscall_64+0x96/0x180
>   entry_SYSCALL_64_after_hwframe+0x71/0x79
>
> Fix this ensuring that lockdep_unregister_key() is called before the
> qdisc struct is freed, also in the error path of qdisc_create() and
> qdisc_alloc().
>
> Fixes: af0cb3fa3f9e ("net/sched: fix false lockdep warning on qdisc root lock")
> Reported-by: Linux Kernel Functional Testing <lkft@...aro.org>
> Closes: https://lore.kernel.org/netdev/20240429221706.1492418-1-naresh.kamboju@linaro.org/
> CC: Naresh Kamboju <naresh.kamboju@...aro.org>
> CC: Eric Dumazet <edumazet@...gle.com>
> Signed-off-by: Davide Caratti <dcaratti@...hat.com>
> ---
>  net/sched/sch_api.c     | 1 +
>  net/sched/sch_generic.c | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index 60239378d43f..6292d6d73b72 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -1389,6 +1389,7 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
>                 ops->destroy(sch);
>         qdisc_put_stab(rtnl_dereference(sch->stab));
>  err_out3:
> +       lockdep_unregister_key(&sch->root_lock_key);
>         netdev_put(dev, &sch->dev_tracker);
>         qdisc_free(sch);
>  err_out2:

For consistency with the other path, what about this instead ?

This would also  allow a qdisc goten from an rcu lookup to allow its
spinlock to be acquired.
(I am not saying this can happen, but who knows...)

Ie defer the  lockdep_unregister_key() right before the kfree()


diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 31dfd6c7405b01e22fe1b8c80944e2bed7d30ddc..edc9e4d240410d2f98dec04d3fac40983e808c28
100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -1042,6 +1042,7 @@ void qdisc_free(struct Qdisc *qdisc)
                free_percpu(qdisc->cpu_qstats);
        }

+       lockdep_unregister_key(&qdisc->root_lock_key);
        kfree(qdisc);
 }

@@ -1070,7 +1071,6 @@ static void __qdisc_destroy(struct Qdisc *qdisc)
        if (ops->destroy)
                ops->destroy(qdisc);

-       lockdep_unregister_key(&qdisc->root_lock_key);
        module_put(ops->owner);
        netdev_put(dev, &qdisc->dev_tracker);

Powered by blists - more mailing lists