From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiri Pirko Subject: Re: [PATCH] net: do not pass vlan pkts to real dev pkt handler also Date: Tue, 13 Dec 2011 22:45:38 +0100 Message-ID: <20111213214537.GC2702@minipsycho> References: <20111212221923.5356.43629.stgit@vifc.jf.intel.com> <20111212225622.GA7083@minipsycho> <1323738532.8333.7.camel@vi> <20111213142114.GB2702@minipsycho> <1323796263.8333.20.camel@vi> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Vasu Dev , netdev@vger.kernel.org, devel@open-fcoe.org, eric.dumazet@gmail.com To: Vasu Dev Return-path: Received: from mx1.redhat.com ([209.132.183.28]:56758 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754630Ab1LMVpv (ORCPT ); Tue, 13 Dec 2011 16:45:51 -0500 Content-Disposition: inline In-Reply-To: <1323796263.8333.20.camel@vi> Sender: netdev-owner@vger.kernel.org List-ID: Tue, Dec 13, 2011 at 06:11:03PM CET, vasu.dev@linux.intel.com wrote: >On Tue, 2011-12-13 at 15:21 +0100, Jiri Pirko wrote: >> Tue, Dec 13, 2011 at 02:08:52AM CET, vasu.dev@linux.intel.com wrote: >> >On Mon, 2011-12-12 at 23:56 +0100, Jiri Pirko wrote: >> >> Mon, Dec 12, 2011 at 11:19:23PM CET, vasu.dev@intel.com wrote: >> >> >The orig_dev has to be updated before going another round >> >> >for vlan pkts, otherwise currently unmodified real orig_dev >> >> >causes vlan pkt delivered to real orig_dev also. >> >> > >> >> >The fcoe stack doesn't expects its vlan pkts on real dev >> >> >and it causes crash in fcoe stack. >> >> >> >> Could you please provide more info on where exactly it would crash and >> >> why? >> > >> >Its in fcoe stack due to its fip rx skb list getting corrupt as same skb >> >instance getting queued twice without being cloned, though list was well >> >protected by its spin lock, it was queued on its two fcoe instances, one >> >on real dev and other on its vlan. >> > >> >I could also handle this gracefully in fcoe stack by cloning but any >> >case netdev should not forward vlan pkt to its read dev pkt handler also >> >and that is getting fixed with this patch, so patch will restore >> >orig_dev uses for *only* vlan pkts as it was with recursive >> >__netif_receive_skb calling prior to commit 0dfe178. >> >> >> I do not see into fcoe code, but wouldn't it be good to do skb >> skb_share_check in fcoe_ctlr_recv? I suppose that would solve your >> problem and looks legal to me. >> > >Yes that will fix along with dropping vlan pkts on real dev, so some >additional checking for dropping also. In fact that is what I meant in >my last response by "I could also handle this gracefully in fcoe stack >by cloning" as skb_share_check() does that conditionally. > >But as far as this patch goes, are you okay with the fix to not forward >vlan pkt on real dev pkt handler ? I think this is required regardless >of fcoe stack fixing for shared skb since otherwise all upper layers of >real dev pkt handler has to handle with un-expected vlan pkts also. I think that's what orig_dev is destined for. To provide a possiblility to do this. I would like to leave that as it is. > >Thanks for your review. >Vasu > > >> >> > >> >Here is the detailed crash log: >> > >> >[ 340.679591] BUG: unable to handle kernel NULL pointer dereference at >> >0000000000000008 >> >[ 340.680112] IP: [] skb_dequeue+0x55/0x90 >> >[ 340.680112] PGD 0 >> >[ 340.680112] Oops: 0002 [#1] SMP >> >[ 340.680112] CPU 3 >> >[ 340.680112] Modules linked in: fcoe libfcoe libfc scsi_transport_fc >> >8021q e1000 virtio_balloon ixgbe mdio virtio_blk virtio_pci virtio_ring >> >virtio [last unloaded: scsi_wait_scan] >> >[ 340.680112] >> >[ 340.680112] Pid: 442, comm: kworker/3:1 Not tainted 3.2.0-rc4+ #53 >> >Bochs Bochs >> >[ 340.680112] RIP: 0010:[] [] >> >skb_dequeue+0x55/0x90 >> >[ 340.680112] RSP: 0018:ffff88007c963c80 EFLAGS: 00010097 >> >[ 340.680112] RAX: 0000000000000282 RBX: ffff88007baee9b4 RCX: >> >0000000000000000 >> >[ 340.680112] RDX: 0000000000000000 RSI: 0000000000000286 RDI: >> >ffff88007baee9b4 >> >[ 340.680112] RBP: ffff88007c963ca0 R08: ffff88007c35ddc0 R09: >> >0000000000000001 >> >[ 340.680112] R10: 0000000000000006 R11: 0000000000000001 R12: >> >ffff88007bedca00 >> >[ 340.680112] R13: ffff88007baee9a0 R14: ffff88007c963d80 R15: >> >ffff88007baeea00 >> >[ 340.680112] FS: 0000000000000000(0000) GS:ffff88007fd80000(0000) >> >knlGS:0000000000000000 >> >[ 340.680112] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b >> >[ 340.680112] CR2: 0000000000000008 CR3: 0000000001c05000 CR4: >> >00000000000006e0 >> >[ 340.680112] DR0: 0000000000000000 DR1: 0000000000000000 DR2: >> >0000000000000000 >> >[ 340.680112] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: >> >0000000000000400 >> >[ 340.680112] Process kworker/3:1 (pid: 442, threadinfo >> >ffff88007c962000, task ffff88007c9f60b0) >> >[ 340.680112] Stack: >> >[ 340.680112] ffff88007baee9a0 ffff88007baee8c0 ffff88007baee9a0 >> >ffff88007bedca00 >> >[ 340.680112] ffff88007c963df0 ffffffffa00af7a7 ffff88007baad4d4 >> >ffffffff81060dee >> >[ 340.680112] 0000000000000001 ffff88007c35ddc0 ffff88007c963fd8 >> >0000000000000000 >> >[ 340.680112] Call Trace: >> >[ 340.680112] [] fcoe_ctlr_recv_work+0x147/0x1870 >> >[libfcoe] >> >[ 340.680112] [] ? queue_delayed_work_on+0x9e/0x170 >> >[ 340.680112] [] ? fcoe_ctlr_vn_recv+0x9a0/0x9a0 >> >[libfcoe] >> >[ 340.680112] [] process_one_work+0x11e/0x460 >> >[ 340.680112] [] worker_thread+0x178/0x400 >> >[ 340.680112] [] ? manage_workers+0x210/0x210 >> >[ 340.680112] [] kthread+0x96/0xa0 >> >[ 340.680112] [] kernel_thread_helper+0x4/0x10 >> >[ 340.680112] [] ? kthread_worker_fn+0x1a0/0x1a0 >> >[ 340.680112] [] ? gs_change+0xb/0xb >> >[ 340.680112] Code: 65 00 4d 39 e5 74 4f 4d 85 e4 74 26 41 83 6d 10 01 >> >49 8b 0c 24 49 8b 54 24 08 49 c7 04 24 00 00 00 00 49 c7 44 24 08 00 00 >> >00 00 >> >[ 340.680112] 89 51 08 48 89 0a 48 89 c6 48 89 df e8 39 1b 15 00 4c 89 >> >e0 >> >[ 340.680112] RIP [] skb_dequeue+0x55/0x90 >> >[ 340.680112] RSP >> >[ 340.680112] CR2: 0000000000000008 >> > >> > >> > >> >Thanks >> >Vasu >> > >> >> >> >> Thanks. >> >> >> >> Jirka >> >> >> >> > >> >> >This wasn't issue untill __netif_receive_skb recursive calling >> >> >was removed with this commit 0dfe178, so this patch restores >> >> >orig_dev uses as it was prior to that commit but still w/o >> >> >recursive calling to __netif_receive_skb. >> >> > >> >> >Signed-off-by: Vasu Dev >> >> >--- >> >> > >> >> > net/core/dev.c | 5 +++-- >> >> > 1 files changed, 3 insertions(+), 2 deletions(-) >> >> > >> >> >diff --git a/net/core/dev.c b/net/core/dev.c >> >> >index f494675..adbcd7a 100644 >> >> >--- a/net/core/dev.c >> >> >+++ b/net/core/dev.c >> >> >@@ -3222,9 +3222,10 @@ ncls: >> >> > ret = deliver_skb(skb, pt_prev, orig_dev); >> >> > pt_prev = NULL; >> >> > } >> >> >- if (vlan_do_receive(&skb, !rx_handler)) >> >> >+ if (vlan_do_receive(&skb, !rx_handler)) { >> >> >+ orig_dev = skb->dev; >> >> > goto another_round; >> >> >- else if (unlikely(!skb)) >> >> >+ } else if (unlikely(!skb)) >> >> > goto out; >> >> > } >> >> > >> >> > >> >> -- >> >> To unsubscribe from this list: send the line "unsubscribe netdev" in >> >> the body of a message to majordomo@vger.kernel.org >> >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > >> > >> > >> -- >> To unsubscribe from this list: send the line "unsubscribe netdev" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >