[PATCH v3 for-rc] RDMA/rxe: Fix panic when calling kmem_cache_create()

[PATCH v3 for-rc] RDMA/rxe: Fix panic when calling kmem_cache_create()

[Kamal Heib]

To avoid the following kernel panic when calling kmem_cache_create()
with a NULL pointer from pool_cache(), Block the rxe_param_set_add()
from running if the rdma_rxe module is not loaded.

 BUG: unable to handle kernel NULL pointer dereference at 000000000000000b
 PGD 0 P4D 0
 Oops: 0000 [#1] SMP NOPTI
 CPU: 4 PID: 8512 Comm: modprobe Kdump: loaded Not tainted 4.18.0-231.el8.x86_64 #1
 Hardware name: HPE ProLiant DL385 Gen10/ProLiant DL385 Gen10, BIOS A40 10/02/2018
 RIP: 0010:kmem_cache_alloc+0xd1/0x1b0
 Code: 8b 57 18 45 8b 77 1c 48 8b 5c 24 30 0f 1f 44 00 00 5b 48 89 e8 5d 41 5c 41 5d 41 5e 41 5f c3 81 e3 00 00 10 00 75 0e 4d 89 fe <41> f6 47 0b 04 0f 84 6c ff ff ff 4c 89 ff e8 cc da 01 00 49 89 c6
 RSP: 0018:ffffa2b8c773f9d0 EFLAGS: 00010246
 RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000005
 RDX: 0000000000000004 RSI: 00000000006080c0 RDI: 0000000000000000
 RBP: ffff8ea0a8634fd0 R08: ffffa2b8c773f988 R09: 00000000006000c0
 R10: 0000000000000000 R11: 0000000000000230 R12: 00000000006080c0
 R13: ffffffffc0a97fc8 R14: 0000000000000000 R15: 0000000000000000
 FS:  00007f9138ed9740(0000) GS:ffff8ea4ae800000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 000000000000000b CR3: 000000046d59a000 CR4: 00000000003406e0
 Call Trace:
  rxe_alloc+0xc8/0x160 [rdma_rxe]
  rxe_get_dma_mr+0x25/0xb0 [rdma_rxe]
  __ib_alloc_pd+0xcb/0x160 [ib_core]
  ib_mad_init_device+0x296/0x8b0 [ib_core]
  add_client_context+0x11a/0x160 [ib_core]
  enable_device_and_get+0xdc/0x1d0 [ib_core]
  ib_register_device+0x572/0x6b0 [ib_core]
  ? crypto_create_tfm+0x32/0xe0
  ? crypto_create_tfm+0x7a/0xe0
  ? crypto_alloc_tfm+0x58/0xf0
  rxe_register_device+0x19d/0x1c0 [rdma_rxe]
  rxe_net_add+0x3d/0x70 [rdma_rxe]
  ? dev_get_by_name_rcu+0x73/0x90
  rxe_param_set_add+0xaf/0xc0 [rdma_rxe]
  parse_args+0x179/0x370
  ? ref_module+0x1b0/0x1b0
  load_module+0x135e/0x17e0
  ? ref_module+0x1b0/0x1b0
  ? __do_sys_init_module+0x13b/0x180
  __do_sys_init_module+0x13b/0x180
  do_syscall_64+0x5b/0x1a0
  entry_SYSCALL_64_after_hwframe+0x65/0xca
 RIP: 0033:0x7f9137ed296e

Fixes: 8700e3e7c485 ("Soft RoCE driver")
Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe.c       | 4 ++++
 drivers/infiniband/sw/rxe/rxe.h       | 2 ++
 drivers/infiniband/sw/rxe/rxe_sysfs.c | 5 +++++
 3 files changed, 11 insertions(+)

diff --git a/drivers/infiniband/sw/rxe/rxe.c b/drivers/infiniband/sw/rxe/rxe.c
index 907203afbd99..88b5c866f5ab 100644
--- a/drivers/infiniband/sw/rxe/rxe.c
+++ b/drivers/infiniband/sw/rxe/rxe.c
@@ -40,6 +40,8 @@ MODULE_AUTHOR("Bob Pearson, Frank Zago, John Groves, Kamal Heib");
 MODULE_DESCRIPTION("Soft RDMA transport");
 MODULE_LICENSE("Dual BSD/GPL");
 
+bool rxe_is_loaded;
+
 /* free resources for a rxe device all objects created for this device must
  * have been destroyed
  */
@@ -315,6 +317,7 @@ static int __init rxe_module_init(void)
 		return err;
 
 	rdma_link_register(&rxe_link_ops);
+	rxe_is_loaded = true;
 	pr_info("loaded\n");
 	return 0;
 }
