[PATCH v2 net-next 0/2] net: ubuf_info.refcnt conversion

[PATCH v2 net-next 0/2] net: ubuf_info.refcnt conversion

[Eric Dumazet]

Yet another atomic_t -> refcount_t conversion, split in two patches.

First patch prepares the automatic conversion done in the second patch.

Eric Dumazet (2):
  net: prepare (struct ubuf_info)->refcnt conversion
  net: convert (struct ubuf_info)->refcnt to refcount_t

 drivers/vhost/net.c    |  2 +-
 include/linux/skbuff.h |  5 +++--
 net/core/skbuff.c      | 14 ++++----------
 net/ipv4/tcp.c         |  2 --
 4 files changed, 8 insertions(+), 15 deletions(-)

-- 
2.14.1.581.gf28d330327-goog

[PATCH v2 net-next 1/2] net: prepare (struct ubuf_info)->refcnt conversion

[Eric Dumazet]

In order to convert this atomic_t refcnt to refcount_t,
we need to init the refcount to one to not trigger
a 0 -> 1 transition.

This also removes one atomic operation in fast path.

v2: removed dead code in sock_zerocopy_put_abort()
as suggested by Willem.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/skbuff.c | 10 ++--------
 net/ipv4/tcp.c    |  2 --
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 917da73d3ab3b82163cf0a9ee944da09cb5a391f..1a754d7896ceac1eb85e3b13c1422b4d6a88fedf 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -963,7 +963,7 @@ struct ubuf_info *sock_zerocopy_alloc(struct sock *sk, size_t size)
 	uarg->len = 1;
 	uarg->bytelen = size;
 	uarg->zerocopy = 1;
-	atomic_set(&uarg->refcnt, 0);
+	atomic_set(&uarg->refcnt, 1);
 	sock_hold(sk);
 
 	return uarg;
@@ -1005,6 +1005,7 @@ struct ubuf_info *sock_zerocopy_realloc(struct sock *sk, size_t size,
 			uarg->len++;
 			uarg->bytelen = bytelen;
 			atomic_set(&sk->sk_zckey, ++next);
+			sock_zerocopy_get(uarg);
 			return uarg;
 		}
 	}
@@ -1102,13 +1103,6 @@ void sock_zerocopy_put_abort(struct ubuf_info *uarg)
 		atomic_dec(&sk->sk_zckey);
 		uarg->len--;
 
-		/* sock_zerocopy_put expects a ref. Most sockets take one per
-		 * skb, which is zero on abort. tcp_sendmsg holds one extra, to
-		 * avoid an skb send inside the main loop triggering uarg free.
-		 */
-		if (sk->sk_type != SOCK_STREAM)
-			atomic_inc(&uarg->refcnt);
-
 		sock_zerocopy_put(uarg);
 	}
 }
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 21ca2df274c5130a13d31a391a1408d779af34af..fff4d7bc3b8b46174e7bd0a04d7c212307e55cb5 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1190,8 +1190,6 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
 			goto out_err;
 		}
 
-		/* skb may be freed in main loop, keep extra ref on uarg */
-		sock_zerocopy_get(uarg);
 		if (!(sk_check_csum_caps(sk) && sk->sk_route_caps & NETIF_F_SG))
 			uarg->zerocopy = 0;
 	}
-- 
2.14.1.581.gf28d330327-goog

[PATCH v2 net-next 2/2] net: convert (struct ubuf_info)->refcnt to refcount_t

[Eric Dumazet]

refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.

v2: added the change in drivers/vhost/net.c as spotted
by Willem.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 drivers/vhost/net.c    | 2 +-
 include/linux/skbuff.h | 5 +++--
 net/core/skbuff.c      | 6 +++---
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index ba08b78ed630c429ccf415af69bf27f2433af4f0..8d2bcae53a2ec9ea1c876625e581bcd429abe365 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -533,7 +533,7 @@ static void handle_tx(struct vhost_net *net)
 			ubuf->callback = vhost_zerocopy_callback;
 			ubuf->ctx = nvq->ubufs;
 			ubuf->desc = nvq->upend_idx;
-			atomic_set(&ubuf->refcnt, 1);
+			refcount_set(&ubuf->refcnt, 1);
 			msg.msg_control = ubuf;
 			msg.msg_controllen = sizeof(ubuf);
 			ubufs = nvq->ubufs;
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 7594e19bce622a38dc39c054093c3da15b99b67b..316a92b45351f53709886ee0099cbc83b66f1b15 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -22,6 +22,7 @@
 #include <linux/cache.h>
 #include <linux/rbtree.h>
 #include <linux/socket.h>
+#include <linux/refcount.h>
 
 #include <linux/atomic.h>
 #include <asm/types.h>
@@ -456,7 +457,7 @@ struct ubuf_info {
 			u32 bytelen;
 		};
 	};
