[PATCH rdma-rc 0/6] Batch of mlx5 fixes for v6.10

[PATCH rdma-rc 0/6] Batch of mlx5 fixes for v6.10

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Various fixes which I collected during the last few weeks.

Thanks

Jason Gunthorpe (3):
  RDMA/mlx5: Remove extra unlock on error path
  RDMA/mlx5: Follow rb_key.ats when creating new mkeys
  RDMA/mlx5: Ensure created mkeys always have a populated rb_key

Leon Romanovsky (1):
  RDMA/cache: Release GID table even if leak is detected

Patrisious Haddad (1):
  RDMA/mlx5: Add check for srq max_sge attribute

Yishai Hadas (1):
  RDMA/mlx5: Fix unwind flow as part of mlx5_ib_stage_init_init

 drivers/infiniband/core/cache.c   | 13 ++++---------
 drivers/infiniband/hw/mlx5/main.c |  4 ++--
 drivers/infiniband/hw/mlx5/mr.c   |  8 ++++----
 drivers/infiniband/hw/mlx5/srq.c  | 13 ++++++++-----
 4 files changed, 18 insertions(+), 20 deletions(-)

-- 
2.45.1

[PATCH rdma-rc 1/6] RDMA/cache: Release GID table even if leak is detected

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

When the table is released, we nullify pointer to GID table, it means
that in case GID entry leak is detected, we will leak table too.

Delete code that prevents table destruction.

