[PATCH net] gve: ignore nonrelevant GSO type bits when processing TSO headers

[PATCH net] gve: ignore nonrelevant GSO type bits when processing TSO headers

[joshwash]

From: Joshua Washington <joshwash@google.com>

TSO currently fails when the skb's gso_type field has more than one bit
set.

TSO packets can be passed from userspace using PF_PACKET, TUNTAP and a
few others, using virtio_net_hdr (e.g., PACKET_VNET_HDR). This includes
virtualization, such as QEMU, a real use-case.

The gso_type and gso_size fields as passed from userspace in
virtio_net_hdr are not trusted blindly by the kernel. It adds gso_type
|= SKB_GSO_DODGY to force the packet to enter the software GSO stack
for verification.

This issue might similarly come up when the CWR bit is set in the TCP
header for congestion control, causing the SKB_GSO_TCP_ECN gso_type bit
to be set.

Fixes: a57e5de476be ("gve: DQO: Add TX path")

Signed-off-by: Joshua Washington <joshwash@google.com>
Reviewed-by: Praveen Kaligineedi <pkaligineedi@google.com>
Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com>
Suggested-by: Eric Dumazet <edumazet@google.com>
---
 drivers/net/ethernet/google/gve/gve_tx_dqo.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
index fe1b26a4d736..04cb43a97c96 100644
--- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c
+++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
@@ -555,6 +555,10 @@ static int gve_prep_tso(struct sk_buff *skb)
 	if (unlikely(skb_shinfo(skb)->gso_size < GVE_TX_MIN_TSO_MSS_DQO))
 		return -1;
 
+	/* We only deal with TCP at this point. */
+	if (!(skb_shinfo(skb)->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)))
+		return -EINVAL;
+
 	/* Needed because we will modify header. */
 	err = skb_cow_head(skb, 0);
 	if (err < 0)
@@ -565,18 +569,10 @@ static int gve_prep_tso(struct sk_buff *skb)
 	/* Remove payload length from checksum. */
 	paylen = skb->len - skb_transport_offset(skb);
 
-	switch (skb_shinfo(skb)->gso_type) {
-	case SKB_GSO_TCPV4:
-	case SKB_GSO_TCPV6:
-		csum_replace_by_diff(&tcp->check,
-				     (__force __wsum)htonl(paylen));
+	csum_replace_by_diff(&tcp->check, (__force __wsum)htonl(paylen));
 
-		/* Compute length of segmentation header. */
-		header_len = skb_tcp_all_headers(skb);
-		break;
-	default:
-		return -EINVAL;
-	}
+	/* Compute length of segmentation header. */
+	header_len = skb_tcp_all_headers(skb);
 
 	if (unlikely(header_len > GVE_TX_MAX_HDR_SIZE_DQO))
 		return -EINVAL;
-- 
2.45.1.288.g0e0cd299f1-goog

Re: [PATCH net] gve: ignore nonrelevant GSO type bits when processing TSO headers

[Willem de Bruijn]

joshwash@ wrote:
> From: Joshua Washington <joshwash@google.com>
> 
> TSO currently fails when the skb's gso_type field has more than one bit
> set.
> 
> TSO packets can be passed from userspace using PF_PACKET, TUNTAP and a
> few others, using virtio_net_hdr (e.g., PACKET_VNET_HDR). This includes
> virtualization, such as QEMU, a real use-case.
> 
> The gso_type and gso_size fields as passed from userspace in
> virtio_net_hdr are not trusted blindly by the kernel. It adds gso_type
> |= SKB_GSO_DODGY to force the packet to enter the software GSO stack
> for verification.
> 
> This issue might similarly come up when the CWR bit is set in the TCP
> header for congestion control, causing the SKB_GSO_TCP_ECN gso_type bit
> to be set.
> 
> Fixes: a57e5de476be ("gve: DQO: Add TX path")

nit: no empty line

> Signed-off-by: Joshua Washington <joshwash@google.com>
> Reviewed-by: Praveen Kaligineedi <pkaligineedi@google.com>
> Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com>
> Suggested-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

> ---
>  drivers/net/ethernet/google/gve/gve_tx_dqo.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> index fe1b26a4d736..04cb43a97c96 100644
> --- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> @@ -555,6 +555,10 @@ static int gve_prep_tso(struct sk_buff *skb)
>  	if (unlikely(skb_shinfo(skb)->gso_size < GVE_TX_MIN_TSO_MSS_DQO))
>  		return -1;
>  
> +	/* We only deal with TCP at this point. */
> +	if (!(skb_shinfo(skb)->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)))
> +		return -EINVAL;
> +

NETIF_F_TSO and NETIF_F_TSO6 are the only terminal/L4 segmentation
offload types that gve advertises in hw_features. So I think that this
will always be true.

