netdev - Re: [PATCH][PPPOL2TP]: Fix SMP oops in pppol2tp driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080220183837.GA2881@ami.dom.local>
Date:	Wed, 20 Feb 2008 19:38:38 +0100
From:	Jarek Poplawski <jarkao2@...il.com>
To:	James Chapman <jchapman@...alix.com>
Cc:	David Miller <davem@...emloft.net>,
	Paul Mackerras <paulus@...ba.org>, netdev@...r.kernel.org
Subject: Re: [PATCH][PPPOL2TP]: Fix SMP oops in pppol2tp driver

On Wed, Feb 20, 2008 at 04:02:52PM +0000, James Chapman wrote:
...
> I tried your ppp_generic patch with only the hlist_lock bh patch in  
> pppol2tp and it seems to fix the ppp create/delete issue. However, when  
> I added much more traffic into the test (flood pings over ppp interfaces  
> while repeatedly creating/deleting the L2TP (PPP) sessions) I get a soft  
> lockup detected in pppol2tp_xmit() after anything between 1 minute and  
> an hour. :( I'm investigating that now.
>
> Thanks for your help!

Not at all!

>
>> (testing patch #1)

But I hope you tested with the fixed (take 2) version of this patch...

Since it's quite experimental (testing) this patch could be wrong
as it is, but I hope it should show the proper way to solve this
problem. Probably you did some of these, but here are a few of my
suggestions for testing this:

1) try my patch with your full bh locking changing patch;
2) add while loops to these trylocks on failure, with e.g.  __delay(1);
   this should work like full locks again, but there should be no (this
   kind of) lockdep reports;
3) I send here another testing patch with this second way to do this:
   on the write side, but it's even more "experimental" and only a
   proof of concept (should be applied on vanilla ppp_generic).

Regards,
Jarek P.

(testing patch #2)

---

 drivers/net/ppp_generic.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ppp_generic.c b/drivers/net/ppp_generic.c
index 4dc5b4b..70bd255 100644
--- a/drivers/net/ppp_generic.c
+++ b/drivers/net/ppp_generic.c
@@ -2606,11 +2606,16 @@ ppp_connect_channel(struct channel *pch, int unit)
 	ppp = ppp_find_unit(unit);
 	if (!ppp)
 		goto out;
-	write_lock_bh(&pch->upl);
+
 	ret = -EINVAL;
-	if (pch->ppp)
-		goto outl;
+	read_lock_bh(&pch->upl);
+	if (pch->ppp) {
+		read_unlock_bh(&pch->upl);
+		goto out;
+	}
+	read_unlock_bh(&pch->upl);
 
+	atomic_inc(&ppp->file.refcnt);
 	ppp_lock(ppp);
 	if (pch->file.hdrlen > ppp->file.hdrlen)
 		ppp->file.hdrlen = pch->file.hdrlen;
@@ -2619,13 +2624,14 @@ ppp_connect_channel(struct channel *pch, int unit)
 		ppp->dev->hard_header_len = hdrlen;
 	list_add_tail(&pch->clist, &ppp->channels);
 	++ppp->n_channels;
-	pch->ppp = ppp;
-	atomic_inc(&ppp->file.refcnt);
 	ppp_unlock(ppp);
-	ret = 0;
 
- outl:
+	/* avoid lock dependency with ppp_locks */
+	write_lock_bh(&pch->upl);
+	BUG_ON(pch->ppp);
+	pch->ppp = ppp;
 	write_unlock_bh(&pch->upl);
+	ret = 0;
  out:
 	mutex_unlock(&all_ppp_mutex);
 	return ret;
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html