linux-ppp.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] ppp: fix xmit recursion detection on ppp channels
@ 2017-08-08  9:43 Guillaume Nault
  2017-08-08 13:16 ` Gao Feng
  2017-08-09  4:07 ` David Miller
  0 siblings, 2 replies; 5+ messages in thread
From: Guillaume Nault @ 2017-08-08  9:43 UTC (permalink / raw)
  To: netdev; +Cc: Gao Feng, Liu Jianying, David Miller, linux-ppp, Paul Mackerras

Commit e5dadc65f9e0 ("ppp: Fix false xmit recursion detect with two ppp
devices") dropped the xmit_recursion counter incrementation in
ppp_channel_push() and relied on ppp_xmit_process() for this task.
But __ppp_channel_push() can also send packets directly (using the
.start_xmit() channel callback), in which case the xmit_recursion
counter isn't incremented anymore. If such packets get routed back to
the parent ppp unit, ppp_xmit_process() won't notice the recursion and
will call ppp_channel_push() on the same channel, effectively creating
the deadlock situation that the xmit_recursion mechanism was supposed
to prevent.

This patch re-introduces the xmit_recursion counter incrementation in
ppp_channel_push(). Since the xmit_recursion variable is now part of
the parent ppp unit, incrementation is skipped if the channel doesn't
have any. This is fine because only packets routed through the parent
unit may enter the channel recursively.

Finally, we have to ensure that pch->ppp is not going to be modified
while executing ppp_channel_push(). Instead of taking this lock only
while calling ppp_xmit_process(), we now have to hold it for the full
ppp_channel_push() execution. This respects the ppp locks ordering
which requires locking ->upl before ->downl.

Fixes: e5dadc65f9e0 ("ppp: Fix false xmit recursion detect with two ppp devices")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
---
 drivers/net/ppp/ppp_generic.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index bd4303944e44..a404552555d4 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -1915,21 +1915,23 @@ static void __ppp_channel_push(struct channel *pch)
 	spin_unlock(&pch->downl);
 	/* see if there is anything from the attached unit to be sent */
 	if (skb_queue_empty(&pch->file.xq)) {
-		read_lock(&pch->upl);
 		ppp = pch->ppp;
 		if (ppp)
-			ppp_xmit_process(ppp);
-		read_unlock(&pch->upl);
+			__ppp_xmit_process(ppp);
 	}
 }
 
 static void ppp_channel_push(struct channel *pch)
 {
-	local_bh_disable();
-
-	__ppp_channel_push(pch);
-
-	local_bh_enable();
+	read_lock_bh(&pch->upl);
+	if (pch->ppp) {
+		(*this_cpu_ptr(pch->ppp->xmit_recursion))++;
+		__ppp_channel_push(pch);
+		(*this_cpu_ptr(pch->ppp->xmit_recursion))--;
+	} else {
+		__ppp_channel_push(pch);
+	}
+	read_unlock_bh(&pch->upl);
 }
 
 /*
-- 
2.14.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re:[PATCH net] ppp: fix xmit recursion detection on ppp channels
  2017-08-08  9:43 [PATCH net] ppp: fix xmit recursion detection on ppp channels Guillaume Nault
@ 2017-08-08 13:16 ` Gao Feng
  2017-08-08 13:58   ` [PATCH " Guillaume Nault
  2017-08-09  4:07 ` David Miller
  1 sibling, 1 reply; 5+ messages in thread
From: Gao Feng @ 2017-08-08 13:16 UTC (permalink / raw)
  To: Guillaume Nault
  Cc: netdev, Liu Jianying, David Miller, linux-ppp, Paul Mackerras

QXQgMjAxNy0wOC0wOCAxNzo0MzoyNCwgIkd1aWxsYXVtZSBOYXVsdCIgPGcubmF1bHRAYWxwaGFs
aW5rLmZyPiB3cm90ZToKPkNvbW1pdCBlNWRhZGM2NWY5ZTAgKCJwcHA6IEZpeCBmYWxzZSB4bWl0
IHJlY3Vyc2lvbiBkZXRlY3Qgd2l0aCB0d28gcHBwCj5kZXZpY2VzIikgZHJvcHBlZCB0aGUgeG1p
dF9yZWN1cnNpb24gY291bnRlciBpbmNyZW1lbnRhdGlvbiBpbgo+cHBwX2NoYW5uZWxfcHVzaCgp
IGFuZCByZWxpZWQgb24gcHBwX3htaXRfcHJvY2VzcygpIGZvciB0aGlzIHRhc2suCj5CdXQgX19w
cHBfY2hhbm5lbF9wdXNoKCkgY2FuIGFsc28gc2VuZCBwYWNrZXRzIGRpcmVjdGx5ICh1c2luZyB0
aGUKPi5zdGFydF94bWl0KCkgY2hhbm5lbCBjYWxsYmFjayksIGluIHdoaWNoIGNhc2UgdGhlIHht
aXRfcmVjdXJzaW9uCgpZb3UncmUgcmlnaHQuIFdlIGxvc3QgdGhpcyBjYXNlLgoKPmNvdW50ZXIg
aXNuJ3QgaW5jcmVtZW50ZWQgYW55bW9yZS4gSWYgc3VjaCBwYWNrZXRzIGdldCByb3V0ZWQgYmFj
ayB0bwo+dGhlIHBhcmVudCBwcHAgdW5pdCwgcHBwX3htaXRfcHJvY2VzcygpIHdvbid0IG5vdGlj
ZSB0aGUgcmVjdXJzaW9uIGFuZAo+d2lsbCBjYWxsIHBwcF9jaGFubmVsX3B1c2goKSBvbiB0aGUg
c2FtZSBjaGFubmVsLCBlZmZlY3RpdmVseSBjcmVhdGluZwo+dGhlIGRlYWRsb2NrIHNpdHVhdGlv
biB0aGF0IHRoZSB4bWl0X3JlY3Vyc2lvbiBtZWNoYW5pc20gd2FzIHN1cHBvc2VkCj50byBwcmV2
ZW50Lgo+Cj5UaGlzIHBhdGNoIHJlLWludHJvZHVjZXMgdGhlIHhtaXRfcmVjdXJzaW9uIGNvdW50
ZXIgaW5jcmVtZW50YXRpb24gaW4KPnBwcF9jaGFubmVsX3B1c2goKS4gU2luY2UgdGhlIHhtaXRf
cmVjdXJzaW9uIHZhcmlhYmxlIGlzIG5vdyBwYXJ0IG9mCj50aGUgcGFyZW50IHBwcCB1bml0LCBp
bmNyZW1lbnRhdGlvbiBpcyBza2lwcGVkIGlmIHRoZSBjaGFubmVsIGRvZXNuJ3QKPmhhdmUgYW55
LiBUaGlzIGlzIGZpbmUgYmVjYXVzZSBvbmx5IHBhY2tldHMgcm91dGVkIHRocm91Z2ggdGhlIHBh
cmVudAo+dW5pdCBtYXkgZW50ZXIgdGhlIGNoYW5uZWwgcmVjdXJzaXZlbHkuCj4KPkZpbmFsbHks
IHdlIGhhdmUgdG8gZW5zdXJlIHRoYXQgcGNoLT5wcHAgaXMgbm90IGdvaW5nIHRvIGJlIG1vZGlm
aWVkCj53aGlsZSBleGVjdXRpbmcgcHBwX2NoYW5uZWxfcHVzaCgpLiBJbnN0ZWFkIG9mIHRha2lu
ZyB0aGlzIGxvY2sgb25seQo+d2hpbGUgY2FsbGluZyBwcHBfeG1pdF9wcm9jZXNzKCksIHdlIG5v
dyBoYXZlIHRvIGhvbGQgaXQgZm9yIHRoZSBmdWxsCj5wcHBfY2hhbm5lbF9wdXNoKCkgZXhlY3V0
aW9uLiBUaGlzIHJlc3BlY3RzIHRoZSBwcHAgbG9ja3Mgb3JkZXJpbmcKPndoaWNoIHJlcXVpcmVz
IGxvY2tpbmcgLT51cGwgYmVmb3JlIC0+ZG93bmwuCj4KPkZpeGVzOiBlNWRhZGM2NWY5ZTAgKCJw
cHA6IEZpeCBmYWxzZSB4bWl0IHJlY3Vyc2lvbiBkZXRlY3Qgd2l0aCB0d28gcHBwIGRldmljZXMi
KQo+U2lnbmVkLW9mZi1ieTogR3VpbGxhdW1lIE5hdWx0IDxnLm5hdWx0QGFscGhhbGluay5mcj4K
Pi0tLQo+IGRyaXZlcnMvbmV0L3BwcC9wcHBfZ2VuZXJpYy5jIHwgMTggKysrKysrKysrKy0tLS0t
LS0tCj4gMSBmaWxlIGNoYW5nZWQsIDEwIGluc2VydGlvbnMoKyksIDggZGVsZXRpb25zKC0pCj4K
PmRpZmYgLS1naXQgYS9kcml2ZXJzL25ldC9wcHAvcHBwX2dlbmVyaWMuYyBiL2RyaXZlcnMvbmV0
L3BwcC9wcHBfZ2VuZXJpYy5jCj5pbmRleCBiZDQzMDM5NDRlNDQuLmE0MDQ1NTI1NTVkNCAxMDA2
NDQKPi0tLSBhL2RyaXZlcnMvbmV0L3BwcC9wcHBfZ2VuZXJpYy5jCj4rKysgYi9kcml2ZXJzL25l
dC9wcHAvcHBwX2dlbmVyaWMuYwo+QEAgLTE5MTUsMjEgKzE5MTUsMjMgQEAgc3RhdGljIHZvaWQg
X19wcHBfY2hhbm5lbF9wdXNoKHN0cnVjdCBjaGFubmVsICpwY2gpCj4gCXNwaW5fdW5sb2NrKCZw
Y2gtPmRvd25sKTsKPiAJLyogc2VlIGlmIHRoZXJlIGlzIGFueXRoaW5nIGZyb20gdGhlIGF0dGFj
aGVkIHVuaXQgdG8gYmUgc2VudCAqLwo+IAlpZiAoc2tiX3F1ZXVlX2VtcHR5KCZwY2gtPmZpbGUu
eHEpKSB7Cj4tCQlyZWFkX2xvY2soJnBjaC0+dXBsKTsKPiAJCXBwcCA9IHBjaC0+cHBwOwo+IAkJ
aWYgKHBwcCkKPi0JCQlwcHBfeG1pdF9wcm9jZXNzKHBwcCk7Cj4tCQlyZWFkX3VubG9jaygmcGNo
LT51cGwpOwo+KwkJCV9fcHBwX3htaXRfcHJvY2VzcyhwcHApOwo+IAl9Cj4gfQo+IAo+IHN0YXRp
YyB2b2lkIHBwcF9jaGFubmVsX3B1c2goc3RydWN0IGNoYW5uZWwgKnBjaCkKPiB7Cj4tCWxvY2Fs
X2JoX2Rpc2FibGUoKTsKPi0KPi0JX19wcHBfY2hhbm5lbF9wdXNoKHBjaCk7Cj4tCj4tCWxvY2Fs
X2JoX2VuYWJsZSgpOwo+KwlyZWFkX2xvY2tfYmgoJnBjaC0+dXBsKTsKPisJaWYgKHBjaC0+cHBw
KSB7Cj4rCQkoKnRoaXNfY3B1X3B0cihwY2gtPnBwcC0+eG1pdF9yZWN1cnNpb24pKSsrOwo+KwkJ
X19wcHBfY2hhbm5lbF9wdXNoKHBjaCk7Cj4rCQkoKnRoaXNfY3B1X3B0cihwY2gtPnBwcC0+eG1p
dF9yZWN1cnNpb24pKS0tOwo+Kwl9IGVsc2Ugewo+KwkJX19wcHBfY2hhbm5lbF9wdXNoKHBjaCk7
Cj4rCX0KPisJcmVhZF91bmxvY2tfYmgoJnBjaC0+dXBsKTsKCklmIGludm9rZWQgcmVhZF9sb2Nr
X2JoIGluIHBwcF9jaGFubmVsX3B1c2gsIGl0IHdvdWxkIGJlIHVubmVjZXNzYXJ5IHRvIGludm9r
ZSByZWFkX2xvY2soJnBjaC0+dXBsKQppbiB0aGUgX19wcHBfY2hhbm5lbF9wdXNoLgoKQmVzdCBS
ZWdhcmRzCkZlbmcKCj4gfQo+IAo+IC8qCj4tLSAKPjIuMTQuMAo+Cg=

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] ppp: fix xmit recursion detection on ppp channels
  2017-08-08 13:16 ` Gao Feng
@ 2017-08-08 13:58   ` Guillaume Nault
  2017-08-08 14:21     ` Gao Feng
  0 siblings, 1 reply; 5+ messages in thread
From: Guillaume Nault @ 2017-08-08 13:58 UTC (permalink / raw)
  To: Gao Feng; +Cc: netdev, Liu Jianying, David Miller, linux-ppp, Paul Mackerras

On Tue, Aug 08, 2017 at 09:16:33PM +0800, Gao Feng wrote:
> At 2017-08-08 17:43:24, "Guillaume Nault" <g.nault@alphalink.fr> wrote:
> >--- a/drivers/net/ppp/ppp_generic.c
> >+++ b/drivers/net/ppp/ppp_generic.c
> >@@ -1915,21 +1915,23 @@ static void __ppp_channel_push(struct channel *pch)
> > 	spin_unlock(&pch->downl);
> > 	/* see if there is anything from the attached unit to be sent */
> > 	if (skb_queue_empty(&pch->file.xq)) {
> >-		read_lock(&pch->upl);
> > 		ppp = pch->ppp;
> > 		if (ppp)
> >-			ppp_xmit_process(ppp);
> >-		read_unlock(&pch->upl);
> >+			__ppp_xmit_process(ppp);
> > 	}
> > }
> > 
> > static void ppp_channel_push(struct channel *pch)
> > {
> >-	local_bh_disable();
> >-
> >-	__ppp_channel_push(pch);
> >-
> >-	local_bh_enable();
> >+	read_lock_bh(&pch->upl);
> >+	if (pch->ppp) {
> >+		(*this_cpu_ptr(pch->ppp->xmit_recursion))++;
> >+		__ppp_channel_push(pch);
> >+		(*this_cpu_ptr(pch->ppp->xmit_recursion))--;
> >+	} else {
> >+		__ppp_channel_push(pch);
> >+	}
> >+	read_unlock_bh(&pch->upl);
> 
> If invoked read_lock_bh in ppp_channel_push, it would be unnecessary to invoke read_lock(&pch->upl)
> in the __ppp_channel_push.
> 
But this patch does remove read_lock(&pch->upl) from
__ppp_channel_push(). Or have I misunderstood your point?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re:Re: [PATCH net] ppp: fix xmit recursion detection on ppp channels
  2017-08-08 13:58   ` [PATCH " Guillaume Nault
@ 2017-08-08 14:21     ` Gao Feng
  0 siblings, 0 replies; 5+ messages in thread
From: Gao Feng @ 2017-08-08 14:21 UTC (permalink / raw)
  To: Guillaume Nault
  Cc: netdev, Liu Jianying, David Miller, linux-ppp, Paul Mackerras

CkF0IDIwMTctMDgtMDggMjE6NTg6MjcsICJHdWlsbGF1bWUgTmF1bHQiIDxnLm5hdWx0QGFscGhh
bGluay5mcj4gd3JvdGU6Cj5PbiBUdWUsIEF1ZyAwOCwgMjAxNyBhdCAwOToxNjozM1BNICswODAw
LCBHYW8gRmVuZyB3cm90ZToKPj4gQXQgMjAxNy0wOC0wOCAxNzo0MzoyNCwgIkd1aWxsYXVtZSBO
YXVsdCIgPGcubmF1bHRAYWxwaGFsaW5rLmZyPiB3cm90ZToKPj4gPi0tLSBhL2RyaXZlcnMvbmV0
L3BwcC9wcHBfZ2VuZXJpYy5jCj4+ID4rKysgYi9kcml2ZXJzL25ldC9wcHAvcHBwX2dlbmVyaWMu
Ywo+PiA+QEAgLTE5MTUsMjEgKzE5MTUsMjMgQEAgc3RhdGljIHZvaWQgX19wcHBfY2hhbm5lbF9w
dXNoKHN0cnVjdCBjaGFubmVsICpwY2gpCj4+ID4gCXNwaW5fdW5sb2NrKCZwY2gtPmRvd25sKTsK
Pj4gPiAJLyogc2VlIGlmIHRoZXJlIGlzIGFueXRoaW5nIGZyb20gdGhlIGF0dGFjaGVkIHVuaXQg
dG8gYmUgc2VudCAqLwo+PiA+IAlpZiAoc2tiX3F1ZXVlX2VtcHR5KCZwY2gtPmZpbGUueHEpKSB7
Cj4+ID4tCQlyZWFkX2xvY2soJnBjaC0+dXBsKTsKPj4gPiAJCXBwcCA9IHBjaC0+cHBwOwo+PiA+
IAkJaWYgKHBwcCkKPj4gPi0JCQlwcHBfeG1pdF9wcm9jZXNzKHBwcCk7Cj4+ID4tCQlyZWFkX3Vu
bG9jaygmcGNoLT51cGwpOwo+PiA+KwkJCV9fcHBwX3htaXRfcHJvY2VzcyhwcHApOwo+PiA+IAl9
Cj4+ID4gfQo+PiA+IAo+PiA+IHN0YXRpYyB2b2lkIHBwcF9jaGFubmVsX3B1c2goc3RydWN0IGNo
YW5uZWwgKnBjaCkKPj4gPiB7Cj4+ID4tCWxvY2FsX2JoX2Rpc2FibGUoKTsKPj4gPi0KPj4gPi0J
X19wcHBfY2hhbm5lbF9wdXNoKHBjaCk7Cj4+ID4tCj4+ID4tCWxvY2FsX2JoX2VuYWJsZSgpOwo+
PiA+KwlyZWFkX2xvY2tfYmgoJnBjaC0+dXBsKTsKPj4gPisJaWYgKHBjaC0+cHBwKSB7Cj4+ID4r
CQkoKnRoaXNfY3B1X3B0cihwY2gtPnBwcC0+eG1pdF9yZWN1cnNpb24pKSsrOwo+PiA+KwkJX19w
cHBfY2hhbm5lbF9wdXNoKHBjaCk7Cj4+ID4rCQkoKnRoaXNfY3B1X3B0cihwY2gtPnBwcC0+eG1p
dF9yZWN1cnNpb24pKS0tOwo+PiA+Kwl9IGVsc2Ugewo+PiA+KwkJX19wcHBfY2hhbm5lbF9wdXNo
KHBjaCk7Cj4+ID4rCX0KPj4gPisJcmVhZF91bmxvY2tfYmgoJnBjaC0+dXBsKTsKPj4gCj4+IElm
IGludm9rZWQgcmVhZF9sb2NrX2JoIGluIHBwcF9jaGFubmVsX3B1c2gsIGl0IHdvdWxkIGJlIHVu
bmVjZXNzYXJ5IHRvIGludm9rZSByZWFkX2xvY2soJnBjaC0+dXBsKQo+PiBpbiB0aGUgX19wcHBf
Y2hhbm5lbF9wdXNoLgo+PiAKPkJ1dCB0aGlzIHBhdGNoIGRvZXMgcmVtb3ZlIHJlYWRfbG9jaygm
cGNoLT51cGwpIGZyb20KPl9fcHBwX2NoYW5uZWxfcHVzaCgpLiBPciBoYXZlIEkgbWlzdW5kZXJz
dG9vZCB5b3VyIHBvaW50PwoKU29ycnksIGl0J3MgbXkgZmF1bHQuCkkgZm9yZ290IHlvdXIgZm9y
bWVyIGNoYW5nZXMgd2hlbiB0aGluayBhYm91dCB0aGUgdXBkYXRlcyBpbiBwcHBfY2hhbm5lbF9w
dXNoCgpCZXN0IFJlZ2FyZHMKRmVuZwoK

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] ppp: fix xmit recursion detection on ppp channels
  2017-08-08  9:43 [PATCH net] ppp: fix xmit recursion detection on ppp channels Guillaume Nault
  2017-08-08 13:16 ` Gao Feng
@ 2017-08-09  4:07 ` David Miller
  1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2017-08-09  4:07 UTC (permalink / raw)
  To: g.nault; +Cc: netdev, gfree.wind, jianying.liu, linux-ppp, paulus

