[PATCH] virtio: don't set VIRTIO_NET_HDR_F_DATA_VALID on xmit

[PATCH] virtio: don't set VIRTIO_NET_HDR_F_DATA_VALID on xmit

[Rolf Neugebauer]

This patch part reverts fd2a0437dc33 and e858fae2b0b8 which introduced a
subtle change in how the virtio_net flags are derived from the SKBs
ip_summed field.

With the above commits, the flags are set to VIRTIO_NET_HDR_F_DATA_VALID
when ip_summed == CHECKSUM_UNNECESSARY, thus treating it differently to
ip_summed == CHECKSUM_NONE, which should be the same.

Further, the virtio spec 1.0 / CS04 explicitly says that
VIRTIO_NET_HDR_F_DATA_VALID must not be set by the driver.

Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>
---
 include/linux/virtio_net.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 66204007d7ac..56436472ccc7 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -91,8 +91,6 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
 				skb_checksum_start_offset(skb));
 		hdr->csum_offset = __cpu_to_virtio16(little_endian,
 				skb->csum_offset);
-	} else if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
-		hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
 	} /* else everything is zero */
 
 	return 0;
-- 
2.11.0

Re: [PATCH] virtio: don't set VIRTIO_NET_HDR_F_DATA_VALID on xmit

[Michael S. Tsirkin]

On Tue, Jan 17, 2017 at 06:13:51PM +0000, Rolf Neugebauer wrote:
> This patch part reverts fd2a0437dc33 and e858fae2b0b8 which introduced a
> subtle change in how the virtio_net flags are derived from the SKBs
> ip_summed field.
> 
> With the above commits, the flags are set to VIRTIO_NET_HDR_F_DATA_VALID
> when ip_summed == CHECKSUM_UNNECESSARY, thus treating it differently to
> ip_summed == CHECKSUM_NONE, which should be the same.
> 
> Further, the virtio spec 1.0 / CS04 explicitly says that
> VIRTIO_NET_HDR_F_DATA_VALID must not be set by the driver.
> 
> Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>

Fixes: fd2a0437dc33 ("virtio_net: introduce virtio_net_hdr_{from,to}_skb")
Fixes: e858fae2b0b8 (" virtio_net: use common code for virtio_net_hdr and skb GSO conversion")
Acked-by: Michael S. Tsirkin <mst@redhat.com>

Should be backported into stable as well.



> ---
>  include/linux/virtio_net.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 66204007d7ac..56436472ccc7 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -91,8 +91,6 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
>  				skb_checksum_start_offset(skb));
>  		hdr->csum_offset = __cpu_to_virtio16(little_endian,
>  				skb->csum_offset);
> -	} else if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
> -		hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
>  	} /* else everything is zero */
>  
>  	return 0;
> -- 
> 2.11.0

Re: [PATCH] virtio: don't set VIRTIO_NET_HDR_F_DATA_VALID on xmit

[David Miller]

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Tue, 17 Jan 2017 20:27:33 +0200

> On Tue, Jan 17, 2017 at 06:13:51PM +0000, Rolf Neugebauer wrote:
>> This patch part reverts fd2a0437dc33 and e858fae2b0b8 which introduced a
>> subtle change in how the virtio_net flags are derived from the SKBs
>> ip_summed field.
>> 
>> With the above commits, the flags are set to VIRTIO_NET_HDR_F_DATA_VALID
>> when ip_summed == CHECKSUM_UNNECESSARY, thus treating it differently to
>> ip_summed == CHECKSUM_NONE, which should be the same.
>> 
>> Further, the virtio spec 1.0 / CS04 explicitly says that
>> VIRTIO_NET_HDR_F_DATA_VALID must not be set by the driver.
>> 
>> Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>
> 
> Fixes: fd2a0437dc33 ("virtio_net: introduce virtio_net_hdr_{from,to}_skb")
> Fixes: e858fae2b0b8 (" virtio_net: use common code for virtio_net_hdr and skb GSO conversion")
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Should be backported into stable as well.

