[PATCH rdma-next 0/3] Various fixes collected over time

[PATCH rdma-next 0/3] Various fixes collected over time

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Hi,

This is set of various and unrelated fixes that we collected over time.

Thanks

Avihai Horon (1):
  RDMA/uverbs: Fix incorrect variable type

Jack Morgenstein (2):
  RDMA/core: Clean up cq pool mechanism
  RDMA/core: Do not indicate device ready when device enablement fails

 drivers/infiniband/core/core_priv.h              |  3 +--
 drivers/infiniband/core/cq.c                     | 12 ++----------
 drivers/infiniband/core/device.c                 | 16 ++++++++++------
 .../infiniband/core/uverbs_std_types_device.c    | 14 +++++---------
 include/rdma/uverbs_ioctl.h                      | 10 ++++++++++
 5 files changed, 28 insertions(+), 27 deletions(-)

--
2.28.0

[PATCH rdma-next 1/3] RDMA/core: Clean up cq pool mechanism

[Leon Romanovsky]

From: Jack Morgenstein <jackm@dev.mellanox.co.il>

The CQ pool mechanism had two problems:
1. The CQ pool lists were uninitialized in the device registration
   error flow.  As a result, all the list pointers remained NULL.
   This caused the kernel to crash (in procedure ib_cq_pool_destroy)
   when that error flow was taken (and unregister called).
   The stack trace snippet:
   [ 4162.222276] BUG: kernel NULL pointer dereference, address: 0000000000000000
   [ 4162.222693] #PF: supervisor read access in kernel mode
   [ 4162.223184] #PF: error_code(0×0000) ? not-present page
   [ 4162.223602] PGD 0 P4D 0
   [ 4162.224060] Oops: 0000 [#1] SMP PTI
   . . .
   [ 4162.225312] RIP: 0010:ib_cq_pool_destroy+0x1b/0×70 [ib_core]
   . . .
   [ 4162.230853] Call Trace:
   [ 4162.231345]  disable_device+0x9f/0×130 [ib_core]
   [ 4162.232309]  __ib_unregister_device+0x35/0×90 [ib_core]
   [ 4162.232786]  ib_register_device+0x529/0×610 [ib_core]
   [ 4162.234259]  __mlx5_ib_add+0x3a/0×70 [mlx5_ib]
   [ 4162.234747]  mlx5_add_device+0x87/0×1c0 [mlx5_core]
   [ 4162.235263]  mlx5_register_interface+0x74/0xc0 [mlx5_core]
   [ 4162.236253]  do_one_initcall+0x4b/0×1f4
   [ 4162.237660]  do_init_module+0x5a/0×223
   [ 4162.238148]  load_module+0x1938/0×1d40

2. At device unregister, when cleaning up the cq pool, the cq's in the
   pool lists were freed, but the cq entries were left in the list.

The fix for the first issue is to initialize the cq pool lists when
the ib_device structure is allocated for a new device (in procedure
_ib_alloc_device).

The fix for the second problem is to delete cq entries from the pool
lists when cleaning up the cq pool.

In addition, procedure ib_cq_pool_destroy() is renamed to the more
appropriate name ib_cq_pool_cleanup().

Fixes: 4aa1615268a8 ("RDMA/core: Fix ordering of CQ pool destruction")
Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/core_priv.h |  3 +--
 drivers/infiniband/core/cq.c        | 12 ++----------
 drivers/infiniband/core/device.c    |  9 ++++++---
 3 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/infiniband/core/core_priv.h b/drivers/infiniband/core/core_priv.h
index f905c5a6456a..aec7005d4bbf 100644
--- a/drivers/infiniband/core/core_priv.h
+++ b/drivers/infiniband/core/core_priv.h
@@ -410,7 +410,6 @@ void rdma_umap_priv_init(struct rdma_umap_priv *priv,
 			 struct vm_area_struct *vma,
 			 struct rdma_user_mmap_entry *entry);
 
-void ib_cq_pool_init(struct ib_device *dev);
-void ib_cq_pool_destroy(struct ib_device *dev);
+void ib_cq_pool_cleanup(struct ib_device *dev);
 
 #endif /* _CORE_PRIV_H */
diff --git a/drivers/infiniband/core/cq.c b/drivers/infiniband/core/cq.c
index f357cf59333a..03c0641c65ce 100644
--- a/drivers/infiniband/core/cq.c
+++ b/drivers/infiniband/core/cq.c
@@ -364,16 +364,7 @@ void ib_free_cq(struct ib_cq *cq)
 }
 EXPORT_SYMBOL(ib_free_cq);
 
