Re: [PATCH rdma-next v2 15/15] RDMA/mlx5: Enforce relaxed ordering when HW requires it

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] RDMA/mlx5: stripped RO flag in FRMR key breaks mlx5r_create_mkeys()
- [High] RDMA/mlx5: ordering check bypass in reg_user_mr_dmabuf_by_data_direct()
--

commit 939d1d09c02a86f15171e7a38c5bf57aaefbee3f
Author: Michael Gur <michaelgur@nvidia.com>

RDMA/mlx5: Enforce relaxed ordering when HW requires it

This commit adds validate_ordering_access() to reject MR registrations and
FRMR pool requests that do not specify IB_ACCESS_RELAXED_ORDERING or
IB_ACCESS_UNORDERED when the hardware sets
mkc_order_write_after_write_ro_only.

> diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
> index 6848515e06d77..7952d80349dc6 100644
> --- a/drivers/infiniband/hw/mlx5/mr.c
> +++ b/drivers/infiniband/hw/mlx5/mr.c
> @@ -78,6 +78,16 @@ static void set_mkc_access_pd_addr_fields(void *mkc, int acc, u64 start_addr,
>  	MLX5_SET64(mkc, mkc, start_addr, start_addr);
>  }
>  
> +static int validate_ordering_access(struct mlx5_ib_dev *dev, int acc)
> +{
> +	/* If HW disallows Strong Ordered writes, RO/UNORDERED must be set */
> +	if (MLX5_CAP_GEN(dev->mdev, mkc_order_write_after_write_ro_only)) {
> +		if (!(acc & (IB_ACCESS_RELAXED_ORDERING | IB_ACCESS_UNORDERED)))
> +			return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
>  static void assign_mkey_variant(struct mlx5_ib_dev *dev, u32 *mkey, u32 *in)
>  {
>  	u8 key = atomic_inc_return(&dev->mkey_var);
> @@ -288,6 +298,7 @@ static int mlx5r_build_frmr_key(struct ib_device *device,
>  				struct ib_frmr_key *out)
>  {
>  	struct mlx5_ib_dev *dev = to_mdev(device);
> +	int err;
>  
>  	/* check HW capabilities of users requested frmr key */
>  	if ((in->ats && !MLX5_CAP_GEN(dev->mdev, ats)) ||
> @@ -297,6 +308,10 @@ static int mlx5r_build_frmr_key(struct ib_device *device,
>  	if (in->vendor_key & ~MLX5_FRMR_POOLS_KEY_VENDOR_KEY_SUPPORTED)
>  		return -EOPNOTSUPP;
>  
> +	err = validate_ordering_access(dev, in->access_flags);
> +	if (err)
> +		return err;
> +
>  	out->ats = in->ats;
>  	out->access_flags =
>  		mlx5r_umr_get_unchangeable_access_flags(dev, in->access_flags);

[Severity: High]
Can this cause mkey creation to fail when mkc_order_write_after_write_ro_only
is set and the device supports modifying relaxed ordering via UMR?

In mlx5r_build_frmr_key() (as well as _mlx5_frmr_pool_alloc() and
mlx5_mr_cache_alloc()), out->access_flags is filtered through
mlx5r_umr_get_unchangeable_access_flags():

drivers/infiniband/hw/mlx5/umr.h:mlx5r_umr_get_unchangeable_access_flags() {
	unsigned int ret = access_flags & IB_ACCESS_UNORDERED;
	...
	if ((access_flags & IB_ACCESS_RELAXED_ORDERING) &&
	    MLX5_CAP_GEN(dev->mdev, mkc_order_write_after_write_ro) &&
	    !MLX5_CAP_GEN(dev->mdev, order_write_after_write_umr))
		ret |= IB_ACCESS_RELAXED_ORDERING;

	if ((access_flags & IB_ACCESS_RELAXED_ORDERING) &&
	    (MLX5_CAP_GEN(dev->mdev, pci_relaxed_ordered_read) ||
	     MLX5_CAP_GEN(dev->mdev, relaxed_ordering_read_pci_enabled)) &&
	    !MLX5_CAP_GEN(dev->mdev, pci_relaxed_ordered_read_umr))
		ret |= IB_ACCESS_RELAXED_ORDERING;

	return ret;
}

When order_write_after_write_umr and pci_relaxed_ordered_read_umr are
supported, IB_ACCESS_RELAXED_ORDERING is stripped from key.access_flags.

When ib_frmr_pools_set_pinned() or ib_frmr_pool_pop() later calls
mlx5r_create_mkeys(), it passes that stripped key->access_flags to
set_mkc_access_pd_addr_fields():

drivers/infiniband/hw/mlx5/mr.c:mlx5r_create_mkeys() {
	...
	set_mkc_access_pd_addr_fields(mkc, key->access_flags, 0, dev->umrc.pd);
	MLX5_SET(mkc, mkc, free, 1);
	MLX5_SET(mkc, mkc, umr_en, 1);
	...
}

Because neither IB_ACCESS_RELAXED_ORDERING nor IB_ACCESS_UNORDERED remains in
key->access_flags, mlx5r_access_to_mkey_order(key->access_flags) returns
MLX5_MKEY_ORDER_STRICT, leaving order_write_after_write as 0 in mkc on
hardware that requires relaxed ordering.

In addition, because the stored pool->key.access_flags has
IB_ACCESS_RELAXED_ORDERING stripped, querying a pool via
RDMA_NLDEV_CMD_FRMR_POOLS_GET and passing that key back to
RDMA_NLDEV_CMD_FRMR_POOLS_SET will fail validate_ordering_access() in
mlx5r_build_frmr_key() with -EOPNOTSUPP.

Should mlx5r_create_mkeys() ensure free mkeys are created with relaxed
ordering (similar to how mlx5_set_umr_free_mkey() passes
IB_ACCESS_RELAXED_ORDERING), or should mlx5r_umr_get_unchangeable_access_flags()
and mlx5r_build_frmr_key() account for mkc_order_write_after_write_ro_only?

[ ... ]

> @@ -1028,6 +1052,10 @@ struct ib_mr *mlx5_ib_reg_user_mr_dmabuf(struct ib_pd *pd, u64 offset,
>  		    "offset 0x%llx, virt_addr 0x%llx, length 0x%llx, fd %d, access_flags 0x%x, mlx5_access_flags 0x%x\n",
>  		    offset, virt_addr, length, fd, access_flags, mlx5_access_flags);
>  
> +	err = validate_ordering_access(dev, access_flags);
> +	if (err)
> +		return ERR_PTR(err);
> +
>  	/* dmabuf requires xlt update via umr to work. */
>  	if (!mlx5r_umr_can_load_pas(dev, length))
>  		return ERR_PTR(-EINVAL);

[Severity: High]
As reported by Sashiko on v1 [1], if MLX5_IB_UAPI_REG_DMABUF_ACCESS_DATA_DIRECT
is set in mlx5_ib_reg_user_mr_dmabuf(), doesn't
reg_user_mr_dmabuf_by_data_direct() bypass this ordering check when
mkey_ro_valid is false?

In mlx5_ib_reg_user_mr_dmabuf(), validate_ordering_access() checks
access_flags before calling reg_user_mr_dmabuf_by_data_direct():

drivers/infiniband/hw/mlx5/mr.c:reg_user_mr_dmabuf_by_data_direct() {
	...
	if (!dev->mdev->data_direct->mkey_ro_valid)
		access_flags &= ~IB_ACCESS_RELAXED_ORDERING;
	crossed_mr = reg_user_mr_dmabuf(pd, &data_direct_dev->pdev->dev,
					offset, length, virt_addr, fd,
					access_flags, MLX5_MKC_ACCESS_MODE_KSM,
					NULL);
	...
	crossing_mr = reg_create_crossing_vhca_mr(pd, virt_addr, length,
						  crossed_mr->ibmr.lkey,
						  access_flags);
	...
}

If the user passes IB_ACCESS_RELAXED_ORDERING and mkey_ro_valid is false,
IB_ACCESS_RELAXED_ORDERING is cleared after validate_ordering_access() has
already succeeded, so reg_user_mr_dmabuf() and reg_create_crossing_vhca_mr()
proceed with strong-ordered access_flags.

Furthermore, if the user passes IB_ACCESS_UNORDERED (without
IB_ACCESS_RELAXED_ORDERING), validate_ordering_access() succeeds, but
_mlx5r_umr_update_mr_pas() only checks IB_ACCESS_RELAXED_ORDERING when
selecting between mdd->mkey_ro and the strong-ordered mdd->mkey:

drivers/infiniband/hw/mlx5/umr.c:_mlx5r_umr_update_mr_pas() {
	...
			if (mr->access_flags & IB_ACCESS_RELAXED_ORDERING &&
			    mdd->mkey_ro_valid)
				cur_ksm->key = cpu_to_be32(mdd->mkey_ro);
			else
				cur_ksm->key = cpu_to_be32(mdd->mkey);
	...
}

Should validate_ordering_access() also be checked after adjusting
access_flags in reg_user_mr_dmabuf_by_data_direct(), and should the Data
Direct path also handle IB_ACCESS_UNORDERED consistently with
IB_ACCESS_RELAXED_ORDERING?

[1] https://lore.kernel.org/r/20260726092943.2880176-16-michaelgur@nvidia.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102223325.2115128-1-michaelgur@nvidia.com?part=15