[PATCH] net/packet: fix packet drop as of virtio gso

[PATCH] net/packet: fix packet drop as of virtio gso

[Jianfeng Tan]

When we use raw socket as the vhost backend, a packet from virito with
gso offloading information, cannot be sent out in later validaton at
xmit path, as we did not set correct skb->protocol which is further used
for looking up the gso function.

To fix this, we set this field according to virito hdr information.

Fixes: e858fae2b0b8f4 ("virtio_net: use common code for virtio_net_hdr and skb GSO conversion")

Cc: stable@vger.kernel.org
Signed-off-by: Jianfeng Tan <jianfeng.tan@linux.alibaba.com>
---
 include/linux/virtio_net.h | 18 ++++++++++++++++++
 net/packet/af_packet.c     | 11 +++++++----
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 9397628a1967..cb462f9ab7dd 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -5,6 +5,24 @@
 #include <linux/if_vlan.h>
 #include <uapi/linux/virtio_net.h>
 
+static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
+					   const struct virtio_net_hdr *hdr)
+{
+	switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
+	case VIRTIO_NET_HDR_GSO_TCPV4:
+	case VIRTIO_NET_HDR_GSO_UDP:
+		skb->protocol = cpu_to_be16(ETH_P_IP);
+		break;
+	case VIRTIO_NET_HDR_GSO_TCPV6:
+		skb->protocol = cpu_to_be16(ETH_P_IPV6);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 					const struct virtio_net_hdr *hdr,
 					bool little_endian)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 75c92a87e7b2..d6e94dc7e290 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2715,10 +2715,12 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 			}
 		}
 
-		if (po->has_vnet_hdr && virtio_net_hdr_to_skb(skb, vnet_hdr,
-							      vio_le())) {
-			tp_len = -EINVAL;
-			goto tpacket_error;
+		if (po->has_vnet_hdr) {
+			if (virtio_net_hdr_to_skb(skb, vnet_hdr, vio_le())) {
+				tp_len = -EINVAL;
+				goto tpacket_error;
+			}
+			virtio_net_hdr_set_proto(skb, vnet_hdr);
 		}
 
 		skb->destructor = tpacket_destruct_skb;
@@ -2915,6 +2917,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 		if (err)
 			goto out_free;
 		len += sizeof(vnet_hdr);
+		virtio_net_hdr_set_proto(skb, &vnet_hdr);
 	}
 
 	skb_probe_transport_header(skb, reserve);
-- 
2.17.1

Re: [PATCH] net/packet: fix packet drop as of virtio gso

[David Miller]

From: Jianfeng Tan <jianfeng.tan@linux.alibaba.com>
Date: Sat, 29 Sep 2018 15:41:27 +0000

> When we use raw socket as the vhost backend, a packet from virito with
> gso offloading information, cannot be sent out in later validaton at
> xmit path, as we did not set correct skb->protocol which is further used
> for looking up the gso function.
> 
> To fix this, we set this field according to virito hdr information.
> 
> Fixes: e858fae2b0b8f4 ("virtio_net: use common code for virtio_net_hdr and skb GSO conversion")
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Jianfeng Tan <jianfeng.tan@linux.alibaba.com>

Applied and queued up for -stable.

Re: [PATCH] net/packet: fix packet drop as of virtio gso

[Jason Wang]

On 2018年09月29日 23:41, Jianfeng Tan wrote:
> When we use raw socket as the vhost backend, a packet from virito with
> gso offloading information, cannot be sent out in later validaton at
> xmit path, as we did not set correct skb->protocol which is further used
> for looking up the gso function.

Hi:

May I ask the reason for using raw socket for vhost? It was not a common 
setup with little care in the past few years. And it was slow since it 
lacks some recent improvements. Can it be replaced with e.g macvtap?

Thanks

