| Message ID | 1354058916.2534.25.camel@shinybook.infradead.org |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Nov 27, 2012 at 11:28:36PM +0000, David Woodhouse wrote: > Avoid submitting patches to a vcc which is being closed. Things go badly > wrong when the ->pop method gets later called after everything's been > torn down. > > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com> > --- > On Tue, 2012-11-27 at 22:36 +0000, David Woodhouse wrote: > > Nathan, does this help? > > I think that's necessary, but not sufficient. You'll want something like > this too... I can now kill br2684ctl while there's a flood of outgoing > packets, and get a handful of the printks that I had in here until a few > seconds ago when I edited it out of the patch in my mail client... and > no more panic. > > I do also now have Krzysztof's patch 1/7 (detach protocol before closing > vcc) but I don't think it actually matters any more. If you do this actually it's better to don't use patch 1/7 because it introduces race condition that you found earlier. > > --- a/net/atm/br2684.c~ 2012-11-23 23:14:29.000000000 +0000 > +++ b/net/atm/br2684.c 2012-11-27 23:09:18.502403881 +0000 > @@ -249,6 +249,12 @@ static int br2684_xmit_vcc(struct sk_buf > skb_debug(skb); > > ATM_SKB(skb)->vcc = atmvcc = brvcc->atmvcc; > + if (test_bit(ATM_VF_RELEASED, &atmvcc->flags) > + || test_bit(ATM_VF_CLOSE, &atmvcc->flags) > + || !test_bit(ATM_VF_READY, &atmvcc->flags)) { > + dev_kfree_skb(skb); > + return 0; > + } > pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n", skb, atmvcc, atmvcc->dev); > atomic_add(skb->truesize, &sk_atm(atmvcc)->sk_wmem_alloc); > ATM_SKB(skb)->atm_options = atmvcc->atm_options; > With this patch you have still theoretical race that was fixed in patches 5 and 8 in pppoatm series, but I never seen that in practice. Acked-by: Krzysztof Mazur <krzysiek@podlesie.net> Krzysiek -- 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
On Wed, 2012-11-28 at 00:51 +0100, Krzysztof Mazur wrote: > If you do this actually it's better to don't use patch 1/7 because > it introduces race condition that you found earlier. Right. I've omitted that from the git tree I just pushed out. > With this patch you have still theoretical race that was fixed in patches > 5 and 8 in pppoatm series, but I never seen that in practice. And I think it's even less likely for br2684. At least with pppoatm you might have had pppd sending frames. But for br2684 they *only* come from its start_xmit function... which is serialised anyway. I do get strange oopses when I try to add BQL to br2684, but that's not something to be looking at at 1am... I *do* need the equivalent of your patch 4, which is the module_put race.
From: David Woodhouse <dwmw2@infradead.org> Date: Tue, 27 Nov 2012 23:28:36 +0000 > + if (test_bit(ATM_VF_RELEASED, &atmvcc->flags) > + || test_bit(ATM_VF_CLOSE, &atmvcc->flags) > + || !test_bit(ATM_VF_READY, &atmvcc->flags)) { Please: if (X || Y || Z) not: if (X || Y || Z) Thanks. -- 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
On Wed, 2012-11-28 at 11:41 -0500, David Miller wrote: > > Please: > > if (X || > Y || > Z) > > not: > > if (X > || Y > || Z) Thanks. Fixed in both Krzysztof's original pppoatm version, and my br2684 patch, in the git tree at git://git.infradead.org/~dwmw2/atm.git I knew there was something else that offended me about the original, other than just the whitespace (which is also fixed).
From: David Woodhouse <dwmw2@infradead.org> Date: Wed, 28 Nov 2012 17:01:10 +0000 > On Wed, 2012-11-28 at 11:41 -0500, David Miller wrote: >> >> Please: >> >> if (X || >> Y || >> Z) >> >> not: >> >> if (X >> || Y >> || Z) > > Thanks. Fixed in both Krzysztof's original pppoatm version, and my > br2684 patch, in the git tree at git://git.infradead.org/~dwmw2/atm.git > > I knew there was something else that offended me about the original, > other than just the whitespace (which is also fixed). Do you want me to pull that tree into net-next or is there a plan to repost the entire series of work for a final submission? -- 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
On Wed, 2012-11-28 at 12:04 -0500, David Miller wrote: > Do you want me to pull that tree into net-next or is there a plan to > repost the entire series of work for a final submission? I think it needs a little more testing/consensus first. I'd like an ack from Chas on the atm ->release_cb() thing, at least. And I wouldn't mind confirmation from Nathan's customer that they're no longer seeing the panics. And then I'll either send an explicit pull request, or submit it as patches — whichever you prefer. Hopefully in the next day or so; the merge window is approaching...
From: David Woodhouse <dwmw2@infradead.org> Date: Wed, 28 Nov 2012 17:09:15 +0000 > And then I'll either send an explicit pull request, or submit it as > patches ― whichever you prefer. The canonical thing is to do both, send the pull request in the "[PATCH 0/N]" email, and then the patches so everyone can see the final form of the changes. > Hopefully in the next day or so; the merge window is approaching... Yep, thanks. -- 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
On Wed, 2012-11-28 at 17:09 +0000, David Woodhouse wrote: > On Wed, 2012-11-28 at 12:04 -0500, David Miller wrote: > > Do you want me to pull that tree into net-next or is there a plan to > > repost the entire series of work for a final submission? > > I think it needs a little more testing/consensus first. I'd like an ack > from Chas on the atm ->release_cb() thing, at least. And I wouldn't mind > confirmation from Nathan's customer that they're no longer seeing the > panics. > The customer has confirmed that they haven't seen any panics. I tested these patches on OpenWrt with Kernel 3.3.8 and couldn't get a panic: c118dc5 solos-pci: Fix leak of skb received for unknown vcc e539793 br2684: fix module_put() race 3656320 br2684: don't send frames on not-ready vcc 753f920 solos-pci: Wait for pending TX to complete when releasing vcc 91ab2cf pppoatm: do not inline pppoatm_may_send() 85b48fa pppoatm: drop frames to not-ready vcc 3ac1080 pppoatm: take ATM socket lock in pppoatm_send() e41faed pppoatm: fix module_put() race 3b1a914 pppoatm: allow assign only on a connected socket ec809bd atm: add owner of push() callback to atmvcc ae088d6 atm: br2684: Fix excessive queue bloat I haven't tested these ones: 230a012 pppoatm: fix missing wakeup in pppoatm_send() 1c0c800 atm: Add release_cb() callback to vcc -- 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
On Fri, 2012-11-30 at 12:18 +1100, Nathan Williams wrote: > The customer has confirmed that they haven't seen any panics. I tested > these patches on OpenWrt with Kernel 3.3.8 and couldn't get a panic: Thanks. > I haven't tested these ones: > > 230a012 pppoatm: fix missing wakeup in pppoatm_send() > 1c0c800 atm: Add release_cb() callback to vcc The race that fixes is fairly unlikely, but if you do see transmit stalls you can make them apply to your 3.3.8 kernel by applying the same preliminary patch that is now in the OpenWRT Attitude Adjustment branch: https://dev.openwrt.org/browser/branches/attitude_adjustment/target/linux/generic/patches-3.3/080-prot-release-cb.patch
--- a/net/atm/br2684.c~ 2012-11-23 23:14:29.000000000 +0000 +++ b/net/atm/br2684.c 2012-11-27 23:09:18.502403881 +0000 @@ -249,6 +249,12 @@ static int br2684_xmit_vcc(struct sk_buf skb_debug(skb); ATM_SKB(skb)->vcc = atmvcc = brvcc->atmvcc; + if (test_bit(ATM_VF_RELEASED, &atmvcc->flags) + || test_bit(ATM_VF_CLOSE, &atmvcc->flags) + || !test_bit(ATM_VF_READY, &atmvcc->flags)) { + dev_kfree_skb(skb); + return 0; + } pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n", skb, atmvcc, atmvcc->dev); atomic_add(skb->truesize, &sk_atm(atmvcc)->sk_wmem_alloc); ATM_SKB(skb)->atm_options = atmvcc->atm_options;