Messages in this thread | | | From | Dmitry Vyukov <> | Date | Mon, 9 Sep 2019 07:45:53 +0100 | Subject | Re: general protection fault in qdisc_put |
| |
On Sun, Sep 8, 2019 at 6:19 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Sat, Sep 7, 2019 at 11:08 PM syzbot > <syzbot+d5870a903591faaca4ae@syzkaller.appspotmail.com> wrote: > > > > The bug was bisected to: > > > > commit e41d58185f1444368873d4d7422f7664a68be61d > > Author: Dmitry Vyukov <dvyukov@google.com> > > Date: Wed Jul 12 21:34:35 2017 +0000 > > > > fault-inject: support systematic fault injection > > That commit does seem a bit questionable, but not the cause of this > problem (just the trigger). > > I think the questionable part is that the new code doesn't honor the > task filtering, and will fail even for protected tasks. Dmitry?
That commit added a new fault injection mode with a new API that is used by syzkaller to inject faults. Before that commit the fault inject is not working for syzkaller at all. I think this bisection result simply means "the GPF is related to an earlier failure".
> > kasan: GPF could be caused by NULL-ptr deref or user memory access > > general protection fault: 0000 [#1] PREEMPT SMP KASAN > > CPU: 1 PID: 9699 Comm: syz-executor169 Not tainted 5.3.0-rc7+ #0 > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS > > Google 01/01/2011 > > RIP: 0010:qdisc_put+0x25/0x90 net/sched/sch_generic.c:983 > > Yes, looks like 'qdisc' is NULL. > > This is the > > qdisc_put(q->qdisc); > > in sfb_destroy(), called from qdisc_create(). > > I think what is happening is this (in qdisc_create()): > > if (ops->init) { > err = ops->init(sch, tca[TCA_OPTIONS], extack); > if (err != 0) > goto err_out5; > } > ... > err_out5: > /* ops->init() failed, we call ->destroy() like qdisc_create_dflt() */ > if (ops->destroy) > ops->destroy(sch); > > and "ops->init" is sfb_init(), which will not initialize q->qdisc if > tcf_block_get() fails. > > I see two solutions: > > (a) move the > > q->qdisc = &noop_qdisc; > > up earlier in sfb_init(), so that qdisc is always initialized > after sfb_init(), even on failure. > > (b) just make qdisc_put(NULL) just silently work as a no-op. > > (c) change all the semantics to not call ->destroy if ->init failed. > > Honestly, (a) seems very fragile - do all the other init routines do > this? And (c) sounds like a big change, and very fragile too. > > So I'd suggest that qdisc_put() be made to just ignore a NULL pointer > (and maybe an error pointer too?). > > But I'll leave it to the maintainers to sort out the proper fix. > Maybe people prefer (a)? > > Linus
| |