-	atomic_t refcnt;
+	refcount_t refcnt;
 
 	struct mmpin {
 		struct user_struct *user;
@@ -472,7 +473,7 @@ struct ubuf_info *sock_zerocopy_realloc(struct sock *sk, size_t size,
 
 static inline void sock_zerocopy_get(struct ubuf_info *uarg)
 {
-	atomic_inc(&uarg->refcnt);
+	refcount_inc(&uarg->refcnt);
 }
 
 void sock_zerocopy_put(struct ubuf_info *uarg);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 1a754d7896ceac1eb85e3b13c1422b4d6a88fedf..06b9ddca3188442d1d1d2052fc060cf5b1e2a6f4 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -963,7 +963,7 @@ struct ubuf_info *sock_zerocopy_alloc(struct sock *sk, size_t size)
 	uarg->len = 1;
 	uarg->bytelen = size;
 	uarg->zerocopy = 1;
-	atomic_set(&uarg->refcnt, 1);
+	refcount_set(&uarg->refcnt, 1);
 	sock_hold(sk);
 
 	return uarg;
@@ -1086,7 +1086,7 @@ EXPORT_SYMBOL_GPL(sock_zerocopy_callback);
 
 void sock_zerocopy_put(struct ubuf_info *uarg)
 {
-	if (uarg && atomic_dec_and_test(&uarg->refcnt)) {
+	if (uarg && refcount_dec_and_test(&uarg->refcnt)) {
 		if (uarg->callback)
 			uarg->callback(uarg, uarg->zerocopy);
 		else
@@ -1483,7 +1483,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 		if (skb_orphan_frags(skb, gfp_mask))
 			goto nofrags;
 		if (skb_zcopy(skb))
-			atomic_inc(&skb_uarg(skb)->refcnt);
+			refcount_inc(&skb_uarg(skb)->refcnt);
 		for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
 			skb_frag_ref(skb, i);
 
-- 
2.14.1.581.gf28d330327-goog

Re: [PATCH v2 net-next 1/2] net: prepare (struct ubuf_info)->refcnt conversion

[Willem de Bruijn]

On Thu, Aug 31, 2017 at 7:48 PM, Eric Dumazet <edumazet@google.com> wrote:
> In order to convert this atomic_t refcnt to refcount_t,
> we need to init the refcount to one to not trigger
> a 0 -> 1 transition.
>
> This also removes one atomic operation in fast path.
>
> v2: removed dead code in sock_zerocopy_put_abort()
> as suggested by Willem.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Acked-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH v2 net-next 2/2] net: convert (struct ubuf_info)->refcnt to refcount_t

[Willem de Bruijn]

On Thu, Aug 31, 2017 at 7:48 PM, Eric Dumazet <edumazet@google.com> wrote:
> refcount_t type and corresponding API should be
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.
>
> v2: added the change in drivers/vhost/net.c as spotted
> by Willem.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Acked-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH v2 net-next 0/2] net: ubuf_info.refcnt conversion

[Eric Dumazet]

On Thu, 2017-08-31 at 16:48 -0700, Eric Dumazet wrote:
> Yet another atomic_t -> refcount_t conversion, split in two patches.
> 
> First patch prepares the automatic conversion done in the second patch.
> 
> Eric Dumazet (2):
>   net: prepare (struct ubuf_info)->refcnt conversion
>   net: convert (struct ubuf_info)->refcnt to refcount_t
> 
>  drivers/vhost/net.c    |  2 +-
>  include/linux/skbuff.h |  5 +++--
>  net/core/skbuff.c      | 14 ++++----------
>  net/ipv4/tcp.c         |  2 --
>  4 files changed, 8 insertions(+), 15 deletions(-)
> 

David please ignore this series, I will send a V3 :)

Re: [PATCH v2 net-next 0/2] net: ubuf_info.refcnt conversion

[Eric Dumazet]

On Thu, 2017-08-31 at 17:04 -0700, Eric Dumazet wrote:
> On Thu, 2017-08-31 at 16:48 -0700, Eric Dumazet wrote:
> > Yet another atomic_t -> refcount_t conversion, split in two patches.
> > 
> > First patch prepares the automatic conversion done in the second patch.
> > 
> > Eric Dumazet (2):
> >   net: prepare (struct ubuf_info)->refcnt conversion
> >   net: convert (struct ubuf_info)->refcnt to refcount_t
> > 
> >  drivers/vhost/net.c    |  2 +-
> >  include/linux/skbuff.h |  5 +++--
> >  net/core/skbuff.c      | 14 ++++----------
> >  net/ipv4/tcp.c         |  2 --
> >  4 files changed, 8 insertions(+), 15 deletions(-)
> > 
> 
> David please ignore this series, I will send a V3 :)
> 

No need for a V3, sorry for the confusion, but we had to double check
with Willem that everything had been covered.

Please tell me if I need to resend, thanks !

Re: [PATCH v2 net-next 0/2] net: ubuf_info.refcnt conversion

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 01 Sep 2017 10:36:29 -0700

> On Thu, 2017-08-31 at 17:04 -0700, Eric Dumazet wrote:
>> On Thu, 2017-08-31 at 16:48 -0700, Eric Dumazet wrote:
>> > Yet another atomic_t -> refcount_t conversion, split in two patches.
>> > 
>> > First patch prepares the automatic conversion done in the second patch.
>> > 
>> > Eric Dumazet (2):
>> >   net: prepare (struct ubuf_info)->refcnt conversion
>> >   net: convert (struct ubuf_info)->refcnt to refcount_t
>> > 
>> >  drivers/vhost/net.c    |  2 +-
>> >  include/linux/skbuff.h |  5 +++--
>> >  net/core/skbuff.c      | 14 ++++----------
>> >  net/ipv4/tcp.c         |  2 --
>> >  4 files changed, 8 insertions(+), 15 deletions(-)
>> > 
>> 
>> David please ignore this series, I will send a V3 :)
>> 
> 
> No need for a V3, sorry for the confusion, but we had to double check
> with Willem that everything had been covered.
> 
> Please tell me if I need to resend, thanks !

Ok, series applied, thanks Eric.