From mboxrd@z Thu Jan 1 00:00:00 1970 From: Guillaume Nault Subject: Re: syzbot rcu/debugobjects warning Date: Mon, 26 Mar 2018 19:38:55 +0200 Message-ID: <20180326173855.GP1351@alphalink.fr> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Thomas Gleixner , Paul McKenney , LKML , Todd Poynor , "open list:BPF (Safe dynamic programs and tools)" , Ben Hutchings , Greg Kroah-Hartman To: Joel Fernandes Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org On Sat, Mar 24, 2018 at 11:29:42PM -0700, Joel Fernandes wrote: > On Fri, Mar 23, 2018 at 1:41 PM, Thomas Gleixner wrote: > > On Fri, 23 Mar 2018, Joel Fernandes wrote: > >> On Fri, Mar 23, 2018 at 2:11 AM, Thomas Gleixner wrote: > >> > On Thu, 22 Mar 2018, Joel Fernandes wrote: > >> Sorry. Here is the raw crash log: https://pastebin.com/raw/puvh0cXE > >> (The kernel logs are toward the end with the above). > > > > And that is interesting: > > > > [ 150.629667] [ 150.631700] [] dump_stack+0xc1/0x128 > > [ 150.637051] [] ? __debug_object_init+0x526/0xc40 > > [ 150.643431] [] panic+0x1bc/0x3a8 > > [ 150.648416] [] ? percpu_up_read_preempt_enable.constprop.53+0xd7/0xd7 > > [ 150.656611] [] ? load_image_and_restore+0xf9/0xf9 > > [ 150.663070] [] ? vprintk_default+0x1d/0x30 > > [ 150.668925] [] ? __warn+0x1a9/0x1e0 > > [ 150.674170] [] ? __debug_object_init+0x526/0xc40 > > [ 150.680543] [] __warn+0x1c4/0x1e0 > > [ 150.685614] [] warn_slowpath_null+0x2c/0x40 > > [ 150.691972] [] __debug_object_init+0x526/0xc40 > > [ 150.698174] [] ? debug_object_fixup+0x30/0x30 > > [ 150.704283] [] debug_object_init_on_stack+0x19/0x20 > > [ 150.710917] [] __wait_rcu_gp+0x93/0x1b0 > > [ 150.716508] [] synchronize_rcu.part.65+0x101/0x110 > > [ 150.723054] [] ? rcu_pm_notify+0xc0/0xc0 > > [ 150.728735] [] ? __call_rcu.constprop.72+0x910/0x910 > > [ 150.735459] [] ? __lock_is_held+0xa1/0xf0 > > [ 150.741223] [] synchronize_rcu+0x27/0x90 > > > > So this calls synchronize_rcu from a rcu callback. That's a nono. This is > > on the back of an interrupt in softirq context and __wait_rcu_gp() can > > sleep, which is obviously a bad idea in softirq context.... > > > > Cc'ed netdev .... > > > > And that also explains the debug object splat because this is not running > > on the task stack. It's running on the softirq stack .... > > > > [ 150.746908] [] __l2tp_session_unhash+0x3d5/0x550 > > [ 150.753281] [] ? __l2tp_session_unhash+0x1bf/0x550 > > [ 150.759828] [] ? __local_bh_enable_ip+0x6a/0xd0 > > [ 150.766123] [] ? l2tp_udp_encap_recv+0xd90/0xd90 > > [ 150.772497] [] l2tp_tunnel_closeall+0x1e7/0x3a0 > > [ 150.778782] [] l2tp_tunnel_destruct+0x30e/0x5a0 > > [ 150.785067] [] ? l2tp_tunnel_destruct+0x1aa/0x5a0 > > [ 150.791537] [] ? l2tp_tunnel_del_work+0x460/0x460 > > [ 150.797997] [] __sk_destruct+0x53/0x570 > > [ 150.803588] [] rcu_process_callbacks+0x898/0x1300 > > [ 150.810048] [] ? rcu_process_callbacks+0x977/0x1300 > > [ 150.816684] [] ? __sk_dst_check+0x240/0x240 > > [ 150.822625] [] __do_softirq+0x206/0x951 > > [ 150.828223] [] irq_exit+0x165/0x190 > > [ 150.833557] [] smp_apic_timer_interrupt+0x7b/0xa0 > > [ 150.840018] [] apic_timer_interrupt+0xa0/0xb0 > > [ 150.846132] [ 150.848166] [] ? native_safe_halt+0x6/0x10 > > [ 150.854036] [] ? trace_hardirqs_on+0xd/0x10 > > [ 150.859973] [] default_idle+0x55/0x360 > > [ 150.865478] [] arch_cpu_idle+0xa/0x10 > > [ 150.870896] [] default_idle_call+0x36/0x60 > > [ 150.876751] [] cpu_startup_entry+0x2b0/0x380 > > [ 150.882787] [] ? cpu_in_idle+0x20/0x20 > > [ 150.888291] [] ? clockevents_register_device+0x123/0x200 > > [ 150.895358] [] start_secondary+0x303/0x3e0 > > [ 150.901209] [] ? set_cpu_sibling_map+0x11f0/0x11f0 > > Thomas, thanks a lot. It appears this issue will not happen on > mainline since from commit 765924e362d1 (subject "l2tp: don't close > sessions in l2tp_tunnel_destruct()"), l2tp_tunnel_closeall is no > longer called from l2tp_tunnel_destruct. From that commit message it > seems one of the motivations is to solve scheduling from atomic issue. > I agree that this patch should fix the above splat. > However for this change to be applied to android-4.9 and/or 4.9 > stable, it depends on several other l2p patches and they aren't > straight forward cherry-picks from mainline (and I don't have much > background with this driver). > > v3.16.56 stable seems to be further along with l2tp than v4.9.89, in > that it atleast has more of the upstream patches adapted for it, that > the above patch depends on. Since this also related to stable, I am > CC'ing Greg kh and Ben. > I generally review l2tp patches proposed for -stable trees (although not in time). If a patch has been ported to 3.16.y and is missing in another tree, then it should be safe to port it there too. > Here are some of the commits in 3.16 stable that I couldn't find > applied to v4.9 stable. The above fix quotes the below patches as > dependencies so they would need to be stable backported. Also CC'ing > Guillaume since he authored the above mentioned fix. > > 0c15ddabbcf l2tp: don't register sessions in l2tp_session_create() > a3c5d5b70f4e l2tp: fix race condition in l2tp_tunnel_delete > 5b216e8dcda2 l2tp: prevent creation of sessions on terminated tunnels > 76ff5e22f1e0 l2tp: hold tunnel while looking up sessions in l2tp_netlink > ceb8f6b23a38 l2tp: define parameters of l2tp_session_get*() as "const" > 0295d020b63f l2tp: initialise session's refcount before making it reachable > 29a77518927e l2tp: take reference on sessions being dumped > b301c9b7782f l2tp: take a reference on sessions used in genetlink handlers > Yes, I think it'd make sense to port these patches.