Patchwork br2684: don't send frames on not-ready vcc

login
register
mail settings
Submitter David Woodhouse
Date Nov. 27, 2012, 11:28 p.m.
Message ID <1354058916.2534.25.camel@shinybook.infradead.org>
Download mbox | patch
Permalink /patch/202319/
State RFC
Delegated to: David Miller
Headers show

Comments

David Woodhouse - Nov. 27, 2012, 11:28 p.m.
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.
Krzysztof Mazur - Nov. 27, 2012, 11:51 p.m.
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
David Woodhouse - Nov. 28, 2012, 12:54 a.m.
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.
David Miller - Nov. 28, 2012, 4:41 p.m.
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
David Woodhouse - Nov. 28, 2012, 5:01 p.m.
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).
David Miller - Nov. 28, 2012, 5:04 p.m.
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
David Woodhouse - Nov. 28, 2012, 5:09 p.m.
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...
David Miller - Nov. 28, 2012, 5:11 p.m.
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
Nathan Williams - Nov. 30, 2012, 1:18 a.m.
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
David Woodhouse - Nov. 30, 2012, 1:34 a.m.
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

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;