suspicius csum initialization in vmxnet3_rx_csum

suspicius csum initialization in vmxnet3_rx_csum

[Paolo Abeni]

Hi all,

we are hitting the BUG() condition in skb_checksum_help() -
skb_checksum_start_offset(skb) >= skb_headlen(skb) for skb received
from vmnxnet3 and queued from OVS to user-space

I think that the root cause is in vmxnet3_rx_csum():

			if (gdesc->rcd.csum) {
                                skb->csum = htons(gdesc->rcd.csum);
                                skb->ip_summed = CHECKSUM_PARTIAL;

CHECKSUM_PARTIAL looks suspicious here, as the csum field is
initialized, instead of csum_offset/csum_start. To be honest I find
also strange that the csum value is converted from host byte order.

I'm wild guessing something like the below patch should fix the issue,
but I'm not familiar with the vmxnet3 code. Can you please have a look?

Thank you,

Paolo
---
diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
index 9ebe2a689966..06ade074c32c 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -1172,7 +1172,7 @@ vmxnet3_rx_csum(struct vmxnet3_adapter *adapter,
 		} else {
 			if (gdesc->rcd.csum) {
 				skb->csum = htons(gdesc->rcd.csum);
-				skb->ip_summed = CHECKSUM_PARTIAL;
+				skb->ip_summed = CHECKSUM_COMPLETE;
 			} else {
 				skb_checksum_none_assert(skb);
 			}

Re: suspicius csum initialization in vmxnet3_rx_csum

[Paolo Abeni]

Hi,

On Thu, 2018-05-24 at 21:48 +0000, Guolin Yang wrote:
> Yes, that code  is not correct, we should fix that code

Did you have any chance to address the issue and/or to give a more in-
deepth look to the change proposed in my initial email?

Thanks,

Paolo 

Re: suspicius csum initialization in vmxnet3_rx_csum

[Ronak Doshi]

On Wed, 30 May 2018, Paolo Abeni wrote:

> Hi,
> 
> On Thu, 2018-05-24 at 21:48 +0000, Guolin Yang wrote:
> > Yes, that code  is not correct, we should fix that code
> 
> Did you have any chance to address the issue and/or to give a more in-
> deepth look to the change proposed in my initial email?
>  
Hi Paolo,

Can you provide the esx build you are using? It can be found using 
"vmware -vl" on ESX host.

Did you try your proposed fix and did it work? Are you sure the packet
hits the below if block and not the else block? I still don't think the
ICMP packet will go through the below if block.

   if (gdesc->rcd.csum) {
	skb->csum = htons(gdesc->rcd.csum);
	skb->ip_summed = CHECKSUM_PARTIAL;
   } else {
	skb_checksum_none_assert(skb);
   }

The vmxnet3 emulation does not calculate rcd.csum for ICMP packet and
hence should go through the else block i.e. checksum none.

Thanks,
Ronak

Re: suspicius csum initialization in vmxnet3_rx_csum

[Neil Horman]

On Thu, May 31, 2018 at 11:02:34AM -0700, Ronak Doshi wrote:
> 
> On Wed, 30 May 2018, Paolo Abeni wrote:
> 
> > Hi,
> > 
> > On Thu, 2018-05-24 at 21:48 +0000, Guolin Yang wrote:
> > > Yes, that code  is not correct, we should fix that code
> > 
> > Did you have any chance to address the issue and/or to give a more in-
> > deepth look to the change proposed in my initial email?
> >  
> Hi Paolo,
> 
> Can you provide the esx build you are using? It can be found using 
> "vmware -vl" on ESX host.
> 
> Did you try your proposed fix and did it work? Are you sure the packet
> hits the below if block and not the else block? I still don't think the
> ICMP packet will go through the below if block.
> 
>    if (gdesc->rcd.csum) {
> 	skb->csum = htons(gdesc->rcd.csum);
> 	skb->ip_summed = CHECKSUM_PARTIAL;
>    } else {
> 	skb_checksum_none_assert(skb);
>    }
> 
> The vmxnet3 emulation does not calculate rcd.csum for ICMP packet and
> hence should go through the else block i.e. checksum none.
> 
What packet types will rcd.csum be set for?
Neil

> Thanks,
> Ronak
> 

Re: suspicius csum initialization in vmxnet3_rx_csum

[Ronak Doshi]

On Thu, 31 May 2018, Neil Horman wrote:

> On Thu, May 31, 2018 at 11:02:34AM -0700, Ronak Doshi wrote:
> > 
> > On Wed, 30 May 2018, Paolo Abeni wrote:
> > 
> > > Hi,
> > > 
> > > On Thu, 2018-05-24 at 21:48 +0000, Guolin Yang wrote:
> > > > Yes, that code  is not correct, we should fix that code
> > > 
> > > Did you have any chance to address the issue and/or to give a more in-
> > > deepth look to the change proposed in my initial email?
> > >  
> > Hi Paolo,
> > 
> > Can you provide the esx build you are using? It can be found using 
> > "vmware -vl" on ESX host.
> > 
> > Did you try your proposed fix and did it work? Are you sure the packet
> > hits the below if block and not the else block? I still don't think the
> > ICMP packet will go through the below if block.
> > 
> >    if (gdesc->rcd.csum) {
> > 	skb->csum = htons(gdesc->rcd.csum);
> > 	skb->ip_summed = CHECKSUM_PARTIAL;
> >    } else {
> > 	skb_checksum_none_assert(skb);
> >    }
> > 
> > The vmxnet3 emulation does not calculate rcd.csum for ICMP packet and
> > hence should go through the else block i.e. checksum none.
> > 
> What packet types will rcd.csum be set for?
> Neil 
> 
I looked thorugh the emulation code and found that rcd.csum is not set. 
For valid v4/v6, TCP/UDP packets the code block above the mentioend "if" 
block will be executed or else it will go through checksum none.

That's why I wanted to know (in previous emails) which ESX build is being 
used while this was tested. The code block under "if (gdesc->rcd.csum)" 
block might seem incorrect but it shouldn't be hit as rcd.csum is not set. 
Hence, I asked did the fix provided by Paolo worked for the icmp test?

Thanks,
Ronak

Re: suspicius csum initialization in vmxnet3_rx_csum

[Neil Horman]

On Fri, Jun 01, 2018 at 11:10:01AM -0700, Ronak Doshi wrote:
> 
> 
> On Thu, 31 May 2018, Neil Horman wrote:
> 
> > On Thu, May 31, 2018 at 11:02:34AM -0700, Ronak Doshi wrote:
> > > 
> > > On Wed, 30 May 2018, Paolo Abeni wrote:
> > > 
> > > > Hi,
> > > > 
> > > > On Thu, 2018-05-24 at 21:48 +0000, Guolin Yang wrote:
> > > > > Yes, that code  is not correct, we should fix that code
> > > > 
> > > > Did you have any chance to address the issue and/or to give a more in-
> > > > deepth look to the change proposed in my initial email?
> > > >  
> > > Hi Paolo,
> > > 
> > > Can you provide the esx build you are using? It can be found using 
> > > "vmware -vl" on ESX host.
> > > 
> > > Did you try your proposed fix and did it work? Are you sure the packet
> > > hits the below if block and not the else block? I still don't think the
> > > ICMP packet will go through the below if block.
> > > 
> > >    if (gdesc->rcd.csum) {
> > > 	skb->csum = htons(gdesc->rcd.csum);
> > > 	skb->ip_summed = CHECKSUM_PARTIAL;
> > >    } else {
> > > 	skb_checksum_none_assert(skb);
> > >    }
> > > 
> > > The vmxnet3 emulation does not calculate rcd.csum for ICMP packet and
> > > hence should go through the else block i.e. checksum none.
> > > 
> > What packet types will rcd.csum be set for?
> > Neil 
> > 
> I looked thorugh the emulation code and found that rcd.csum is not set. 
> For valid v4/v6, TCP/UDP packets the code block above the mentioend "if" 
> block will be executed or else it will go through checksum none.
> 
> That's why I wanted to know (in previous emails) which ESX build is being 
> used while this was tested. The code block under "if (gdesc->rcd.csum)" 
> block might seem incorrect but it shouldn't be hit as rcd.csum is not set. 
> Hence, I asked did the fix provided by Paolo worked for the icmp test?
> 
Paolo will have to tell you that specifically I'm afraid.  I'm not sure if the patch was tested or not.
> Thanks,
> Ronak
> 

Re: suspicius csum initialization in vmxnet3_rx_csum

[Paolo Abeni]

Hi,

I'm sorry for the long delay in my answer, I've been travelling.

On Fri, 2018-06-01 at 11:10 -0700, Ronak Doshi wrote:
> On Thu, 31 May 2018, Neil Horman wrote:
> > What packet types will rcd.csum be set for?
> > Neil 
> 
> I looked thorugh the emulation code and found that rcd.csum is not set. 
> For valid v4/v6, TCP/UDP packets the code block above the mentioend "if" 
> block will be executed or else it will go through checksum none.
> 
> That's why I wanted to know (in previous emails) which ESX build is being 
> used while this was tested. The code block under "if (gdesc->rcd.csum)" 
> block might seem incorrect but it shouldn't be hit as rcd.csum is not set. 

I'm unsure if I read the above correctly. Do you mean that the relevant
code-path is never hit? If so, can we simply drop it, as we agreed that
such code is uncorrect? Elsewhere, could you plese specify under which
circumstances gdesc->rcd.csum is filled by the hypervisor?

> Hence, I asked did the fix provided by Paolo worked for the icmp test?

Unfortunatelly so far I've not been able to reproduce the issue outside
a production environment and I can't run test kernel there.

Thanks,

Paolo

Re: suspicius csum initialization in vmxnet3_rx_csum

[Ronak Doshi]

On Tue, 5 Jun 2018, Paolo Abeni wrote:

> Hi,
> 
> I'm sorry for the long delay in my answer, I've been travelling.
> 
> On Fri, 2018-06-01 at 11:10 -0700, Ronak Doshi wrote:
> > On Thu, 31 May 2018, Neil Horman wrote:
> > > What packet types will rcd.csum be set for?
> > > Neil 
> > 
> > I looked thorugh the emulation code and found that rcd.csum is not set. 
> > For valid v4/v6, TCP/UDP packets the code block above the mentioend "if" 
> > block will be executed or else it will go through checksum none.
> > 
> > That's why I wanted to know (in previous emails) which ESX build is being 
> > used while this was tested. The code block under "if (gdesc->rcd.csum)" 
> > block might seem incorrect but it shouldn't be hit as rcd.csum is not set. 
> 
> I'm unsure if I read the above correctly. Do you mean that the relevant
> code-path is never hit? If so, can we simply drop it, as we agreed that
> such code is uncorrect? Elsewhere, could you plese specify under which
> circumstances gdesc->rcd.csum is filled by the hypervisor?
>
I do not see hypervisor populating rcd.csum field or may be the code has 
changed over the years. So, the codepath should not be hit as it is not 
populated. I will check and fix it or remove the block if not required. 
But as far as your issue is concerned, that code block is not hit.
 
Thanks,
Ronak