[Patch] mqueue: fix netlink sock refcnt and skb refcnt

[Patch] mqueue: fix netlink sock refcnt and skb refcnt

[Cong Wang]

netlink_sendskb() is problematic, it releases sock refcnt
silently which could cause troubles we can call it multiple
times. info->notify_sock is a good example where we
setup once and use it to send netlink skb's for many times.
It should not hold or release any refcnt, but needs to rely
on netlink_attachskb()/netlink_detachskb() to hold/release
the corresponding refcnt.

Same for the skb attached to this sock, it is allocated once
and used for multiple times, so we should hold its refcnt
in netlink_attachskb().

At last, we need to call netlink_detachskb() to release
both refcnt's after we remove the notification.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Manfred Spraul <manfred@colorfullife.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 ipc/mqueue.c             |  1 +
 net/netlink/af_netlink.c | 25 ++++++++++---------------
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index eb1391b..8b0a0ce 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -683,6 +683,7 @@ static void remove_notification(struct mqueue_inode_info *info)
 	    info->notify.sigev_notify == SIGEV_THREAD) {
 		set_cookie(info->notify_cookie, NOTIFY_REMOVED);
 		netlink_sendskb(info->notify_sock, info->notify_cookie);
+		netlink_detachskb(info->notify_sock, info->notify_cookie);
 	}
 	put_pid(info->notify_owner);
 	put_user_ns(info->notify_user_ns);
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 5acee49..9f2d6ca 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1159,8 +1159,8 @@ static struct sk_buff *netlink_alloc_large_skb(unsigned int size,
  * all error checks are performed and memory in the queue is reserved.
  * Return values:
  * < 0: error. skb freed, reference to sock dropped.
- * 0: continue
- * 1: repeat lookup - reference dropped while waiting for socket memory.
+ * 0: continue - skb reference is held.
+ * 1: repeat lookup - sock reference dropped while waiting for socket memory.
  */
 int netlink_attachskb(struct sock *sk, struct sk_buff *skb,
 		      long *timeo, struct sock *ssk)
@@ -1198,11 +1198,12 @@ int netlink_attachskb(struct sock *sk, struct sk_buff *skb,
 		}
 		return 1;
 	}
+	skb_get(skb);
 	netlink_skb_set_owner_r(skb, sk);
 	return 0;
 }
 
-static int __netlink_sendskb(struct sock *sk, struct sk_buff *skb)
+int netlink_sendskb(struct sock *sk, struct sk_buff *skb)
 {
 	int len = skb->len;
 
@@ -1213,14 +1214,6 @@ static int __netlink_sendskb(struct sock *sk, struct sk_buff *skb)
 	return len;
 }
 
-int netlink_sendskb(struct sock *sk, struct sk_buff *skb)
-{
-	int len = __netlink_sendskb(sk, skb);
-
-	sock_put(sk);
-	return len;
-}
-
 void netlink_detachskb(struct sock *sk, struct sk_buff *skb)
 {
 	kfree_skb(skb);
@@ -1303,7 +1296,9 @@ int netlink_unicast(struct sock *ssk, struct sk_buff *skb,
 	if (err)
 		return err;
 
-	return netlink_sendskb(sk, skb);
+	err = netlink_sendskb(sk, skb);
+	netlink_detachskb(sk, skb);
+	return err;
 }
 EXPORT_SYMBOL(netlink_unicast);
 
@@ -1333,7 +1328,7 @@ static int netlink_broadcast_deliver(struct sock *sk, struct sk_buff *skb)
 	if (atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf &&
 	    !test_bit(NETLINK_S_CONGESTED, &nlk->state)) {
 		netlink_skb_set_owner_r(skb, sk);
-		__netlink_sendskb(sk, skb);
+		netlink_sendskb(sk, skb);
 		return atomic_read(&sk->sk_rmem_alloc) > (sk->sk_rcvbuf >> 1);
 	}
 	return -1;
@@ -2183,7 +2178,7 @@ static int netlink_dump(struct sock *sk)
 		if (sk_filter(sk, skb))
 			kfree_skb(skb);
 		else
-			__netlink_sendskb(sk, skb);
+			netlink_sendskb(sk, skb);
 		return 0;
 	}
 
@@ -2198,7 +2193,7 @@ static int netlink_dump(struct sock *sk)
 	if (sk_filter(sk, skb))
 		kfree_skb(skb);
 	else
-		__netlink_sendskb(sk, skb);
+		netlink_sendskb(sk, skb);
 
 	if (cb->done)
 		cb->done(cb);
-- 
2.5.5

Re: [Patch] mqueue: fix netlink sock refcnt and skb refcnt

[Cong Wang]

On Sun, Jul 9, 2017 at 10:08 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> netlink_sendskb() is problematic, it releases sock refcnt
> silently which could cause troubles we can call it multiple
> times. info->notify_sock is a good example where we
> setup once and use it to send netlink skb's for many times.
> It should not hold or release any refcnt, but needs to rely
> on netlink_attachskb()/netlink_detachskb() to hold/release
> the corresponding refcnt.
>
> Same for the skb attached to this sock, it is allocated once
> and used for multiple times, so we should hold its refcnt
> in netlink_attachskb().
>
> At last, we need to call netlink_detachskb() to release
> both refcnt's after we remove the notification.

Hmm, the info->notify_owner is NULL'ed after sending
the notification, so probably we don't put the sock refcnt
repeatly. Not sure about the skb though...

Re: [Patch] mqueue: fix netlink sock refcnt and skb refcnt

[Linus Torvalds]

This thing is definitely not cc'd to the right people:

On Sun, Jul 9, 2017 at 10:08 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Manfred Spraul <manfred@colorfullife.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Unlike your previous patch, this seems to be more of a generic netlink
interface issue, so you should primarily talk to the networking
people, I'm not sure it makes much sense to cc me/Andrew/Manfred.

             Linus

Re: [Patch] mqueue: fix netlink sock refcnt and skb refcnt

[Cong Wang]

On Mon, Jul 10, 2017 at 11:09 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> This thing is definitely not cc'd to the right people:
>
> On Sun, Jul 9, 2017 at 10:08 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>
>> Cc: Linus Torvalds <torvalds@linux-foundation.org>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Manfred Spraul <manfred@colorfullife.com>
>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>
> Unlike your previous patch, this seems to be more of a generic netlink
> interface issue, so you should primarily talk to the networking
> people, I'm not sure it makes much sense to cc me/Andrew/Manfred.
>

OK, actually this patch is not needed, because we only send notification
once, although the code itself does need some cleanup.

Sorry for the noise.