Re: [PATCH net-next V4 10/14] net/mlx5e: shampo: Flush session on PSP mismatch

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] Understated impact / missing Fixes: tag.

--

net/mlx5e: shampo: Flush session on PSP mismatch

This adds mlx5e_hw_gro_psp_match() and wires it into the SHAMPO session
match test in mlx5e_handle_rx_cqe_mpwrq_shampo(), so a session is flushed
when the PSP status changes, when SPI/version differ, or when the CQE
indicates a non-decapsulated PSP packet.

(No Link: tags present in the commit header.)

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> index e3f915beebe1..9eb11be7abe0 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> @@ -2224,6 +2224,33 @@ static bool mlx5e_hw_gro_skb_has_enough_space(struct sk_buff *skb,
>  		return page_size * nr_frags + data_bcnt <= GRO_LEGACY_MAX_SIZE;
>  }
>  
> +static bool mlx5e_hw_gro_psp_match(struct sk_buff *skb, struct mlx5_cqe64 *cqe)
> +{
> +#ifdef CONFIG_MLX5_EN_PSP
> +	struct psp_skb_ext *pse = skb_ext_find(skb, SKB_EXT_PSP);
> +	bool is_psp = mlx5e_psp_is_rx_flow(cqe);
> +
> +	if (likely(!is_psp && !pse))
> +		return true;
> +
> +	/* No match on PSP status change (no crypto -> crypto or vice-versa). */
> +	if (unlikely(is_psp != !!pse))
> +		return false;
> +
> +	/* SPI and version are only available in CQE metadata for decap flows.
> +	 * Non-decap PSP cannot be matched here, force a flush.
> +	 */
> +	if (unlikely(!mlx5e_psp_is_decap(cqe)))
> +		return false;

[Severity: Medium]
Is the commit message understating what this particular hunk fixes?  The
message says:

    The session is also forcefully flushed if non-decapsulated packets are
    received. This could happen if, for example, the decap table could not
    be created so HW GRO is active but there's no decapsulation.

At the baseline commit (bc6fe9d301d5), non-decap looks like the only PSP
RX mode rather than an exceptional fallback:
mlx5e_psp_offload_handle_rx_skb() in en_accel/psp_rxtx.c only calls
psp_dev_rcv(), and mlx5e_psp_update_rx()/shampo_enabled() coupling between
PSP and NETIF_F_GRO_HW is only added by a later patch in this series.

If that is right, does the following sequence mis-deliver payload bytes
today, without this patch?

mlx5e_handle_rx_cqe_mpwrq_shampo()
  -> HW coalesces several PSP datagrams sharing the outer IP/UDP 5-tuple
     (mlx5e_shampo_update_hdr() -> mlx5e_shampo_update_ipv4_udp_hdr()
      sets SKB_GSO_UDP_L4, so UDP flows are aggregated too)
  -> mlx5e_shampo_complete_rx_cqe():

	if (NAPI_GRO_CB(skb)->count != 1)
		return false;

	if (mlx5e_build_rx_skb(cqe, cqe_bcnt, rq, skb))
		return true;

so mlx5e_build_rx_skb() -> mlx5e_psp_offload_handle_rx_skb() ->
psp_dev_rcv() runs only for the first record of a session.  psp_dev_rcv()
in net/psp/psp_main.c strips one 16-byte PSP header plus one 16-byte
trailer and adjusts iph->tot_len once, so do the PSP headers and ICVs of
records 2..N remain embedded in the payload handed to the socket?

If so, should the commit message say that PSP + HW GRO
(ethtool -K <dev> rx-gro-hw on, also implied by tcp-data-split) is broken
in the current tree, and should the patch carry a Fixes: tag so stable
kernels shipping mlx5e PSP pick it up?  A natural candidate looks like
29d7f433fcec ("net/mlx5e: Add Rx data path offload"), even if a backport
needs adjustment because mlx5e_psp_is_decap() is new in this series.

> +
> +	/* No match on security parameters change. */
> +	return pse->spi == mlx5e_psp_get_spi(cqe) &&
> +		pse->version == mlx5e_psp_get_version(cqe);
> +#else
> +	return true;
> +#endif
> +}
> +
>  static void mlx5e_handle_rx_cqe_mpwrq_shampo(struct mlx5e_rq *rq, struct mlx5_cqe64 *cqe)
>  {
>  	u16 data_bcnt		= mpwrq_get_cqe_byte_cnt(cqe) - cqe->shampo.header_size;
> @@ -2266,8 +2293,9 @@ static void mlx5e_handle_rx_cqe_mpwrq_shampo(struct mlx5e_rq *rq, struct mlx5_cq
>  	}
>  
>  	if (*skb &&
> -	    !(match && mlx5e_hw_gro_skb_has_enough_space(*skb, data_bcnt,
> -							 page_size))) {
> +	    !(match &&
> +	      mlx5e_hw_gro_skb_has_enough_space(*skb, data_bcnt, page_size) &&
> +	      mlx5e_hw_gro_psp_match(*skb, cqe))) {
>  		match = false;
>  		mlx5e_shampo_flush_skb(rq, cqe, match);
>  	}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908065244.3799142-1-tariqt%40nvidia.com