Applied and queued up for -stable, thanks everyone.

Re: [PATCH] virtio: don't set VIRTIO_NET_HDR_F_DATA_VALID on xmit

[Jason Wang]

On 2017年01月18日 02:27, Michael S. Tsirkin wrote:
> On Tue, Jan 17, 2017 at 06:13:51PM +0000, Rolf Neugebauer wrote:
>> This patch part reverts fd2a0437dc33 and e858fae2b0b8 which introduced a
>> subtle change in how the virtio_net flags are derived from the SKBs
>> ip_summed field.
>>
>> With the above commits, the flags are set to VIRTIO_NET_HDR_F_DATA_VALID
>> when ip_summed == CHECKSUM_UNNECESSARY, thus treating it differently to
>> ip_summed == CHECKSUM_NONE, which should be the same.
>>
>> Further, the virtio spec 1.0 / CS04 explicitly says that
>> VIRTIO_NET_HDR_F_DATA_VALID must not be set by the driver.
>>
>> Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>
> Fixes: fd2a0437dc33 ("virtio_net: introduce virtio_net_hdr_{from,to}_skb")
> Fixes: e858fae2b0b8 (" virtio_net: use common code for virtio_net_hdr and skb GSO conversion")
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> Should be backported into stable as well.

Looks like a side effect is that we will never see this on receive path? 
We probably need a hint for virtio_net_hdr_from_skb().

Thanks

>
>
>> ---
>>   include/linux/virtio_net.h | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
>> index 66204007d7ac..56436472ccc7 100644
>> --- a/include/linux/virtio_net.h
>> +++ b/include/linux/virtio_net.h
>> @@ -91,8 +91,6 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
>>   				skb_checksum_start_offset(skb));
>>   		hdr->csum_offset = __cpu_to_virtio16(little_endian,
>>   				skb->csum_offset);
>> -	} else if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
>> -		hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
>>   	} /* else everything is zero */
>>   
>>   	return 0;
>> -- 
>> 2.11.0

Re: [PATCH] virtio: don't set VIRTIO_NET_HDR_F_DATA_VALID on xmit

[Sergei Shtylyov]

Hello!

On 1/17/2017 9:13 PM, Rolf Neugebauer wrote:

> This patch part reverts fd2a0437dc33 and e858fae2b0b8 which introduced a

    You also need to specify the patch summary lines enclosed in ("").

> subtle change in how the virtio_net flags are derived from the SKBs
> ip_summed field.
>
> With the above commits, the flags are set to VIRTIO_NET_HDR_F_DATA_VALID
> when ip_summed == CHECKSUM_UNNECESSARY, thus treating it differently to
> ip_summed == CHECKSUM_NONE, which should be the same.
>
> Further, the virtio spec 1.0 / CS04 explicitly says that
> VIRTIO_NET_HDR_F_DATA_VALID must not be set by the driver.
>
> Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>
[...]

MBR, Sergei

Re: [PATCH] virtio: don't set VIRTIO_NET_HDR_F_DATA_VALID on xmit

[Michael S. Tsirkin]

On Wed, Jan 18, 2017 at 11:54:21AM +0800, Jason Wang wrote:
> 
> 
> On 2017年01月18日 02:27, Michael S. Tsirkin wrote:
> > On Tue, Jan 17, 2017 at 06:13:51PM +0000, Rolf Neugebauer wrote:
> > > This patch part reverts fd2a0437dc33 and e858fae2b0b8 which introduced a
> > > subtle change in how the virtio_net flags are derived from the SKBs
> > > ip_summed field.
> > > 
> > > With the above commits, the flags are set to VIRTIO_NET_HDR_F_DATA_VALID
> > > when ip_summed == CHECKSUM_UNNECESSARY, thus treating it differently to
> > > ip_summed == CHECKSUM_NONE, which should be the same.
> > > 
> > > Further, the virtio spec 1.0 / CS04 explicitly says that
> > > VIRTIO_NET_HDR_F_DATA_VALID must not be set by the driver.
> > > 
> > > Signed-off-by: Rolf Neugebauer <rolf.neugebauer@docker.com>
> > Fixes: fd2a0437dc33 ("virtio_net: introduce virtio_net_hdr_{from,to}_skb")
> > Fixes: e858fae2b0b8 (" virtio_net: use common code for virtio_net_hdr and skb GSO conversion")
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > Should be backported into stable as well.
> 
> Looks like a side effect is that we will never see this on receive path?

