[PATCH iwl-next v2 0/4] return value of skb_checksum_help()

[PATCH iwl-next v2 0/4] return value of skb_checksum_help()

[Michal Swiatkowski]

Hi,

most intel drivers ignore the return value of skb_checksum_help(). It
can fail, so pass this value to the caller, to allow dropping packet
without correct checksum.

Michal Swiatkowski (4):
  ice: pass the return value of skb_checksum_help()
  i40e: pass the return value of skb_checksum_help()
  iavf: pass the return value of skb_checksum_help()
  idpf: pass the return value of skb_checksum_help()

 drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 18 +++++++--------
 drivers/net/ethernet/intel/iavf/iavf_txrx.c   | 17 +++++++-------
 drivers/net/ethernet/intel/ice/ice_txrx.c     | 20 ++++++++---------
 .../ethernet/intel/idpf/idpf_singleq_txrx.c   | 22 ++++++++-----------
 4 files changed, 34 insertions(+), 43 deletions(-)

-- 
2.49.0

[PATCH iwl-next v2 1/4] ice: pass the return value of skb_checksum_help()

[Michal Swiatkowski]

skb_checksum_help() can fail. Pass its return value back to the caller.

Commonize this software path in goto.

Instead of just returning error try calculating software checksum first.
There is a check for TSO in checksum_sw_fb.

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
---
 drivers/net/ethernet/intel/ice/ice_txrx.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
index 94129a7a9e12..f52a4af36eb0 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
@@ -1673,7 +1673,7 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
 			ret = ipv6_skip_exthdr(skb, exthdr - skb->data,
 					       &l4_proto, &frag_off);
 			if (ret < 0)
-				return -1;
+				goto checksum_sw_fb;
 		}
 
 		/* define outer transport */
@@ -1692,11 +1692,7 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
 			l4.hdr = skb_inner_network_header(skb);
 			break;
 		default:
-			if (first->tx_flags & ICE_TX_FLAGS_TSO)
-				return -1;
-
-			skb_checksum_help(skb);
-			return 0;
+			goto checksum_sw_fb;
 		}
 
 		/* compute outer L3 header size */
@@ -1755,7 +1751,7 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
 			ipv6_skip_exthdr(skb, exthdr - skb->data, &l4_proto,
 					 &frag_off);
 	} else {
-		return -1;
+		goto checksum_sw_fb;
 	}
 
 	/* compute inner L3 header size */
@@ -1808,15 +1804,17 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
 		break;
 
 	default:
-		if (first->tx_flags & ICE_TX_FLAGS_TSO)
-			return -1;
-		skb_checksum_help(skb);
-		return 0;
+		goto checksum_sw_fb;
 	}
 
 	off->td_cmd |= cmd;
 	off->td_offset |= offset;
 	return 1;
+
+checksum_sw_fb:
+	if (first->tx_flags & ICE_TX_FLAGS_TSO)
+		return -1;
+	return skb_checksum_help(skb);
 }
 
 /**
-- 
2.49.0

[PATCH iwl-next v2 2/4] i40e: pass the return value of skb_checksum_help()

[Michal Swiatkowski]

skb_checksum_help() can fail. Pass its return value back to the caller.

Commonize this software path in goto.

Try calculating in software instead of returning error.

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index cfaf724ee7ff..6e336696276f 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -3306,7 +3306,7 @@ static int i40e_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
 			ret = ipv6_skip_exthdr(skb, exthdr - skb->data,
 					       &l4_proto, &frag_off);
 			if (ret < 0)
-				return -1;
+				goto checksum_sw_fb;
 		}
 
 		/* define outer transport */
@@ -3325,11 +3325,7 @@ static int i40e_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
 			l4.hdr = skb_inner_network_header(skb);
 			break;
 		default:
-			if (*tx_flags & I40E_TX_FLAGS_TSO)
-				return -1;
-
-			skb_checksum_help(skb);
-			return 0;
+			goto checksum_sw_fb;
 		}
 
 		/* compute outer L3 header size */
@@ -3406,16 +3402,18 @@ static int i40e_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
 			  I40E_TX_DESC_LENGTH_L4_FC_LEN_SHIFT;
 		break;
 	default:
-		if (*tx_flags & I40E_TX_FLAGS_TSO)
-			return -1;
-		skb_checksum_help(skb);
-		return 0;
+		goto checksum_sw_fb;
 	}
 
 	*td_cmd |= cmd;
 	*td_offset |= offset;
 
 	return 1;
+
+checksum_sw_fb:
+	if (*tx_flags & I40E_TX_FLAGS_TSO)
+		return -1;
+	return skb_checksum_help(skb);
 }
 
 /**
-- 
2.49.0

[PATCH iwl-next v2 3/4] iavf: pass the return value of skb_checksum_help()

[Michal Swiatkowski]

skb_checksum_help() can fail. Pass its return value back to the caller.

Commonize this software path in goto.

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_txrx.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_txrx.c b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
index ab46a49bb5e0..c5d4486c0396 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_txrx.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
@@ -1909,11 +1909,7 @@ static int iavf_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
 			l4.hdr = skb_inner_network_header(skb);
 			break;
 		default:
-			if (*tx_flags & IAVF_TX_FLAGS_TSO)
-				return -1;
-
-			skb_checksum_help(skb);
-			return 0;
+			goto checksum_sw_fb;
 		}
 
 		/* compute outer L3 header size */
@@ -1990,16 +1986,19 @@ static int iavf_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
 			  IAVF_TX_DESC_LENGTH_L4_FC_LEN_SHIFT;
 		break;
 	default:
