[ovs-dev] [PATCH] datapath: Orphan frags in skb_zerocopy and handle errors

[Zoltan Kiss]

I don't know if Kyle posted this already, but I needed it anyway, so I 
sent in my version.

Zoli

On 09/04/14 17:20, Zoltan Kiss wrote:
> This is the ported version of commit 36d5fe6a with the same name from net-next.
> Apart from the small datapath.c changes it adjust the compat layer files as
> well. This is the original commit message:
>
> "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 at citrix.com>
> ---
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 25edd7d..5e5aa71 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -485,7 +485,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)) {
> @@ -499,6 +501,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;
>   }
> diff --git a/datapath/linux/compat/include/linux/skbuff.h b/datapath/linux/compat/include/linux/skbuff.h
> index de0c56a..c709aca 100644
> --- a/datapath/linux/compat/include/linux/skbuff.h
> +++ b/datapath/linux/compat/include/linux/skbuff.h
> @@ -260,7 +260,7 @@ static inline __u32 skb_get_rxhash(struct sk_buff *skb)
>
>   #if LINUX_VERSION_CODE < KERNEL_VERSION(3,14,0)
>   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 skb_zerocopy(struct sk_buff *to, struct sk_buff *from, int len,
>   		  int hlen);
>   #endif
>
> diff --git a/datapath/linux/compat/skbuff-openvswitch.c b/datapath/linux/compat/skbuff-openvswitch.c
> index ddd7bc8..96c27d0 100644
> --- a/datapath/linux/compat/skbuff-openvswitch.c
> +++ b/datapath/linux/compat/skbuff-openvswitch.c
> @@ -61,25 +61,31 @@ skb_zerocopy_headlen(const struct sk_buff *from)
>    *
>    *	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
> -skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, int len, int hlen)
> +int
> +skb_zerocopy(struct sk_buff *to, 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(!head_frag(from) && !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);
> @@ -97,6 +103,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;
> @@ -107,5 +118,7 @@ skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, int len, int hlen)
>   		j++;
>   	}
>   	skb_shinfo(to)->nr_frags = j;
> +
> +	return 0;
>   }
>   #endif
>