I thought receive path is using virtio_net_hdr_to_skb - isn't it?

> We
> probably need a hint for virtio_net_hdr_from_skb().
> 
> Thanks
> 
> > 
> > 
> > > ---
> > >   include/linux/virtio_net.h | 2 --
> > >   1 file changed, 2 deletions(-)
> > > 
> > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > > index 66204007d7ac..56436472ccc7 100644
> > > --- a/include/linux/virtio_net.h
> > > +++ b/include/linux/virtio_net.h
> > > @@ -91,8 +91,6 @@ static inline int virtio_net_hdr_from_skb(const struct sk_buff *skb,
> > >   				skb_checksum_start_offset(skb));
> > >   		hdr->csum_offset = __cpu_to_virtio16(little_endian,
> > >   				skb->csum_offset);
> > > -	} else if (skb->ip_summed == CHECKSUM_UNNECESSARY) {
> > > -		hdr->flags = VIRTIO_NET_HDR_F_DATA_VALID;
> > >   	} /* else everything is zero */
> > >   	return 0;
> > > -- 
> > > 2.11.0

Re: [PATCH] virtio: don't set VIRTIO_NET_HDR_F_DATA_VALID on xmit

[Jason Wang]

On 2017年01月18日 23:18, Michael S. Tsirkin wrote:
> On Wed, Jan 18, 2017 at 11:54:21AM +0800, Jason Wang wrote:
>> On 2017年01月18日 02:27, Michael S. Tsirkin wrote:
>>> On Tue, Jan 17, 2017 at 06:13:51PM +0000, Rolf Neugebauer wrote:
>>>> This patch part reverts fd2a0437dc33 and e858fae2b0b8 which introduced a
>>>> subtle change in how the virtio_net flags are derived from the SKBs
>>>> ip_summed field.
>>>>
>>>> With the above commits, the flags are set to VIRTIO_NET_HDR_F_DATA_VALID
>>>> when ip_summed == CHECKSUM_UNNECESSARY, thus treating it differently to
>>>> ip_summed == CHECKSUM_NONE, which should be the same.
>>>>
>>>> Further, the virtio spec 1.0 / CS04 explicitly says that
>>>> VIRTIO_NET_HDR_F_DATA_VALID must not be set by the driver.
>>>>
>>>> Signed-off-by: Rolf Neugebauer<rolf.neugebauer@docker.com>
>>> Fixes: fd2a0437dc33 ("virtio_net: introduce virtio_net_hdr_{from,to}_skb")
>>> Fixes: e858fae2b0b8 (" virtio_net: use common code for virtio_net_hdr and skb GSO conversion")
>>> Acked-by: Michael S. Tsirkin<mst@redhat.com>
>>>
>>> Should be backported into stable as well.
>> Looks like a side effect is that we will never see this on receive path?
> I thought receive path is using virtio_net_hdr_to_skb - isn't it?
>

The problem is tun use this on xmit.

Thanks

Re: [PATCH] virtio: don't set VIRTIO_NET_HDR_F_DATA_VALID on xmit

[Michael S. Tsirkin]

