Re: [PATCH net] net/mlx5e: macsec: free SC lookup entries after an RCU grace period

[Simon Horman <horms@kernel.org>]

On Sat, Oct 24, 2026 at 01:21:28PM -0300, Joas Antonio dos Santos wrote:
> macsec_del_rxsc_ctx() removes rx_sc->sc_xarray_element from
> macsec->sc_xarray with xa_erase() and frees it with kfree() right away.
> mlx5e_macsec_offload_handle_rx_skb() looks the element up with
> xa_load() under rcu_read_lock() and then dereferences ->rx_sc, so an RX
> completion that loaded the element just before xa_erase() reads freed
> memory. rx_sc itself is already freed with kfree_rcu_mightsleep(); only
> the element pointing to it was missed.
> 
> macsec_fs_id_del() has the same problem on the TX side: it removes
> fs_id_found from the SCI rhashtable and frees it with kfree(), while
> mlx5e_macsec_handle_tx_skb() and mlx5e_macsec_tx_build_eseg() read it
> through mlx5_macsec_fs_get_fs_id_from_hashtable(), which uses
> rhashtable_lookup() under rcu_read_lock().
> 
> Free both with kvfree_rcu_mightsleep(). Both run in process context
> from the macsec offload ops, outside any spinlock, so sleeping is fine.
> 
> The sc_xarray_element issue was pointed out during review of
> "net/mlx5e: macsec: fix use-after-free of metadata_dst on RX SC delete"
> and left for a follow-up; this is that follow-up.
> 
> Fixes: b7c9400cbc48 ("net/mlx5e: Implement MACsec Rx data path using MACsec skb_metadata_dst")
> Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
> Assisted-by: Yaga Agent HackerSec Claude:claude-opus-5-5
> ---
> Testing: compile-tested only (arm64, W=1, CONFIG_MLX5_MACSEC=y, no new
> warnings). I do not have MACsec-capable ConnectX hardware, so this was
> not run on a real device and I have no reproducer against the driver
> itself. The free-while-reader-holds-RCU pattern was reproduced with a
> standalone test module under KASAN (slab-use-after-free on kmalloc-16),
> and the same module showed no reports after switching to
> kvfree_rcu_mightsleep().
> 
>  drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 2 +-
>  drivers/net/ethernet/mellanox/mlx5/core/lib/macsec_fs.c   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> index 38a3415ac..fb9126f6b 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> @@ -839,7 +839,7 @@ static void macsec_del_rxsc_ctx(struct mlx5e_macsec *macsec, struct mlx5e_macsec
>  	list_del_rcu(&rx_sc->rx_sc_list_element);
>  	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);
> +	kvfree_rcu_mightsleep(rx_sc->sc_xarray_element);
>  	kfree_rcu_mightsleep(rx_sc);
>  }
>  

Hi Joas,

This patch does not apply because the problem in macsec_del_rxsc_ctx() has
already been addressed by ad17ad50b8dc ("net: mlx5e: Fix use-after-free of
the MACsec xarray entry on RX SC deletion") using kfree_rcu.

I would suggest taking that approach in macsec_fs_id_del() too
unless there is a reason to use mightsleep.

Quoting the documentation of kfree_rcu_mightsleep():

/**
 * kfree_rcu_mightsleep() - kfree an object after a grace period.
 * @ptr: pointer to kfree for single-argument invocations.
 *
 * When it comes to head-less variant, only one argument
 * is passed and that is just a pointer which has to be
 * freed after a grace period. Therefore the semantic is
 *
 *     kfree_rcu_mightsleep(ptr);
 *
 * where @ptr is the pointer to be freed by kvfree().
 *
 * Please note, head-less way of freeing is permitted to
 * use from a context that has to follow might_sleep()
 * annotation. Otherwise, please switch and embed the
 * rcu_head structure within the type of @ptr.
 */

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/macsec_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/macsec_fs.c
> index efc516749..910fafe40 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/macsec_fs.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/macsec_fs.c
> @@ -684,7 +684,7 @@ static void macsec_fs_id_del(struct list_head *macsec_devices_list, u32 fs_id,
>  
>  	xa_erase(fs_id_xa, fs_id);
>  
> -	kfree(fs_id_found);
> +	kvfree_rcu_mightsleep(fs_id_found);
>  
>  	if (macsec_fs_is_macsec_device_empty(macsec_device)) {
>  		list_del(&macsec_device->macsec_devices_list_entry);

-- 
pw-bot: changes-requested