From: Guillaume Nault <g.nault@alphalink.fr>
Date: Tue, 8 Aug 2017 11:43:24 +0200

> Commit e5dadc65f9e0 ("ppp: Fix false xmit recursion detect with two ppp
> devices") dropped the xmit_recursion counter incrementation in
> ppp_channel_push() and relied on ppp_xmit_process() for this task.
> But __ppp_channel_push() can also send packets directly (using the
> .start_xmit() channel callback), in which case the xmit_recursion
> counter isn't incremented anymore. If such packets get routed back to
> the parent ppp unit, ppp_xmit_process() won't notice the recursion and
> will call ppp_channel_push() on the same channel, effectively creating
> the deadlock situation that the xmit_recursion mechanism was supposed
> to prevent.
> 
> This patch re-introduces the xmit_recursion counter incrementation in
> ppp_channel_push(). Since the xmit_recursion variable is now part of
> the parent ppp unit, incrementation is skipped if the channel doesn't
> have any. This is fine because only packets routed through the parent
> unit may enter the channel recursively.
> 
> Finally, we have to ensure that pch->ppp is not going to be modified
> while executing ppp_channel_push(). Instead of taking this lock only
> while calling ppp_xmit_process(), we now have to hold it for the full
> ppp_channel_push() execution. This respects the ppp locks ordering
> which requires locking ->upl before ->downl.
> 
> Fixes: e5dadc65f9e0 ("ppp: Fix false xmit recursion detect with two ppp devices")
> Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>

Applied, thank you.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-08-09  4:07 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-08  9:43 [PATCH net] ppp: fix xmit recursion detection on ppp channels Guillaume Nault
2017-08-08 13:16 ` Gao Feng
2017-08-08 13:58   ` [PATCH " Guillaume Nault
2017-08-08 14:21     ` Gao Feng
2017-08-09  4:07 ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).