[PATCH net] vmxnet3: correctly report gso type for UDP tunnels

[PATCH net] vmxnet3: correctly report gso type for UDP tunnels

[Ronak Doshi]

Commit 3d010c8031e3 ("udp: do not accept non-tunnel GSO skbs landing
in a tunnel") added checks in linux stack to not accept non-tunnel
GRO packets landing in a tunnel. This exposed an issue in vmxnet3
which was not correctly reporting GRO packets for tunnel packets.

This patch fixes this issue by setting correct GSO type for the
tunnel packets.

Fixes: dacce2be3312 ("vmxnet3: add geneve and vxlan tunnel offload support")
Signed-off-by: Ronak Doshi <ronak.doshi@broadcom.com>
Acked-by: Guolin Yang <guolin.yang@broadcom.com>
---
 drivers/net/vmxnet3/vmxnet3_drv.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c b/drivers/net/vmxnet3/vmxnet3_drv.c
index 58027e82de88..9d84ad96ae54 100644
--- a/drivers/net/vmxnet3/vmxnet3_drv.c
+++ b/drivers/net/vmxnet3/vmxnet3_drv.c
@@ -1452,6 +1452,7 @@ vmxnet3_rx_csum(struct vmxnet3_adapter *adapter,
 			if ((le32_to_cpu(gdesc->dword[0]) &
 				     (1UL << VMXNET3_RCD_HDR_INNER_SHIFT))) {
 				skb->csum_level = 1;
+				skb->encapsulation = 1;
 			}
 			WARN_ON_ONCE(!(gdesc->rcd.tcp || gdesc->rcd.udp) &&
 				     !(le32_to_cpu(gdesc->dword[0]) &
@@ -1465,6 +1466,7 @@ vmxnet3_rx_csum(struct vmxnet3_adapter *adapter,
 			if ((le32_to_cpu(gdesc->dword[0]) &
 				     (1UL << VMXNET3_RCD_HDR_INNER_SHIFT))) {
 				skb->csum_level = 1;
+				skb->encapsulation = 1;
 			}
 			WARN_ON_ONCE(!(gdesc->rcd.tcp || gdesc->rcd.udp) &&
 				     !(le32_to_cpu(gdesc->dword[0]) &
@@ -1568,6 +1570,30 @@ vmxnet3_get_hdr_len(struct vmxnet3_adapter *adapter, struct sk_buff *skb,
 	return (hlen + (hdr.tcp->doff << 2));
 }
 
+static void
+vmxnet3_lro_tunnel(struct sk_buff *skb, __be16 ip_proto)
+{
+	struct udphdr *uh = NULL;
+
+	if (ip_proto == htons(ETH_P_IP)) {
+		struct iphdr *iph = (struct iphdr *)skb->data;
+
+		if (iph->protocol == IPPROTO_UDP)
+			uh = (struct udphdr *)(iph + 1);
+	} else {
+		struct ipv6hdr *iph = (struct ipv6hdr *)skb->data;
+
+		if (iph->nexthdr == IPPROTO_UDP)
+			uh = (struct udphdr *)(iph + 1);
+	}
+	if (uh) {
+		if (uh->check)
+			skb_shinfo(skb)->gso_type |= SKB_GSO_UDP_TUNNEL_CSUM;
+		else
+			skb_shinfo(skb)->gso_type |= SKB_GSO_UDP_TUNNEL;
+	}
+}
+
 static int
 vmxnet3_rq_rx_complete(struct vmxnet3_rx_queue *rq,
 		       struct vmxnet3_adapter *adapter, int quota)
@@ -1881,6 +1907,8 @@ vmxnet3_rq_rx_complete(struct vmxnet3_rx_queue *rq,
 			if (segCnt != 0 && mss != 0) {
 				skb_shinfo(skb)->gso_type = rcd->v4 ?
 					SKB_GSO_TCPV4 : SKB_GSO_TCPV6;
+				if (encap_lro)
+					vmxnet3_lro_tunnel(skb, skb->protocol);
 				skb_shinfo(skb)->gso_size = mss;
 				skb_shinfo(skb)->gso_segs = segCnt;
 			} else if ((segCnt != 0 || skb->len > mtu) && !encap_lro) {
-- 
2.45.2

Re: [PATCH net] vmxnet3: correctly report gso type for UDP tunnels

[Jakub Kicinski]

On Tue, 13 May 2025 21:05:02 +0000 Ronak Doshi wrote:
> +				skb->encapsulation = 1;
>  			}
>  			WARN_ON_ONCE(!(gdesc->rcd.tcp || gdesc->rcd.udp) &&
>  				     !(le32_to_cpu(gdesc->dword[0]) &
> @@ -1465,6 +1466,7 @@ vmxnet3_rx_csum(struct vmxnet3_adapter *adapter,
>  			if ((le32_to_cpu(gdesc->dword[0]) &
>  				     (1UL << VMXNET3_RCD_HDR_INNER_SHIFT))) {
>  				skb->csum_level = 1;
> +				skb->encapsulation = 1;

IIRC ->encapsulation means that ->inner.. fields are valid, no?
And I don't see you setting any of these.

Paolo, please keep me honest, IIUC you have very recent and very
relevant experience with virtio.

Re: [PATCH net] vmxnet3: correctly report gso type for UDP tunnels

[Ronak Doshi]

On Thu, May 15, 2025 at 7:02 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 13 May 2025 21:05:02 +0000 Ronak Doshi wrote:
> > +                             skb->encapsulation = 1;
> >                       }
> >                       WARN_ON_ONCE(!(gdesc->rcd.tcp || gdesc->rcd.udp) &&
> >                                    !(le32_to_cpu(gdesc->dword[0]) &
> > @@ -1465,6 +1466,7 @@ vmxnet3_rx_csum(struct vmxnet3_adapter *adapter,
> >                       if ((le32_to_cpu(gdesc->dword[0]) &
> >                                    (1UL << VMXNET3_RCD_HDR_INNER_SHIFT))) {
> >                               skb->csum_level = 1;
> > +                             skb->encapsulation = 1;
>
> IIRC ->encapsulation means that ->inner.. fields are valid, no?
> And I don't see you setting any of these.
>
> Paolo, please keep me honest, IIUC you have very recent and very
> relevant experience with virtio.

I did not hit any issues during Vxlan and Geneve tunnel testing. I did not find
the code which validates inner fields being set. Maybe I missed something. If
you and Paolo think inner fields are indeed required, then I will remove these
lines.

Thanks,
Ronak

Re: [PATCH net] vmxnet3: correctly report gso type for UDP tunnels

[Jakub Kicinski]

On Thu, 15 May 2025 13:38:49 -0700 Ronak Doshi wrote:
> > IIRC ->encapsulation means that ->inner.. fields are valid, no?
> > And I don't see you setting any of these.
> >
> > Paolo, please keep me honest, IIUC you have very recent and very
> > relevant experience with virtio.  
> 
> I did not hit any issues during Vxlan and Geneve tunnel testing. I did not find
> the code which validates inner fields being set. Maybe I missed something. If
> you and Paolo think inner fields are indeed required, then I will remove these
> lines.

Not sure if the stack itself cares, but drivers look at those 
fields for TSO. I see a call to skb_inner_transport_offset() 
in vmxnet3_parse_hdr(). One thing to try would be to configure
the machine for forwarding so that the packet comes via LRO
and leaves via TSO.

Re: [PATCH net] vmxnet3: correctly report gso type for UDP tunnels

[Ronak Doshi]

On Thu, May 15, 2025 at 2:25 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Not sure if the stack itself cares, but drivers look at those
> fields for TSO. I see a call to skb_inner_transport_offset()
> in vmxnet3_parse_hdr(). One thing to try would be to configure
> the machine for forwarding so that the packet comes via LRO
> and leaves via TSO.

I see. Yes, drivers do check for it. Sure, let me update the patch to not set
encapsulation.

Thanks,
Ronak

Re: [PATCH net] vmxnet3: correctly report gso type for UDP tunnels

[Paolo Abeni]

On 5/15/25 4:02 PM, Jakub Kicinski wrote:
> On Tue, 13 May 2025 21:05:02 +0000 Ronak Doshi wrote:
>> +				skb->encapsulation = 1;
>>  			}
>>  			WARN_ON_ONCE(!(gdesc->rcd.tcp || gdesc->rcd.udp) &&
>>  				     !(le32_to_cpu(gdesc->dword[0]) &
>> @@ -1465,6 +1466,7 @@ vmxnet3_rx_csum(struct vmxnet3_adapter *adapter,
>>  			if ((le32_to_cpu(gdesc->dword[0]) &
>>  				     (1UL << VMXNET3_RCD_HDR_INNER_SHIFT))) {
>>  				skb->csum_level = 1;
>> +				skb->encapsulation = 1;
> 
> IIRC ->encapsulation means that ->inner.. fields are valid, no?
> And I don't see you setting any of these.
> 
> Paolo, please keep me honest, IIUC you have very recent and very
> relevant experience with virtio.

Yes. Specifically the GSO code expect the inner headers to be set,
otherwise the segmentation will yield quite wrong results.

Note that reproducing the issue requires a quite specific setup, i.e.
bridging (or tc redirecting) the ingress traffic from an
UDP-tunnel-HW-GRO enabled device into an egress device not supporting
tx-udp_tnl-segmentation.

If otherwise the traffic goes into the UDP tunnel rx path, such
processing will set the needed field correctly and no issue could/should
be observed AFAICS.

@Ronak: I think the problem pre-exists this specific patch, but since
you are fixing the relevant offload, I think it should be better to
address the problem now.

Thanks,

Paolo

Re: [PATCH net] vmxnet3: correctly report gso type for UDP tunnels

[Paolo Abeni]

On 5/16/25 1:38 AM, Ronak Doshi wrote:
> On Thu, May 15, 2025 at 2:25 PM Jakub Kicinski <kuba@kernel.org> wrote:
>>
>> Not sure if the stack itself cares, but drivers look at those
>> fields for TSO. I see a call to skb_inner_transport_offset()
>> in vmxnet3_parse_hdr(). One thing to try would be to configure
>> the machine for forwarding so that the packet comes via LRO
>> and leaves via TSO.
> 
> I see. Yes, drivers do check for it. Sure, let me update the patch to not set
> encapsulation.

AFAICS, not setting skb->encapsulation, but still building a GSO packet
would still break the s/w segmentation in the scenario described by my
previous email.

/P

Re: [PATCH net] vmxnet3: correctly report gso type for UDP tunnels

[Ronak Doshi]

On Mon, May 19, 2025 at 12:30 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> If otherwise the traffic goes into the UDP tunnel rx path, such
> processing will set the needed field correctly and no issue could/should
> be observed AFAICS.
>
> @Ronak: I think the problem pre-exists this specific patch, but since
> you are fixing the relevant offload, I think it should be better to
> address the problem now.
>
Can we apply this fix which unblocks one of our customer case and address this
concern as a separate patch as it has been there for a while and it
has a workaround
of enabling tnl segmentation on the redirected interface? I think it
might require quite
some change in vmxnet3 to address this concern and can be done as a
different patch.
Meanwhile, I will raise an internal (broadcom) PR for recreating this
specific issue.

Thanks,
Ronak

Re: [PATCH net] vmxnet3: correctly report gso type for UDP tunnels

[Ronak Doshi]

On Tue, May 27, 2025 at 9:10 AM Ronak Doshi <ronak.doshi@broadcom.com> wrote:
>
> On Mon, May 19, 2025 at 12:30 AM Paolo Abeni <pabeni@redhat.com> wrote:
> >
> > If otherwise the traffic goes into the UDP tunnel rx path, such
> > processing will set the needed field correctly and no issue could/should
> > be observed AFAICS.
> >
> > @Ronak: I think the problem pre-exists this specific patch, but since
> > you are fixing the relevant offload, I think it should be better to
> > address the problem now.
> >
> Can we apply this fix which unblocks one of our customer case and address this
> concern as a separate patch as it has been there for a while and it
> has a workaround
> of enabling tnl segmentation on the redirected interface? I think it
> might require quite
> some change in vmxnet3 to address this concern and can be done as a
> different patch.
> Meanwhile, I will raise an internal (broadcom) PR for recreating this
> specific issue.
>
Hello Jakub,
Any update on this? Can you help apply this patch?

Thanks,
Ronak

Re: [PATCH net] vmxnet3: correctly report gso type for UDP tunnels

[Jakub Kicinski]

On Thu, 29 May 2025 14:55:20 -0700 Ronak Doshi wrote:
> On Tue, May 27, 2025 at 9:10 AM Ronak Doshi <ronak.doshi@broadcom.com> wrote:
> > On Mon, May 19, 2025 at 12:30 AM Paolo Abeni <pabeni@redhat.com> wrote:  
> > >
> > > If otherwise the traffic goes into the UDP tunnel rx path, such
> > > processing will set the needed field correctly and no issue could/should
> > > be observed AFAICS.
> > >
> > > @Ronak: I think the problem pre-exists this specific patch, but since
> > > you are fixing the relevant offload, I think it should be better to
> > > address the problem now.
> > >  
> > Can we apply this fix which unblocks one of our customer case and address this
> > concern as a separate patch as it has been there for a while and it
> > has a workaround
> > of enabling tnl segmentation on the redirected interface? I think it
> > might require quite
> > some change in vmxnet3 to address this concern and can be done as a
> > different patch.
> > Meanwhile, I will raise an internal (broadcom) PR for recreating this
> > specific issue.
> >  
> Hello Jakub,
> Any update on this? Can you help apply this patch?

You put Paolo in the To: field, so I assumed your messages are directed
to him. I'm not entirely sure what you're proposing, to apply this
patch as is? Whether your driver supports segmentation or not - it
should not send mangled skbs into the stack. Maybe send a v2 and
explain next steps in the commit message, less guessing the better..

Re: [PATCH net] vmxnet3: correctly report gso type for UDP tunnels

[Ronak Doshi]

On Thu, May 29, 2025 at 4:33 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> You put Paolo in the To: field, so I assumed your messages are directed
> to him. I'm not entirely sure what you're proposing, to apply this
> patch as is? Whether your driver supports segmentation or not - it
> should not send mangled skbs into the stack. Maybe send a v2 and
> explain next steps in the commit message, less guessing the better..

Sure, let me send a v2 patch to get this patch applied first and
explain next steps
to get the inner fields fixed in a future patch.


Thanks,
Ronak