Re: [PATCH net-next] net/sched: unregister lockdep keys in qdisc_create/qdisc_alloc error path

[Davide Caratti <dcaratti@redhat.com>]

hello Eric,

On Tue, Apr 30, 2024 at 08:43:22PM +0200, Eric Dumazet wrote:
> On Tue, Apr 30, 2024 at 8:35 PM Davide Caratti <dcaratti@redhat.com> wrote:
> >

[...]

> > > 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()
> >
> > the problem is, qdisc_free() is called also in a RCU callback. So, if we move
> > lockdep_unregister_key() inside the function, the non-error path is
> > going to splat like this
> 
> Got it, but we do have ways of running a work queue after rcu grace period.

this would imply scheduling a work that does qdisc_free() + lockdep_unregister_key()
in qdisc_free_cb(). I can try that, but maybe the issue is different:

> Let's use your patch, but I suspect we could have other issues.
> 
> Full disclosure, I have the following syzbot report:
> 
> WARNING: bad unlock balance detected!
> 6.9.0-rc5-syzkaller-01413-gdd1941f801bc #0 Not tainted
> -------------------------------------
> kworker/u8:6/2474 is trying to release lock (&sch->root_lock_key) at:
> [<ffffffff897300c5>] spin_unlock_bh include/linux/spinlock.h:396 [inline]
> [<ffffffff897300c5>] dev_reset_queue+0x145/0x1b0 net/sched/sch_generic.c:1304
> but there are no more locks to release!

I don't understand how can this "imbalance" be caused by lockdep_unregister_key()
being called too early. I'm more inclined to think that this splat is due to UaF
similar to those that we saw a couples of days ago. Is syzbot still
generating report like the one above?

thanks,

-- 
davide