* [PATCH 0/5] skbuff: fix skb_segment with zero copy skbs
@ 2014-03-10 16:29 Michael S. Tsirkin
2014-03-10 16:29 ` [PATCH 1/5] skbuff: skb_segment: s/frag/nskb_frag/ Michael S. Tsirkin
` (5 more replies)
0 siblings, 6 replies; 20+ messages in thread
From: Michael S. Tsirkin @ 2014-03-10 16:29 UTC (permalink / raw
To: linux-kernel; +Cc: jbenc, netdev, David Miller
This fixes a bug in skb_segment where it moves frags
between skbs without orphaning them.
This causes userspace to assume it's safe to
reuse the buffer, and receiver gets corrupted data.
This further might leak information from the
transmitter on the wire.
To fix track which skb does a copied frag belong
to, and orphan frags when copying them.
As we are tracking multiple skbs here, using
short names (skb,nskb,fskb,skb_frag,frag) becomes confusing.
So before adding another one, I refactor these names
slightly.
Patch is split out to make it easier to
verify that all trasformations are trivially correct.
The problem was observed in the field,
so I think that the patch is necessary on stable
as well.
Michael S. Tsirkin (5):
skbuff: skb_segment: s/frag/nskb_frag/
skbuff: skb_segment: s/skb_frag/frag/
skbuff: skb_segment: s/skb/head_skb/
skbuff: skb_segment: s/fskb/list_skb/
skbuff: skb_segment: orphan frags before copying
net/core/skbuff.c | 100 +++++++++++++++++++++++++++++-------------------------
1 file changed, 54 insertions(+), 46 deletions(-)
--
MST
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/5] skbuff: skb_segment: s/frag/nskb_frag/
2014-03-10 16:29 [PATCH 0/5] skbuff: fix skb_segment with zero copy skbs Michael S. Tsirkin
@ 2014-03-10 16:29 ` Michael S. Tsirkin
2014-03-10 16:29 ` [PATCH 2/5] skbuff: skb_segment: s/skb_frag/frag/ Michael S. Tsirkin
` (4 subsequent siblings)
5 siblings, 0 replies; 20+ messages in thread
From: Michael S. Tsirkin @ 2014-03-10 16:29 UTC (permalink / raw
To: linux-kernel
Cc: jbenc, David Miller, Eric Dumazet, Daniel Borkmann, Simon Horman,
Paul Durrant, Thomas Graf, Herbert Xu, Miklos Szeredi,
=?UTF-8?q?Peter=20Pan=28=E6=BD=98=E5=8D=AB=E5=B9=B3=29?=, netdev
frag points at nskb, so name it appropriately
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
net/core/skbuff.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 5976ef0..3bc6d75 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2879,7 +2879,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
do {
struct sk_buff *nskb;
- skb_frag_t *frag;
+ skb_frag_t *nskb_frag;
int hsize;
int size;
@@ -2972,7 +2972,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
continue;
}
- frag = skb_shinfo(nskb)->frags;
+ nskb_frag = skb_shinfo(nskb)->frags;
skb_copy_from_linear_data_offset(skb, offset,
skb_put(nskb, hsize), hsize);
@@ -3000,13 +3000,13 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
goto err;
}
- *frag = *skb_frag;
- __skb_frag_ref(frag);
- size = skb_frag_size(frag);
+ *nskb_frag = *skb_frag;
+ __skb_frag_ref(nskb_frag);
+ size = skb_frag_size(nskb_frag);
if (pos < offset) {
- frag->page_offset += offset - pos;
- skb_frag_size_sub(frag, offset - pos);
+ nskb_frag->page_offset += offset - pos;
+ skb_frag_size_sub(nskb_frag, offset - pos);
}
skb_shinfo(nskb)->nr_frags++;
@@ -3016,11 +3016,11 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
skb_frag++;
pos += size;
} else {
- skb_frag_size_sub(frag, pos + size - (offset + len));
+ skb_frag_size_sub(nskb_frag, pos + size - (offset + len));
goto skip_fraglist;
}
- frag++;
+ nskb_frag++;
}
skip_fraglist:
--
MST
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/5] skbuff: skb_segment: s/skb_frag/frag/
2014-03-10 16:29 [PATCH 0/5] skbuff: fix skb_segment with zero copy skbs Michael S. Tsirkin
2014-03-10 16:29 ` [PATCH 1/5] skbuff: skb_segment: s/frag/nskb_frag/ Michael S. Tsirkin
@ 2014-03-10 16:29 ` Michael S. Tsirkin
2014-03-10 16:29 ` [PATCH 3/5] skbuff: skb_segment: s/skb/head_skb/ Michael S. Tsirkin
` (3 subsequent siblings)
5 siblings, 0 replies; 20+ messages in thread
From: Michael S. Tsirkin @ 2014-03-10 16:29 UTC (permalink / raw
To: linux-kernel
Cc: jbenc, David Miller, Eric Dumazet, Daniel Borkmann, Simon Horman,
Paul Durrant, Thomas Graf, Herbert Xu, Miklos Szeredi,
=?UTF-8?q?Peter=20Pan=28=E6=BD=98=E5=8D=AB=E5=B9=B3=29?=, netdev
skb_frag can in fact point at either skb
or fskb so rename it generally "frag".
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
net/core/skbuff.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 3bc6d75..917cb60 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2853,7 +2853,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
struct sk_buff *segs = NULL;
struct sk_buff *tail = NULL;
struct sk_buff *fskb = skb_shinfo(skb)->frag_list;
- skb_frag_t *skb_frag = skb_shinfo(skb)->frags;
+ skb_frag_t *frag = skb_shinfo(skb)->frags;
unsigned int mss = skb_shinfo(skb)->gso_size;
unsigned int doffset = skb->data - skb_mac_header(skb);
unsigned int offset = doffset;
@@ -2899,19 +2899,19 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
i = 0;
nfrags = skb_shinfo(fskb)->nr_frags;
- skb_frag = skb_shinfo(fskb)->frags;
+ frag = skb_shinfo(fskb)->frags;
pos += skb_headlen(fskb);
while (pos < offset + len) {
BUG_ON(i >= nfrags);
- size = skb_frag_size(skb_frag);
+ size = skb_frag_size(frag);
if (pos + size > offset + len)
break;
i++;
pos += size;
- skb_frag++;
+ frag++;
}
nskb = skb_clone(fskb, GFP_ATOMIC);
@@ -2985,7 +2985,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
i = 0;
nfrags = skb_shinfo(fskb)->nr_frags;
- skb_frag = skb_shinfo(fskb)->frags;
+ frag = skb_shinfo(fskb)->frags;
BUG_ON(!nfrags);
@@ -3000,7 +3000,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
goto err;
}
- *nskb_frag = *skb_frag;
+ *nskb_frag = *frag;
__skb_frag_ref(nskb_frag);
size = skb_frag_size(nskb_frag);
@@ -3013,7 +3013,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
if (pos + size <= offset + len) {
i++;
- skb_frag++;
+ frag++;
pos += size;
} else {
skb_frag_size_sub(nskb_frag, pos + size - (offset + len));
--
MST
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/5] skbuff: skb_segment: s/skb/head_skb/
2014-03-10 16:29 [PATCH 0/5] skbuff: fix skb_segment with zero copy skbs Michael S. Tsirkin
2014-03-10 16:29 ` [PATCH 1/5] skbuff: skb_segment: s/frag/nskb_frag/ Michael S. Tsirkin
2014-03-10 16:29 ` [PATCH 2/5] skbuff: skb_segment: s/skb_frag/frag/ Michael S. Tsirkin
@ 2014-03-10 16:29 ` Michael S. Tsirkin
2014-03-10 17:27 ` [PATCH 4/5] skbuff: skb_segment: s/fskb/list_skb/ Michael S. Tsirkin
` (2 subsequent siblings)
5 siblings, 0 replies; 20+ messages in thread
From: Michael S. Tsirkin @ 2014-03-10 16:29 UTC (permalink / raw
To: linux-kernel
Cc: jbenc, David Miller, Eric Dumazet, Daniel Borkmann, Simon Horman,
Paul Durrant, Thomas Graf, Herbert Xu, Miklos Szeredi,
=?UTF-8?q?Peter=20Pan=28=E6=BD=98=E5=8D=AB=E5=B9=B3=29?=, netdev
rename local variable to make it easier to tell at a glance that we are
dealing with a head skb.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
net/core/skbuff.c | 46 ++++++++++++++++++++++++----------------------
1 file changed, 24 insertions(+), 22 deletions(-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 917cb60..af386e1 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2841,41 +2841,42 @@ EXPORT_SYMBOL_GPL(skb_pull_rcsum);
/**
* skb_segment - Perform protocol segmentation on skb.
- * @skb: buffer to segment
+ * @head_skb: buffer to segment
* @features: features for the output path (see dev->features)
*
* This function performs segmentation on the given skb. It returns
* a pointer to the first in a list of new skbs for the segments.
* In case of error it returns ERR_PTR(err).
*/
-struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
+struct sk_buff *skb_segment(struct sk_buff *head_skb,
+ netdev_features_t features)
{
struct sk_buff *segs = NULL;
struct sk_buff *tail = NULL;
- struct sk_buff *fskb = skb_shinfo(skb)->frag_list;
- skb_frag_t *frag = skb_shinfo(skb)->frags;
- unsigned int mss = skb_shinfo(skb)->gso_size;
- unsigned int doffset = skb->data - skb_mac_header(skb);
+ struct sk_buff *fskb = skb_shinfo(head_skb)->frag_list;
+ skb_frag_t *frag = skb_shinfo(head_skb)->frags;
+ unsigned int mss = skb_shinfo(head_skb)->gso_size;
+ unsigned int doffset = head_skb->data - skb_mac_header(head_skb);
unsigned int offset = doffset;
- unsigned int tnl_hlen = skb_tnl_header_len(skb);
+ unsigned int tnl_hlen = skb_tnl_header_len(head_skb);
unsigned int headroom;
unsigned int len;
__be16 proto;
bool csum;
int sg = !!(features & NETIF_F_SG);
- int nfrags = skb_shinfo(skb)->nr_frags;
+ int nfrags = skb_shinfo(head_skb)->nr_frags;
int err = -ENOMEM;
int i = 0;
int pos;
- proto = skb_network_protocol(skb);
+ proto = skb_network_protocol(head_skb);
if (unlikely(!proto))
return ERR_PTR(-EINVAL);
csum = !!can_checksum_protocol(features, proto);
- __skb_push(skb, doffset);
- headroom = skb_headroom(skb);
- pos = skb_headlen(skb);
+ __skb_push(head_skb, doffset);
+ headroom = skb_headroom(head_skb);
+ pos = skb_headlen(head_skb);
do {
struct sk_buff *nskb;
@@ -2883,11 +2884,11 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
int hsize;
int size;
- len = skb->len - offset;
+ len = head_skb->len - offset;
if (len > mss)
len = mss;
- hsize = skb_headlen(skb) - offset;
+ hsize = skb_headlen(head_skb) - offset;
if (hsize < 0)
hsize = 0;
if (hsize > len || !sg)
@@ -2936,7 +2937,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
__skb_push(nskb, doffset);
} else {
nskb = __alloc_skb(hsize + doffset + headroom,
- GFP_ATOMIC, skb_alloc_rx_flag(skb),
+ GFP_ATOMIC, skb_alloc_rx_flag(head_skb),
NUMA_NO_NODE);
if (unlikely(!nskb))
@@ -2952,12 +2953,12 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
segs = nskb;
tail = nskb;
- __copy_skb_header(nskb, skb);
- nskb->mac_len = skb->mac_len;
+ __copy_skb_header(nskb, head_skb);
+ nskb->mac_len = head_skb->mac_len;
skb_headers_offset_update(nskb, skb_headroom(nskb) - headroom);
- skb_copy_from_linear_data_offset(skb, -tnl_hlen,
+ skb_copy_from_linear_data_offset(head_skb, -tnl_hlen,
nskb->data - tnl_hlen,
doffset + tnl_hlen);
@@ -2966,7 +2967,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
if (!sg) {
nskb->ip_summed = CHECKSUM_NONE;
- nskb->csum = skb_copy_and_csum_bits(skb, offset,
+ nskb->csum = skb_copy_and_csum_bits(head_skb, offset,
skb_put(nskb, len),
len, 0);
continue;
@@ -2974,10 +2975,11 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
nskb_frag = skb_shinfo(nskb)->frags;
- skb_copy_from_linear_data_offset(skb, offset,
+ skb_copy_from_linear_data_offset(head_skb, offset,
skb_put(nskb, hsize), hsize);
- skb_shinfo(nskb)->tx_flags = skb_shinfo(skb)->tx_flags & SKBTX_SHARED_FRAG;
+ skb_shinfo(nskb)->tx_flags = skb_shinfo(head_skb)->tx_flags &
+ SKBTX_SHARED_FRAG;
while (pos < offset + len) {
if (i >= nfrags) {
@@ -3034,7 +3036,7 @@ perform_csum_check:
nskb->len - doffset, 0);
nskb->ip_summed = CHECKSUM_NONE;
}
- } while ((offset += len) < skb->len);
+ } while ((offset += len) < head_skb->len);
return segs;
--
MST
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 4/5] skbuff: skb_segment: s/fskb/list_skb/
2014-03-10 16:29 [PATCH 0/5] skbuff: fix skb_segment with zero copy skbs Michael S. Tsirkin
` (2 preceding siblings ...)
2014-03-10 16:29 ` [PATCH 3/5] skbuff: skb_segment: s/skb/head_skb/ Michael S. Tsirkin
@ 2014-03-10 17:27 ` Michael S. Tsirkin
2014-03-10 17:40 ` David Miller
2014-03-10 17:28 ` [PATCH 5/5] skbuff: skb_segment: orphan frags before copying Michael S. Tsirkin
2014-03-11 20:27 ` [PATCH 0/5] skbuff: fix skb_segment with zero copy skbs David Miller
5 siblings, 1 reply; 20+ messages in thread
From: Michael S. Tsirkin @ 2014-03-10 17:27 UTC (permalink / raw
To: linux-kernel
Cc: jbenc, David Miller, Eric Dumazet, Daniel Borkmann, Simon Horman,
Paul Durrant, Thomas Graf, Herbert Xu, Miklos Szeredi,
=?us-ascii?B?PT9VVEYtOD9xP1BldGVyPTIwUGFuPTI4PUU2PUJEPTk4PUU1PThEPUFC?= =?us-ascii?B?PUU1PUI5PUIzPTI5Pz0=?=,
netdev
fskb is unrelated to frag: it's coming from
frag_list. Rename it list_skb to avoid confusion.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
net/core/skbuff.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index af386e1..64ee4e6 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2853,7 +2853,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
{
struct sk_buff *segs = NULL;
struct sk_buff *tail = NULL;
- struct sk_buff *fskb = skb_shinfo(head_skb)->frag_list;
+ struct sk_buff *list_skb = skb_shinfo(head_skb)->frag_list;
skb_frag_t *frag = skb_shinfo(head_skb)->frags;
unsigned int mss = skb_shinfo(head_skb)->gso_size;
unsigned int doffset = head_skb->data - skb_mac_header(head_skb);
@@ -2894,14 +2894,14 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
if (hsize > len || !sg)
hsize = len;
- if (!hsize && i >= nfrags && skb_headlen(fskb) &&
- (skb_headlen(fskb) == len || sg)) {
- BUG_ON(skb_headlen(fskb) > len);
+ if (!hsize && i >= nfrags && skb_headlen(list_skb) &&
+ (skb_headlen(list_skb) == len || sg)) {
+ BUG_ON(skb_headlen(list_skb) > len);
i = 0;
- nfrags = skb_shinfo(fskb)->nr_frags;
- frag = skb_shinfo(fskb)->frags;
- pos += skb_headlen(fskb);
+ nfrags = skb_shinfo(list_skb)->nr_frags;
+ frag = skb_shinfo(list_skb)->frags;
+ pos += skb_headlen(list_skb);
while (pos < offset + len) {
BUG_ON(i >= nfrags);
@@ -2915,8 +2915,8 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
frag++;
}
- nskb = skb_clone(fskb, GFP_ATOMIC);
- fskb = fskb->next;
+ nskb = skb_clone(list_skb, GFP_ATOMIC);
+ list_skb = list_skb->next;
if (unlikely(!nskb))
goto err;
@@ -2983,15 +2983,15 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
while (pos < offset + len) {
if (i >= nfrags) {
- BUG_ON(skb_headlen(fskb));
+ BUG_ON(skb_headlen(list_skb));
i = 0;
- nfrags = skb_shinfo(fskb)->nr_frags;
- frag = skb_shinfo(fskb)->frags;
+ nfrags = skb_shinfo(list_skb)->nr_frags;
+ frag = skb_shinfo(list_skb)->frags;
BUG_ON(!nfrags);
- fskb = fskb->next;
+ list_skb = list_skb->next;
}
if (unlikely(skb_shinfo(nskb)->nr_frags >=
--
MST
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 5/5] skbuff: skb_segment: orphan frags before copying
2014-03-10 16:29 [PATCH 0/5] skbuff: fix skb_segment with zero copy skbs Michael S. Tsirkin
` (3 preceding siblings ...)
2014-03-10 17:27 ` [PATCH 4/5] skbuff: skb_segment: s/fskb/list_skb/ Michael S. Tsirkin
@ 2014-03-10 17:28 ` Michael S. Tsirkin
2014-03-10 21:34 ` Herbert Xu
2014-03-11 20:27 ` [PATCH 0/5] skbuff: fix skb_segment with zero copy skbs David Miller
5 siblings, 1 reply; 20+ messages in thread
From: Michael S. Tsirkin @ 2014-03-10 17:28 UTC (permalink / raw
To: linux-kernel
Cc: jbenc, David Miller, Eric Dumazet, Daniel Borkmann, Simon Horman,
Paul Durrant, Thomas Graf, Herbert Xu, Miklos Szeredi,
=?us-ascii?B?PT9VVEYtOD9xP1BldGVyPTIwUGFuPTI4PUU2PUJEPTk4PUU1PThEPUFC?= =?us-ascii?B?PUU1PUI5PUIzPTI5Pz0=?=,
netdev
skb_segment copies frags around, so we need
to copy them carefully to avoid accessing
user memory after reporting completion to userspace
through a callback.
skb_segment doesn't normally happen on datapath:
TSO needs to be disabled - so disabling zero copy
in this case does not look like a big deal.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
net/core/skbuff.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 64ee4e6..ae11f78 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2857,6 +2857,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
skb_frag_t *frag = skb_shinfo(head_skb)->frags;
unsigned int mss = skb_shinfo(head_skb)->gso_size;
unsigned int doffset = head_skb->data - skb_mac_header(head_skb);
+ struct sk_buff *frag_skb = head_skb;
unsigned int offset = doffset;
unsigned int tnl_hlen = skb_tnl_header_len(head_skb);
unsigned int headroom;
@@ -2901,6 +2902,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
i = 0;
nfrags = skb_shinfo(list_skb)->nr_frags;
frag = skb_shinfo(list_skb)->frags;
+ frag_skb = list_skb;
pos += skb_headlen(list_skb);
while (pos < offset + len) {
@@ -2988,6 +2990,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
i = 0;
nfrags = skb_shinfo(list_skb)->nr_frags;
frag = skb_shinfo(list_skb)->frags;
+ frag_skb = list_skb;
BUG_ON(!nfrags);
@@ -3002,6 +3005,9 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
goto err;
}
+ if (unlikely(skb_orphan_frags(frag_skb, GFP_ATOMIC)))
+ goto err;
+
*nskb_frag = *frag;
__skb_frag_ref(nskb_frag);
size = skb_frag_size(nskb_frag);
--
MST
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 4/5] skbuff: skb_segment: s/fskb/list_skb/
2014-03-10 17:27 ` [PATCH 4/5] skbuff: skb_segment: s/fskb/list_skb/ Michael S. Tsirkin
@ 2014-03-10 17:40 ` David Miller
2014-03-10 17:43 ` David Miller
0 siblings, 1 reply; 20+ messages in thread
From: David Miller @ 2014-03-10 17:40 UTC (permalink / raw
To: mst
Cc: linux-kernel, jbenc, edumazet, dborkman, horms, Paul.Durrant,
tgraf, herbert, mszeredi, panweiping3, netdev
As a netdev reader, I don't see patches 1-3, did you post them somewhere else
and not CC: netdev?
If so, please don't do that without at least making a mention of what's going
on in the 0/N posting.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/5] skbuff: skb_segment: s/fskb/list_skb/
2014-03-10 17:40 ` David Miller
@ 2014-03-10 17:43 ` David Miller
2014-03-10 17:56 ` Michael S. Tsirkin
0 siblings, 1 reply; 20+ messages in thread
From: David Miller @ 2014-03-10 17:43 UTC (permalink / raw
To: mst
Cc: linux-kernel, jbenc, edumazet, dborkman, horms, Paul.Durrant,
tgraf, herbert, mszeredi, panweiping3, netdev
From: David Miller <davem@davemloft.net>
Date: Mon, 10 Mar 2014 13:40:54 -0400 (EDT)
>
> As a netdev reader, I don't see patches 1-3, did you post them somewhere else
> and not CC: netdev?
>
> If so, please don't do that without at least making a mention of what's going
> on in the 0/N posting.
Nevermind, I see them as older postings in my queue.
Are you using the commit date as the date for your outgoing emails?
Don't do that, it causes a patch series get chopped up and intertwined
with other ones in patchwork and therefore makes more work for me.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/5] skbuff: skb_segment: s/fskb/list_skb/
2014-03-10 17:43 ` David Miller
@ 2014-03-10 17:56 ` Michael S. Tsirkin
2014-03-10 20:45 ` David Miller
0 siblings, 1 reply; 20+ messages in thread
From: Michael S. Tsirkin @ 2014-03-10 17:56 UTC (permalink / raw
To: David Miller
Cc: linux-kernel, jbenc, edumazet, dborkman, horms, Paul.Durrant,
tgraf, herbert, mszeredi, panweiping3, netdev
On Mon, Mar 10, 2014 at 01:43:59PM -0400, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Mon, 10 Mar 2014 13:40:54 -0400 (EDT)
>
> >
> > As a netdev reader, I don't see patches 1-3, did you post them somewhere else
> > and not CC: netdev?
> >
> > If so, please don't do that without at least making a mention of what's going
> > on in the 0/N posting.
>
> Nevermind, I see them as older postings in my queue.
>
> Are you using the commit date as the date for your outgoing emails?
I don't really know, I just used git send-email to prepare them.
I saw a large delay after I sent them all until patches 4/5
appeared on list. Could just be some hiccup in the smtp server
(using ssmtp).
> Don't do that, it causes a patch series get chopped up and intertwined
> with other ones in patchwork and therefore makes more work for me.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/5] skbuff: skb_segment: s/fskb/list_skb/
2014-03-10 17:56 ` Michael S. Tsirkin
@ 2014-03-10 20:45 ` David Miller
0 siblings, 0 replies; 20+ messages in thread
From: David Miller @ 2014-03-10 20:45 UTC (permalink / raw
To: mst
Cc: linux-kernel, jbenc, edumazet, dborkman, horms, Paul.Durrant,
tgraf, herbert, mszeredi, panweiping3, netdev
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 10 Mar 2014 19:56:06 +0200
> On Mon, Mar 10, 2014 at 01:43:59PM -0400, David Miller wrote:
>> From: David Miller <davem@davemloft.net>
>> Date: Mon, 10 Mar 2014 13:40:54 -0400 (EDT)
>>
>> >
>> > As a netdev reader, I don't see patches 1-3, did you post them somewhere else
>> > and not CC: netdev?
>> >
>> > If so, please don't do that without at least making a mention of what's going
>> > on in the 0/N posting.
>>
>> Nevermind, I see them as older postings in my queue.
>>
>> Are you using the commit date as the date for your outgoing emails?
>
> I don't really know, I just used git send-email to prepare them.
>
> I saw a large delay after I sent them all until patches 4/5
> appeared on list. Could just be some hiccup in the smtp server
> (using ssmtp).
Yeah that sounds like a better explanation then.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] skbuff: skb_segment: orphan frags before copying
2014-03-10 17:28 ` [PATCH 5/5] skbuff: skb_segment: orphan frags before copying Michael S. Tsirkin
@ 2014-03-10 21:34 ` Herbert Xu
2014-03-10 21:52 ` Michael S. Tsirkin
0 siblings, 1 reply; 20+ messages in thread
From: Herbert Xu @ 2014-03-10 21:34 UTC (permalink / raw
To: Michael S. Tsirkin
Cc: linux-kernel, jbenc, David Miller, Eric Dumazet, Daniel Borkmann,
Simon Horman, Paul Durrant, Thomas Graf, Miklos Szeredi,
=?UTF-8?q?Peter=20Pan=28=E6=BD=98=E5=8D=AB=E5=B9=B3=29?=, netdev
On Mon, Mar 10, 2014 at 06:29:36PM +0200, Michael S. Tsirkin wrote:
> skb_segment copies frags around, so we need
> to copy them carefully to avoid accessing
> user memory after reporting completion to userspace
> through a callback.
>
> skb_segment doesn't normally happen on datapath:
> TSO needs to be disabled - so disabling zero copy
> in this case does not look like a big deal.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Why do we need this patch at all? Contrary to what you said,
this path is definitely performance-critical since the basis
of being able to enable GSO by default is that it causes no
regressions in non-TSO cases.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] skbuff: skb_segment: orphan frags before copying
2014-03-10 21:34 ` Herbert Xu
@ 2014-03-10 21:52 ` Michael S. Tsirkin
2014-03-10 21:56 ` Herbert Xu
0 siblings, 1 reply; 20+ messages in thread
From: Michael S. Tsirkin @ 2014-03-10 21:52 UTC (permalink / raw
To: Herbert Xu
Cc: linux-kernel, jbenc, David Miller, Eric Dumazet, Daniel Borkmann,
Simon Horman, Paul Durrant, Thomas Graf, Miklos Szeredi,
=?UTF-8?q?Peter=20Pan=28=E6=BD=98=E5=8D=AB=E5=B9=B3=29?=, netdev
On Tue, Mar 11, 2014 at 05:34:05AM +0800, Herbert Xu wrote:
> On Mon, Mar 10, 2014 at 06:29:36PM +0200, Michael S. Tsirkin wrote:
> > skb_segment copies frags around, so we need
> > to copy them carefully to avoid accessing
> > user memory after reporting completion to userspace
> > through a callback.
> >
> > skb_segment doesn't normally happen on datapath:
> > TSO needs to be disabled - so disabling zero copy
> > in this case does not look like a big deal.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Why do we need this patch at all?
The cover letter has more detail:
skb_segment ... moves frags
between skbs without orphaning them.
This causes userspace to assume it's safe to
reuse the buffer, and receiver gets corrupted data.
This further might leak information from the
transmitter on the wire.
if still unclear, pls let me know.
> Contrary to what you said,
> this path is definitely performance-critical since the basis
> of being able to enable GSO by default is that it causes no
> regressions in non-TSO cases.
>
> Cheers,
Well it's a single non taken branch.
If the branch is taken it's more expensive, but once we
take it once callback is invoked and zero copy is
disabled for future users, so it will be non taken from here on.
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] skbuff: skb_segment: orphan frags before copying
2014-03-10 21:52 ` Michael S. Tsirkin
@ 2014-03-10 21:56 ` Herbert Xu
2014-03-10 22:07 ` Michael S. Tsirkin
0 siblings, 1 reply; 20+ messages in thread
From: Herbert Xu @ 2014-03-10 21:56 UTC (permalink / raw
To: Michael S. Tsirkin
Cc: linux-kernel, jbenc, David Miller, Eric Dumazet, Daniel Borkmann,
Simon Horman, Paul Durrant, Thomas Graf, Miklos Szeredi,
=?UTF-8?q?Peter=20Pan=28=E6=BD=98=E5=8D=AB=E5=B9=B3=29?=, netdev
On Mon, Mar 10, 2014 at 11:52:36PM +0200, Michael S. Tsirkin wrote:
>
> The cover letter has more detail:
For some reason I didn't receive this cover letter.
> skb_segment ... moves frags
> between skbs without orphaning them.
> This causes userspace to assume it's safe to
> reuse the buffer, and receiver gets corrupted data.
> This further might leak information from the
> transmitter on the wire.
>
> if still unclear, pls let me know.
Why can't we deal with this by simply postponing the copy until
later? IOW if we pass along SKBTX_SHARED_FRAG will it work?
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] skbuff: skb_segment: orphan frags before copying
2014-03-10 21:56 ` Herbert Xu
@ 2014-03-10 22:07 ` Michael S. Tsirkin
2014-03-10 22:12 ` Herbert Xu
0 siblings, 1 reply; 20+ messages in thread
From: Michael S. Tsirkin @ 2014-03-10 22:07 UTC (permalink / raw
To: Herbert Xu
Cc: linux-kernel, jbenc, David Miller, Eric Dumazet, Daniel Borkmann,
Simon Horman, Paul Durrant, Thomas Graf, Miklos Szeredi,
=?UTF-8?q?Peter=20Pan=28=E6=BD=98=E5=8D=AB=E5=B9=B3=29?=, netdev
On Tue, Mar 11, 2014 at 05:56:40AM +0800, Herbert Xu wrote:
> On Mon, Mar 10, 2014 at 11:52:36PM +0200, Michael S. Tsirkin wrote:
> >
> > The cover letter has more detail:
>
> For some reason I didn't receive this cover letter.
> > skb_segment ... moves frags
> > between skbs without orphaning them.
> > This causes userspace to assume it's safe to
> > reuse the buffer, and receiver gets corrupted data.
> > This further might leak information from the
> > transmitter on the wire.
> >
> > if still unclear, pls let me know.
>
> Why can't we deal with this by simply postponing the copy until
> later?
>
> Thanks,
Once one skb completes the callback is invoked and userspace
reuses this buffer for something else.
At that point it's too late to do the copy.
> IOW if we pass along SKBTX_SHARED_FRAG will it work?
I don't see how would SKBTX_SHARED_FRAG help with this at all.
That only works for pages gifted to kernel by e.g. vmsplice
that aren't reused by userspace.
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] skbuff: skb_segment: orphan frags before copying
2014-03-10 22:07 ` Michael S. Tsirkin
@ 2014-03-10 22:12 ` Herbert Xu
2014-03-10 22:30 ` Michael S. Tsirkin
0 siblings, 1 reply; 20+ messages in thread
From: Herbert Xu @ 2014-03-10 22:12 UTC (permalink / raw
To: Michael S. Tsirkin
Cc: linux-kernel, jbenc, David Miller, Eric Dumazet, Daniel Borkmann,
Simon Horman, Paul Durrant, Thomas Graf, Miklos Szeredi,
=?UTF-8?q?Peter=20Pan=28=E6=BD=98=E5=8D=AB=E5=B9=B3=29?=, netdev
On Tue, Mar 11, 2014 at 12:07:17AM +0200, Michael S. Tsirkin wrote:
>
> Once one skb completes the callback is invoked and userspace
> reuses this buffer for something else.
> At that point it's too late to do the copy.
OK, would it be possible to delay the callback until all generated
skbs are completed?
> > IOW if we pass along SKBTX_SHARED_FRAG will it work?
>
> I don't see how would SKBTX_SHARED_FRAG help with this at all.
> That only works for pages gifted to kernel by e.g. vmsplice
> that aren't reused by userspace.
Sorry cut-n-paste error, I meant SKBTX_DEV_ZEROCOPY.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] skbuff: skb_segment: orphan frags before copying
2014-03-10 22:12 ` Herbert Xu
@ 2014-03-10 22:30 ` Michael S. Tsirkin
2014-03-11 0:39 ` Herbert Xu
0 siblings, 1 reply; 20+ messages in thread
From: Michael S. Tsirkin @ 2014-03-10 22:30 UTC (permalink / raw
To: Herbert Xu
Cc: linux-kernel, jbenc, David Miller, Eric Dumazet, Daniel Borkmann,
Simon Horman, Paul Durrant, Thomas Graf, Miklos Szeredi,
=?UTF-8?q?Peter=20Pan=28=E6=BD=98=E5=8D=AB=E5=B9=B3=29?=, netdev
On Tue, Mar 11, 2014 at 06:12:23AM +0800, Herbert Xu wrote:
> On Tue, Mar 11, 2014 at 12:07:17AM +0200, Michael S. Tsirkin wrote:
> >
> > Once one skb completes the callback is invoked and userspace
> > reuses this buffer for something else.
> > At that point it's too late to do the copy.
>
> OK, would it be possible to delay the callback until all generated
> skbs are completed?
Hmm this won't be easy since devices can complete them in any order.
See below for some ideas.
> > > IOW if we pass along SKBTX_SHARED_FRAG will it work?
> >
> > I don't see how would SKBTX_SHARED_FRAG help with this at all.
> > That only works for pages gifted to kernel by e.g. vmsplice
> > that aren't reused by userspace.
>
> Sorry cut-n-paste error, I meant SKBTX_DEV_ZEROCOPY.
>
> Cheers,
Ah ok. Well we'd need to reference-count them somehow.
Maybe skb_get and stick the pointer in ubuf callback for the
new skbs, make ubuf callback kfree_skb?
Not sure if that's too much memory ...
I also worry a bit that we'll lose the success flag value which is used
to disable zero copy on OOM errors more aggressively.
How about we do the simple thing as the first step and try to optimize
as a separate patch on top?
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] skbuff: skb_segment: orphan frags before copying
2014-03-10 22:30 ` Michael S. Tsirkin
@ 2014-03-11 0:39 ` Herbert Xu
2014-03-11 6:41 ` Michael S. Tsirkin
0 siblings, 1 reply; 20+ messages in thread
From: Herbert Xu @ 2014-03-11 0:39 UTC (permalink / raw
To: Michael S. Tsirkin
Cc: linux-kernel, jbenc, David Miller, Eric Dumazet, Daniel Borkmann,
Simon Horman, Paul Durrant, Thomas Graf, Miklos Szeredi,
=?UTF-8?q?Peter=20Pan=28=E6=BD=98=E5=8D=AB=E5=B9=B3=29?=, netdev
On Tue, Mar 11, 2014 at 12:30:01AM +0200, Michael S. Tsirkin wrote:
>
> How about we do the simple thing as the first step and try to optimize
> as a separate patch on top?
Sure let's do that.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] skbuff: skb_segment: orphan frags before copying
2014-03-11 0:39 ` Herbert Xu
@ 2014-03-11 6:41 ` Michael S. Tsirkin
2014-03-11 7:21 ` Herbert Xu
0 siblings, 1 reply; 20+ messages in thread
From: Michael S. Tsirkin @ 2014-03-11 6:41 UTC (permalink / raw
To: Herbert Xu
Cc: linux-kernel, jbenc, David Miller, Eric Dumazet, Daniel Borkmann,
Simon Horman, Paul Durrant, Thomas Graf, Miklos Szeredi,
=?UTF-8?q?Peter=20Pan=28=E6=BD=98=E5=8D=AB=E5=B9=B3=29?=, netdev
On Tue, Mar 11, 2014 at 08:39:41AM +0800, Herbert Xu wrote:
> On Tue, Mar 11, 2014 at 12:30:01AM +0200, Michael S. Tsirkin wrote:
> >
> > How about we do the simple thing as the first step and try to optimize
> > as a separate patch on top?
>
> Sure let's do that.
>
> Cheers,
Could you send an ack to my patchset then?
Or do you have something else in mind?
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/5] skbuff: skb_segment: orphan frags before copying
2014-03-11 6:41 ` Michael S. Tsirkin
@ 2014-03-11 7:21 ` Herbert Xu
0 siblings, 0 replies; 20+ messages in thread
From: Herbert Xu @ 2014-03-11 7:21 UTC (permalink / raw
To: Michael S. Tsirkin
Cc: linux-kernel, jbenc, David Miller, Eric Dumazet, Daniel Borkmann,
Simon Horman, Paul Durrant, Thomas Graf, Miklos Szeredi,
=?UTF-8?q?Peter=20Pan=28=E6=BD=98=E5=8D=AB=E5=B9=B3=29?=, netdev
On Tue, Mar 11, 2014 at 08:41:50AM +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 11, 2014 at 08:39:41AM +0800, Herbert Xu wrote:
> > On Tue, Mar 11, 2014 at 12:30:01AM +0200, Michael S. Tsirkin wrote:
> > >
> > > How about we do the simple thing as the first step and try to optimize
> > > as a separate patch on top?
> >
> > Sure let's do that.
> >
> > Cheers,
>
> Could you send an ack to my patchset then?
Sure,
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 0/5] skbuff: fix skb_segment with zero copy skbs
2014-03-10 16:29 [PATCH 0/5] skbuff: fix skb_segment with zero copy skbs Michael S. Tsirkin
` (4 preceding siblings ...)
2014-03-10 17:28 ` [PATCH 5/5] skbuff: skb_segment: orphan frags before copying Michael S. Tsirkin
@ 2014-03-11 20:27 ` David Miller
5 siblings, 0 replies; 20+ messages in thread
From: David Miller @ 2014-03-11 20:27 UTC (permalink / raw
To: mst; +Cc: linux-kernel, jbenc, netdev
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Mon, 10 Mar 2014 18:29:01 +0200
> This fixes a bug in skb_segment where it moves frags
> between skbs without orphaning them.
> This causes userspace to assume it's safe to
> reuse the buffer, and receiver gets corrupted data.
> This further might leak information from the
> transmitter on the wire.
>
> To fix track which skb does a copied frag belong
> to, and orphan frags when copying them.
>
> As we are tracking multiple skbs here, using
> short names (skb,nskb,fskb,skb_frag,frag) becomes confusing.
> So before adding another one, I refactor these names
> slightly.
>
> Patch is split out to make it easier to
> verify that all trasformations are trivially correct.
>
> The problem was observed in the field,
> so I think that the patch is necessary on stable
> as well.
Series applied and queued up for -stable, thanks for fixing this.
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2014-03-11 20:27 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-10 16:29 [PATCH 0/5] skbuff: fix skb_segment with zero copy skbs Michael S. Tsirkin
2014-03-10 16:29 ` [PATCH 1/5] skbuff: skb_segment: s/frag/nskb_frag/ Michael S. Tsirkin
2014-03-10 16:29 ` [PATCH 2/5] skbuff: skb_segment: s/skb_frag/frag/ Michael S. Tsirkin
2014-03-10 16:29 ` [PATCH 3/5] skbuff: skb_segment: s/skb/head_skb/ Michael S. Tsirkin
2014-03-10 17:27 ` [PATCH 4/5] skbuff: skb_segment: s/fskb/list_skb/ Michael S. Tsirkin
2014-03-10 17:40 ` David Miller
2014-03-10 17:43 ` David Miller
2014-03-10 17:56 ` Michael S. Tsirkin
2014-03-10 20:45 ` David Miller
2014-03-10 17:28 ` [PATCH 5/5] skbuff: skb_segment: orphan frags before copying Michael S. Tsirkin
2014-03-10 21:34 ` Herbert Xu
2014-03-10 21:52 ` Michael S. Tsirkin
2014-03-10 21:56 ` Herbert Xu
2014-03-10 22:07 ` Michael S. Tsirkin
2014-03-10 22:12 ` Herbert Xu
2014-03-10 22:30 ` Michael S. Tsirkin
2014-03-11 0:39 ` Herbert Xu
2014-03-11 6:41 ` Michael S. Tsirkin
2014-03-11 7:21 ` Herbert Xu
2014-03-11 20:27 ` [PATCH 0/5] skbuff: fix skb_segment with zero copy skbs David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.