If nothing else, it documents the assumption, so fine to keep.

Careful about comments that just repeat what the code does. More
informative are comments that why non-obvious code exists (where
applicable, which is not here).

>  	/* Needed because we will modify header. */
>  	err = skb_cow_head(skb, 0);
>  	if (err < 0)
> @@ -565,18 +569,10 @@ static int gve_prep_tso(struct sk_buff *skb)
>  	/* Remove payload length from checksum. */
>  	paylen = skb->len - skb_transport_offset(skb);
>  
> -	switch (skb_shinfo(skb)->gso_type) {
> -	case SKB_GSO_TCPV4:
> -	case SKB_GSO_TCPV6:
> -		csum_replace_by_diff(&tcp->check,
> -				     (__force __wsum)htonl(paylen));
> +	csum_replace_by_diff(&tcp->check, (__force __wsum)htonl(paylen));
>  
> -		/* Compute length of segmentation header. */
> -		header_len = skb_tcp_all_headers(skb);
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> +	/* Compute length of segmentation header. */
> +	header_len = skb_tcp_all_headers(skb);
>  
>  	if (unlikely(header_len > GVE_TX_MAX_HDR_SIZE_DQO))
>  		return -EINVAL;
> -- 
> 2.45.1.288.g0e0cd299f1-goog
> 

Re: [PATCH net] gve: ignore nonrelevant GSO type bits when processing TSO headers

[Andrei Vagin]

On Thu, Jun 6, 2024 at 12:22 PM <joshwash@google.com> wrote:
>
> From: Joshua Washington <joshwash@google.com>
>
> TSO currently fails when the skb's gso_type field has more than one bit
> set.
>
> TSO packets can be passed from userspace using PF_PACKET, TUNTAP and a
> few others, using virtio_net_hdr (e.g., PACKET_VNET_HDR). This includes
> virtualization, such as QEMU, a real use-case.

Here is the bug report where this issue was triggered by gVisor:
https://github.com/google/gvisor/issues/10344

>
> The gso_type and gso_size fields as passed from userspace in
> virtio_net_hdr are not trusted blindly by the kernel. It adds gso_type
> |= SKB_GSO_DODGY to force the packet to enter the software GSO stack
> for verification.
>
> This issue might similarly come up when the CWR bit is set in the TCP
> header for congestion control, causing the SKB_GSO_TCP_ECN gso_type bit
> to be set.
>
> Fixes: a57e5de476be ("gve: DQO: Add TX path")
>
> Signed-off-by: Joshua Washington <joshwash@google.com>
> Reviewed-by: Praveen Kaligineedi <pkaligineedi@google.com>
> Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com>
> Suggested-by: Eric Dumazet <edumazet@google.com>

Acked-by: Andrei Vagin <avagin@gmail.com>

Thanks,
Andrei

[PATCH net v2] gve: ignore nonrelevant GSO type bits when processing TSO headers

[joshwash]

From: Joshua Washington <joshwash@google.com>

TSO currently fails when the skb's gso_type field has more than one bit
set.

TSO packets can be passed from userspace using PF_PACKET, TUNTAP and a
few others, using virtio_net_hdr (e.g., PACKET_VNET_HDR). This includes
virtualization, such as QEMU, a real use-case.

The gso_type and gso_size fields as passed from userspace in
virtio_net_hdr are not trusted blindly by the kernel. It adds gso_type
|= SKB_GSO_DODGY to force the packet to enter the software GSO stack
for verification.

This issue might similarly come up when the CWR bit is set in the TCP
header for congestion control, causing the SKB_GSO_TCP_ECN gso_type bit
to be set.

Fixes: a57e5de476be ("gve: DQO: Add TX path")
Signed-off-by: Joshua Washington <joshwash@google.com>
Reviewed-by: Praveen Kaligineedi <pkaligineedi@google.com>
Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com>
Reviewed-by: Willem de Bruijn <willemb@google.com>
Suggested-by: Eric Dumazet <edumazet@google.com>
Acked-by: Andrei Vagin <avagin@gmail.com>
---
 drivers/net/ethernet/google/gve/gve_tx_dqo.c | 21 +++++---------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
index fe1b26a4d736..a76b407a981b 100644
--- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c
+++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
@@ -551,32 +551,21 @@ static int gve_prep_tso(struct sk_buff *skb)
 	 * - Hypervisor enforces a limit of 9K MTU
 	 * - Kernel will not produce a TSO larger than 64k
 	 */
-
 	if (unlikely(skb_shinfo(skb)->gso_size < GVE_TX_MIN_TSO_MSS_DQO))
 		return -1;
 
+	if (!(skb_shinfo(skb)->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)))
+		return -EINVAL;
+
 	/* Needed because we will modify header. */
 	err = skb_cow_head(skb, 0);
 	if (err < 0)
 		return err;
 
 	tcp = tcp_hdr(skb);
