| Message ID | 1395397894-9493-1-git-send-email-zoltan.kiss@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/21/2014 11:31 AM, Zoltan Kiss wrote: > skb_zerocopy can copy elements of the frags array between skbs, but it doesn't > orphan them. Also, it doesn't handle errors, so this patch takes care of that > as well, and modify the callers accordingly. skb_tx_error() is also added to > the callers so they will signal the failed delivery towards the creator of the > skb. > > Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com> > --- > v2: orphan the frags right before touching the frags > > v3: > - orphan 'from' instead of 'to' > - call skb_tx_error() in the callers if something went wrong > > v4: correctly use error path in queue_userspace_packet LGTM now Acked-by: Thomas Graf <tgraf@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Zoltan Kiss <zoltan.kiss@citrix.com> Date: Fri, 21 Mar 2014 10:31:34 +0000 > skb_zerocopy can copy elements of the frags array between skbs, but it doesn't > orphan them. Also, it doesn't handle errors, so this patch takes care of that > as well, and modify the callers accordingly. skb_tx_error() is also added to > the callers so they will signal the failed delivery towards the creator of the > skb. > > Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com> Applied, thanks Zoltan. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RnJvbTogRGF2aWQgTWlsbGVyIDxkYXZlbUBkYXZlbWxvZnQubmV0Pg0KRGF0ZTogV2VkLCAyNiBN YXIgMjAxNCAxNTo1OTo1OCAtMDQwMCAoRURUKQ0KDQo+IEZyb206IFpvbHRhbiBLaXNzIDx6b2x0 YW4ua2lzc0BjaXRyaXguY29tPg0KPiBEYXRlOiBGcmksIDIxIE1hciAyMDE0IDEwOjMxOjM0ICsw MDAwDQo+IA0KPj4gc2tiX3plcm9jb3B5IGNhbiBjb3B5IGVsZW1lbnRzIG9mIHRoZSBmcmFncyBh cnJheSBiZXR3ZWVuIHNrYnMsIGJ1dCBpdCBkb2Vzbid0DQo+PiBvcnBoYW4gdGhlbS4gQWxzbywg aXQgZG9lc24ndCBoYW5kbGUgZXJyb3JzLCBzbyB0aGlzIHBhdGNoIHRha2VzIGNhcmUgb2YgdGhh dA0KPj4gYXMgd2VsbCwgYW5kIG1vZGlmeSB0aGUgY2FsbGVycyBhY2NvcmRpbmdseS4gc2tiX3R4 X2Vycm9yKCkgaXMgYWxzbyBhZGRlZCB0bw0KPj4gdGhlIGNhbGxlcnMgc28gdGhleSB3aWxsIHNp Z25hbCB0aGUgZmFpbGVkIGRlbGl2ZXJ5IHRvd2FyZHMgdGhlIGNyZWF0b3Igb2YgdGhlDQo+PiBz a2IuDQo+PiANCj4+IFNpZ25lZC1vZmYtYnk6IFpvbHRhbiBLaXNzIDx6b2x0YW4ua2lzc0BjaXRy aXguY29tPg0KPiANCj4gQXBwbGllZCwgdGhhbmtzIFpvbHRhbi4NCg0KQWN0dWFsbHksIFpvbHRh biwgeW91IGhhdmUgdG8gZml4IHRoaXM6DQoNCm5ldC9jb3JlL3NrYnVmZi5jOiBJbiBmdW5jdGlv biChc2tiX3plcm9jb3B5ojoNCm5ldC9jb3JlL3NrYnVmZi5jOjIxNzI6Mjogd2FybmluZzogcGFz c2luZyBhcmd1bWVudCAxIG9mIKFza2Jfb3JwaGFuX2ZyYWdzoiBkaXNjYXJkcyChY29uc3SiIHF1 YWxpZmkNCmVyIGZyb20gcG9pbnRlciB0YXJnZXQgdHlwZSBbZW5hYmxlZCBieSBkZWZhdWx0XQ0K SW4gZmlsZSBpbmNsdWRlZCBmcm9tIGluY2x1ZGUvbGludXgvdGNwLmg6MjE6MCwNCiAgICAgICAg ICAgICAgICAgZnJvbSBuZXQvY29yZS9za2J1ZmYuYzo1MDoNCmluY2x1ZGUvbGludXgvc2tidWZm Lmg6MTkwNDoxOTogbm90ZTogZXhwZWN0ZWQgoXN0cnVjdCBza19idWZmICqiIGJ1dCBhcmd1bWVu dCBpcyBvZiB0eXBlIKFjb25zdCBzdHINCnVjdCBza19idWZmICqiDQpuZXQvY29yZS9za2J1ZmYu YzoyMTczOjM6IHdhcm5pbmc6IHBhc3NpbmcgYXJndW1lbnQgMSBvZiChc2tiX3R4X2Vycm9yoiBk aXNjYXJkcyChY29uc3SiIHF1YWxpZmllciBmDQpyb20gcG9pbnRlciB0YXJnZXQgdHlwZSBbZW5h YmxlZCBieSBkZWZhdWx0XQ0KbmV0L2NvcmUvc2tidWZmLmM6NjQyOjY6IG5vdGU6IGV4cGVjdGVk IKFzdHJ1Y3Qgc2tfYnVmZiAqoiBidXQgYXJndW1lbnQgaXMgb2YgdHlwZSChY29uc3Qgc3RydWN0 IHNrXw0KYnVmZiAqog0K -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26/03/14 20:12, David Miller wrote: > From: David Miller <davem@davemloft.net> > Date: Wed, 26 Mar 2014 15:59:58 -0400 (EDT) > >> From: Zoltan Kiss <zoltan.kiss@citrix.com> >> Date: Fri, 21 Mar 2014 10:31:34 +0000 >> >>> skb_zerocopy can copy elements of the frags array between skbs, but it doesn't >>> orphan them. Also, it doesn't handle errors, so this patch takes care of that >>> as well, and modify the callers accordingly. skb_tx_error() is also added to >>> the callers so they will signal the failed delivery towards the creator of the >>> skb. >>> >>> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com> >> >> Applied, thanks Zoltan. > > Actually, Zoltan, you have to fix this: > > net/core/skbuff.c: In function ‘skb_zerocopy’: > net/core/skbuff.c:2172:2: warning: passing argument 1 of ‘skb_orphan_frags’ discards ‘const’ qualifi > er from pointer target type [enabled by default] > In file included from include/linux/tcp.h:21:0, > from net/core/skbuff.c:50: > include/linux/skbuff.h:1904:19: note: expected ‘struct sk_buff *’ but argument is of type ‘const str > uct sk_buff *’ > net/core/skbuff.c:2173:3: warning: passing argument 1 of ‘skb_tx_error’ discards ‘const’ qualifier f > rom pointer target type [enabled by default] > net/core/skbuff.c:642:6: note: expected ‘struct sk_buff *’ but argument is of type ‘const struct sk_ > buff *’ > Ok, resubmitted. 'from' is now not a const parameter, because skb->pfmemalloc might change. Zoli -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 03db95a..35c4e85 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -2508,8 +2508,8 @@ int skb_splice_bits(struct sk_buff *skb, unsigned int offset, unsigned int flags); void skb_copy_and_csum_dev(const struct sk_buff *skb, u8 *to); unsigned int skb_zerocopy_headlen(const struct sk_buff *from); -void skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, - int len, int hlen); +int skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, + int len, int hlen); void skb_split(struct sk_buff *skb, struct sk_buff *skb1, const u32 len); int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen); void skb_scrub_packet(struct sk_buff *skb, bool xnet); diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 3f14c63..4cf0ee5 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -2127,25 +2127,31 @@ EXPORT_SYMBOL_GPL(skb_zerocopy_headlen); * * The `hlen` as calculated by skb_zerocopy_headlen() specifies the * headroom in the `to` buffer. + * + * Return value: + * 0: everything is OK + * -ENOMEM: couldn't orphan frags of @from due to lack of memory + * -EFAULT: skb_copy_bits() found some problem with skb geometry */ -void +int skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, int len, int hlen) { int i, j = 0; int plen = 0; /* length of skb->head fragment */ + int ret; struct page *page; unsigned int offset; BUG_ON(!from->head_frag && !hlen); /* dont bother with small payloads */ - if (len <= skb_tailroom(to)) { - skb_copy_bits(from, 0, skb_put(to, len), len); - return; - } + if (len <= skb_tailroom(to)) + return skb_copy_bits(from, 0, skb_put(to, len), len); if (hlen) { - skb_copy_bits(from, 0, skb_put(to, hlen), hlen); + ret = skb_copy_bits(from, 0, skb_put(to, hlen), hlen); + if (unlikely(ret)) + return ret; len -= hlen; } else { plen = min_t(int, skb_headlen(from), len); @@ -2163,6 +2169,11 @@ skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, int len, int hlen) to->len += len + plen; to->data_len += len + plen; + if (unlikely(skb_orphan_frags(from, GFP_ATOMIC))) { + skb_tx_error(from); + return -ENOMEM; + } + for (i = 0; i < skb_shinfo(from)->nr_frags; i++) { if (!len) break; @@ -2173,6 +2184,8 @@ skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, int len, int hlen) j++; } skb_shinfo(to)->nr_frags = j; + + return 0; } EXPORT_SYMBOL_GPL(skb_zerocopy); diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c index f072fe8..108120f 100644 --- a/net/netfilter/nfnetlink_queue_core.c +++ b/net/netfilter/nfnetlink_queue_core.c @@ -354,13 +354,16 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue, skb = nfnetlink_alloc_skb(net, size, queue->peer_portid, GFP_ATOMIC); - if (!skb) + if (!skb) { + skb_tx_error(entskb); return NULL; + } nlh = nlmsg_put(skb, 0, 0, NFNL_SUBSYS_QUEUE << 8 | NFQNL_MSG_PACKET, sizeof(struct nfgenmsg), 0); if (!nlh) { + skb_tx_error(entskb); kfree_skb(skb); return NULL; } @@ -488,13 +491,15 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue, nla->nla_type = NFQA_PAYLOAD; nla->nla_len = nla_attr_size(data_len); - skb_zerocopy(skb, entskb, data_len, hlen); + if (skb_zerocopy(skb, entskb, data_len, hlen)) + goto nla_put_failure; } nlh->nlmsg_len = skb->len; return skb; nla_put_failure: + skb_tx_error(entskb); kfree_skb(skb); net_err_ratelimited("nf_queue: error creating packet message\n"); return NULL; diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c index c53fe0c..9230223 100644 --- a/net/openvswitch/datapath.c +++ b/net/openvswitch/datapath.c @@ -464,7 +464,9 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb, } nla->nla_len = nla_attr_size(skb->len); - skb_zerocopy(user_skb, skb, skb->len, hlen); + err = skb_zerocopy(user_skb, skb, skb->len, hlen); + if (err) + goto out; /* Pad OVS_PACKET_ATTR_PACKET if linear copy was performed */ if (!(dp->user_features & OVS_DP_F_UNALIGNED)) { @@ -478,6 +480,8 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb, err = genlmsg_unicast(ovs_dp_get_net(dp), user_skb, upcall_info->portid); out: + if (err) + skb_tx_error(skb); kfree_skb(nskb); return err; }
skb_zerocopy can copy elements of the frags array between skbs, but it doesn't orphan them. Also, it doesn't handle errors, so this patch takes care of that as well, and modify the callers accordingly. skb_tx_error() is also added to the callers so they will signal the failed delivery towards the creator of the skb. Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com> --- v2: orphan the frags right before touching the frags v3: - orphan 'from' instead of 'to' - call skb_tx_error() in the callers if something went wrong v4: correctly use error path in queue_userspace_packet -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html