diff mbox series

vhost: fix IPv4 csum calculation

Message ID 20191024142814.10979-1-fbl@sysclose.org (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series vhost: fix IPv4 csum calculation |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-compilation success Compile Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/Intel-compilation success Compilation OK

Commit Message

Flavio Leitner Oct. 24, 2019, 2:28 p.m. UTC 
  Currently the IPv4 header checksum is calculated including its
current value, which can be a valid checksum or just garbage.
In any case, if the original value is not zero, then the result
is always wrong.

The IPv4 checksum is defined in RFC791, page 14 says:
  Header Checksum:  16 bits

  The checksum algorithm is:
  The checksum field is the 16 bit one's complement of the one's
  complement sum of all 16 bit words in the header.  For purposes of
  computing the checksum, the value of the checksum field is zero.

Thus force the csum field to always be zero.

Signed-off-by: Flavio Leitner <fbl@sysclose.org>
---
 lib/librte_vhost/virtio_net.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Maxime Coquelin Oct. 24, 2019, 2:32 p.m. UTC | #1 
Hi Flavio,

On 10/24/19 4:28 PM, Flavio Leitner wrote:
> Currently the IPv4 header checksum is calculated including its
> current value, which can be a valid checksum or just garbage.
> In any case, if the original value is not zero, then the result
> is always wrong.
> 
> The IPv4 checksum is defined in RFC791, page 14 says:
>   Header Checksum:  16 bits
> 
>   The checksum algorithm is:
>   The checksum field is the 16 bit one's complement of the one's
>   complement sum of all 16 bit words in the header.  For purposes of
>   computing the checksum, the value of the checksum field is zero.
> 
> Thus force the csum field to always be zero.

I think we need Fixes tag:

Fixes: b08b8cfeb2ae ("vhost: fix IP checksum")
Cc: stable@dpdk.org

Also adding stable in Cc.


> Signed-off-by: Flavio Leitner <fbl@sysclose.org>
> ---
>  lib/librte_vhost/virtio_net.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
> index eae7825f04..cde7498c76 100644
> --- a/lib/librte_vhost/virtio_net.c
> +++ b/lib/librte_vhost/virtio_net.c
> @@ -445,6 +445,7 @@ virtio_enqueue_offload(struct rte_mbuf *m_buf, struct virtio_net_hdr *net_hdr)
>  
>  		ipv4_hdr = rte_pktmbuf_mtod_offset(m_buf, struct rte_ipv4_hdr *,
>  						   m_buf->l2_len);
> +		ipv4_hdr->hdr_checksum = 0;
>  		ipv4_hdr->hdr_checksum = rte_ipv4_cksum(ipv4_hdr);
>  	}
>  
> 

Other than that:

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

I'll add the Fixes tag while applying.

Thanks!
Maxime
Flavio Leitner Oct. 24, 2019, 4:31 p.m. UTC | #2 
On Thu, 24 Oct 2019 16:32:43 +0200
Maxime Coquelin <maxime.coquelin@redhat.com> wrote:

> Hi Flavio,
> 
> On 10/24/19 4:28 PM, Flavio Leitner wrote:
> > Currently the IPv4 header checksum is calculated including its
> > current value, which can be a valid checksum or just garbage.
> > In any case, if the original value is not zero, then the result
> > is always wrong.
> > 
> > The IPv4 checksum is defined in RFC791, page 14 says:
> >   Header Checksum:  16 bits
> > 
> >   The checksum algorithm is:
> >   The checksum field is the 16 bit one's complement of the one's
> >   complement sum of all 16 bit words in the header.  For purposes of
> >   computing the checksum, the value of the checksum field is zero.
> > 
> > Thus force the csum field to always be zero.  
> 
> I think we need Fixes tag:
> 
> Fixes: b08b8cfeb2ae ("vhost: fix IP checksum")
> Cc: stable@dpdk.org
> 
> Also adding stable in Cc.
> 
> 
> > Signed-off-by: Flavio Leitner <fbl@sysclose.org>
> > ---
> >  lib/librte_vhost/virtio_net.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/lib/librte_vhost/virtio_net.c
> > b/lib/librte_vhost/virtio_net.c index eae7825f04..cde7498c76 100644
> > --- a/lib/librte_vhost/virtio_net.c
> > +++ b/lib/librte_vhost/virtio_net.c
> > @@ -445,6 +445,7 @@ virtio_enqueue_offload(struct rte_mbuf *m_buf,
> > struct virtio_net_hdr *net_hdr) 
> >  		ipv4_hdr = rte_pktmbuf_mtod_offset(m_buf, struct
> > rte_ipv4_hdr *, m_buf->l2_len);
> > +		ipv4_hdr->hdr_checksum = 0;
> >  		ipv4_hdr->hdr_checksum = rte_ipv4_cksum(ipv4_hdr);
> >  	}
> >  
> >   
> 
> Other than that:
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> I'll add the Fixes tag while applying.