-
-	/* Remove payload length from checksum. */
 	paylen = skb->len - skb_transport_offset(skb);
-
-	switch (skb_shinfo(skb)->gso_type) {
-	case SKB_GSO_TCPV4:
-	case SKB_GSO_TCPV6:
-		csum_replace_by_diff(&tcp->check,
-				     (__force __wsum)htonl(paylen));
-
-		/* Compute length of segmentation header. */
-		header_len = skb_tcp_all_headers(skb);
-		break;
-	default:
-		return -EINVAL;
-	}
+	csum_replace_by_diff(&tcp->check, (__force __wsum)htonl(paylen));
+	header_len = skb_tcp_all_headers(skb);
 
 	if (unlikely(header_len > GVE_TX_MAX_HDR_SIZE_DQO))
 		return -EINVAL;
-- 
2.45.2.505.gda0bf45e8d-goog

Re: [PATCH net v2] gve: ignore nonrelevant GSO type bits when processing TSO headers

[Eric Dumazet]

On Fri, Jun 7, 2024 at 8:10 AM <joshwash@google.com> wrote:
>
> From: Joshua Washington <joshwash@google.com>
>
> TSO currently fails when the skb's gso_type field has more than one bit
> set.
>
> TSO packets can be passed from userspace using PF_PACKET, TUNTAP and a
> few others, using virtio_net_hdr (e.g., PACKET_VNET_HDR). This includes
> virtualization, such as QEMU, a real use-case.
>
> The gso_type and gso_size fields as passed from userspace in
> virtio_net_hdr are not trusted blindly by the kernel. It adds gso_type
> |= SKB_GSO_DODGY to force the packet to enter the software GSO stack
> for verification.
>
> This issue might similarly come up when the CWR bit is set in the TCP
> header for congestion control, causing the SKB_GSO_TCP_ECN gso_type bit
> to be set.
>
> Fixes: a57e5de476be ("gve: DQO: Add TX path")
> Signed-off-by: Joshua Washington <joshwash@google.com>
> Reviewed-by: Praveen Kaligineedi <pkaligineedi@google.com>
> Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com>
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Acked-by: Andrei Vagin <avagin@gmail.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net v2] gve: ignore nonrelevant GSO type bits when processing TSO headers

[Willem de Bruijn]

joshwash@ wrote:
> From: Joshua Washington <joshwash@google.com>
> 
> TSO currently fails when the skb's gso_type field has more than one bit
> set.
> 
> TSO packets can be passed from userspace using PF_PACKET, TUNTAP and a
> few others, using virtio_net_hdr (e.g., PACKET_VNET_HDR). This includes
> virtualization, such as QEMU, a real use-case.
> 
> The gso_type and gso_size fields as passed from userspace in
> virtio_net_hdr are not trusted blindly by the kernel. It adds gso_type
> |= SKB_GSO_DODGY to force the packet to enter the software GSO stack
> for verification.
> 
> This issue might similarly come up when the CWR bit is set in the TCP
> header for congestion control, causing the SKB_GSO_TCP_ECN gso_type bit
> to be set.
> 
> Fixes: a57e5de476be ("gve: DQO: Add TX path")
> Signed-off-by: Joshua Washington <joshwash@google.com>
> Reviewed-by: Praveen Kaligineedi <pkaligineedi@google.com>
> Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com>
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Acked-by: Andrei Vagin <avagin@gmail.com>

I did not mean to ask for a revision. When you send a v2, please do include
a changelog

> ---
>  drivers/net/ethernet/google/gve/gve_tx_dqo.c | 21 +++++---------------
>  1 file changed, 5 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> index fe1b26a4d736..a76b407a981b 100644
> --- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> @@ -551,32 +551,21 @@ static int gve_prep_tso(struct sk_buff *skb)
>  	 * - Hypervisor enforces a limit of 9K MTU
>  	 * - Kernel will not produce a TSO larger than 64k
>  	 */
> -

Accidental removal?

>  	if (unlikely(skb_shinfo(skb)->gso_size < GVE_TX_MIN_TSO_MSS_DQO))
>  		return -1;
>  
> +	if (!(skb_shinfo(skb)->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)))
> +		return -EINVAL;
> +

[PATCH net v3] gve: ignore nonrelevant GSO type bits when processing TSO headers

[joshwash]

From: Joshua Washington <joshwash@google.com>

TSO currently fails when the skb's gso_type field has more than one bit
set.

TSO packets can be passed from userspace using PF_PACKET, TUNTAP and a
few others, using virtio_net_hdr (e.g., PACKET_VNET_HDR). This includes
virtualization, such as QEMU, a real use-case.