>
> To fix this, we set this field according to virito hdr information.
>
> Fixes: e858fae2b0b8f4 ("virtio_net: use common code for virtio_net_hdr and skb GSO conversion")
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jianfeng Tan <jianfeng.tan@linux.alibaba.com>
> ---
>   include/linux/virtio_net.h | 18 ++++++++++++++++++
>   net/packet/af_packet.c     | 11 +++++++----
>   2 files changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 9397628a1967..cb462f9ab7dd 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -5,6 +5,24 @@
>   #include <linux/if_vlan.h>
>   #include <uapi/linux/virtio_net.h>
>   
> +static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
> +					   const struct virtio_net_hdr *hdr)
> +{
> +	switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> +	case VIRTIO_NET_HDR_GSO_TCPV4:
> +	case VIRTIO_NET_HDR_GSO_UDP:
> +		skb->protocol = cpu_to_be16(ETH_P_IP);
> +		break;
> +	case VIRTIO_NET_HDR_GSO_TCPV6:
> +		skb->protocol = cpu_to_be16(ETH_P_IPV6);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>   static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
>   					const struct virtio_net_hdr *hdr,
>   					bool little_endian)
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 75c92a87e7b2..d6e94dc7e290 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -2715,10 +2715,12 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
>   			}
>   		}
>   
> -		if (po->has_vnet_hdr && virtio_net_hdr_to_skb(skb, vnet_hdr,
> -							      vio_le())) {
> -			tp_len = -EINVAL;
> -			goto tpacket_error;
> +		if (po->has_vnet_hdr) {
> +			if (virtio_net_hdr_to_skb(skb, vnet_hdr, vio_le())) {
> +				tp_len = -EINVAL;
> +				goto tpacket_error;
> +			}
> +			virtio_net_hdr_set_proto(skb, vnet_hdr);
>   		}
>   
>   		skb->destructor = tpacket_destruct_skb;
> @@ -2915,6 +2917,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>   		if (err)
>   			goto out_free;
>   		len += sizeof(vnet_hdr);
> +		virtio_net_hdr_set_proto(skb, &vnet_hdr);
>   	}
>   
>   	skb_probe_transport_header(skb, reserve);

Re: [PATCH] net/packet: fix packet drop as of virtio gso

[Jianfeng Tan]

On 10/8/2018 11:14 AM, Jason Wang wrote:
>
>
> On 2018年09月29日 23:41, Jianfeng Tan wrote:
>> When we use raw socket as the vhost backend, a packet from virito with
>> gso offloading information, cannot be sent out in later validaton at
>> xmit path, as we did not set correct skb->protocol which is further used
>> for looking up the gso function.
>
> Hi:
>
> May I ask the reason for using raw socket for vhost? It was not a 
> common setup with little care in the past few years. And it was slow 
> since it lacks some recent improvements. Can it be replaced with e.g 
> macvtap?

Hi Jason,

Apologize for late response. We are in container environment, in which 
case veth is used mostly. Either tap or macvtap cannot be put into an 
isolated netns. Another thing could be macvlan as the backend of vhost, 
which is not supported either. So unfortunately, improving raw socket is 
the only choice I suppose.

Thanks,
Jianfeng


>
> Thanks
>
>>
>> To fix this, we set this field according to virito hdr information.
>>
>> Fixes: e858fae2b0b8f4 ("virtio_net: use common code for 
>> virtio_net_hdr and skb GSO conversion")
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jianfeng Tan <jianfeng.tan@linux.alibaba.com>
>> ---
>>   include/linux/virtio_net.h | 18 ++++++++++++++++++
>>   net/packet/af_packet.c     | 11 +++++++----
>>   2 files changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
>> index 9397628a1967..cb462f9ab7dd 100644
>> --- a/include/linux/virtio_net.h
>> +++ b/include/linux/virtio_net.h
>> @@ -5,6 +5,24 @@
>>   #include <linux/if_vlan.h>
>>   #include <uapi/linux/virtio_net.h>
>>   +static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
>> +                       const struct virtio_net_hdr *hdr)
>> +{
>> +    switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
>> +    case VIRTIO_NET_HDR_GSO_TCPV4:
>> +    case VIRTIO_NET_HDR_GSO_UDP:
>> +        skb->protocol = cpu_to_be16(ETH_P_IP);
>> +        break;
>> +    case VIRTIO_NET_HDR_GSO_TCPV6:
>> +        skb->protocol = cpu_to_be16(ETH_P_IPV6);
>> +        break;
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
>>                       const struct virtio_net_hdr *hdr,
>>                       bool little_endian)
>> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
>> index 75c92a87e7b2..d6e94dc7e290 100644
>> --- a/net/packet/af_packet.c
>> +++ b/net/packet/af_packet.c
>> @@ -2715,10 +2715,12 @@ static int tpacket_snd(struct packet_sock 
>> *po, struct msghdr *msg)
>>               }
>>           }
>>   -        if (po->has_vnet_hdr && virtio_net_hdr_to_skb(skb, vnet_hdr,
>> -                                  vio_le())) {
>> -            tp_len = -EINVAL;
>> -            goto tpacket_error;
>> +        if (po->has_vnet_hdr) {
>> +            if (virtio_net_hdr_to_skb(skb, vnet_hdr, vio_le())) {
>> +                tp_len = -EINVAL;
>> +                goto tpacket_error;
>> +            }
>> +            virtio_net_hdr_set_proto(skb, vnet_hdr);
>>           }
>>             skb->destructor = tpacket_destruct_skb;
>> @@ -2915,6 +2917,7 @@ static int packet_snd(struct socket *sock, 
>> struct msghdr *msg, size_t len)
>>           if (err)
>>               goto out_free;
>>           len += sizeof(vnet_hdr);
>> +        virtio_net_hdr_set_proto(skb, &vnet_hdr);
>>       }
>>         skb_probe_transport_header(skb, reserve);