-		if (*tx_flags & IAVF_TX_FLAGS_TSO)
-			return -1;
-		skb_checksum_help(skb);
-		return 0;
+		goto checksum_sw_fb;
 	}
 
 	*td_cmd |= cmd;
 	*td_offset |= offset;
 
 	return 1;
+
+checksum_sw_fb:
+	if (*tx_flags & IAVF_TX_FLAGS_TSO)
+		return -1;
+
+	return skb_checksum_help(skb);
 }
 
 /**
-- 
2.49.0

[PATCH iwl-next v2 4/4] idpf: pass the return value of skb_checksum_help()

[Michal Swiatkowski]

skb_checksum_help() can fail. Pass its return value back to the caller.

Commonize this software path in goto.

Instead of just returning error try calculating software checksum first.
There is a check for TSO in checksum_sw_fb.

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
---
 .../ethernet/intel/idpf/idpf_singleq_txrx.c   | 22 ++++++++-----------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
index e3ddf18dcbf5..dde986706a37 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
@@ -78,12 +78,7 @@ static int idpf_tx_singleq_csum(struct sk_buff *skb,
 			l4.hdr = skb_inner_network_header(skb);
 			break;
 		default:
-			if (is_tso)
-				return -1;
-
-			skb_checksum_help(skb);
-
-			return 0;
+			goto checksum_sw_fb;
 		}
 		off->tx_flags |= IDPF_TX_FLAGS_TUNNEL;
 
@@ -138,7 +133,7 @@ static int idpf_tx_singleq_csum(struct sk_buff *skb,
 					 sizeof(*ip.v6), &l4_proto,
 					 &frag_off);
 	} else {
-		return -1;
+		goto checksum_sw_fb;
 	}
 
 	/* compute inner L3 header size */
@@ -163,12 +158,7 @@ static int idpf_tx_singleq_csum(struct sk_buff *skb,
 		l4_len = sizeof(struct sctphdr) >> 2;
 		break;
 	default:
-		if (is_tso)
-			return -1;
-
-		skb_checksum_help(skb);
-
-		return 0;
+		goto checksum_sw_fb;
 	}
 
 	offset |= l4_len << IDPF_TX_DESC_LEN_L4_LEN_S;
@@ -176,6 +166,12 @@ static int idpf_tx_singleq_csum(struct sk_buff *skb,
 	off->hdr_offsets |= offset;
 
 	return 1;
+
+checksum_sw_fb:
+	if (is_tso)
+		return -1;
+
+	return skb_checksum_help(skb);
 }
 
 /**
-- 
2.49.0

Re: [Intel-wired-lan] [PATCH iwl-next v2 1/4] ice: pass the return value of skb_checksum_help()

[Simon Horman]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/

Simon says:
There is also an AI-generated review of this patch available on sashiko.dev
However I believe that can be treated in the context of possible follow-up.

---
ice: pass the return value of skb_checksum_help()

This patch propagates the return value of skb_checksum_help() back to
the caller and consolidates the software checksum fallback path under a
single checksum_sw_fb label in ice_tx_csum().

> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> index 4ca1a0602307..c04c5856dad6 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> @@ -1654,7 +1654,7 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
>  			ret = ipv6_skip_exthdr(skb, exthdr - skb->data,
>  					       &l4_proto, &frag_off);
>  			if (ret < 0)
> -				return -1;
> +				goto checksum_sw_fb;
>  		}
>  
>  		/* define outer transport */
> @@ -1673,11 +1673,7 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
>  			l4.hdr = skb_inner_network_header(skb);
>  			break;
>  		default:
> -			if (first->tx_flags & ICE_TX_FLAGS_TSO)
> -				return -1;
> -
> -			skb_checksum_help(skb);
> -			return 0;
> +			goto checksum_sw_fb;
>  		}
>  
>  		/* compute outer L3 header size */

[ ... ]

> @@ -1736,7 +1732,7 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
>  			ipv6_skip_exthdr(skb, exthdr - skb->data, &l4_proto,
>  					 &frag_off);
>  	} else {
> -		return -1;
> +		goto checksum_sw_fb;
>  	}

[Low]

In the encapsulated branch, when the inner IP version is neither 4 nor 6
the new behaviour is to jump to checksum_sw_fb instead of unconditionally
returning -1. Before this patch the non-TSO path also bailed via
return 0 after skb_checksum_help(), and the caller would not emit a
context descriptor for that packet. After this patch the encapsulation
block has already populated off->cd_tunnel_params and set
off->cd_qw1 |= ICE_TX_DESC_DTYPE_CTX:

	/* record tunnel offload values */
	off->cd_tunnel_params |= tunnel;

	off->cd_qw1 |= (u64)ICE_TX_DESC_DTYPE_CTX;

If skb_checksum_help() then returns 0, ice_tx_csum() returns 0 and
ice_xmit_frame_ring() will still build a Tx context descriptor with the
tunnel parameters even though no L3/L4 offload bits ever get programmed
into the data descriptor.

Is that descriptor pair intentional for these malformed encapsulated
packets, or should the encapsulated default case clear the tunnel state
(or just return -1) before falling through to the software fallback?

> @@ -1789,15 +1785,17 @@ int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
>  		break;
>  
>  	default:
> -		if (first->tx_flags & ICE_TX_FLAGS_TSO)
> -			return -1;
> -		skb_checksum_help(skb);
> -		return 0;
> +		goto checksum_sw_fb;
>  	}
>  
>  	off->td_cmd |= cmd;
>  	off->td_offset |= offset;
>  	return 1;
> +
> +checksum_sw_fb:
> +	if (first->tx_flags & ICE_TX_FLAGS_TSO)
> +		return -1;
> +	return skb_checksum_help(skb);
>  }