| Message ID | 20190522072340.9042-1-kamalheib1@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [for-next] RDMA/core: Avoid panic when port_data isn't initialized | expand |
On Wed, May 22, 2019 at 10:23:40AM +0300, Kamal Heib wrote: > A panic could occur when calling ib_device_release() and port_data > isn't initialized, To avoid that a check was added to verify that > port_data isn't NULL. This is a terrible commit message, describe the case that causes this. The check should be in ib_device_release(), not in the functions. Jason
On Wed, 2019-05-22 at 09:15 -0300, Jason Gunthorpe wrote: > On Wed, May 22, 2019 at 10:23:40AM +0300, Kamal Heib wrote: > > A panic could occur when calling ib_device_release() and port_data > > isn't initialized, To avoid that a check was added to verify that > > port_data isn't NULL. > > This is a terrible commit message, describe the case that causes > this. > This happen if assign_name() return failure when called from ib_register_device() - The following panic will happen and in every function that touches the port_data's data members. [ 33.668236] BUG: unable to handle kernel NULL pointer dereference at 00000000000000c0 [ 33.668238] #PF error: [WRITE] [ 33.668239] PGD 0 P4D 0 [ 33.668241] Oops: 0002 [#1] SMP PTI [ 33.668244] CPU: 19 PID: 1994 Comm: systemd-udevd Not tainted 5.1.0- rc5+ #1 [ 33.668245] Hardware name: HP ProLiant DL360p Gen8, BIOS P71 12/20/2013 [ 33.668252] RIP: 0010:_raw_spin_lock_irqsave+0x1e/0x40 [ 33.668254] Code: 85 ff 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90 53 9c 58 66 66 90 66 90 48 89 c3 fa 66 66 90 66 66 90 31 c0 ba 01 00 00 00 <f0> 0f b1 17 0f 94 c2 84 d2 74 05 48 89 d8 5b c3 89 c6 e8 b4 85 8a [ 33.668254] RSP: 0018:ffffa8d7079a7c08 EFLAGS: 00010046 [ 33.668256] RAX: 0000000000000000 RBX: 0000000000000202 RCX: ffffa8d7079a7bf8 [ 33.668256] RDX: 0000000000000001 RSI: ffff93607c990000 RDI: 00000000000000c0 [ 33.668257] RBP: 0000000000000001 R08: 0000000000000000 R09: ffffffffc08c4dd8 [ 33.668258] R10: 0000000000000000 R11: 0000000000000001 R12: 00000000000000c0 [ 33.668259] R13: ffff93607c990000 R14: ffffffffc05a9740 R15: ffffa8d7079a7e98 [ 33.668260] FS: 00007f1c6ee438c0(0000) GS:ffff93609f6c0000(0000) knlGS:0000000000000000 [ 33.668261] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 33.668261] CR2: 00000000000000c0 CR3: 0000000819fca002 CR4: 00000000000606e0 [ 33.668262] Call Trace: [ 33.668280] free_netdevs+0x4d/0xe0 [ib_core] [ 33.668289] ib_dealloc_device+0x51/0xb0 [ib_core] [ 33.668300] __mlx5_ib_add+0x5e/0x70 [mlx5_ib] [ 33.668321] mlx5_add_device+0x57/0xe0 [mlx5_core] [ 33.668335] mlx5_register_interface+0x85/0xc0 [mlx5_core] [ 33.668337] ? 0xffffffffc0474000 [ 33.668340] do_one_initcall+0x4e/0x1d4 [ 33.668342] ? _cond_resched+0x15/0x30 [ 33.668345] ? kmem_cache_alloc_trace+0x15f/0x1c0 [ 33.668348] do_init_module+0x5a/0x218 [ 33.668350] load_module+0x186b/0x1e40 [ 33.668351] ? m_show+0x1c0/0x1c0 [ 33.668354] __do_sys_finit_module+0x94/0xe0 [ 33.668356] do_syscall_64+0x5b/0x180 [ 33.668358] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 33.668360] RIP: 0033:0x7f1c6daa0be9 [ 33.668361] Code: 01 00 48 81 c4 80 00 00 00 e9 f1 fe ff ff 0f 1f 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 57 e2 2c 00 f7 d8 64 89 01 48 [ 33.668362] RSP: 002b:00007ffd2d71a3e8 EFLAGS: 00000246 ORIG_RAX: 0000000000000139 [ 33.668363] RAX: ffffffffffffffda RBX: 00005576c3936f60 RCX: 00007f1c6daa0be9 [ 33.668364] RDX: 0000000000000000 RSI: 00007f1c6e3c2039 RDI: 000000000000000e [ 33.668364] RBP: 00007f1c6e3c2039 R08: 0000000000000000 R09: 00005576c39374c0 [ 33.668365] R10: 000000000000000e R11: 0000000000000246 R12: 0000000000000000 [ 33.668365] R13: 00005576c39fd9a0 R14: 0000000000020000 R15: 0000000000000000 [ 33.668367] Modules linked in: mlx5_ib(+) kvm irqbypass ib_uverbs ipmi_ssif crct10dif_pclmul ib_core crc32_pclmul ghash_clmulni_intel ipmi_si aesni_intel iTCO_wdt iTCO_vendor_support crypto_simd cryptd glue_helper hpilo sg ipmi_devintf pcspkr ioatdma ipmi_msghandler hpwdt acpi_power_meter pcc_cpufreq dca lpc_ich ip_tables xfs libcrc32c mgag200 ata_generic sd_mod pata_acpi i2c_algo_bit drm_kms_helper mlx5_core syscopyarea sysfillrect sysimgblt fb_sys_fops ttm tg3 mlxfw drm ata_piix hpsa ptp libata crc32c_intel serio_raw pps_core scsi_transport_sas dm_mirror dm_region_hash dm_log dm_mod [ 33.668387] CR2: 00000000000000c0 [ 33.668414] ---[ end trace e1a60554f6535c71 ]--- [ 33.675166] XFS (sda1): Mounting V5 Filesystem [ 33.676498] RIP: 0010:_raw_spin_lock_irqsave+0x1e/0x40 [ 33.676500] Code: 85 ff 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90 53 9c 58 66 66 90 66 90 48 89 c3 fa 66 66 90 66 66 90 31 c0 ba 01 00 00 00 <f0> 0f b1 17 0f 94 c2 84 d2 74 05 48 89 d8 5b c3 89 c6 e8 b4 85 8a [ 33.676500] RSP: 0018:ffffa8d7079a7c08 EFLAGS: 00010046 [ 33.676501] RAX: 0000000000000000 RBX: 0000000000000202 RCX: ffffa8d7079a7bf8 [ 33.676502] RDX: 0000000000000001 RSI: ffff93607c990000 RDI: 00000000000000c0 [ 33.676503] RBP: 0000000000000001 R08: 0000000000000000 R09: ffffffffc08c4dd8 [ 33.676503] R10: 0000000000000000 R11: 0000000000000001 R12: 00000000000000c0 [ 33.676504] R13: ffff93607c990000 R14: ffffffffc05a9740 R15: ffffa8d7079a7e98 [ 33.676505] FS: 00007f1c6ee438c0(0000) GS:ffff93609f6c0000(0000) knlGS:0000000000000000 [ 33.676506] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 33.676507] CR2: 00000000000000c0 CR3: 0000000819fca002 CR4: 00000000000606e0 [ 33.676508] Kernel panic - not syncing: Fatal exception [ 33.706833] Kernel Offset: 0x2b200000 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffffbfffffff) > The check should be in ib_device_release(), not in the functions. > > Jason Why? static void ib_device_release(struct device *device) { struct ib_device *dev = container_of(device, struct ib_device, dev); free_netdevs(dev); WARN_ON(refcount_read(&dev->refcount)); ib_cache_release_one(dev); ib_security_release_port_pkey_list(dev); xa_destroy(&dev->compat_devs); xa_destroy(&dev->client_data); if (dev->port_data) kfree_rcu(container_of(dev->port_data, struct ib_port_data_rcu, pdata[0]), rcu_head); kfree_rcu(dev, rcu_head); } Thanks, Kamal
On Wed, May 22, 2019 at 03:42:47PM +0300, Kamal Heib wrote: > On Wed, 2019-05-22 at 09:15 -0300, Jason Gunthorpe wrote: > > On Wed, May 22, 2019 at 10:23:40AM +0300, Kamal Heib wrote: > > > A panic could occur when calling ib_device_release() and port_data > > > isn't initialized, To avoid that a check was added to verify that > > > port_data isn't NULL. > > > > This is a terrible commit message, describe the case that causes > > this. > > > > This happen if assign_name() return failure when called from > ib_register_device() - The following panic will happen and in every > function that touches the port_data's data members. Then this should be the commit message, with the oops. > > The check should be in ib_device_release(), not in the functions. > > > > Jason > > Why? Because if the device has not progressed to be setup enough we shouldn't be freeing things that aren't started yet. However, it is a bit weird because the cache_release was intended to not have dependencies - but the cache also can't be started until the port_data is setup The real problem is that we can't allocate the port data during ib_alloc_device.. Jason
diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c index 18e476b3ced0..87801e3848a8 100644 --- a/drivers/infiniband/core/cache.c +++ b/drivers/infiniband/core/cache.c @@ -1520,6 +1520,9 @@ void ib_cache_release_one(struct ib_device *device) { unsigned int p; + if (!device->port_data) + return; + /* * The release function frees all the cache elements. * This function should be called as part of freeing diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c index 78dc07c6ac4b..3a497e57f673 100644 --- a/drivers/infiniband/core/device.c +++ b/drivers/infiniband/core/device.c @@ -1935,6 +1935,9 @@ static void free_netdevs(struct ib_device *ib_dev) unsigned long flags; unsigned int port; + if (!ib_dev->port_data) + return; + rdma_for_each_port (ib_dev, port) { struct ib_port_data *pdata = &ib_dev->port_data[port]; struct net_device *ndev; diff --git a/drivers/infiniband/core/security.c b/drivers/infiniband/core/security.c index 1ab423b19f77..fba7bd5d4a2e 100644 --- a/drivers/infiniband/core/security.c +++ b/drivers/infiniband/core/security.c @@ -564,6 +564,9 @@ void ib_security_release_port_pkey_list(struct ib_device *device) struct pkey_index_qp_list *pkey, *tmp_pkey; unsigned int i; + if (!device->port_data) + return; + rdma_for_each_port (device, i) { list_for_each_entry_safe(pkey, tmp_pkey,
A panic could occur when calling ib_device_release() and port_data isn't initialized, To avoid that a check was added to verify that port_data isn't NULL. Signed-off-by: Kamal Heib <kamalheib1@gmail.com> --- drivers/infiniband/core/cache.c | 3 +++ drivers/infiniband/core/device.c | 3 +++ drivers/infiniband/core/security.c | 3 +++ 3 files changed, 9 insertions(+)