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/[email protected]/
> - 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/[email protected]/
> - Use XFail in patch 13 (Jakub).
>
> V1:
> https://lore.kernel.org/all/[email protected]/
>
> 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.
