Re: [PATCH mlx5-next 07/10] RDMA/mlx5: Extract IB specific lock out of data direct

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] core: missing smp_store_release() in lockless pointer publication
--

commit 70f41e46e61426ca24948348bb11ea96b2dc863f
Author: Dragos Tatulea <dtatulea@nvidia.com>

RDMA/mlx5: Extract IB specific lock out of data direct

The data_direct_lock from struct mlx5_ib_dev is an IB specific lock.
It protects from unbind while mlx5_data_direct_dev is being accessed.
So move data_direct_lock out of the generic bind/unbind functions
and into the IB specific notifier callback.

> diff --git a/drivers/infiniband/hw/mlx5/data_direct.c b/drivers/infiniband/hw/mlx5/data_direct.c
> index f871e3ce9f73d..16afacdd36018 100644
> --- a/drivers/infiniband/hw/mlx5/data_direct.c
> +++ b/drivers/infiniband/hw/mlx5/data_direct.c
> @@ -197,9 +197,7 @@ mlx5_data_direct_get_reg(struct mlx5_ib_dev *ibdev)
>  static void mlx5_data_direct_bind(struct mlx5_ib_dev *ibdev,
>  				  struct mlx5_data_direct_dev *dev)
>  {
> -	mutex_lock(&ibdev->data_direct_lock);
> -	ibdev->data_direct_dev = dev;
> -	mutex_unlock(&ibdev->data_direct_lock);
> +	WRITE_ONCE(ibdev->data_direct_dev, dev);

[Severity: High]
Is a release barrier needed here?

By removing the shared lock, this becomes a lockless publication.
mutex_lock() only provides ACQUIRE semantics, so the writer lacks a RELEASE
barrier like smp_store_release().

Could weakly-ordered CPUs reorder the initialization of dev to become
visible after the pointer publication?  This might allow a concurrent
lockless reader to observe the newly assigned pointer but read uninitialized
structure fields (e.g., dev->pdev being NULL), leading to a kernel panic
when dereferenced.

[ ... ]
> diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
> index e6b74955d95dc..1fc61a680f8af 100644
> --- a/drivers/infiniband/hw/mlx5/mr.c
> +++ b/drivers/infiniband/hw/mlx5/mr.c
> @@ -997,7 +997,7 @@ reg_user_mr_dmabuf_by_data_direct(struct ib_pd *pd, u64 offset,
>  		return ERR_PTR(-EOPNOTSUPP);
>  
>  	mutex_lock(&dev->data_direct_lock);
> -	data_direct_dev = dev->data_direct_dev;
> +	data_direct_dev = READ_ONCE(dev->data_direct_dev);

[Severity: High]
Is an acquire barrier like smp_load_acquire() needed here instead of
READ_ONCE()?

Since the reader in reg_user_mr_dmabuf_by_data_direct() takes a different
lock (dev->data_direct_lock) than the writer (which takes
mlx5_data_direct_mutex), it loads the pointer locklessly with respect to the
writer.

Could an unprivileged userspace issuing an RDMA memory registration
concurrently with a data direct device hotplug or bind observe uninitialized
memory fields because of this missing barrier?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917111422.4054862-1-tariqt@nvidia.com?part=7