-void ib_cq_pool_init(struct ib_device *dev)
-{
-	unsigned int i;
-
-	spin_lock_init(&dev->cq_pools_lock);
-	for (i = 0; i < ARRAY_SIZE(dev->cq_pools); i++)
-		INIT_LIST_HEAD(&dev->cq_pools[i]);
-}
-
-void ib_cq_pool_destroy(struct ib_device *dev)
+void ib_cq_pool_cleanup(struct ib_device *dev)
 {
 	struct ib_cq *cq, *n;
 	unsigned int i;
@@ -382,6 +373,7 @@ void ib_cq_pool_destroy(struct ib_device *dev)
 		list_for_each_entry_safe(cq, n, &dev->cq_pools[i],
 					 pool_entry) {
 			WARN_ON(cq->cqe_used);
+			list_del(&cq->pool_entry);
 			cq->shared = false;
 			ib_free_cq(cq);
 		}
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 3ab1edea6acb..11485b8748a2 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -570,6 +570,7 @@ static void rdma_init_coredev(struct ib_core_device *coredev,
 struct ib_device *_ib_alloc_device(size_t size)
 {
 	struct ib_device *device;
+	unsigned int i;
 
 	if (WARN_ON(size < sizeof(struct ib_device)))
 		return NULL;
@@ -601,6 +602,10 @@ struct ib_device *_ib_alloc_device(size_t size)
 	init_completion(&device->unreg_completion);
 	INIT_WORK(&device->unregistration_work, ib_unregister_work);
 
+	spin_lock_init(&device->cq_pools_lock);
+	for (i = 0; i < ARRAY_SIZE(device->cq_pools); i++)
+		INIT_LIST_HEAD(&device->cq_pools[i]);
+
 	device->uverbs_cmd_mask =
 		BIT_ULL(IB_USER_VERBS_CMD_ALLOC_MW) |
 		BIT_ULL(IB_USER_VERBS_CMD_ALLOC_PD) |
@@ -1262,7 +1267,7 @@ static void disable_device(struct ib_device *device)
 		remove_client_context(device, cid);
 	}
 
-	ib_cq_pool_destroy(device);
+	ib_cq_pool_cleanup(device);
 
 	/* Pairs with refcount_set in enable_device */
 	ib_device_put(device);
@@ -1307,8 +1312,6 @@ static int enable_device_and_get(struct ib_device *device)
 			goto out;
 	}
 
-	ib_cq_pool_init(device);
-
 	down_read(&clients_rwsem);
 	xa_for_each_marked (&clients, index, client, CLIENT_REGISTERED) {
 		ret = add_client_context(device, client);
-- 
2.28.0

[PATCH rdma-next 2/3] RDMA/core: Do not indicate device ready when device enablement fails

[Leon Romanovsky]

From: Jack Morgenstein <jackm@dev.mellanox.co.il>

In procedure ib_register_device, procedure kobject_uevent is called
(advertising that the device is ready for userspace usage) even when
device_enable_and_get() returned an error.

As a result, various RDMA modules attempted to register for the device
even while the device driver was preparing to unregister the device.

Fix this by advertising the device availability only after enabling
the device succeeds.

Fixes: e7a5b4aafd82 ("RDMA/device: Don't fire uevent before device is fully initialized")
Suggested-by: Leon Romanovsky <leonro@mellanox.com>
Signed-off-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/device.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 11485b8748a2..e96f979e6d52 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -1397,9 +1397,6 @@ int ib_register_device(struct ib_device *device, const char *name,
 	}
 
 	ret = enable_device_and_get(device);
-	dev_set_uevent_suppress(&device->dev, false);
-	/* Mark for userspace that device is ready */
-	kobject_uevent(&device->dev.kobj, KOBJ_ADD);
 	if (ret) {
 		void (*dealloc_fn)(struct ib_device *);
 
@@ -1419,8 +1416,12 @@ int ib_register_device(struct ib_device *device, const char *name,
 		ib_device_put(device);
 		__ib_unregister_device(device);
 		device->ops.dealloc_driver = dealloc_fn;
+		dev_set_uevent_suppress(&device->dev, false);
 		return ret;
 	}
+	dev_set_uevent_suppress(&device->dev, false);
+	/* Mark for userspace that device is ready */
+	kobject_uevent(&device->dev.kobj, KOBJ_ADD);
 	ib_device_put(device);
 
 	return 0;
-- 
2.28.0

[PATCH rdma-next 3/3] RDMA/uverbs: Fix incorrect variable type

[Leon Romanovsky]

From: Avihai Horon <avihaih@nvidia.com>

Fix incorrect type of max_entries in UVERBS_METHOD_QUERY_GID_TABLE -
max_entries is of type size_t although it can take negative values.

The following static check revealed it:

drivers/infiniband/core/uverbs_std_types_device.c:338
ib_uverbs_handler_UVERBS_METHOD_QUERY_GID_TABLE()
warn: 'max_entries' unsigned <= 0

Fixes: 9f85cbe50aa0 ("RDMA/uverbs: Expose the new GID query API to user space")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Avihai Horon <avihaih@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/uverbs_std_types_device.c | 14 +++++---------
 include/rdma/uverbs_ioctl.h                       | 10 ++++++++++
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_std_types_device.c b/drivers/infiniband/core/uverbs_std_types_device.c
index 302f898c5833..9ec6971056fa 100644
--- a/drivers/infiniband/core/uverbs_std_types_device.c
+++ b/drivers/infiniband/core/uverbs_std_types_device.c
@@ -317,8 +317,7 @@ static int UVERBS_HANDLER(UVERBS_METHOD_QUERY_GID_TABLE)(
 	struct ib_device *ib_dev;
 	size_t user_entry_size;
 	ssize_t num_entries;
-	size_t max_entries;
-	size_t num_bytes;
+	int max_entries;
 	u32 flags;
 	int ret;
 
@@ -336,19 +335,16 @@ static int UVERBS_HANDLER(UVERBS_METHOD_QUERY_GID_TABLE)(
 		attrs, UVERBS_ATTR_QUERY_GID_TABLE_RESP_ENTRIES,
 		user_entry_size);
 	if (max_entries <= 0)
-		return -EINVAL;
+		return max_entries ?: -EINVAL;
 
 	ucontext = ib_uverbs_get_ucontext(attrs);
 	if (IS_ERR(ucontext))
 		return PTR_ERR(ucontext);
 	ib_dev = ucontext->device;
 
-	if (check_mul_overflow(max_entries, sizeof(*entries), &num_bytes))
-		return -EINVAL;
-
-	entries = uverbs_zalloc(attrs, num_bytes);
-	if (!entries)
-		return -ENOMEM;
+	entries = uverbs_kcalloc(attrs, max_entries, sizeof(*entries));
+	if (IS_ERR(entries))
+		return PTR_ERR(entries);
 
 	num_entries = rdma_query_gid_table(ib_dev, entries, max_entries);
 	if (num_entries < 0)
diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
index bf167ef6c688..39ef204753ec 100644
--- a/include/rdma/uverbs_ioctl.h
+++ b/include/rdma/uverbs_ioctl.h
@@ -865,6 +865,16 @@ static inline __malloc void *uverbs_zalloc(struct uverbs_attr_bundle *bundle,
 {
 	return _uverbs_alloc(bundle, size, GFP_KERNEL | __GFP_ZERO);
 }
+
+static inline __malloc void *uverbs_kcalloc(struct uverbs_attr_bundle *bundle,
+					    size_t n, size_t size)
+{
+	size_t bytes;
+
+	if (unlikely(check_mul_overflow(n, size, &bytes)))
+		return ERR_PTR(-EOVERFLOW);
+	return uverbs_zalloc(bundle, bytes);
+}
 int _uverbs_get_const(s64 *to, const struct uverbs_attr_bundle *attrs_bundle,
 		      size_t idx, s64 lower_bound, u64 upper_bound,
 		      s64 *def_val);
-- 
2.28.0

Re: [PATCH rdma-next 3/3] RDMA/uverbs: Fix incorrect variable type

[Dan Carpenter]

On Tue, Dec 08, 2020 at 09:35:45AM +0200, Leon Romanovsky wrote:
> @@ -336,19 +335,16 @@ static int UVERBS_HANDLER(UVERBS_METHOD_QUERY_GID_TABLE)(
>  		attrs, UVERBS_ATTR_QUERY_GID_TABLE_RESP_ENTRIES,
>  		user_entry_size);
>  	if (max_entries <= 0)
> -		return -EINVAL;
> +		return max_entries ?: -EINVAL;
>  
>  	ucontext = ib_uverbs_get_ucontext(attrs);
>  	if (IS_ERR(ucontext))
>  		return PTR_ERR(ucontext);
>  	ib_dev = ucontext->device;
>  
> -	if (check_mul_overflow(max_entries, sizeof(*entries), &num_bytes))
> -		return -EINVAL;
> -
> -	entries = uverbs_zalloc(attrs, num_bytes);
> -	if (!entries)
> -		return -ENOMEM;
> +	entries = uverbs_kcalloc(attrs, max_entries, sizeof(*entries));
> +	if (IS_ERR(entries))
> +		return PTR_ERR(entries);

This isn't right.  The uverbs_kcalloc() should match every other
kcalloc() function and return NULL on error.  This actually buggy
because it returns both is error pointers and NULL so it will lead to
a NULL dereference.

Btw, when a function returns both error pointers and NULL the NULL
return means that the feature has been deliberately disabled.  It's not
an error pointer because it's deliberate.

regards,
dan carpenter

>  
>  	num_entries = rdma_query_gid_table(ib_dev, entries, max_entries);
>  	if (num_entries < 0)
> diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
> index bf167ef6c688..39ef204753ec 100644
> --- a/include/rdma/uverbs_ioctl.h
> +++ b/include/rdma/uverbs_ioctl.h
> @@ -865,6 +865,16 @@ static inline __malloc void *uverbs_zalloc(struct uverbs_attr_bundle *bundle,
>  {
>  	return _uverbs_alloc(bundle, size, GFP_KERNEL | __GFP_ZERO);
>  }
> +
> +static inline __malloc void *uverbs_kcalloc(struct uverbs_attr_bundle *bundle,
> +					    size_t n, size_t size)
> +{
> +	size_t bytes;
> +
> +	if (unlikely(check_mul_overflow(n, size, &bytes)))
> +		return ERR_PTR(-EOVERFLOW);
> +	return uverbs_zalloc(bundle, bytes);
> +}
>  int _uverbs_get_const(s64 *to, const struct uverbs_attr_bundle *attrs_bundle,
>  		      size_t idx, s64 lower_bound, u64 upper_bound,
>  		      s64 *def_val);
> -- 
> 2.28.0

Re: [PATCH rdma-next 3/3] RDMA/uverbs: Fix incorrect variable type

[Leon Romanovsky]

On Tue, Dec 08, 2020 at 10:55:39AM +0300, Dan Carpenter wrote:
> On Tue, Dec 08, 2020 at 09:35:45AM +0200, Leon Romanovsky wrote:
> > @@ -336,19 +335,16 @@ static int UVERBS_HANDLER(UVERBS_METHOD_QUERY_GID_TABLE)(
> >  		attrs, UVERBS_ATTR_QUERY_GID_TABLE_RESP_ENTRIES,
> >  		user_entry_size);
> >  	if (max_entries <= 0)
> > -		return -EINVAL;
> > +		return max_entries ?: -EINVAL;
> >
> >  	ucontext = ib_uverbs_get_ucontext(attrs);
> >  	if (IS_ERR(ucontext))
> >  		return PTR_ERR(ucontext);
> >  	ib_dev = ucontext->device;
> >
> > -	if (check_mul_overflow(max_entries, sizeof(*entries), &num_bytes))
> > -		return -EINVAL;
> > -
> > -	entries = uverbs_zalloc(attrs, num_bytes);
> > -	if (!entries)
> > -		return -ENOMEM;
> > +	entries = uverbs_kcalloc(attrs, max_entries, sizeof(*entries));
> > +	if (IS_ERR(entries))
> > +		return PTR_ERR(entries);
>
> This isn't right.  The uverbs_kcalloc() should match every other
> kcalloc() function and return NULL on error.  This actually buggy
> because it returns both is error pointers and NULL so it will lead to
> a NULL dereference.

The actual bug was before, when an error result from uverbs_zalloc()
was treated as NULL. The uverbs_kcalloc/uverbs_zalloc will call to
_uverbs_alloc() that doesn't return NULL.

>
> Btw, when a function returns both error pointers and NULL the NULL
> return means that the feature has been deliberately disabled.  It's not
> an error pointer because it's deliberate.
>
> regards,
> dan carpenter
>
> >
> >  	num_entries = rdma_query_gid_table(ib_dev, entries, max_entries);
> >  	if (num_entries < 0)
> > diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
> > index bf167ef6c688..39ef204753ec 100644
> > --- a/include/rdma/uverbs_ioctl.h
> > +++ b/include/rdma/uverbs_ioctl.h
> > @@ -865,6 +865,16 @@ static inline __malloc void *uverbs_zalloc(struct uverbs_attr_bundle *bundle,
> >  {
> >  	return _uverbs_alloc(bundle, size, GFP_KERNEL | __GFP_ZERO);
> >  }
> > +
> > +static inline __malloc void *uverbs_kcalloc(struct uverbs_attr_bundle *bundle,
> > +					    size_t n, size_t size)
> > +{
> > +	size_t bytes;
> > +
> > +	if (unlikely(check_mul_overflow(n, size, &bytes)))
> > +		return ERR_PTR(-EOVERFLOW);
> > +	return uverbs_zalloc(bundle, bytes);
> > +}
> >  int _uverbs_get_const(s64 *to, const struct uverbs_attr_bundle *attrs_bundle,
> >  		      size_t idx, s64 lower_bound, u64 upper_bound,
> >  		      s64 *def_val);
> > --
> > 2.28.0

Re: [PATCH rdma-next 3/3] RDMA/uverbs: Fix incorrect variable type

[Dan Carpenter]

On Tue, Dec 08, 2020 at 10:54:05AM +0200, Leon Romanovsky wrote:
> On Tue, Dec 08, 2020 at 10:55:39AM +0300, Dan Carpenter wrote:
> > On Tue, Dec 08, 2020 at 09:35:45AM +0200, Leon Romanovsky wrote:
> > > @@ -336,19 +335,16 @@ static int UVERBS_HANDLER(UVERBS_METHOD_QUERY_GID_TABLE)(
> > >  		attrs, UVERBS_ATTR_QUERY_GID_TABLE_RESP_ENTRIES,
> > >  		user_entry_size);
> > >  	if (max_entries <= 0)
> > > -		return -EINVAL;
> > > +		return max_entries ?: -EINVAL;
> > >
> > >  	ucontext = ib_uverbs_get_ucontext(attrs);
> > >  	if (IS_ERR(ucontext))
> > >  		return PTR_ERR(ucontext);
> > >  	ib_dev = ucontext->device;
> > >
> > > -	if (check_mul_overflow(max_entries, sizeof(*entries), &num_bytes))
> > > -		return -EINVAL;
> > > -
> > > -	entries = uverbs_zalloc(attrs, num_bytes);
> > > -	if (!entries)
> > > -		return -ENOMEM;
> > > +	entries = uverbs_kcalloc(attrs, max_entries, sizeof(*entries));
> > > +	if (IS_ERR(entries))
> > > +		return PTR_ERR(entries);
> >
> > This isn't right.  The uverbs_kcalloc() should match every other
> > kcalloc() function and return NULL on error.  This actually buggy
> > because it returns both is error pointers and NULL so it will lead to
> > a NULL dereference.
> 
> The actual bug was before, when an error result from uverbs_zalloc()
> was treated as NULL. The uverbs_kcalloc/uverbs_zalloc will call to
> _uverbs_alloc() that doesn't return NULL.
> 

Ah....  Thanks.

regards,
dan carpenter

Re: [PATCH rdma-next 3/3] RDMA/uverbs: Fix incorrect variable type

[Jason Gunthorpe]

On Tue, Dec 08, 2020 at 10:55:39AM +0300, Dan Carpenter wrote:
> On Tue, Dec 08, 2020 at 09:35:45AM +0200, Leon Romanovsky wrote:
> > @@ -336,19 +335,16 @@ static int UVERBS_HANDLER(UVERBS_METHOD_QUERY_GID_TABLE)(
> >  		attrs, UVERBS_ATTR_QUERY_GID_TABLE_RESP_ENTRIES,
> >  		user_entry_size);
> >  	if (max_entries <= 0)
> > -		return -EINVAL;
> > +		return max_entries ?: -EINVAL;
> >  
> >  	ucontext = ib_uverbs_get_ucontext(attrs);
> >  	if (IS_ERR(ucontext))
> >  		return PTR_ERR(ucontext);
> >  	ib_dev = ucontext->device;
> >  
> > -	if (check_mul_overflow(max_entries, sizeof(*entries), &num_bytes))
> > -		return -EINVAL;
> > -
> > -	entries = uverbs_zalloc(attrs, num_bytes);
> > -	if (!entries)
> > -		return -ENOMEM;
> > +	entries = uverbs_kcalloc(attrs, max_entries, sizeof(*entries));
> > +	if (IS_ERR(entries))
> > +		return PTR_ERR(entries);
> 
> This isn't right.  The uverbs_kcalloc() should match every other
> kcalloc() function and return NULL on error.  This actually buggy
> because it returns both is error pointers and NULL so it will lead to
> a NULL dereference.

It is abnormal, but returing the EOVERFLOW to the caller vs ENOMEM
does seem somewhat relevant for debuggability..

If anything on the uverbs_*alloc* path returns NULL it is a bug.

Jason

Re: [PATCH rdma-next 0/3] Various fixes collected over time

[Jason Gunthorpe]

On Tue, Dec 08, 2020 at 09:35:42AM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Hi,
> 
> This is set of various and unrelated fixes that we collected over time.
> 
> Thanks
> 
> Avihai Horon (1):
>   RDMA/uverbs: Fix incorrect variable type
> 
> Jack Morgenstein (2):
>   RDMA/core: Clean up cq pool mechanism
>   RDMA/core: Do not indicate device ready when device enablement fails
> 
>  drivers/infiniband/core/core_priv.h              |  3 +--
>  drivers/infiniband/core/cq.c                     | 12 ++----------
>  drivers/infiniband/core/device.c                 | 16 ++++++++++------
>  .../infiniband/core/uverbs_std_types_device.c    | 14 +++++---------
>  include/rdma/uverbs_ioctl.h                      | 10 ++++++++++
>  5 files changed, 28 insertions(+), 27 deletions(-)

Applied to for-next, thanks

Jason