[PATCH net 0/2] zerocopy fixes

[PATCH net 0/2] zerocopy fixes

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

The removal of UFO hardware offload support exposed a bug in
segmentation of zerocopy skbs.

The reference counting mechanism for msg_zerocopy was incorrectly
applied to vhost_net zerocopy packets.

The other issue observed through analysis. We do not cook skbs
with skb_zcopy(skb) but no frags, but this is possible in principle.
Correctly call skb_zcopy_clear on those.

Willem de Bruijn (2):
  skbuff: orphan frags before zerocopy clone
  skbuff: skb_copy_ubufs must release uarg even without user frags

 net/core/skbuff.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
2.15.1.620.gb9897f4670-goog

[PATCH net 1/2] skbuff: orphan frags before zerocopy clone

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Call skb_zerocopy_clone after skb_orphan_frags, to avoid duplicate
calls to skb_uarg(skb)->callback for the same data.

skb_zerocopy_clone associates skb_shinfo(skb)->uarg from frag_skb
with each segment. This is only safe for uargs that do refcounting,
which is those that pass skb_orphan_frags without dropping their
shared frags. For others, skb_orphan_frags drops the user frags and
sets the uarg to NULL, after which sock_zerocopy_clone has no effect.

Qemu hangs were reported due to duplicate vhost_net_zerocopy_callback
calls for the same data causing the vhost_net_ubuf_ref_>refcount to
drop below zero.

Link: http://lkml.kernel.org/r/<CAF=yD-LWyCD4Y0aJ9O0e_CHLR+3JOeKicRRTEVCPxgw4XOcqGQ@mail.gmail.com>
Fixes: 1f8b977ab32d ("sock: enable MSG_ZEROCOPY")
Reported-by: Andreas Hartmann <andihartmann@01019freenet.de>
Reported-by: David Hill <dhill@redhat.com>
Signed-off-by: Willem de Bruijn <willemb@google.com>

---

This fix causes skb_zerocopy_clone to be called for each frag in the
array. I will follow-up with a patch to net-next that will call both
skb_orphan_frags and skb_zerocopy_clone once per skb only.
---
 net/core/skbuff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index a592ca025fc4..edf40ac0cd07 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3654,8 +3654,6 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 
 		skb_shinfo(nskb)->tx_flags |= skb_shinfo(head_skb)->tx_flags &
 					      SKBTX_SHARED_FRAG;
-		if (skb_zerocopy_clone(nskb, head_skb, GFP_ATOMIC))
-			goto err;
 
 		while (pos < offset + len) {
 			if (i >= nfrags) {
@@ -3681,6 +3679,8 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 
 			if (unlikely(skb_orphan_frags(frag_skb, GFP_ATOMIC)))
 				goto err;
+			if (skb_zerocopy_clone(nskb, frag_skb, GFP_ATOMIC))
+				goto err;
 
 			*nskb_frag = *frag;
 			__skb_frag_ref(nskb_frag);
-- 
2.15.1.620.gb9897f4670-goog

[PATCH net 2/2] skbuff: skb_copy_ubufs must release uarg even without user frags

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

skb_copy_ubufs creates a private copy of frags[] to release its hold
on user frags, then calls uarg->callback to notify the owner.

Call uarg->callback even when no frags exist. This edge case can
happen when zerocopy_sg_from_iter finds enough room in skb_headlen
to copy all the data.

Fixes: 3ece782693c4 ("sock: skb_copy_ubufs support for compound pages")
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 net/core/skbuff.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index edf40ac0cd07..a3cb0be4c6f3 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1178,7 +1178,7 @@ int skb_copy_ubufs(struct sk_buff *skb, gfp_t gfp_mask)
 	u32 d_off;
 
 	if (!num_frags)
-		return 0;
+		goto release;
 
 	if (skb_shared(skb) || skb_unclone(skb, gfp_mask))
 		return -EINVAL;
@@ -1238,6 +1238,7 @@ int skb_copy_ubufs(struct sk_buff *skb, gfp_t gfp_mask)
 	__skb_fill_page_desc(skb, new_frags - 1, head, 0, d_off);
 	skb_shinfo(skb)->nr_frags = new_frags;
 
+release:
 	skb_zcopy_clear(skb, false);
 	return 0;
 }
-- 
2.15.1.620.gb9897f4670-goog

Re: [PATCH net 0/2] zerocopy fixes

[David Miller]

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Wed, 20 Dec 2017 17:37:48 -0500

> From: Willem de Bruijn <willemb@google.com>
> 
> The removal of UFO hardware offload support exposed a bug in
> segmentation of zerocopy skbs.
> 
> The reference counting mechanism for msg_zerocopy was incorrectly
> applied to vhost_net zerocopy packets.
> 
> The other issue observed through analysis. We do not cook skbs
> with skb_zcopy(skb) but no frags, but this is possible in principle.
> Correctly call skb_zcopy_clear on those.

Series applied and queued up for -stable.