Re: [PATCH net-next v17 02/15] net/mlx5e: add TLS 1.3 hardware offload support

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Concern (INVALIDATED by series context): 'net/mlx5e: add TLS 1.3
  hardware offload support' is claimed to be dead code because net/tls…
- [Low] Concern (no defect proven): fill_static_params() writes the TLS
  1.3 8-byte IV tail into the device's implicit_iv field by pointer…

--

net/mlx5e: add TLS 1.3 hardware offload support

This adds TLS 1.3 capability checks to mlx5e_is_ktls_device() and
mlx5e_ktls_type_check(), and teaches fill_static_params() to program the
TLS 1.3 static params context including the 8-byte IV tail that follows
the 4-byte salt.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls.h b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls.h
> index 07a04a142a2ea..0469ca6a0762e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls.h
> @@ -30,7 +30,9 @@ static inline bool mlx5e_is_ktls_device(struct mlx5_core_dev *mdev)
>  		return false;
>  
>  	return (MLX5_CAP_TLS(mdev, tls_1_2_aes_gcm_128) ||
> -		MLX5_CAP_TLS(mdev, tls_1_2_aes_gcm_256));
> +		MLX5_CAP_TLS(mdev, tls_1_2_aes_gcm_256) ||
> +		MLX5_CAP_TLS(mdev, tls_1_3_aes_gcm_128) ||
> +		MLX5_CAP_TLS(mdev, tls_1_3_aes_gcm_256));
>  }
>  
>  static inline bool mlx5e_ktls_type_check(struct mlx5_core_dev *mdev,
> @@ -40,10 +42,14 @@ static inline bool mlx5e_ktls_type_check(struct mlx5_core_dev *mdev,
>  	case TLS_CIPHER_AES_GCM_128:
>  		if (crypto_info->version == TLS_1_2_VERSION)
>  			return MLX5_CAP_TLS(mdev,  tls_1_2_aes_gcm_128);
> +		else if (crypto_info->version == TLS_1_3_VERSION)
> +			return MLX5_CAP_TLS(mdev,  tls_1_3_aes_gcm_128);
>  		break;
>  	case TLS_CIPHER_AES_GCM_256:
>  		if (crypto_info->version == TLS_1_2_VERSION)
>  			return MLX5_CAP_TLS(mdev,  tls_1_2_aes_gcm_256);
> +		else if (crypto_info->version == TLS_1_3_VERSION)
> +			return MLX5_CAP_TLS(mdev,  tls_1_3_aes_gcm_256);
>  		break;
>  	}

[Severity: Low]
This isn't a bug, but could the commit message mention the core dependency?

At this commit the new TLS_1_3_VERSION arms in mlx5e_ktls_type_check() and
the TLS 1.3 branch in fill_static_params() cannot be reached, because
net/tls still rejects 1.3 for device offload:

net/tls/tls_device.c:tls_set_device_offload() {
	crypto_info = &ctx->crypto_send.info;
	if (crypto_info->version != TLS_1_2_VERSION) {
		rc = -EOPNOTSUPP;
		goto release_netdev;
	}
}

net/tls/tls_device.c:tls_set_device_offload_rx() {
	if (ctx->crypto_recv.info.version != TLS_1_2_VERSION)
		return -EOPNOTSUPP;
}

Those two guards are dropped by "tls: add TLS 1.3 hardware offload support"
later in the same series, so the driver code does become live by the end of
the series and there is no functional problem here.

Since the message says "Tested with TLS 1.3 AES-GCM-128 and AES-GCM-256
cipher suites", would it help to state that the test depends on the later
core patch, or to reorder so the core change lands first?

Related, mlx5e_is_ktls_device() starts advertising kTLS on hardware that
only reports tls_1_3_aes_gcm_128/256 caps while the core still offers only
1.2 sessions, which mlx5e_ktls_type_check() then refuses. Is that
intermediate state intentional for the two commits it lasts?

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_txrx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_txrx.c
> index 570a912dd6faf..f3f1be1d40343 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_txrx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_txrx.c

[ ... ]

> @@ -15,8 +16,10 @@ enum {
>  #define EXTRACT_INFO_FIELDS do { \
>  	salt    = info->salt;    \
>  	rec_seq = info->rec_seq; \
> +	iv      = info->iv;      \
>  	salt_sz    = sizeof(info->salt);    \
>  	rec_seq_sz = sizeof(info->rec_seq); \
> +	iv_sz      = sizeof(info->iv);      \
>  } while (0)

[ ... ]

> @@ -59,7 +62,12 @@ fill_static_params(struct mlx5_wqe_tls_static_params_seg *params,
>  	memcpy(gcm_iv,      salt,    salt_sz);
>  	memcpy(initial_rn,  rec_seq, rec_seq_sz);
>  
> -	tls_version = MLX5E_STATIC_PARAMS_CONTEXT_TLS_1_2;
> +	if (crypto_info->crypto_info.version == TLS_1_3_VERSION) {
> +		memcpy(gcm_iv + salt_sz, iv, iv_sz);
                        ^^^^^^^^^^^^^^^^

[Severity: Low]
This isn't a bug today, but would it be clearer to address implicit_iv by
name rather than walking off the end of gcm_iv?

The layout is:

include/linux/mlx5/mlx5_ifc.h:
struct mlx5_ifc_tls_static_params_bits {
	...
	u8         gcm_iv[0x20];

	u8         implicit_iv[0x40];

	u8         reserved_at_100[0x8];
	u8         dek_index[0x18];
	...
};

gcm_iv is 4 bytes, so gcm_iv + salt_sz with salt_sz == 4 and iv_sz == 8
lands exactly on the 8 bytes of implicit_iv for both AES-GCM-128 and
AES-GCM-256, which is why nothing is currently overwritten.

The write does depend on salt_sz staying 4 and on gcm_iv/implicit_iv staying
adjacent though. Would using

	implicit_iv = MLX5_ADDR_OF(tls_static_params, ctx, implicit_iv);

plus a BUILD_BUG_ON() tying sizeof(info->salt) and sizeof(info->iv) to the
field widths be preferable, so a future cipher suite with a different salt
size or a PRM layout change fails to build instead of writing into
dek_index?

> +		tls_version = MLX5E_STATIC_PARAMS_CONTEXT_TLS_1_3;
> +	} else {
> +		tls_version = MLX5E_STATIC_PARAMS_CONTEXT_TLS_1_2;
> +	}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917224355.2288021-1-rjethwani%40purestorage.com