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

[Tariq Toukan <tariqt@nvidia.com>]

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.

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.
> @@ -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.
Change-Id: Ib52fd4bfc205ea2e4da47e07a54cc0b727cc225a

Cosmin Ratiu (13):
  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: Fix responder parsing
  selftests: drv-net: psp: Add a test for PSP with HW-GRO

 .../net/ethernet/mellanox/mlx5/core/en/fs.h   |   1 +
 .../mellanox/mlx5/core/en_accel/en_accel.h    |  31 ++
 .../mellanox/mlx5/core/en_accel/flow_tag.h    |  45 +++
 .../mellanox/mlx5/core/en_accel/ipsec_fs.c    |  70 ++--
 .../mellanox/mlx5/core/en_accel/ipsec_rxtx.h  |   9 +-
 .../mellanox/mlx5/core/en_accel/macsec.c      |  34 +-
 .../mellanox/mlx5/core/en_accel/macsec.h      |   5 +-
 .../mellanox/mlx5/core/en_accel/psp.c         | 319 ++++++++++++++++--
 .../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 |   8 +-
 .../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        |  17 +-
 .../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/psp.py    | 129 ++++++-
 .../selftests/drivers/net/psp_responder.c     |  87 +++--
 21 files changed, 763 insertions(+), 163 deletions(-)
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en_accel/flow_tag.h


base-commit: 2bb54b49e9d522f54dc9c0fe10ba40fbc56041c8
-- 
2.44.0