@@ -326,6 +329,7 @@ static void __exit rxe_module_exit(void)
 	rxe_net_exit();
 	rxe_cache_exit();
 
+	rxe_is_loaded = false;
 	pr_info("unloaded\n");
 }
 
diff --git a/drivers/infiniband/sw/rxe/rxe.h b/drivers/infiniband/sw/rxe/rxe.h
index fb07eed9e402..d9b71b5e2fba 100644
--- a/drivers/infiniband/sw/rxe/rxe.h
+++ b/drivers/infiniband/sw/rxe/rxe.h
@@ -67,6 +67,8 @@
 
 #define RXE_ROCE_V2_SPORT		(0xc000)
 
+extern bool rxe_is_loaded;
+
 static inline u32 rxe_crc32(struct rxe_dev *rxe,
 			    u32 crc, void *next, size_t len)
 {
diff --git a/drivers/infiniband/sw/rxe/rxe_sysfs.c b/drivers/infiniband/sw/rxe/rxe_sysfs.c
index ccda5f5a3bc0..12c7ca0764d5 100644
--- a/drivers/infiniband/sw/rxe/rxe_sysfs.c
+++ b/drivers/infiniband/sw/rxe/rxe_sysfs.c
@@ -61,6 +61,11 @@ static int rxe_param_set_add(const char *val, const struct kernel_param *kp)
 	struct net_device *ndev;
 	struct rxe_dev *exists;
 
+	if (!rxe_is_loaded) {
+		pr_err("Please make sure to load the rdma_rxe module first\n");
+		return -EINVAL;
+	}
+
 	len = sanitize_arg(val, intf, sizeof(intf));
 	if (!len) {
 		pr_err("add: invalid interface name\n");
-- 
2.26.2

Re: [PATCH v3 for-rc] RDMA/rxe: Fix panic when calling kmem_cache_create()

[Bart Van Assche]

On 8/24/20 8:52 AM, Kamal Heib wrote:
> +bool rxe_is_loaded;

The name of this variable seems wrong to me. My understanding is that rxe_module_init() is
called whether or not rxe has been built as a module. Consider renaming this variable into
e.g. "rxe_initialized".

> diff --git a/drivers/infiniband/sw/rxe/rxe_sysfs.c b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> index ccda5f5a3bc0..12c7ca0764d5 100644
> --- a/drivers/infiniband/sw/rxe/rxe_sysfs.c
> +++ b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> @@ -61,6 +61,11 @@ static int rxe_param_set_add(const char *val, const struct kernel_param *kp)
>   	struct net_device *ndev;
>   	struct rxe_dev *exists;
>   
> +	if (!rxe_is_loaded) {
> +		pr_err("Please make sure to load the rdma_rxe module first\n");
> +		return -EINVAL;
> +	}
> +
>   	len = sanitize_arg(val, intf, sizeof(intf));
>   	if (!len) {
>   		pr_err("add: invalid interface name\n");

The above message is misleading. Consider changing it into e.g. the following:

     Please wait until initialization of the rdma_rxe module has finished.

Additionally, how about returning -EAGAIN instead of -EINVAL?

Thanks,

Bart.

Re: [PATCH v3 for-rc] RDMA/rxe: Fix panic when calling kmem_cache_create()

[Jason Gunthorpe]

On Mon, Aug 24, 2020 at 09:41:14AM -0700, Bart Van Assche wrote:
> On 8/24/20 8:52 AM, Kamal Heib wrote:
> > +bool rxe_is_loaded;
> 
> The name of this variable seems wrong to me. My understanding is that rxe_module_init() is
> called whether or not rxe has been built as a module. Consider renaming this variable into
> e.g. "rxe_initialized".
> 
> > diff --git a/drivers/infiniband/sw/rxe/rxe_sysfs.c b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> > index ccda5f5a3bc0..12c7ca0764d5 100644
> > +++ b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> > @@ -61,6 +61,11 @@ static int rxe_param_set_add(const char *val, const struct kernel_param *kp)
> >   	struct net_device *ndev;
> >   	struct rxe_dev *exists;
> > +	if (!rxe_is_loaded) {
> > +		pr_err("Please make sure to load the rdma_rxe module first\n");
> > +		return -EINVAL;
> > +	}
> > +
> >   	len = sanitize_arg(val, intf, sizeof(intf));
> >   	if (!len) {
> >   		pr_err("add: invalid interface name\n");
> 
> The above message is misleading. Consider changing it into e.g. the following:
> 
>     Please wait until initialization of the rdma_rxe module has finished.

How about "Module parameters are not supported, use rdma link add"

Jason

Re: [PATCH v3 for-rc] RDMA/rxe: Fix panic when calling kmem_cache_create()

[Kamal Heib]

On Mon, Aug 24, 2020 at 09:41:14AM -0700, Bart Van Assche wrote:
> On 8/24/20 8:52 AM, Kamal Heib wrote:
> > +bool rxe_is_loaded;
> 
> The name of this variable seems wrong to me. My understanding is that rxe_module_init() is
> called whether or not rxe has been built as a module. Consider renaming this variable into
> e.g. "rxe_initialized".
>
OK, I'll change it.

> > diff --git a/drivers/infiniband/sw/rxe/rxe_sysfs.c b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> > index ccda5f5a3bc0..12c7ca0764d5 100644
> > --- a/drivers/infiniband/sw/rxe/rxe_sysfs.c
> > +++ b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> > @@ -61,6 +61,11 @@ static int rxe_param_set_add(const char *val, const struct kernel_param *kp)
> >   	struct net_device *ndev;
> >   	struct rxe_dev *exists;
> > +	if (!rxe_is_loaded) {
> > +		pr_err("Please make sure to load the rdma_rxe module first\n");
> > +		return -EINVAL;
> > +	}
> > +
> >   	len = sanitize_arg(val, intf, sizeof(intf));
> >   	if (!len) {
> >   		pr_err("add: invalid interface name\n");
> 
> The above message is misleading. Consider changing it into e.g. the following:
> 
>     Please wait until initialization of the rdma_rxe module has finished.
> 
> Additionally, how about returning -EAGAIN instead of -EINVAL?
>
Yes, this makes more sense, I'll change it.

Thanks,
Kamal

> Thanks,
> 
> Bart.
> 
> 

Re: [PATCH v3 for-rc] RDMA/rxe: Fix panic when calling kmem_cache_create()

[Kamal Heib]

On Mon, Aug 24, 2020 at 01:51:11PM -0300, Jason Gunthorpe wrote:
> On Mon, Aug 24, 2020 at 09:41:14AM -0700, Bart Van Assche wrote:
> > On 8/24/20 8:52 AM, Kamal Heib wrote:
> > > +bool rxe_is_loaded;
> > 
> > The name of this variable seems wrong to me. My understanding is that rxe_module_init() is
> > called whether or not rxe has been built as a module. Consider renaming this variable into
> > e.g. "rxe_initialized".
> > 
> > > diff --git a/drivers/infiniband/sw/rxe/rxe_sysfs.c b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> > > index ccda5f5a3bc0..12c7ca0764d5 100644
> > > +++ b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> > > @@ -61,6 +61,11 @@ static int rxe_param_set_add(const char *val, const struct kernel_param *kp)
> > >   	struct net_device *ndev;
> > >   	struct rxe_dev *exists;
> > > +	if (!rxe_is_loaded) {
> > > +		pr_err("Please make sure to load the rdma_rxe module first\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > >   	len = sanitize_arg(val, intf, sizeof(intf));
> > >   	if (!len) {
> > >   		pr_err("add: invalid interface name\n");
> > 
> > The above message is misleading. Consider changing it into e.g. the following:
> > 
> >     Please wait until initialization of the rdma_rxe module has finished.
> 
> How about "Module parameters are not supported, use rdma link add"
> 
I don't think so, This patch is targeted to for-rc (stable) and the
support of "rdma link add" is not part of all the stable versions.

Thanks,
Kamal

> Jason

Re: [PATCH v3 for-rc] RDMA/rxe: Fix panic when calling kmem_cache_create()

[Jason Gunthorpe]

On Tue, Aug 25, 2020 at 12:36:24PM +0300, Kamal Heib wrote:
> On Mon, Aug 24, 2020 at 01:51:11PM -0300, Jason Gunthorpe wrote:
> > On Mon, Aug 24, 2020 at 09:41:14AM -0700, Bart Van Assche wrote:
> > > On 8/24/20 8:52 AM, Kamal Heib wrote:
> > > > +bool rxe_is_loaded;
> > > 
> > > The name of this variable seems wrong to me. My understanding is that rxe_module_init() is
> > > called whether or not rxe has been built as a module. Consider renaming this variable into
> > > e.g. "rxe_initialized".
> > > 
> > > > diff --git a/drivers/infiniband/sw/rxe/rxe_sysfs.c b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> > > > index ccda5f5a3bc0..12c7ca0764d5 100644
> > > > +++ b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> > > > @@ -61,6 +61,11 @@ static int rxe_param_set_add(const char *val, const struct kernel_param *kp)
> > > >   	struct net_device *ndev;
> > > >   	struct rxe_dev *exists;
> > > > +	if (!rxe_is_loaded) {
> > > > +		pr_err("Please make sure to load the rdma_rxe module first\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > >   	len = sanitize_arg(val, intf, sizeof(intf));
> > > >   	if (!len) {
> > > >   		pr_err("add: invalid interface name\n");
> > > 
> > > The above message is misleading. Consider changing it into e.g. the following:
> > > 
> > >     Please wait until initialization of the rdma_rxe module has finished.
> > 
> > How about "Module parameters are not supported, use rdma link add"
>
> I don't think so, This patch is targeted to for-rc (stable) and the
> support of "rdma link add" is not part of all the stable versions.

then add "or rxe_cfg"

Jason

Re: [PATCH v3 for-rc] RDMA/rxe: Fix panic when calling kmem_cache_create()

[Kamal Heib]

On Tue, Aug 25, 2020 at 10:25:57AM -0300, Jason Gunthorpe wrote:
> On Tue, Aug 25, 2020 at 12:36:24PM +0300, Kamal Heib wrote:
> > On Mon, Aug 24, 2020 at 01:51:11PM -0300, Jason Gunthorpe wrote:
> > > On Mon, Aug 24, 2020 at 09:41:14AM -0700, Bart Van Assche wrote:
> > > > On 8/24/20 8:52 AM, Kamal Heib wrote:
> > > > > +bool rxe_is_loaded;
> > > > 
> > > > The name of this variable seems wrong to me. My understanding is that rxe_module_init() is
> > > > called whether or not rxe has been built as a module. Consider renaming this variable into
> > > > e.g. "rxe_initialized".
> > > > 
> > > > > diff --git a/drivers/infiniband/sw/rxe/rxe_sysfs.c b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> > > > > index ccda5f5a3bc0..12c7ca0764d5 100644
> > > > > +++ b/drivers/infiniband/sw/rxe/rxe_sysfs.c
> > > > > @@ -61,6 +61,11 @@ static int rxe_param_set_add(const char *val, const struct kernel_param *kp)
> > > > >   	struct net_device *ndev;
> > > > >   	struct rxe_dev *exists;
> > > > > +	if (!rxe_is_loaded) {
> > > > > +		pr_err("Please make sure to load the rdma_rxe module first\n");
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > >   	len = sanitize_arg(val, intf, sizeof(intf));
> > > > >   	if (!len) {
> > > > >   		pr_err("add: invalid interface name\n");
> > > > 
> > > > The above message is misleading. Consider changing it into e.g. the following:
> > > > 
> > > >     Please wait until initialization of the rdma_rxe module has finished.
> > > 
> > > How about "Module parameters are not supported, use rdma link add"
> >
> > I don't think so, This patch is targeted to for-rc (stable) and the
> > support of "rdma link add" is not part of all the stable versions.
> 
> then add "or rxe_cfg"
>
Done.

Thanks,
Kamal

> Jason