| Message ID | 20200818142504.917186-1-kamalheib1@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [v2,for-rc] RDMA/rxe: Fix panic when calling kmem_cache_create() | expand |
On Tue, Aug 18, 2020 at 05:25:04PM +0300, Kamal Heib wrote: > To avoid the following kernel panic when calling kmem_cache_create() > with a NULL pointer from pool_cache(), move the rxe_cache_init() to the > context of device initialization. I think you've hit on a bigger bug than just this oops. rxe_net_add() should never be called before rxe_module_init(), that surely subtly breaks all kinds of things. Maybe it is time to remove these module parameters? Jason
On Tue, Aug 18, 2020 at 01:31:57PM -0300, Jason Gunthorpe wrote: > On Tue, Aug 18, 2020 at 05:25:04PM +0300, Kamal Heib wrote: > > To avoid the following kernel panic when calling kmem_cache_create() > > with a NULL pointer from pool_cache(), move the rxe_cache_init() to the > > context of device initialization. > > I think you've hit on a bigger bug than just this oops. > > rxe_net_add() should never be called before rxe_module_init(), that > surely subtly breaks all kinds of things. > > Maybe it is time to remove these module parameters? > Yes, I agree, this can be done in for-next. But at least can we take this patch to for-rc (stable) to fix this issue in stable releases? Thanks, Kamal > Jason
On Wed, Aug 19, 2020 at 12:15:45AM +0300, Kamal Heib wrote: > On Tue, Aug 18, 2020 at 01:31:57PM -0300, Jason Gunthorpe wrote: > > On Tue, Aug 18, 2020 at 05:25:04PM +0300, Kamal Heib wrote: > > > To avoid the following kernel panic when calling kmem_cache_create() > > > with a NULL pointer from pool_cache(), move the rxe_cache_init() to the > > > context of device initialization. > > > > I think you've hit on a bigger bug than just this oops. > > > > rxe_net_add() should never be called before rxe_module_init(), that > > surely subtly breaks all kinds of things. > > > > Maybe it is time to remove these module parameters? > > > Yes, I agree, this can be done in for-next. > > But at least can we take this patch to for-rc (stable) to fix this issue > in stable releases? If you want to fix something in stable then block the module options from working as actual module options - eg before rxe_module_init() runs. Jason
On Thu, Aug 20, 2020 at 08:37:17AM -0300, Jason Gunthorpe wrote: > On Wed, Aug 19, 2020 at 12:15:45AM +0300, Kamal Heib wrote: > > On Tue, Aug 18, 2020 at 01:31:57PM -0300, Jason Gunthorpe wrote: > > > On Tue, Aug 18, 2020 at 05:25:04PM +0300, Kamal Heib wrote: > > > > To avoid the following kernel panic when calling kmem_cache_create() > > > > with a NULL pointer from pool_cache(), move the rxe_cache_init() to the > > > > context of device initialization. > > > > > > I think you've hit on a bigger bug than just this oops. > > > > > > rxe_net_add() should never be called before rxe_module_init(), that > > > surely subtly breaks all kinds of things. > > > > > > Maybe it is time to remove these module parameters? > > > > > Yes, I agree, this can be done in for-next. > > > > But at least can we take this patch to for-rc (stable) to fix this issue > > in stable releases? > > If you want to fix something in stable then block the module options > from working as actual module options - eg before rxe_module_init() > runs. > > Jason Something like the following patch? diff --git a/drivers/infiniband/sw/rxe/rxe.c b/drivers/infiniband/sw/rxe/rxe.c index 907203afbd99..872ebc57ac06 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 = false; + /* 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"); Thanks, Kamal
On Sun, Aug 23, 2020 at 10:45:58PM +0300, Kamal Heib wrote: > On Thu, Aug 20, 2020 at 08:37:17AM -0300, Jason Gunthorpe wrote: > > On Wed, Aug 19, 2020 at 12:15:45AM +0300, Kamal Heib wrote: > > > On Tue, Aug 18, 2020 at 01:31:57PM -0300, Jason Gunthorpe wrote: > > > > On Tue, Aug 18, 2020 at 05:25:04PM +0300, Kamal Heib wrote: > > > > > To avoid the following kernel panic when calling kmem_cache_create() > > > > > with a NULL pointer from pool_cache(), move the rxe_cache_init() to the > > > > > context of device initialization. > > > > > > > > I think you've hit on a bigger bug than just this oops. > > > > > > > > rxe_net_add() should never be called before rxe_module_init(), that > > > > surely subtly breaks all kinds of things. > > > > > > > > Maybe it is time to remove these module parameters? > > > > > > > Yes, I agree, this can be done in for-next. > > > > > > But at least can we take this patch to for-rc (stable) to fix this issue > > > in stable releases? > > > > If you want to fix something in stable then block the module options > > from working as actual module options - eg before rxe_module_init() > > runs. > > > > Jason > > Something like the following patch? If you want to got this path, it will be more like this: request_module("rdma_rxe") Thanks > > diff --git a/drivers/infiniband/sw/rxe/rxe.c b/drivers/infiniband/sw/rxe/rxe.c > index 907203afbd99..872ebc57ac06 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 = false; > + > /* 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"); > > Thanks, > Kamal
On Sun, Aug 23, 2020 at 10:45:58PM +0300, Kamal Heib wrote: > On Thu, Aug 20, 2020 at 08:37:17AM -0300, Jason Gunthorpe wrote: > > On Wed, Aug 19, 2020 at 12:15:45AM +0300, Kamal Heib wrote: > > > On Tue, Aug 18, 2020 at 01:31:57PM -0300, Jason Gunthorpe wrote: > > > > On Tue, Aug 18, 2020 at 05:25:04PM +0300, Kamal Heib wrote: > > > > > To avoid the following kernel panic when calling kmem_cache_create() > > > > > with a NULL pointer from pool_cache(), move the rxe_cache_init() to the > > > > > context of device initialization. > > > > > > > > I think you've hit on a bigger bug than just this oops. > > > > > > > > rxe_net_add() should never be called before rxe_module_init(), that > > > > surely subtly breaks all kinds of things. > > > > > > > > Maybe it is time to remove these module parameters? > > > > > > > Yes, I agree, this can be done in for-next. > > > > > > But at least can we take this patch to for-rc (stable) to fix this issue > > > in stable releases? > > > > If you want to fix something in stable then block the module options > > from working as actual module options - eg before rxe_module_init() > > runs. > > > > Jason > > Something like the following patch? Yes, something more like that Jason
On Mon, Aug 24, 2020 at 10:47:23AM -0300, Jason Gunthorpe wrote: > On Sun, Aug 23, 2020 at 10:45:58PM +0300, Kamal Heib wrote: > > On Thu, Aug 20, 2020 at 08:37:17AM -0300, Jason Gunthorpe wrote: > > > On Wed, Aug 19, 2020 at 12:15:45AM +0300, Kamal Heib wrote: > > > > On Tue, Aug 18, 2020 at 01:31:57PM -0300, Jason Gunthorpe wrote: > > > > > On Tue, Aug 18, 2020 at 05:25:04PM +0300, Kamal Heib wrote: > > > > > > To avoid the following kernel panic when calling kmem_cache_create() > > > > > > with a NULL pointer from pool_cache(), move the rxe_cache_init() to the > > > > > > context of device initialization. > > > > > > > > > > I think you've hit on a bigger bug than just this oops. > > > > > > > > > > rxe_net_add() should never be called before rxe_module_init(), that > > > > > surely subtly breaks all kinds of things. > > > > > > > > > > Maybe it is time to remove these module parameters? > > > > > > > > > Yes, I agree, this can be done in for-next. > > > > > > > > But at least can we take this patch to for-rc (stable) to fix this issue > > > > in stable releases? > > > > > > If you want to fix something in stable then block the module options > > > from working as actual module options - eg before rxe_module_init() > > > runs. > > > > > > Jason > > > > Something like the following patch? > > Yes, something more like that > > Jason OK, Thanks! I'll send v3 soon. Thanks, Kamal
diff --git a/drivers/infiniband/sw/rxe/rxe.c b/drivers/infiniband/sw/rxe/rxe.c index 5642eefb4ba1..c80f7c183957 100644 --- a/drivers/infiniband/sw/rxe/rxe.c +++ b/drivers/infiniband/sw/rxe/rxe.c @@ -252,6 +252,13 @@ static int rxe_init(struct rxe_dev *rxe) /* init default device parameters */ rxe_init_device_param(rxe); + /* initialize slab caches for managed objects */ + err = rxe_cache_init(); + if (err) { + pr_err("unable to init object pools\n"); + return err; + } + err = rxe_init_ports(rxe); if (err) goto err1; @@ -336,13 +343,6 @@ static int __init rxe_module_init(void) { int err; - /* initialize slab caches for managed objects */ - err = rxe_cache_init(); - if (err) { - pr_err("unable to init object pools\n"); - return err; - } - err = rxe_net_init(); if (err) return err; diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c index fbcbac52290b..06c6d1f835b7 100644 --- a/drivers/infiniband/sw/rxe/rxe_pool.c +++ b/drivers/infiniband/sw/rxe/rxe_pool.c @@ -139,6 +139,9 @@ int rxe_cache_init(void) for (i = 0; i < RXE_NUM_TYPES; i++) { type = &rxe_type_info[i]; size = ALIGN(type->size, RXE_POOL_ALIGN); + if (type->cache) + continue; + if (!(type->flags & RXE_POOL_NO_ALLOC)) { type->cache = kmem_cache_create(type->name, size,
To avoid the following kernel panic when calling kmem_cache_create() with a NULL pointer from pool_cache(), move the rxe_cache_init() to the context of device initialization. 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> --- v2: Call rxe_cache_init() from rxe_init(). --- drivers/infiniband/sw/rxe/rxe.c | 14 +++++++------- drivers/infiniband/sw/rxe/rxe_pool.c | 3 +++ 2 files changed, 10 insertions(+), 7 deletions(-)