Re: [PATCH] net/packet: fix packet drop as of virtio gso

[Jason Wang]

On 2018/10/28 上午7:42, Jianfeng Tan wrote:
>
> On 10/8/2018 11:14 AM, Jason Wang wrote:
>>
>>
>> On 2018年09月29日 23:41, Jianfeng Tan wrote:
>>> When we use raw socket as the vhost backend, a packet from virito with
>>> gso offloading information, cannot be sent out in later validaton at
>>> xmit path, as we did not set correct skb->protocol which is further 
>>> used
>>> for looking up the gso function.
>>
>> Hi:
>>
>> May I ask the reason for using raw socket for vhost? It was not a 
>> common setup with little care in the past few years. And it was slow 
>> since it lacks some recent improvements. Can it be replaced with e.g 
>> macvtap?
>
> Hi Jason,
>
> Apologize for late response. We are in container environment, in which 
> case veth is used mostly. Either tap or macvtap cannot be put into an 
> isolated netns. 


I think it can? See 17af2bce88d31e65ed73d638bb752d2e13c66ced.


> Another thing could be macvlan as the backend of vhost, which is not 
> supported either. So unfortunately, improving raw socket is the only 
> choice I suppose.


Btw, you can setup macvtap on top of veth. Does this help?

Thanks


>
> Thanks,
> Jianfeng

Re: [PATCH] net/packet: fix packet drop as of virtio gso

[Jianfeng Tan]

On 10/29/2018 10:40 AM, Jason Wang wrote:
>
> On 2018/10/28 上午7:42, Jianfeng Tan wrote:
>>
>> On 10/8/2018 11:14 AM, Jason Wang wrote:
>>>
>>>
>>> On 2018年09月29日 23:41, Jianfeng Tan wrote:
>>>> When we use raw socket as the vhost backend, a packet from virito with
>>>> gso offloading information, cannot be sent out in later validaton at
>>>> xmit path, as we did not set correct skb->protocol which is further 
>>>> used
>>>> for looking up the gso function.
>>>
>>> Hi:
>>>
>>> May I ask the reason for using raw socket for vhost? It was not a 
>>> common setup with little care in the past few years. And it was slow 
>>> since it lacks some recent improvements. Can it be replaced with e.g 
>>> macvtap?
>>
>> Hi Jason,
>>
>> Apologize for late response. We are in container environment, in 
>> which case veth is used mostly. Either tap or macvtap cannot be put 
>> into an isolated netns. 
>
>
> I think it can? See 17af2bce88d31e65ed73d638bb752d2e13c66ced.

This commit gives an example of creating a macvtap on to of a veth 
interface, which is interesting to try. The shortcoming, if I understand 
it correctly, it still needs the network plugin to create the macvtap 
interface for containers.

>
>
>> Another thing could be macvlan as the backend of vhost, which is not 
>> supported either. So unfortunately, improving raw socket is the only 
>> choice I suppose.
>
>
> Btw, you can setup macvtap on top of veth. Does this help?

Good idea. Will have a try.

Thanks,
Jianfeng

>
> Thanks
>
>
>>
>> Thanks,
>> Jianfeng