Not sure if you will send to stable, so I did v2 just in case.
Feel free to drop either one if you took care of those items already.
Thanks!
fbl
Maxime Coquelin Oct. 24, 2019, 4:32 p.m. UTC | #3 
On 10/24/19 6:31 PM, Flavio Leitner wrote:
> On Thu, 24 Oct 2019 16:32:43 +0200
> Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
> 
>> Hi Flavio,
>>
>> On 10/24/19 4:28 PM, Flavio Leitner wrote:
>>> Currently the IPv4 header checksum is calculated including its
>>> current value, which can be a valid checksum or just garbage.
>>> In any case, if the original value is not zero, then the result
>>> is always wrong.
>>>
>>> The IPv4 checksum is defined in RFC791, page 14 says:
>>>   Header Checksum:  16 bits
>>>
>>>   The checksum algorithm is:
>>>   The checksum field is the 16 bit one's complement of the one's
>>>   complement sum of all 16 bit words in the header.  For purposes of
>>>   computing the checksum, the value of the checksum field is zero.
>>>
>>> Thus force the csum field to always be zero.  
>>
>> I think we need Fixes tag:
>>
>> Fixes: b08b8cfeb2ae ("vhost: fix IP checksum")
>> Cc: stable@dpdk.org
>>
>> Also adding stable in Cc.
>>
>>
>>> Signed-off-by: Flavio Leitner <fbl@sysclose.org>
>>> ---
>>>  lib/librte_vhost/virtio_net.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/lib/librte_vhost/virtio_net.c
>>> b/lib/librte_vhost/virtio_net.c index eae7825f04..cde7498c76 100644
>>> --- a/lib/librte_vhost/virtio_net.c
>>> +++ b/lib/librte_vhost/virtio_net.c
>>> @@ -445,6 +445,7 @@ virtio_enqueue_offload(struct rte_mbuf *m_buf,
>>> struct virtio_net_hdr *net_hdr) 
>>>  		ipv4_hdr = rte_pktmbuf_mtod_offset(m_buf, struct
>>> rte_ipv4_hdr *, m_buf->l2_len);
>>> +		ipv4_hdr->hdr_checksum = 0;
>>>  		ipv4_hdr->hdr_checksum = rte_ipv4_cksum(ipv4_hdr);
>>>  	}
>>>  
>>>   
>>
>> Other than that:
>>
>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>
>> I'll add the Fixes tag while applying.
> 
> Not sure if you will send to stable, so I did v2 just in case.
> Feel free to drop either one if you took care of those items already.
> Thanks!
> fbl
> 
> 

I synced with Ferruh, and he proposed to pick it directly for -rc1.

Thanks!
Maxime
diff mbox series

Patch

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index eae7825f04..cde7498c76 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -445,6 +445,7 @@  virtio_enqueue_offload(struct rte_mbuf *m_buf, struct virtio_net_hdr *net_hdr)
 
 		ipv4_hdr = rte_pktmbuf_mtod_offset(m_buf, struct rte_ipv4_hdr *,
 						   m_buf->l2_len);
+		ipv4_hdr->hdr_checksum = 0;
 		ipv4_hdr->hdr_checksum = rte_ipv4_cksum(ipv4_hdr);
 	}