On Tue, Aug 29, 2017 at 10:58 AM, Nikolay Aleksandrov
<niko...@cumulusnetworks.com> wrote:
> The commit below added a call to the ->destroy() callback for all qdiscs
> which failed in their ->init(), but some were not prepared for such
> change and can't handle partially initialized qdisc. HTB is one of them
> and if any error occurs before the qdisc watchdog timer and qdisc work are
> initialized then we can hit either a null ptr deref (timer->base) when
> canceling in ->destroy or lockdep error info about trying to register
> a non-static key and a stack dump. So to fix these two move the watchdog
> timer and workqueue init before anything that can err out.
> To reproduce userspace needs to send broken htb qdisc create request,
> tested with a modified tc (q_htb.c).
>
> Trace log:
> [ 2710.897602] BUG: unable to handle kernel NULL pointer dereference at (null)
> [ 2710.897977] IP: hrtimer_active+0x17/0x8a
> [ 2710.898174] PGD 58fab067
> [ 2710.898175] P4D 58fab067
> [ 2710.898353] PUD 586c0067
> [ 2710.898531] PMD 0
> [ 2710.898710]
> [ 2710.899045] Oops: 0000 [#1] SMP
> [ 2710.899232] Modules linked in:
> [ 2710.899419] CPU: 1 PID: 950 Comm: tc Not tainted 4.13.0-rc6+ #54
> [ 2710.899646] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> 1.7.5-20140531_083030-gandalf 04/01/2014
> [ 2710.900035] task: ffff880059ed2700 task.stack: ffff88005ad4c000
> [ 2710.900262] RIP: 0010:hrtimer_active+0x17/0x8a
> [ 2710.900467] RSP: 0018:ffff88005ad4f960 EFLAGS: 00010246
> [ 2710.900684] RAX: 0000000000000000 RBX: ffff88003701e298 RCX:
> 0000000000000000
> [ 2710.900933] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
> ffff88003701e298
> [ 2710.901177] RBP: ffff88005ad4f980 R08: 0000000000000001 R09:
> 0000000000000001
> [ 2710.901419] R10: ffff88005ad4f800 R11: 0000000000000400 R12:
> 0000000000000000
> [ 2710.901663] R13: ffff88003701e298 R14: ffffffff822a4540 R15:
> ffff88005ad4fac0
> [ 2710.901907] FS: 00007f2f5e90f740(0000) GS:ffff88005d880000(0000)
> knlGS:0000000000000000
> [ 2710.902277] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 2710.902500] CR2: 0000000000000000 CR3: 0000000058ca3000 CR4:
> 00000000000406e0
> [ 2710.902744] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> 0000000000000000
> [ 2710.902977] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
> 0000000000000400
> [ 2710.903180] Call Trace:
> [ 2710.903332] hrtimer_try_to_cancel+0x1a/0x93
> [ 2710.903504] hrtimer_cancel+0x15/0x20
> [ 2710.903667] qdisc_watchdog_cancel+0x12/0x14
> [ 2710.903866] htb_destroy+0x2e/0xf7
> [ 2710.904097] qdisc_create+0x377/0x3fd
> [ 2710.904330] tc_modify_qdisc+0x4d2/0x4fd
> [ 2710.904511] rtnetlink_rcv_msg+0x188/0x197
> [ 2710.904682] ? rcu_read_unlock+0x3e/0x5f
> [ 2710.904849] ? rtnl_newlink+0x729/0x729
> [ 2710.905017] netlink_rcv_skb+0x6c/0xce
> [ 2710.905183] rtnetlink_rcv+0x23/0x2a
> [ 2710.905345] netlink_unicast+0x103/0x181
> [ 2710.905511] netlink_sendmsg+0x326/0x337
> [ 2710.905679] sock_sendmsg_nosec+0x14/0x3f
> [ 2710.905847] sock_sendmsg+0x29/0x2e
> [ 2710.906010] ___sys_sendmsg+0x209/0x28b
> [ 2710.906176] ? do_raw_spin_unlock+0xcd/0xf8
> [ 2710.906346] ? _raw_spin_unlock+0x27/0x31
> [ 2710.906514] ? __handle_mm_fault+0x651/0xdb1
> [ 2710.906685] ? check_chain_key+0xb0/0xfd
> [ 2710.906855] __sys_sendmsg+0x45/0x63
> [ 2710.907018] ? __sys_sendmsg+0x45/0x63
> [ 2710.907185] SyS_sendmsg+0x19/0x1b
> [ 2710.907344] entry_SYSCALL_64_fastpath+0x23/0xc2
>
> Note that probably this bug goes further back because the default qdisc
> handling always calls ->destroy on init failure too.
>
> Fixes: 87b60cfacf9f ("net_sched: fix error recovery at qdisc creation")
> Signed-off-by: Nikolay Aleksandrov <niko...@cumulusnetworks.com>
> ---
> Always calling qdisc destroy on init failure in the default qdisc handling
> was added in commit 0fbbeb1ba43b. I'm not sure if I should include that
> one as fixes tag.
>
> There're more fixes to come, some are much easier to trigger without
> modifications to tc.
Acked-by: Cong Wang <xiyou.wangc...@gmail.com>
