Re: [PATCH net-next V4 00/14] net/mlx5e: Add support for HW-GRO to PSP

[Cosmin Ratiu <cratiu@nvidia.com>]

On Tue, 2026-09-08 at 09:52 +0300, Tariq Toukan wrote:
> Hi,
> 
> Ingress PSP packets cannot be merged by the HW-GRO HW state machine
> because they are not decapsulated and the current HW-GRO state
> machine
> does not understand PSP.
> 
> This series by Cosmin decapsulates PSP packets in steering, which
> allows
> the now-decapsulated PSP packets (== TCP) to go through HW-GRO and be
> aggregated. The SPI and PSP version from the PSP header are handed
> off
> to the driver in the CQE metadata fields. They are used to terminate
> the
> HW GRO session on mismatch, and are required to construct the skb
> extension which is used higher up in the stack.
> 
> Some preparatory work needed to happen to allow that:
> - All accel protocol markers were moved away from ft_metadata into
>   flow_tag
> - Mutual exclusion between TC and accel protocols was added. Trying
> to
>   configure both IPsec and TC at the same time for example is now
>   detected at config time instead of doing weird things at runtime.
> 
> kperf tests on a pair of CX7 NICs with 200Gbps link speed:
> 
> Streams   Gbps no HW-GRO   Gbps HW-GRO   Speedup
> -------   --------------   -----------   -------
>       1               28            58     2.07x
>       2               67           102     1.52x
>       4              136           180     1.32x
>       8              175           183     1.05x
>         
> Regards,
> Tariq
> 
> Some internal Sashiko findings, plus Cosmin's comments:
> 
> > +        cfg.wait_hw_stats_settle()
> > +        after = cfg.netnl.qstats_get({"ifindex": cfg.ifindex},
> > dump=True)[0]
> Could this lead to flaky test failures on multiqueue NICs?
> By hardcoding the netlink dump array access to index [0], the test
> statically
> verifies statistics for the first queue only. Since the test sends
> traffic over
> an ephemeral random port, Receive Side Scaling (RSS) will hash this
> 4-tuple and
> could deliver the flow to any active RX queue. If the flow lands on a
> queue
> other than queue 0, will the test fail to observe the hardware GRO
> packet
> increments?
> 
> [CR] The premise is wrong. [0] is not queue 0, it's the result for
> the
> requested dev.
> 
> > @@ -1757,11 +1755,10 @@ macsec_fs_rx_add_rule(struct mlx5_macsec_fs
> > *macsec_fs,
> >       rx_tables = &rx_fs->tables;
> >       ft_crypto = &rx_tables->ft_crypto;
> > 
> > -     /* Set bit[31 - 30] macsec marker - 0x01 */
> >       /* Set bit[15-0] fs id */
> >       MLX5_SET(set_action_in, action, action_type,
> > MLX5_ACTION_TYPE_SET);
> >       MLX5_SET(set_action_in, action, field,
> > MLX5_ACTION_IN_FIELD_METADATA_REG_B);
> > -     MLX5_SET(set_action_in, action, data,
> > macsec_fs_set_rx_fs_id(fs_id));
> > +     MLX5_SET(set_action_in, action, data, fs_id);
> Does removing the BIT(30) marker from the MACsec fs_id metadata cause
> a
> regression by exposing RoCE MACsec to an aliasing issue?
> By removing BIT(30) from the fs_id in this patch, the fs_id written
> to
> METADATA_REG_B becomes a small integer (e.g., 1, 2).
> Later in mlx5_macsec_fs_add_roce_rule_rx(), the hardware rule
> ft_macsec_op_check enforces MACsec on RoCE traffic by matching
> metadata_reg_c_5 exactly against this same fs_id value.
> Could any unencrypted packet that has its REG_B set to this small
> integer
> by another NIC feature (such as TC rules setting packet marks, or PSP
> IPsec
> decryption) falsely pass the MACsec check and bypass encryption
> enforcement?
> 
> [CR] Preexisting, TC rules could set the fsid to the the previous
> value even before.
> 'PSP IPsec' decryption doesn't make sense.
> 
> > @@ -1757,11 +1755,10 @@ macsec_fs_rx_add_rule(struct mlx5_macsec_fs
> > *macsec_fs,
> >  	rx_tables = &rx_fs->tables;
> >  	ft_crypto = &rx_tables->ft_crypto;
> > 
> > -	/* Set bit[31 - 30] macsec marker - 0x01 */
> >  	/* Set bit[15-0] fs id */
> >  	MLX5_SET(set_action_in, action, action_type,
> > MLX5_ACTION_TYPE_SET);
> >  	MLX5_SET(set_action_in, action, field,
> > MLX5_ACTION_IN_FIELD_METADATA_REG_B);
> > -	MLX5_SET(set_action_in, action, data,
> > macsec_fs_set_rx_fs_id(fs_id));
> > +	MLX5_SET(set_action_in, action, data, fs_id);
> Does removing the BIT(30) marker allow metadata collisions in the
> hardware
> RoCE datapath?
> If an administrator configures a benign TC rule that sets a mark
> equal to
> the MACsec fs_id (which is just a 16-bit integer), could an attacker
> send
> unauthenticated RoCE traffic that matches the TC mark, falsely
> setting the
> metadata and bypassing the MACsec hardware authentication checks?
> 
> [CR] This series makes TC and MACsec mutually exclusive.
> 
> > @@ -2160,8 +2167,8 @@ static int
> > mlx5_macsec_fs_add_roce_rule_rx(struct mlx5_macsec_fs *macsec_fs,
> > u32
> > 
> >  	spec->match_criteria_enable |=
> > MLX5_MATCH_MISC_PARAMETERS_2;
> >  	MLX5_SET_TO_ONES(fte_match_param, spec->match_criteria,
> > misc_parameters_2.metadata_reg_c_5);
> > -	MLX5_SET(fte_match_param, spec->match_value,
> > misc_parameters_2.metadata_reg_c_5,
> > -		 macsec_fs_set_rx_fs_id(fs_id));
> > +	MLX5_SET(fte_match_param, spec->match_value,
> > +		 misc_parameters_2.metadata_reg_c_5, fs_id);
> By matching on the bare fs_id without the BIT(30) marker in
> metadata_reg_c_5, is this RoCE datapath rule now susceptible to the
> TC
> mark collision issue?
> 
> [CR] See above.
> 
> > @@ -1778,6 +1775,11 @@ macsec_fs_rx_add_rule(struct mlx5_macsec_fs
> > *macsec_fs,
> >  	/* Rx crypto table with SCI rule */
> >  	macsec_fs_rx_setup_fte(spec, &flow_act, attrs, true);
> > 
> > +	spec->flow_context.flags |= FLOW_CONTEXT_HAS_TAG;
> > +	spec->flow_context.flow_tag =
> > +		FIELD_PREP(MLX5E_ACCEL_FLOW_TAG_PROTO_MASK,
> > +			   MLX5E_ACCEL_FLOW_TAG_PROTO_MACSEC);
> Can adding FLOW_CONTEXT_HAS_TAG to the shared MACsec crypto table
> corrupt
> RoCE Queue Pair Numbers (QPN)?
> This unconditionally adds a flow tag to the shared ft_crypto table,
> which
> processes both Ethernet and RoCE MACsec traffic. When
> FLOW_CONTEXT_HAS_TAG
> is set, the hardware writes the flow tag into the sop_drop_qpn field
> of
> the CQE.
> Will this cause the mlx5_ib driver to read the MACsec tag (value 2)
> instead
> of the actual destination QPN for RDMA traffic, breaking RoCE
> delivery?
> 
> [CR] The flow_tag is only given for Eth CQEs. For RoCE/RDMA, that CQE
> field is the QPN, and therefore mlx5_ib is unaffected.
> 
> > @@ -1803,6 +1805,11 @@ macsec_fs_rx_add_rule(struct mlx5_macsec_fs
> > *macsec_fs,
> > 
> >  		macsec_fs_rx_setup_fte(spec, &flow_act, attrs,
> > false);
> > 
> > +		spec->flow_context.flags |= FLOW_CONTEXT_HAS_TAG;
> > +		spec->flow_context.flow_tag =
> > +			FIELD_PREP(MLX5E_ACCEL_FLOW_TAG_PROTO_MASK
> > ,
> > +				  
> > MLX5E_ACCEL_FLOW_TAG_PROTO_MACSEC);
> Is it safe to set the flow tag on the shared crypto table here as
> well?
> This risks corrupting the destination QPN for RoCE traffic as
> mentioned
> above.
> 
> [CR] Same as above, QPN isn't corrupted, those CQEs don't carry
> flow_tag.
> 
> > @@ -1717,7 +1717,7 @@ void
> > mlx5e_macsec_offload_handle_rx_skb(struct net_device *netdev,
> >       if (!macsec)
> >               return;
> > 
> > -     fs_id = MLX5_MACSEC_RX_METADAT_HANDLE(macsec_meta_data);
> > +     fs_id = MLX5_MACSEC_RX_METADATA_HANDLE(macsec_meta_data);
> > 
> >       rcu_read_lock();
> >       sc_xarray_element = xa_load(&macsec->sc_xarray, fs_id);
> This is a pre-existing issue, but can this xa_load() race with an
> immediate
> kfree() during deletion, causing a use-after-free?
> In the deletion path, mlx5e_macsec_del_rxsc() calls xa_erase() and
> then
> immediately frees the element without an RCU grace period:
> mlx5e_macsec_del_rxsc() {
>         ...
>         xa_erase(&macsec->sc_xarray, rx_sc->sc_xarray_element-
> >fs_id);
>         dst_release(&rx_sc->md_dst->dst);
>         kfree(rx_sc->sc_xarray_element);
>         ...
> }
> Since mlx5e_macsec_offload_handle_rx_skb() retrieves
> sc_xarray_element
> locklessly under rcu_read_lock() via xa_load(), would a concurrent
> deletion
> result in dereferencing freed memory?
> 
> [CR] Preexisting.
> 
> > +static inline int mlx5e_accel_block_tc_offload(struct
> > mlx5_core_dev *mdev)
> > +{
> > +     int ret = 0;
> > +
> > +     mutex_lock(&mdev->offload_block.lock);
> > +     if (mdev->offload_block.num_accel)
> > +             ret = -EBUSY;
> > +     else
> > +             mdev->offload_block.num_tc++;
> This isn't a bug, but does the new naming here swap the semantics of
> the
> variables?
> In the original logic, num_block_tc counted the number of blockers on
> TC
> (which are IPsec/accel rules). By renaming it to num_tc,
> mlx5e_accel_block_tc_offload() now increments num_tc when an accel
> rule
> is added.
> Would it be clearer to keep "block" in the names, e.g., num_block_tc,
> so
> it doesn't look like num_tc is tracking the number of TC rules?
> 
> [CR] Intentional, "offload_block.num_{tc, accel}" sounds better.
> 
> > +static inline int mlx5e_accel_block_tc_offload(struct
> > mlx5_core_dev *mdev)
> > +{
> > +	int ret = 0;
> > +
> > +	mutex_lock(&mdev->offload_block.lock);
> > +	if (mdev->offload_block.num_accel)
> > +		ret = -EBUSY;
> > +	else
> > +		mdev->offload_block.num_tc++;
> Are the semantic meanings of the num_tc and num_accel variables
> inverted?
> When an Accel rule is added here in mlx5e_accel_block_tc_offload(),
> it
> increments num_tc.
> 
> [CR] Same thing, it's an intentional rename.
> 
> > @@ -384,6 +429,215 @@ static void fill_transportlayer(void *buf,
> > int seq_offset, int ack_offset,
> [ ... ]
> > +static char psp_scratch[L2_HLEN_MAX + IP_MAXPACKET +
> > PSP_ENCAP_LEN];
> [ ... ]
> > +/* Encapsulates & encrypts @pkt with PSP transport mode into
> > psp_scratch.
> > + * Returns the scratch buffer and updates *@lenp.
> > + */
> > +static char *psp_encapsulate(const char *pkt, int *lenp)
> > +{
> [ ... ]
> > +	memcpy(psp_scratch, pkt, len);
> > +
> > +	if (proto == PF_INET) {
> > +		struct iphdr *iph = (struct iphdr *)(psp_scratch +
> > ETH_HLEN);
> Does this code violate strict aliasing rules?
> Since psp_scratch is declared as a character array, casting it to an
> incompatible structure pointer like struct iphdr * (and later struct
> ipv6hdr *
> and struct udphdr *) violates C11 strict aliasing rules.
> Because the tools/ directory assumes standard -fstrict-aliasing
> optimizations
> are active, could this cause the compiler to incorrectly reorder or
> optimize
> away memory writes to these headers, potentially resulting in
> malformed
> packets and spurious test failures?
> 
> [CR] Maybe, but there are already 20+ similar things in the file.
> 
> V4:
> - Removed dead mlx5_esw_lock() (Daniel).
> - Rebased on top of f225a7317c18 ("selftests: drv-net: split gro.py
> into one test per coalescing mode") (Daniel).
> - Dropped the gro_lib.py extraction, made obsolete by above change.
> - Fixed some typos (Aleksandr).
> - Added CONFIG_INET_PSP=y to the test config (Daniel).
> 
> V3:
> https://lore.kernel.org/netdev/20260903085215.3691657-1-tariqt@nvidia.com/
> - Cleared fs->decap_enabled on config down (Daniel).
> - Made decap support optional (don't fail device reconfig on errors).
> - Used bitfield ops for accel protos & psp ver (Daniel).
> - Renamed psp_responder off -> len (Jakub).
> - Dedicated HW GRO PSP test (Jakub).
> - Extended HW GRO test coverage (Jakub).
> 
> V2:
> https://lore.kernel.org/netdev/20260804083535.2946459-1-tariqt@nvidia.com/
> - Use XFail in patch 13 (Jakub).
> 
> V1:
> https://lore.kernel.org/all/20260730091756.2543777-1-tariqt@nvidia.com/
> 
> Cosmin Ratiu (14):
>   net/mlx5e: Generalize TC <-> IPsec mutual exclusion
>   net/mlx5e: ipsec: Block TC offload when IPsec is enabled
>   net/mlx5e: psp: Block TC offload when PSP is enabled
>   net/mlx5e: macsec: Block TC offload when MACsec is enabled
>   net/mlx5e: psp: Move RX marker from ft_metadata to flow_tag
>   net/mlx5e: ipsec: Move RX marker from ft_metadata to flow_tag
>   net/mlx5e: macsec: Move RX marker from ft_metadata to flow_tag
>   net/mlx5e: psp: Handle HW-decapsulated RX PSP packets
>   net/mlx5e: psp: Add an rx_decap steering table
>   net/mlx5e: shampo: Flush session on PSP mismatch
>   net/mlx5e: psp: Dynamically reconfigure based on SHAMPO mode
>   selftests: drv-net: psp: Extract shared helpers into psp_lib.py
>   selftests: net: gro: Add PSP encapsulation and encryption
>   selftests: drv-net: Add PSP HW GRO conformance tests
> 
>  .../net/ethernet/mellanox/mlx5/core/en/fs.h   |   1 +
>  .../mellanox/mlx5/core/en_accel/en_accel.h    |  27 ++
>  .../mellanox/mlx5/core/en_accel/flow_tag.h    |  49 +++
>  .../mellanox/mlx5/core/en_accel/ipsec_fs.c    |  72 ++--
>  .../mellanox/mlx5/core/en_accel/ipsec_rxtx.h  |   8 +-
>  .../mellanox/mlx5/core/en_accel/macsec.c      |  34 +-
>  .../mellanox/mlx5/core/en_accel/macsec.h      |   4 +-
>  .../mellanox/mlx5/core/en_accel/psp.c         | 316 ++++++++++++++--
>  .../mellanox/mlx5/core/en_accel/psp.h         |   2 +
>  .../mellanox/mlx5/core/en_accel/psp_rxtx.c    |  21 +-
>  .../mellanox/mlx5/core/en_accel/psp_rxtx.h    |  44 ++-
>  .../net/ethernet/mellanox/mlx5/core/en_main.c |  10 +-
>  .../net/ethernet/mellanox/mlx5/core/en_rx.c   |  32 +-
>  .../net/ethernet/mellanox/mlx5/core/en_tc.c   |  46 ++-
>  .../net/ethernet/mellanox/mlx5/core/en_tc.h   |   7 +-
>  .../net/ethernet/mellanox/mlx5/core/eswitch.c |  12 -
>  .../net/ethernet/mellanox/mlx5/core/eswitch.h |   1 -
>  .../mellanox/mlx5/core/lib/macsec_fs.c        |  21 +-
>  .../mellanox/mlx5/core/lib/macsec_fs.h        |   9 +-
>  .../net/ethernet/mellanox/mlx5/core/main.c    |   3 +
>  include/linux/mlx5/driver.h                   |   7 +-
>  tools/testing/selftests/drivers/net/Makefile  |   1 +
>  .../testing/selftests/drivers/net/gro_lib.py  |  15 +-
>  .../testing/selftests/drivers/net/hw/Makefile |  17 +
>  tools/testing/selftests/drivers/net/hw/config |   1 +
>  .../selftests/drivers/net/hw/psp_gro.py       | 157 ++++++++
>  tools/testing/selftests/drivers/net/psp.py    |  77 ++--
>  .../testing/selftests/drivers/net/psp_lib.py  |  57 +++
>  tools/testing/selftests/net/lib/Makefile      |  16 +
>  tools/testing/selftests/net/lib/gro.c         | 337
> +++++++++++++++++-
>  30 files changed, 1198 insertions(+), 206 deletions(-)
>  create mode 100644
> drivers/net/ethernet/mellanox/mlx5/core/en_accel/flow_tag.h
>  create mode 100755 tools/testing/selftests/drivers/net/hw/psp_gro.py
>  create mode 100644 tools/testing/selftests/drivers/net/psp_lib.py
> 
> 
> base-commit: bc6fe9d301d59fe5cb236d646abe792bffae5fa6

Replying here instead of the individual messages, to make it easier to
read.

I read and understood the various bits and feedback from Daniel and
from Sashiko.
Some useful findings, everything possible will be addressed in the next
version, and there will be a net series with fixes for the TC exclusion
and macsec issues raised by Sashiko. This will unfortunately add a week
or so at least of dependency.

Cosmin.