BPF List
 help / color / mirror / Atom feed
* [PATCH net-next V3 00/15] net/mlx5e: Add support for HW-GRO to PSP
@ 2026-09-03  8:52 Tariq Toukan
  2026-09-03  8:52 ` [PATCH net-next V3 01/15] net/mlx5e: Generalize TC <-> IPsec mutual exclusion Tariq Toukan
                   ` (14 more replies)
  0 siblings, 15 replies; 17+ messages in thread
From: Tariq Toukan @ 2026-09-03  8:52 UTC (permalink / raw)
  To: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	netdev, Paolo Abeni, Sabrina Dubroca
  Cc: Aleksandr Loktionov, Alexei Lazar, Alexei Starovoitov,
	Allison Henderson, Antonio Quartulli, Anubhav Singh,
	Bobby Eshleman, Boris Pismenny, bpf, Carolina Jubran, Chris Mi,
	Cosmin Ratiu, Daniel Borkmann, Daniel Zahka, David Wei,
	Doruk Tan Ozturk, Dragos Tatulea, Gal Pressman, Jacob Keller,
	Jesper Dangaard Brouer, Jianbo Liu, John Fastabend, Kees Cook,
	Lama Kayal, Leon Romanovsky, open list, linux-kselftest,
	linux-rdma, Mark Bloch, Matthieu Baerts (NGI0), Patrisious Haddad,
	Petr Machata, Raed Salem, Rahul Rameshbabu, Richard Gobert,
	Saeed Mahameed, Shuah Khan, Shuah Khan, Simon Horman,
	Stanislav Fomichev, Stanislav Fomichev, Tariq Toukan,
	Willem de Bruijn, Willem de Bruijn

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 not
  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.

> @@ -224,6 +253,28 @@ run_session(struct ynl_sock *ys, struct opts *opts,
>                                               fprintf(stderr, "WARN: echo but no data sock\n");
>                                       send_ack(comm_sock);
>                               }
> +                             if (cmd("data send")) {
> +                                     __u32 len;
> +
> +                                     if (data_sock < 0) {
> +                                             fprintf(stderr, "WARN: send but no data sock\n");
> +                                             send_err(comm_sock);
> +                                             continue;
> +                                     }
Is there a race condition between processing the control command and accepting
the data socket?
If the Python client initiates the data socket connection and immediately
sends the "data send" command on the control socket, both the server_sock
(accept queue) and comm_sock (control connection) could become readable
simultaneously before the responder calls poll().
Because the comm_sock is processed first in run_session():
tools/testing/selftests/drivers/net/psp_responder.c:run_session() {
    ...
    /* comm sock */
    if (pfds[1].revents & POLLIN) {
        ...
    }
    /* server sock */
    if (pfds[0].revents & POLLIN) {
        data_sock = accept(server_sock, NULL, NULL);
    }
    ...
}
does this cause the responder to handle the "data send" command before
accepting the data socket, resulting in data_sock < 0 and an immediate failure?

[CR] Possible in theory, not happening in practice today because "data send" is
only used after a few round trips from the Python test, so it is guaranteed
that the new socket is accepted by the time the command gets executed.
Fixing this class of races in the responder would be nice, but outside the
scope of this series.

> @@ -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.

V3:
- 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 (15):
  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: drv-net: gro: Extract shared helpers into gro_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 +-
 .../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  |   5 +
 tools/testing/selftests/drivers/net/gro.py    | 200 ++---------
 .../testing/selftests/drivers/net/gro_lib.py  | 204 +++++++++++
 .../testing/selftests/drivers/net/hw/Makefile |  17 +
 .../selftests/drivers/net/hw/psp_gro.py       | 172 +++++++++
 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 +++++++++++++++++-
 28 files changed, 1431 insertions(+), 367 deletions(-)
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en_accel/flow_tag.h
 create mode 100644 tools/testing/selftests/drivers/net/gro_lib.py
 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: c29b37ed7a4d9856ed758a82282456d69cee2ed1
-- 
2.44.0


^ permalink raw reply	[flat|nested] 17+ messages in thread
end of thread, other threads:[~2026-09-03 10:26 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-03  8:52 [PATCH net-next V3 00/15] net/mlx5e: Add support for HW-GRO to PSP Tariq Toukan
2026-09-03  8:52 ` [PATCH net-next V3 01/15] net/mlx5e: Generalize TC <-> IPsec mutual exclusion Tariq Toukan
2026-09-03  8:52 ` [PATCH net-next V3 02/15] net/mlx5e: ipsec: Block TC offload when IPsec is enabled Tariq Toukan
2026-09-03  8:52 ` [PATCH net-next V3 03/15] net/mlx5e: psp: Block TC offload when PSP " Tariq Toukan
2026-09-03  8:52 ` [PATCH net-next V3 04/15] net/mlx5e: macsec: Block TC offload when MACsec " Tariq Toukan
2026-09-03  8:52 ` [PATCH net-next V3 05/15] net/mlx5e: psp: Move RX marker from ft_metadata to flow_tag Tariq Toukan
2026-09-03  8:52 ` [PATCH net-next V3 06/15] net/mlx5e: ipsec: " Tariq Toukan
2026-09-03  8:52 ` [PATCH net-next V3 07/15] net/mlx5e: macsec: " Tariq Toukan
2026-09-03  8:52 ` [PATCH net-next V3 08/15] net/mlx5e: psp: Handle HW-decapsulated RX PSP packets Tariq Toukan
2026-09-03  8:52 ` [PATCH net-next V3 09/15] net/mlx5e: psp: Add an rx_decap steering table Tariq Toukan
2026-09-03  8:52 ` [PATCH net-next V3 10/15] net/mlx5e: shampo: Flush session on PSP mismatch Tariq Toukan
2026-09-03  8:52 ` [PATCH net-next V3 11/15] net/mlx5e: psp: Dynamically reconfigure based on SHAMPO mode Tariq Toukan
2026-09-03  8:52 ` [PATCH net-next V3 12/15] selftests: drv-net: psp: Extract shared helpers into psp_lib.py Tariq Toukan
2026-09-03  8:52 ` [PATCH net-next V3 13/15] selftests: drv-net: gro: Extract shared helpers into gro_lib.py Tariq Toukan
2026-09-03  8:52 ` [PATCH net-next V3 14/15] selftests: net: gro: Add PSP encapsulation and encryption Tariq Toukan
2026-09-03 10:26   ` Loktionov, Aleksandr
2026-09-03  8:52 ` [PATCH net-next V3 15/15] selftests: drv-net: Add PSP HW GRO conformance tests Tariq Toukan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox