[PATCH v2 net-next 0/2] kcm: Fix two locking issues

[PATCH v2 net-next 0/2] kcm: Fix two locking issues

[Tom Herbert]

One issue is lockdep warnings when sock_owned_by_user returns true
in strparser. Fix is to add and call sock_owned_by_user_nocheck since
the check for owned by user is not an error condition in this case.

The other issue is a potential deadlock between TX and RX paths

KCM socket lock and the psock socket lock are acquired in both
the RX and TX path, however they take the locks in opposite order
which can lead to deadlock. The fix is to add try_sock_lock to see
if psock socket lock can get acquired in the TX path with KCM lock
held. If not, then KCM socket is released and the psock socket lock
and KCM socket lock are acquired in the same order as the RX path.

Tested:

Ran KCM traffic without incident.

v2: Remove patches to address potential deadlock. I couldn't convince
myself this is an issue after looking at the code some more.


Tom Herbert (2):
  sock: Add sock_owned_by_user_nocheck
  strparser: Call sock_owned_by_user_nocheck

 include/net/sock.h        | 5 +++++
 net/strparser/strparser.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

-- 
2.11.0

[PATCH v2 net-next 1/2] sock: Add sock_owned_by_user_nocheck

[Tom Herbert]

This allows checking socket lock ownership with producing lockdep
warnings.

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 include/net/sock.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/net/sock.h b/include/net/sock.h
index 6c1db823f8b9..66fd3951e6f3 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1515,6 +1515,11 @@ static inline bool sock_owned_by_user(const struct sock *sk)
 	return sk->sk_lock.owned;
 }
 
+static inline bool sock_owned_by_user_nocheck(const struct sock *sk)
+{
+	return sk->sk_lock.owned;
+}
+
 /* no reclassification while locks are held */
 static inline bool sock_allow_reclassification(const struct sock *csk)
 {
-- 
2.11.0

[PATCH v2 net-next 2/2] strparser: Call sock_owned_by_user_nocheck

[Tom Herbert]

strparser wants to check socket ownership without producing any
warnings. As indicated by the comment in the code, it is permissible
for owned_by_user to return true.

Fixes: 43a0c6751a322847 ("strparser: Stream parser for messages")
Reported-by: syzbot <syzkaller@googlegroups.com>
Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 net/strparser/strparser.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
index c5fda15ba319..1fdab5c4eda8 100644
--- a/net/strparser/strparser.c
+++ b/net/strparser/strparser.c
@@ -401,7 +401,7 @@ void strp_data_ready(struct strparser *strp)
 	 * allows a thread in BH context to safely check if the process
 	 * lock is held. In this case, if the lock is held, queue work.
 	 */
-	if (sock_owned_by_user(strp->sk)) {
+	if (sock_owned_by_user_nocheck(strp->sk)) {
 		queue_work(strp_wq, &strp->work);
 		return;
 	}
-- 
2.11.0

Re: [PATCH v2 net-next 0/2] kcm: Fix two locking issues

[David Miller]

From: Tom Herbert <tom@quantonium.net>
Date: Sat, 23 Dec 2017 09:17:14 -0800

> One issue is lockdep warnings when sock_owned_by_user returns true
> in strparser. Fix is to add and call sock_owned_by_user_nocheck since
> the check for owned by user is not an error condition in this case.
> 
> The other issue is a potential deadlock between TX and RX paths
> 
> KCM socket lock and the psock socket lock are acquired in both
> the RX and TX path, however they take the locks in opposite order
> which can lead to deadlock. The fix is to add try_sock_lock to see
> if psock socket lock can get acquired in the TX path with KCM lock
> held. If not, then KCM socket is released and the psock socket lock
> and KCM socket lock are acquired in the same order as the RX path.
> 
> Tested:
> 
> Ran KCM traffic without incident.
> 
> v2: Remove patches to address potential deadlock. I couldn't convince
> myself this is an issue after looking at the code some more.

If this fixes real locking bugs you should target them at 'net' not
'net-next'.

I also see you telling some people hitting kcm locking problems to
test "the patch" but you give them no idea what patch you are even
talking about.

Is it this series?  Nobody knows.

Please poing them at exactly what patch you want them to test, get
their testing results, and add appropriate Tested-by: tags as you
respin this for 'net'.

Thanks.