The gso_type and gso_size fields as passed from userspace in
virtio_net_hdr are not trusted blindly by the kernel. It adds gso_type
|= SKB_GSO_DODGY to force the packet to enter the software GSO stack
for verification.

This issue might similarly come up when the CWR bit is set in the TCP
header for congestion control, causing the SKB_GSO_TCP_ECN gso_type bit
to be set.

Fixes: a57e5de476be ("gve: DQO: Add TX path")
Signed-off-by: Joshua Washington <joshwash@google.com>
Reviewed-by: Praveen Kaligineedi <pkaligineedi@google.com>
Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com>
Reviewed-by: Willem de Bruijn <willemb@google.com>
Suggested-by: Eric Dumazet <edumazet@google.com>
Acked-by: Andrei Vagin <avagin@gmail.com>

v2 - Remove unnecessary comments, remove line break between fixes tag
and signoffs.

v3 - Add back unrelated empty line removal.
---
 drivers/net/ethernet/google/gve/gve_tx_dqo.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
index fe1b26a4d736..0b3cca3fc792 100644
--- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c
+++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
@@ -555,28 +555,18 @@ static int gve_prep_tso(struct sk_buff *skb)
 	if (unlikely(skb_shinfo(skb)->gso_size < GVE_TX_MIN_TSO_MSS_DQO))
 		return -1;
 
+	if (!(skb_shinfo(skb)->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)))
+		return -EINVAL;
+
 	/* Needed because we will modify header. */
 	err = skb_cow_head(skb, 0);
 	if (err < 0)
 		return err;
 
 	tcp = tcp_hdr(skb);
-
-	/* Remove payload length from checksum. */
 	paylen = skb->len - skb_transport_offset(skb);
-
-	switch (skb_shinfo(skb)->gso_type) {
-	case SKB_GSO_TCPV4:
-	case SKB_GSO_TCPV6:
-		csum_replace_by_diff(&tcp->check,
-				     (__force __wsum)htonl(paylen));
-
-		/* Compute length of segmentation header. */
-		header_len = skb_tcp_all_headers(skb);
-		break;
-	default:
-		return -EINVAL;
-	}
+	csum_replace_by_diff(&tcp->check, (__force __wsum)htonl(paylen));
+	header_len = skb_tcp_all_headers(skb);
 
 	if (unlikely(header_len > GVE_TX_MAX_HDR_SIZE_DQO))
 		return -EINVAL;
-- 
2.45.2.505.gda0bf45e8d-goog

Re: [PATCH net v3] gve: ignore nonrelevant GSO type bits when processing TSO headers

[Jakub Kicinski]

On Mon, 10 Jun 2024 15:57:18 -0700 joshwash@google.com wrote:
> v2 - Remove unnecessary comments, remove line break between fixes tag
> and signoffs.
> 
> v3 - Add back unrelated empty line removal.

Read the maintainer info again, please:
https://www.kernel.org/doc/html/next/process/maintainer-netdev.html
we prefer no in-reply to postings.

Re: [PATCH net v3] gve: ignore nonrelevant GSO type bits when processing TSO headers

[Joshua Washington]

My apologies. I'll send an updated patch tomorrow without --in-reply-to.

On Mon, Jun 10, 2024 at 5:27 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 10 Jun 2024 15:57:18 -0700 joshwash@google.com wrote:
> > v2 - Remove unnecessary comments, remove line break between fixes tag
> > and signoffs.
> >
> > v3 - Add back unrelated empty line removal.
>
> Read the maintainer info again, please:
> https://www.kernel.org/doc/html/next/process/maintainer-netdev.html
> we prefer no in-reply to postings.



-- 

Joshua Washington | Software Engineer | joshwash@google.com | (414) 366-4423

Re: [PATCH net v3] gve: ignore nonrelevant GSO type bits when processing TSO headers

[Jakub Kicinski]

On Mon, 10 Jun 2024 19:26:32 -0700 Joshua Washington wrote:
> My apologies. I'll send an updated patch tomorrow without --in-reply-to.

No need, it's still in patchwork, it was just a note for the future.
I should have made that more clear, I realized that after hitting send.

Re: [PATCH net v3] gve: ignore nonrelevant GSO type bits when processing TSO headers

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 10 Jun 2024 15:57:18 -0700 you wrote:
> From: Joshua Washington <joshwash@google.com>
> 
> TSO currently fails when the skb's gso_type field has more than one bit
> set.
> 
> TSO packets can be passed from userspace using PF_PACKET, TUNTAP and a
> few others, using virtio_net_hdr (e.g., PACKET_VNET_HDR). This includes
> virtualization, such as QEMU, a real use-case.
> 
> [...]

Here is the summary with links:
  - [net,v3] gve: ignore nonrelevant GSO type bits when processing TSO headers
    https://git.kernel.org/netdev/net/c/1b9f75634441

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html