Fixes: b150c3862d21 ("IB/core: Introduce GID entry reference counts")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/cache.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
index c02a96d3572a..aa62c8c7ca75 100644
--- a/drivers/infiniband/core/cache.c
+++ b/drivers/infiniband/core/cache.c
@@ -794,7 +794,6 @@ static struct ib_gid_table *alloc_gid_table(int sz)
 static void release_gid_table(struct ib_device *device,
 			      struct ib_gid_table *table)
 {
-	bool leak = false;
 	int i;
 
 	if (!table)
@@ -803,15 +802,11 @@ static void release_gid_table(struct ib_device *device,
 	for (i = 0; i < table->sz; i++) {
 		if (is_gid_entry_free(table->data_vec[i]))
 			continue;
-		if (kref_read(&table->data_vec[i]->kref) > 1) {
-			dev_err(&device->dev,
-				"GID entry ref leak for index %d ref=%u\n", i,
-				kref_read(&table->data_vec[i]->kref));
-			leak = true;
-		}
+
+		dev_err(&device->dev,
+			"GID entry ref leak for index %d ref=%u\n", i,
+			kref_read(&table->data_vec[i]->kref));
 	}
-	if (leak)
-		return;
 
 	mutex_destroy(&table->lock);
 	kfree(table->data_vec);
-- 
2.45.1

[PATCH rdma-rc 2/6] RDMA/mlx5: Remove extra unlock on error path

[Leon Romanovsky]

From: Jason Gunthorpe <jgg@nvidia.com>

The below commit lifted the locking out of this function but left this
error path unlock behind resulting in unbalanced locking. Remove the
missed unlock too.

Cc: stable@vger.kernel.org
Fixes: 627122280c87 ("RDMA/mlx5: Add work to remove temporary entries from the cache")
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/hw/mlx5/mr.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
index ecc111ed5d86..38d2c743db87 100644
--- a/drivers/infiniband/hw/mlx5/mr.c
+++ b/drivers/infiniband/hw/mlx5/mr.c
@@ -641,10 +641,8 @@ static int mlx5_cache_ent_insert(struct mlx5_mkey_cache *cache,
 			new = &((*new)->rb_left);
 		if (cmp < 0)
 			new = &((*new)->rb_right);
-		if (cmp == 0) {
-			mutex_unlock(&cache->rb_lock);
+		if (cmp == 0)
 			return -EEXIST;
-		}
 	}
 
 	/* Add new node and rebalance tree. */
-- 
2.45.1

[PATCH rdma-rc 3/6] RDMA/mlx5: Follow rb_key.ats when creating new mkeys

[Leon Romanovsky]

From: Jason Gunthorpe <jgg@nvidia.com>

When a cache ent already exists but doesn't have any mkeys in it the cache
will automatically create a new one based on the specification in the
ent->rb_key.

ent->ats was missed when creating the new key and so ma_translation_mode
was not being set even though the ent requires it.

Cc: stable@vger.kernel.org
Fixes: 73d09b2fe833 ("RDMA/mlx5: Introduce mlx5r_cache_rb_key")
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Michael Guralnik <michaelgur@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/hw/mlx5/mr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
index 38d2c743db87..35dcb9d9e12a 100644
--- a/drivers/infiniband/hw/mlx5/mr.c
+++ b/drivers/infiniband/hw/mlx5/mr.c
@@ -246,6 +246,7 @@ static void set_cache_mkc(struct mlx5_cache_ent *ent, void *mkc)
 	MLX5_SET(mkc, mkc, access_mode_1_0, ent->rb_key.access_mode & 0x3);
 	MLX5_SET(mkc, mkc, access_mode_4_2,
 		(ent->rb_key.access_mode >> 2) & 0x7);
+	MLX5_SET(mkc, mkc, ma_translation_mode, !!ent->rb_key.ats);
 
 	MLX5_SET(mkc, mkc, translations_octword_size,
 		 get_mkc_octo_size(ent->rb_key.access_mode,
-- 
2.45.1

[PATCH rdma-rc 4/6] RDMA/mlx5: Ensure created mkeys always have a populated rb_key

[Leon Romanovsky]

From: Jason Gunthorpe <jgg@nvidia.com>

cachable and mmkey.rb_key together are used by mlx5_revoke_mr() to put the
MR/mkey back into the cache. In all cases they should be set correctly.

alloc_cacheable_mr() was setting cachable but not filling rb_key,
resulting in cache_ent_find_and_store() bucketing them all into a 0 length
entry.

implicit_get_child_mr()/mlx5_ib_alloc_implicit_mr() failed to set cachable
or rb_key at all, so the cache was not working at all for implicit ODP.

Cc: stable@vger.kernel.org
Fixes: 8c1185fef68c ("RDMA/mlx5: Change check for cacheable mkeys")
Fixes: dd1b913fb0d0 ("RDMA/mlx5: Cache all user cacheable mkeys on dereg MR flow")
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/hw/mlx5/mr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
index 35dcb9d9e12a..d3c1f63791a2 100644
--- a/drivers/infiniband/hw/mlx5/mr.c
+++ b/drivers/infiniband/hw/mlx5/mr.c
@@ -718,6 +718,8 @@ static struct mlx5_ib_mr *_mlx5_mr_cache_alloc(struct mlx5_ib_dev *dev,
 	}
 	mr->mmkey.cache_ent = ent;
 	mr->mmkey.type = MLX5_MKEY_MR;
+	mr->mmkey.rb_key = ent->rb_key;
+	mr->mmkey.cacheable = true;
 	init_waitqueue_head(&mr->mmkey.wait);
 	return mr;
 }
@@ -1168,7 +1170,6 @@ static struct mlx5_ib_mr *alloc_cacheable_mr(struct ib_pd *pd,
 	mr->ibmr.pd = pd;
 	mr->umem = umem;
 	mr->page_shift = order_base_2(page_size);
-	mr->mmkey.cacheable = true;
 	set_mr_fields(dev, mr, umem->length, access_flags, iova);
 
 	return mr;
-- 
2.45.1

[PATCH rdma-rc 5/6] RDMA/mlx5: Fix unwind flow as part of mlx5_ib_stage_init_init

[Leon Romanovsky]

From: Yishai Hadas <yishaih@nvidia.com>

Fix unwind flow as part of mlx5_ib_stage_init_init to use the correct
goto upon an error.

Fixes: 758ce14aee82 ("RDMA/mlx5: Implement MACsec gid addition and deletion")
Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
Reviewed-by: Patrisious Haddad <phaddad@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/hw/mlx5/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 2366c46eebc8..43660c831b22 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -3759,10 +3759,10 @@ static int mlx5_ib_stage_init_init(struct mlx5_ib_dev *dev)
 	spin_lock_init(&dev->dm.lock);
 	dev->dm.dev = mdev;
 	return 0;
-err:
-	mlx5r_macsec_dealloc_gids(dev);
 err_mp:
 	mlx5_ib_cleanup_multiport_master(dev);
+err:
+	mlx5r_macsec_dealloc_gids(dev);
 	return err;
 }
 
-- 
2.45.1

[PATCH rdma-rc 6/6] RDMA/mlx5: Add check for srq max_sge attribute

[Leon Romanovsky]

From: Patrisious Haddad <phaddad@nvidia.com>

max_sge attribute is passed by the user, and is inserted and used
unchecked, so verify that the value doesn't exceed maximum allowed value
before using it.

Fixes: e126ba97dba9 ("mlx5: Add driver for Mellanox Connect-IB adapters")
Signed-off-by: Patrisious Haddad <phaddad@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/hw/mlx5/srq.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/srq.c b/drivers/infiniband/hw/mlx5/srq.c
index a056ea835da5..84be0c3d5699 100644
--- a/drivers/infiniband/hw/mlx5/srq.c
+++ b/drivers/infiniband/hw/mlx5/srq.c
@@ -199,17 +199,20 @@ int mlx5_ib_create_srq(struct ib_srq *ib_srq,
 	int err;
 	struct mlx5_srq_attr in = {};
 	__u32 max_srq_wqes = 1 << MLX5_CAP_GEN(dev->mdev, log_max_srq_sz);
+	__u32 max_sge_sz =  MLX5_CAP_GEN(dev->mdev, max_wqe_sz_rq) /
+			    sizeof(struct mlx5_wqe_data_seg);
 
 	if (init_attr->srq_type != IB_SRQT_BASIC &&
 	    init_attr->srq_type != IB_SRQT_XRC &&
 	    init_attr->srq_type != IB_SRQT_TM)
 		return -EOPNOTSUPP;
 
-	/* Sanity check SRQ size before proceeding */
-	if (init_attr->attr.max_wr >= max_srq_wqes) {
-		mlx5_ib_dbg(dev, "max_wr %d, cap %d\n",
-			    init_attr->attr.max_wr,
-			    max_srq_wqes);
+	/* Sanity check SRQ and sge size before proceeding */
+	if (init_attr->attr.max_wr >= max_srq_wqes ||
+	    init_attr->attr.max_sge > max_sge_sz) {
+		mlx5_ib_dbg(dev, "max_wr %d,wr_cap %d,max_sge %d, sge_cap:%d\n",
+			    init_attr->attr.max_wr, max_srq_wqes,
+			    init_attr->attr.max_sge, max_sge_sz);
 		return -EINVAL;
 	}
 
-- 
2.45.1

Re: [PATCH rdma-rc 0/6] Batch of mlx5 fixes for v6.10

[Leon Romanovsky]

On Tue, 28 May 2024 15:52:50 +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Various fixes which I collected during the last few weeks.
> 
> Thanks
> 
> Jason Gunthorpe (3):
>   RDMA/mlx5: Remove extra unlock on error path
>   RDMA/mlx5: Follow rb_key.ats when creating new mkeys
>   RDMA/mlx5: Ensure created mkeys always have a populated rb_key
> 
> [...]

Applied, thanks!

[1/6] RDMA/cache: Release GID table even if leak is detected
      https://git.kernel.org/rdma/rdma/c/3ac844148b9b80
[2/6] RDMA/mlx5: Remove extra unlock on error path
      https://git.kernel.org/rdma/rdma/c/36e1ea42751ce8
[3/6] RDMA/mlx5: Follow rb_key.ats when creating new mkeys
      https://git.kernel.org/rdma/rdma/c/0f28eefd6bdb6c
[4/6] RDMA/mlx5: Ensure created mkeys always have a populated rb_key
      https://git.kernel.org/rdma/rdma/c/7322d666dbd55f
[5/6] RDMA/mlx5: Fix unwind flow as part of mlx5_ib_stage_init_init
      https://git.kernel.org/rdma/rdma/c/6bb41bed95ec39
[6/6] RDMA/mlx5: Add check for srq max_sge attribute
      https://git.kernel.org/rdma/rdma/c/c405e9cac10239

Best regards,
-- 
Leon Romanovsky <leon@kernel.org>

Re: [PATCH rdma-rc 1/6] RDMA/cache: Release GID table even if leak is detected

[Jason Gunthorpe]

On Tue, May 28, 2024 at 03:52:51PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> When the table is released, we nullify pointer to GID table, it means
> that in case GID entry leak is detected, we will leak table too.
> 
> Delete code that prevents table destruction.

This converts a memory leak into a UAF, it doesn't seem like a good direction??

Jason

Re: [PATCH rdma-rc 1/6] RDMA/cache: Release GID table even if leak is detected

[Leon Romanovsky]

On Tue, Jun 04, 2024 at 01:36:36PM -0300, Jason Gunthorpe wrote:
> On Tue, May 28, 2024 at 03:52:51PM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > When the table is released, we nullify pointer to GID table, it means
> > that in case GID entry leak is detected, we will leak table too.
> > 
> > Delete code that prevents table destruction.
> 
> This converts a memory leak into a UAF, it doesn't seem like a good direction??

Maybe we should convert dev_err() to be WARN_ON(). I didn't see any
complains about GID entry leaks. It is debug print.

Thanks

> 
> Jason
> 

Re: [PATCH rdma-rc 1/6] RDMA/cache: Release GID table even if leak is detected

[Jason Gunthorpe]

On Wed, Jun 05, 2024 at 12:44:56PM +0300, Leon Romanovsky wrote:
> On Tue, Jun 04, 2024 at 01:36:36PM -0300, Jason Gunthorpe wrote:
> > On Tue, May 28, 2024 at 03:52:51PM +0300, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@nvidia.com>
> > > 
> > > When the table is released, we nullify pointer to GID table, it means
> > > that in case GID entry leak is detected, we will leak table too.
> > > 
> > > Delete code that prevents table destruction.
> > 
> > This converts a memory leak into a UAF, it doesn't seem like a good direction??
> 
> Maybe we should convert dev_err() to be WARN_ON(). I didn't see any
> complains about GID entry leaks. It is debug print.

Yes WARN_ON is better

Jason

Re: [PATCH rdma-rc 1/6] RDMA/cache: Release GID table even if leak is detected

[Jason Gunthorpe]

On Tue, May 28, 2024 at 03:52:51PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> When the table is released, we nullify pointer to GID table, it means
> that in case GID entry leak is detected, we will leak table too.
> 
> Delete code that prevents table destruction.
> 
> Fixes: b150c3862d21 ("IB/core: Introduce GID entry reference counts")
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  drivers/infiniband/core/cache.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)

Since this is causing syzkaller failures and it really doesn't seem
like stable material I'm dropping it from the rc branch and putting it
in for-next. Lets see if we can fix the failures before the merge window.

Jason

Re: [PATCH rdma-rc 1/6] RDMA/cache: Release GID table even if leak is detected

[Leon Romanovsky]

On Fri, Jun 21, 2024 at 10:21:39AM -0300, Jason Gunthorpe wrote:
> On Tue, May 28, 2024 at 03:52:51PM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > When the table is released, we nullify pointer to GID table, it means
> > that in case GID entry leak is detected, we will leak table too.
> > 
> > Delete code that prevents table destruction.
> > 
> > Fixes: b150c3862d21 ("IB/core: Introduce GID entry reference counts")
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> >  drivers/infiniband/core/cache.c | 13 ++++---------
> >  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> Since this is causing syzkaller failures and it really doesn't seem
> like stable material I'm dropping it from the rc branch and putting it
> in for-next. Lets see if we can fix the failures before the merge window.

I don't see it in the for-next branch, can you please add it?

Thanks

> 
> Jason