On Thu, Jan 19, 2017 at 11:06:59AM +0800, Jason Wang wrote:
> 
> 
> On 2017年01月18日 23:18, Michael S. Tsirkin wrote:
> > On Wed, Jan 18, 2017 at 11:54:21AM +0800, Jason Wang wrote:
> > > On 2017年01月18日 02:27, Michael S. Tsirkin wrote:
> > > > On Tue, Jan 17, 2017 at 06:13:51PM +0000, Rolf Neugebauer wrote:
> > > > > This patch part reverts fd2a0437dc33 and e858fae2b0b8 which introduced a
> > > > > subtle change in how the virtio_net flags are derived from the SKBs
> > > > > ip_summed field.
> > > > > 
> > > > > With the above commits, the flags are set to VIRTIO_NET_HDR_F_DATA_VALID
> > > > > when ip_summed == CHECKSUM_UNNECESSARY, thus treating it differently to
> > > > > ip_summed == CHECKSUM_NONE, which should be the same.
> > > > > 
> > > > > Further, the virtio spec 1.0 / CS04 explicitly says that
> > > > > VIRTIO_NET_HDR_F_DATA_VALID must not be set by the driver.
> > > > > 
> > > > > Signed-off-by: Rolf Neugebauer<rolf.neugebauer@docker.com>
> > > > Fixes: fd2a0437dc33 ("virtio_net: introduce virtio_net_hdr_{from,to}_skb")
> > > > Fixes: e858fae2b0b8 (" virtio_net: use common code for virtio_net_hdr and skb GSO conversion")
> > > > Acked-by: Michael S. Tsirkin<mst@redhat.com>
> > > > 
> > > > Should be backported into stable as well.
> > > Looks like a side effect is that we will never see this on receive path?
> > I thought receive path is using virtio_net_hdr_to_skb - isn't it?
> > 
> 
> The problem is tun use this on xmit.
> 
> Thanks

Oh I missed that. I agree, let's add a flag that allows this.

-- 
MST

Re: [PATCH] virtio: don't set VIRTIO_NET_HDR_F_DATA_VALID on xmit

[Jason Wang]

On 2017年01月20日 06:30, Michael S. Tsirkin wrote:
> On Thu, Jan 19, 2017 at 11:06:59AM +0800, Jason Wang wrote:
>>
>> On 2017年01月18日 23:18, Michael S. Tsirkin wrote:
>>> On Wed, Jan 18, 2017 at 11:54:21AM +0800, Jason Wang wrote:
>>>> On 2017年01月18日 02:27, Michael S. Tsirkin wrote:
>>>>> On Tue, Jan 17, 2017 at 06:13:51PM +0000, Rolf Neugebauer wrote:
>>>>>> This patch part reverts fd2a0437dc33 and e858fae2b0b8 which introduced a
>>>>>> subtle change in how the virtio_net flags are derived from the SKBs
>>>>>> ip_summed field.
>>>>>>
>>>>>> With the above commits, the flags are set to VIRTIO_NET_HDR_F_DATA_VALID
>>>>>> when ip_summed == CHECKSUM_UNNECESSARY, thus treating it differently to
>>>>>> ip_summed == CHECKSUM_NONE, which should be the same.
>>>>>>
>>>>>> Further, the virtio spec 1.0 / CS04 explicitly says that
>>>>>> VIRTIO_NET_HDR_F_DATA_VALID must not be set by the driver.
>>>>>>
>>>>>> Signed-off-by: Rolf Neugebauer<rolf.neugebauer@docker.com>
>>>>> Fixes: fd2a0437dc33 ("virtio_net: introduce virtio_net_hdr_{from,to}_skb")
>>>>> Fixes: e858fae2b0b8 (" virtio_net: use common code for virtio_net_hdr and skb GSO conversion")
>>>>> Acked-by: Michael S. Tsirkin<mst@redhat.com>
>>>>>
>>>>> Should be backported into stable as well.
>>>> Looks like a side effect is that we will never see this on receive path?
>>> I thought receive path is using virtio_net_hdr_to_skb - isn't it?
>>>
>> The problem is tun use this on xmit.
>>
>> Thanks
> Oh I missed that. I agree, let's add a flag that allows this.
>

Will post a fix soon.

Thanks