* [PATCH rdma-rc] IB/mlx5: Fix use-after-free error while accessing ev_file pointer
@ 2019-08-08 8:15 Leon Romanovsky
2019-08-08 12:26 ` Jason Gunthorpe
0 siblings, 1 reply; 7+ messages in thread
From: Leon Romanovsky @ 2019-08-08 8:15 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Yishai Hadas, RDMA mailing list, Leon Romanovsky
From: Yishai Hadas <yishaih@mellanox.com>
Call to uverbs_close_fd() releases file pointer to 'ev_file' and
mlx5_ib_dev is going to be inaccessible. Cache pointer prior cleaning
resources to solve the KASAN warning below.
BUG: KASAN: use-after-free in devx_async_event_close+0x391/0x480 [mlx5_ib]
Read of size 8 at addr ffff888301e3cec0 by task devx_direct_tes/4631
CPU: 1 PID: 4631 Comm: devx_direct_tes Tainted: G OE 5.3.0-rc1-for-upstream-dbg-2019-07-26_01-19-56-93 #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu2 04/01/2014
Call Trace:
dump_stack+0x9a/0xeb
print_address_description+0x1e2/0x400
? devx_async_event_close+0x391/0x480 [mlx5_ib]
__kasan_report+0x15c/0x1df
? devx_async_event_close+0x391/0x480 [mlx5_ib]
kasan_report+0xe/0x20
devx_async_event_close+0x391/0x480 [mlx5_ib]
__fput+0x26a/0x7b0
task_work_run+0x10d/0x180
exit_to_usermode_loop+0x137/0x160
do_syscall_64+0x3c7/0x490
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x7f5df907d664
Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f
80 00 00 00 00 8b 05 6a cd 20 00 48 63 ff 85 c0 75 13 b8
03 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 44 f3 c3 66 90
48 83 ec 18 48 89 7c 24 08 e8
RSP: 002b:00007ffd353cb958 EFLAGS: 00000246 ORIG_RAX: 0000000000000003
RAX: 0000000000000000 RBX: 000056017a88c348 RCX: 00007f5df907d664
RDX: 00007f5df969d400 RSI: 00007f5de8f1ec90 RDI: 0000000000000006
RBP: 00007f5df9681dc0 R08: 00007f5de8736410 R09: 000056017a9d2dd0
R10: 000000000000000b R11: 0000000000000246 R12: 00007f5de899d7d0
R13: 00007f5df96c4248 R14: 00007f5de8f1ecb0 R15: 000056017ae41308
Allocated by task 4631:
save_stack+0x19/0x80
kasan_kmalloc.constprop.3+0xa0/0xd0
alloc_uobj+0x71/0x230 [ib_uverbs]
alloc_begin_fd_uobject+0x2e/0xc0 [ib_uverbs]
rdma_alloc_begin_uobject+0x96/0x140 [ib_uverbs]
ib_uverbs_run_method+0xdf0/0x1940 [ib_uverbs]
ib_uverbs_cmd_verbs+0x57e/0xdb0 [ib_uverbs]
ib_uverbs_ioctl+0x177/0x260 [ib_uverbs]
do_vfs_ioctl+0x18f/0x1010
ksys_ioctl+0x70/0x80
__x64_sys_ioctl+0x6f/0xb0
do_syscall_64+0x95/0x490
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Freed by task 4631:
save_stack+0x19/0x80
__kasan_slab_free+0x11d/0x160
slab_free_freelist_hook+0x67/0x1a0
kfree+0xb9/0x2a0
uverbs_close_fd+0x118/0x1c0 [ib_uverbs]
devx_async_event_close+0x28a/0x480 [mlx5_ib]
__fput+0x26a/0x7b0
task_work_run+0x10d/0x180
exit_to_usermode_loop+0x137/0x160
do_syscall_64+0x3c7/0x490
entry_SYSCALL_64_after_hwframe+0x49/0xbe
The buggy address belongs to the object at ffff888301e3cda8
which belongs to the cache kmalloc-512 of size 512
The buggy address is located 280 bytes inside of 512-byte region
[ffff888301e3cda8, ffff888301e3cfa8)
The buggy address belongs to the page:
page:ffffea000c078e00 refcount:1 mapcount:0
mapping:ffff888352811300 index:0x0 compound_mapcount: 0
flags: 0x2fffff80010200(slab|head)
raw: 002fffff80010200 ffffea000d152608 ffffea000c077808 ffff888352811300
raw: 0000000000000000 0000000000250025 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff888301e3cd80: fc fc fc fc fc fb fb fb fb fb fb fb fb fb fb fb
ffff888301e3ce00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff888301e3ce80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff888301e3cf00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff888301e3cf80: fb fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc
Disabling lock debugging due to kernel taint
Cc: <stable@vger.kernel.org> # 5.2
Fixes: 759738537142 ("IB/mlx5: Enable subscription for device events over DEVX")
Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
drivers/infiniband/hw/mlx5/devx.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/infiniband/hw/mlx5/devx.c b/drivers/infiniband/hw/mlx5/devx.c
index 2d1b3d9609d9..af5bbb35c058 100644
--- a/drivers/infiniband/hw/mlx5/devx.c
+++ b/drivers/infiniband/hw/mlx5/devx.c
@@ -2644,12 +2644,13 @@ static int devx_async_event_close(struct inode *inode, struct file *filp)
struct devx_async_event_file *ev_file = filp->private_data;
struct devx_event_subscription *event_sub, *event_sub_tmp;
struct devx_async_event_data *entry, *tmp;
+ struct mlx5_ib_dev *dev = ev_file->dev;
- mutex_lock(&ev_file->dev->devx_event_table.event_xa_lock);
+ mutex_lock(&dev->devx_event_table.event_xa_lock);
/* delete the subscriptions which are related to this FD */
list_for_each_entry_safe(event_sub, event_sub_tmp,
&ev_file->subscribed_events_list, file_list) {
- devx_cleanup_subscription(ev_file->dev, event_sub);
+ devx_cleanup_subscription(dev, event_sub);
if (event_sub->eventfd)
eventfd_ctx_put(event_sub->eventfd);
@@ -2658,7 +2659,7 @@ static int devx_async_event_close(struct inode *inode, struct file *filp)
kfree_rcu(event_sub, rcu);
}
- mutex_unlock(&ev_file->dev->devx_event_table.event_xa_lock);
+ mutex_unlock(&dev->devx_event_table.event_xa_lock);
/* free the pending events allocation */
if (!ev_file->omit_data) {
@@ -2670,7 +2671,7 @@ static int devx_async_event_close(struct inode *inode, struct file *filp)
}
uverbs_close_fd(filp);
- put_device(&ev_file->dev->ib_dev.dev);
+ put_device(&dev->ib_dev.dev);
return 0;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH rdma-rc] IB/mlx5: Fix use-after-free error while accessing ev_file pointer
2019-08-08 8:15 [PATCH rdma-rc] IB/mlx5: Fix use-after-free error while accessing ev_file pointer Leon Romanovsky
@ 2019-08-08 12:26 ` Jason Gunthorpe
2019-08-08 15:32 ` Yishai Hadas
0 siblings, 1 reply; 7+ messages in thread
From: Jason Gunthorpe @ 2019-08-08 12:26 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Yishai Hadas, RDMA mailing list, Leon Romanovsky
On Thu, Aug 08, 2019 at 11:15:38AM +0300, Leon Romanovsky wrote:
> From: Yishai Hadas <yishaih@mellanox.com>
>
> Call to uverbs_close_fd() releases file pointer to 'ev_file' and
> mlx5_ib_dev is going to be inaccessible. Cache pointer prior cleaning
> resources to solve the KASAN warning below.
>
> BUG: KASAN: use-after-free in devx_async_event_close+0x391/0x480 [mlx5_ib]
> Read of size 8 at addr ffff888301e3cec0 by task devx_direct_tes/4631
> CPU: 1 PID: 4631 Comm: devx_direct_tes Tainted: G OE 5.3.0-rc1-for-upstream-dbg-2019-07-26_01-19-56-93 #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu2 04/01/2014
> Call Trace:
> dump_stack+0x9a/0xeb
> print_address_description+0x1e2/0x400
> ? devx_async_event_close+0x391/0x480 [mlx5_ib]
> __kasan_report+0x15c/0x1df
> ? devx_async_event_close+0x391/0x480 [mlx5_ib]
> kasan_report+0xe/0x20
> devx_async_event_close+0x391/0x480 [mlx5_ib]
> __fput+0x26a/0x7b0
> task_work_run+0x10d/0x180
> exit_to_usermode_loop+0x137/0x160
> do_syscall_64+0x3c7/0x490
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x7f5df907d664
> Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f
> 80 00 00 00 00 8b 05 6a cd 20 00 48 63 ff 85 c0 75 13 b8
> 03 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 44 f3 c3 66 90
> 48 83 ec 18 48 89 7c 24 08 e8
> RSP: 002b:00007ffd353cb958 EFLAGS: 00000246 ORIG_RAX: 0000000000000003
> RAX: 0000000000000000 RBX: 000056017a88c348 RCX: 00007f5df907d664
> RDX: 00007f5df969d400 RSI: 00007f5de8f1ec90 RDI: 0000000000000006
> RBP: 00007f5df9681dc0 R08: 00007f5de8736410 R09: 000056017a9d2dd0
> R10: 000000000000000b R11: 0000000000000246 R12: 00007f5de899d7d0
> R13: 00007f5df96c4248 R14: 00007f5de8f1ecb0 R15: 000056017ae41308
>
> Allocated by task 4631:
> save_stack+0x19/0x80
> kasan_kmalloc.constprop.3+0xa0/0xd0
> alloc_uobj+0x71/0x230 [ib_uverbs]
> alloc_begin_fd_uobject+0x2e/0xc0 [ib_uverbs]
> rdma_alloc_begin_uobject+0x96/0x140 [ib_uverbs]
> ib_uverbs_run_method+0xdf0/0x1940 [ib_uverbs]
> ib_uverbs_cmd_verbs+0x57e/0xdb0 [ib_uverbs]
> ib_uverbs_ioctl+0x177/0x260 [ib_uverbs]
> do_vfs_ioctl+0x18f/0x1010
> ksys_ioctl+0x70/0x80
> __x64_sys_ioctl+0x6f/0xb0
> do_syscall_64+0x95/0x490
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> Freed by task 4631:
> save_stack+0x19/0x80
> __kasan_slab_free+0x11d/0x160
> slab_free_freelist_hook+0x67/0x1a0
> kfree+0xb9/0x2a0
> uverbs_close_fd+0x118/0x1c0 [ib_uverbs]
> devx_async_event_close+0x28a/0x480 [mlx5_ib]
> __fput+0x26a/0x7b0
> task_work_run+0x10d/0x180
> exit_to_usermode_loop+0x137/0x160
> do_syscall_64+0x3c7/0x490
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> The buggy address belongs to the object at ffff888301e3cda8
> which belongs to the cache kmalloc-512 of size 512
> The buggy address is located 280 bytes inside of 512-byte region
> [ffff888301e3cda8, ffff888301e3cfa8)
> The buggy address belongs to the page:
> page:ffffea000c078e00 refcount:1 mapcount:0
> mapping:ffff888352811300 index:0x0 compound_mapcount: 0
> flags: 0x2fffff80010200(slab|head)
> raw: 002fffff80010200 ffffea000d152608 ffffea000c077808 ffff888352811300
> raw: 0000000000000000 0000000000250025 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
> Memory state around the buggy address:
> ffff888301e3cd80: fc fc fc fc fc fb fb fb fb fb fb fb fb fb fb fb
> ffff888301e3ce00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff888301e3ce80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff888301e3cf00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff888301e3cf80: fb fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc
> Disabling lock debugging due to kernel taint
>
> Cc: <stable@vger.kernel.org> # 5.2
> Fixes: 759738537142 ("IB/mlx5: Enable subscription for device events over DEVX")
> Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> drivers/infiniband/hw/mlx5/devx.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mlx5/devx.c b/drivers/infiniband/hw/mlx5/devx.c
> index 2d1b3d9609d9..af5bbb35c058 100644
> +++ b/drivers/infiniband/hw/mlx5/devx.c
> @@ -2644,12 +2644,13 @@ static int devx_async_event_close(struct inode *inode, struct file *filp)
> struct devx_async_event_file *ev_file = filp->private_data;
This line is wrong, it should be
struct devx_async_event_file *ev_file = container_of(struct
devx_async_event_file, filp->private_data, uobj);
It should get fixed in a followup, along with any other places like
it.
It is also a bit redundant to store the mlx5_ib_dev in the
devx_async_event_file as uobj->ucontext->dev is the same pointer.
Otherwise this patch is Ok
Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH rdma-rc] IB/mlx5: Fix use-after-free error while accessing ev_file pointer
2019-08-08 12:26 ` Jason Gunthorpe
@ 2019-08-08 15:32 ` Yishai Hadas
2019-08-08 15:34 ` Jason Gunthorpe
0 siblings, 1 reply; 7+ messages in thread
From: Yishai Hadas @ 2019-08-08 15:32 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Leon Romanovsky, Doug Ledford, Yishai Hadas, RDMA mailing list,
Leon Romanovsky
On 8/8/2019 3:26 PM, Jason Gunthorpe wrote:
> On Thu, Aug 08, 2019 at 11:15:38AM +0300, Leon Romanovsky wrote:
>> From: Yishai Hadas <yishaih@mellanox.com>
>>
>> Call to uverbs_close_fd() releases file pointer to 'ev_file' and
>> mlx5_ib_dev is going to be inaccessible. Cache pointer prior cleaning
>> resources to solve the KASAN warning below.
>>
>> BUG: KASAN: use-after-free in devx_async_event_close+0x391/0x480 [mlx5_ib]
>> Read of size 8 at addr ffff888301e3cec0 by task devx_direct_tes/4631
>> CPU: 1 PID: 4631 Comm: devx_direct_tes Tainted: G OE 5.3.0-rc1-for-upstream-dbg-2019-07-26_01-19-56-93 #1
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu2 04/01/2014
>> Call Trace:
>> dump_stack+0x9a/0xeb
>> print_address_description+0x1e2/0x400
>> ? devx_async_event_close+0x391/0x480 [mlx5_ib]
>> __kasan_report+0x15c/0x1df
>> ? devx_async_event_close+0x391/0x480 [mlx5_ib]
>> kasan_report+0xe/0x20
>> devx_async_event_close+0x391/0x480 [mlx5_ib]
>> __fput+0x26a/0x7b0
>> task_work_run+0x10d/0x180
>> exit_to_usermode_loop+0x137/0x160
>> do_syscall_64+0x3c7/0x490
>> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>> RIP: 0033:0x7f5df907d664
>> Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f
>> 80 00 00 00 00 8b 05 6a cd 20 00 48 63 ff 85 c0 75 13 b8
>> 03 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 44 f3 c3 66 90
>> 48 83 ec 18 48 89 7c 24 08 e8
>> RSP: 002b:00007ffd353cb958 EFLAGS: 00000246 ORIG_RAX: 0000000000000003
>> RAX: 0000000000000000 RBX: 000056017a88c348 RCX: 00007f5df907d664
>> RDX: 00007f5df969d400 RSI: 00007f5de8f1ec90 RDI: 0000000000000006
>> RBP: 00007f5df9681dc0 R08: 00007f5de8736410 R09: 000056017a9d2dd0
>> R10: 000000000000000b R11: 0000000000000246 R12: 00007f5de899d7d0
>> R13: 00007f5df96c4248 R14: 00007f5de8f1ecb0 R15: 000056017ae41308
>>
>> Allocated by task 4631:
>> save_stack+0x19/0x80
>> kasan_kmalloc.constprop.3+0xa0/0xd0
>> alloc_uobj+0x71/0x230 [ib_uverbs]
>> alloc_begin_fd_uobject+0x2e/0xc0 [ib_uverbs]
>> rdma_alloc_begin_uobject+0x96/0x140 [ib_uverbs]
>> ib_uverbs_run_method+0xdf0/0x1940 [ib_uverbs]
>> ib_uverbs_cmd_verbs+0x57e/0xdb0 [ib_uverbs]
>> ib_uverbs_ioctl+0x177/0x260 [ib_uverbs]
>> do_vfs_ioctl+0x18f/0x1010
>> ksys_ioctl+0x70/0x80
>> __x64_sys_ioctl+0x6f/0xb0
>> do_syscall_64+0x95/0x490
>> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>
>> Freed by task 4631:
>> save_stack+0x19/0x80
>> __kasan_slab_free+0x11d/0x160
>> slab_free_freelist_hook+0x67/0x1a0
>> kfree+0xb9/0x2a0
>> uverbs_close_fd+0x118/0x1c0 [ib_uverbs]
>> devx_async_event_close+0x28a/0x480 [mlx5_ib]
>> __fput+0x26a/0x7b0
>> task_work_run+0x10d/0x180
>> exit_to_usermode_loop+0x137/0x160
>> do_syscall_64+0x3c7/0x490
>> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>
>> The buggy address belongs to the object at ffff888301e3cda8
>> which belongs to the cache kmalloc-512 of size 512
>> The buggy address is located 280 bytes inside of 512-byte region
>> [ffff888301e3cda8, ffff888301e3cfa8)
>> The buggy address belongs to the page:
>> page:ffffea000c078e00 refcount:1 mapcount:0
>> mapping:ffff888352811300 index:0x0 compound_mapcount: 0
>> flags: 0x2fffff80010200(slab|head)
>> raw: 002fffff80010200 ffffea000d152608 ffffea000c077808 ffff888352811300
>> raw: 0000000000000000 0000000000250025 00000001ffffffff 0000000000000000
>> page dumped because: kasan: bad access detected
>> Memory state around the buggy address:
>> ffff888301e3cd80: fc fc fc fc fc fb fb fb fb fb fb fb fb fb fb fb
>> ffff888301e3ce00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>> ffff888301e3ce80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>> ffff888301e3cf00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>> ffff888301e3cf80: fb fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc
>> Disabling lock debugging due to kernel taint
>>
>> Cc: <stable@vger.kernel.org> # 5.2
>> Fixes: 759738537142 ("IB/mlx5: Enable subscription for device events over DEVX")
>> Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
>> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
>> drivers/infiniband/hw/mlx5/devx.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/infiniband/hw/mlx5/devx.c b/drivers/infiniband/hw/mlx5/devx.c
>> index 2d1b3d9609d9..af5bbb35c058 100644
>> +++ b/drivers/infiniband/hw/mlx5/devx.c
>> @@ -2644,12 +2644,13 @@ static int devx_async_event_close(struct inode *inode, struct file *filp)
>> struct devx_async_event_file *ev_file = filp->private_data;
>
> This line is wrong, it should be
>
> struct devx_async_event_file *ev_file = container_of(struct
> devx_async_event_file, filp->private_data, uobj);
You suggested the below 2 lines instead of the above one line, correct ?
as struct devx_async_event_file wraps uobj as its first field this is
logically equal, agree ?
struct ib_uobject *uobj = filp->private_data;
struct devx_async_event_file *ev_file = container_of(struct
devx_async_event_file, filp->private_data, uobj);
>
> It should get fixed in a followup, along with any other places like
> it.
>
This can be done if you prefer the extra line pointed above.
> It is also a bit redundant to store the mlx5_ib_dev in the
> devx_async_event_file as uobj->ucontext->dev is the same pointer.
>
Post hot unplug uobj->ucontext might not be accessible, isn't it ?
Current code should be fine for that.
> Otherwise this patch is Ok
>
> Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
>
Thanks.
Are we fine to take this patch ?
Yishai
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH rdma-rc] IB/mlx5: Fix use-after-free error while accessing ev_file pointer
2019-08-08 15:32 ` Yishai Hadas
@ 2019-08-08 15:34 ` Jason Gunthorpe
2019-08-12 14:59 ` Doug Ledford
0 siblings, 1 reply; 7+ messages in thread
From: Jason Gunthorpe @ 2019-08-08 15:34 UTC (permalink / raw)
To: Yishai Hadas
Cc: Leon Romanovsky, Doug Ledford, Yishai Hadas, RDMA mailing list,
Leon Romanovsky
On Thu, Aug 08, 2019 at 06:32:00PM +0300, Yishai Hadas wrote:
> > > diff --git a/drivers/infiniband/hw/mlx5/devx.c b/drivers/infiniband/hw/mlx5/devx.c
> > > index 2d1b3d9609d9..af5bbb35c058 100644
> > > +++ b/drivers/infiniband/hw/mlx5/devx.c
> > > @@ -2644,12 +2644,13 @@ static int devx_async_event_close(struct inode *inode, struct file *filp)
> > > struct devx_async_event_file *ev_file = filp->private_data;
> >
> > This line is wrong, it should be
> >
> > struct devx_async_event_file *ev_file = container_of(struct
> > devx_async_event_file, filp->private_data, uobj);
>
> You suggested the below 2 lines instead of the above one line, correct ? as
> struct devx_async_event_file wraps uobj as its first field this is logically
> equal, agree ?
Yes, it is wrong only in the use of the type system, the private_data
void * should only ever be cast to a ib_uobject.
> struct ib_uobject *uobj = filp->private_data;
This could be done with a cast
> > It is also a bit redundant to store the mlx5_ib_dev in the
> > devx_async_event_file as uobj->ucontext->dev is the same pointer.
> >
> Post hot unplug uobj->ucontext might not be accessible, isn't it ?
> Current code should be fine for that.
That is the other kind of weird thing, all this destruction could be
done on unplug..
> Are we fine to take this patch ?
Yes
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH rdma-rc] IB/mlx5: Fix use-after-free error while accessing ev_file pointer
2019-08-08 15:34 ` Jason Gunthorpe
@ 2019-08-12 14:59 ` Doug Ledford
2019-08-13 10:10 ` Leon Romanovsky
0 siblings, 1 reply; 7+ messages in thread
From: Doug Ledford @ 2019-08-12 14:59 UTC (permalink / raw)
To: Jason Gunthorpe, Yishai Hadas
Cc: Leon Romanovsky, Yishai Hadas, RDMA mailing list, Leon Romanovsky
[-- Attachment #1: Type: text/plain, Size: 1755 bytes --]
On Thu, 2019-08-08 at 15:34 +0000, Jason Gunthorpe wrote:
> On Thu, Aug 08, 2019 at 06:32:00PM +0300, Yishai Hadas wrote:
> > > > diff --git a/drivers/infiniband/hw/mlx5/devx.c
> > > > b/drivers/infiniband/hw/mlx5/devx.c
> > > > index 2d1b3d9609d9..af5bbb35c058 100644
> > > > +++ b/drivers/infiniband/hw/mlx5/devx.c
> > > > @@ -2644,12 +2644,13 @@ static int devx_async_event_close(struct
> > > > inode *inode, struct file *filp)
> > > > struct devx_async_event_file *ev_file = filp-
> > > > >private_data;
> > >
> > > This line is wrong, it should be
> > >
> > > struct devx_async_event_file *ev_file =
> > > container_of(struct
> > > devx_async_event_file, filp-
> > > >private_data, uobj);
> >
> > You suggested the below 2 lines instead of the above one line,
> > correct ? as
> > struct devx_async_event_file wraps uobj as its first field this is
> > logically
> > equal, agree ?
>
> Yes, it is wrong only in the use of the type system, the private_data
> void * should only ever be cast to a ib_uobject.
>
> > struct ib_uobject *uobj = filp->private_data;
>
> This could be done with a cast
>
> > > It is also a bit redundant to store the mlx5_ib_dev in the
> > > devx_async_event_file as uobj->ucontext->dev is the same pointer.
> > >
> > Post hot unplug uobj->ucontext might not be accessible, isn't it ?
> > Current code should be fine for that.
>
> That is the other kind of weird thing, all this destruction could be
> done on unplug..
>
> > Are we fine to take this patch ?
>
> Yes
Thanks, applied to for-rc.
--
Doug Ledford <dledford@redhat.com>
GPG KeyID: B826A3330E572FDD
Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH rdma-rc] IB/mlx5: Fix use-after-free error while accessing ev_file pointer
2019-08-12 14:59 ` Doug Ledford
@ 2019-08-13 10:10 ` Leon Romanovsky
2019-08-13 13:35 ` Doug Ledford
0 siblings, 1 reply; 7+ messages in thread
From: Leon Romanovsky @ 2019-08-13 10:10 UTC (permalink / raw)
To: Doug Ledford
Cc: Jason Gunthorpe, Yishai Hadas, Yishai Hadas, RDMA mailing list
On Mon, Aug 12, 2019 at 10:59:07AM -0400, Doug Ledford wrote:
> On Thu, 2019-08-08 at 15:34 +0000, Jason Gunthorpe wrote:
> > On Thu, Aug 08, 2019 at 06:32:00PM +0300, Yishai Hadas wrote:
> > > > > diff --git a/drivers/infiniband/hw/mlx5/devx.c
> > > > > b/drivers/infiniband/hw/mlx5/devx.c
> > > > > index 2d1b3d9609d9..af5bbb35c058 100644
> > > > > +++ b/drivers/infiniband/hw/mlx5/devx.c
> > > > > @@ -2644,12 +2644,13 @@ static int devx_async_event_close(struct
> > > > > inode *inode, struct file *filp)
> > > > > struct devx_async_event_file *ev_file = filp-
> > > > > >private_data;
> > > >
> > > > This line is wrong, it should be
> > > >
> > > > struct devx_async_event_file *ev_file =
> > > > container_of(struct
> > > > devx_async_event_file, filp-
> > > > >private_data, uobj);
> > >
> > > You suggested the below 2 lines instead of the above one line,
> > > correct ? as
> > > struct devx_async_event_file wraps uobj as its first field this is
> > > logically
> > > equal, agree ?
> >
> > Yes, it is wrong only in the use of the type system, the private_data
> > void * should only ever be cast to a ib_uobject.
> >
> > > struct ib_uobject *uobj = filp->private_data;
> >
> > This could be done with a cast
> >
> > > > It is also a bit redundant to store the mlx5_ib_dev in the
> > > > devx_async_event_file as uobj->ucontext->dev is the same pointer.
> > > >
> > > Post hot unplug uobj->ucontext might not be accessible, isn't it ?
> > > Current code should be fine for that.
> >
> > That is the other kind of weird thing, all this destruction could be
> > done on unplug..
> >
> > > Are we fine to take this patch ?
> >
> > Yes
>
> Thanks, applied to for-rc.
Doug,
I don't see it in WIP branch, did you push it?
Thanks
>
> --
> Doug Ledford <dledford@redhat.com>
> GPG KeyID: B826A3330E572FDD
> Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH rdma-rc] IB/mlx5: Fix use-after-free error while accessing ev_file pointer
2019-08-13 10:10 ` Leon Romanovsky
@ 2019-08-13 13:35 ` Doug Ledford
0 siblings, 0 replies; 7+ messages in thread
From: Doug Ledford @ 2019-08-13 13:35 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Jason Gunthorpe, Yishai Hadas, Yishai Hadas, RDMA mailing list
[-- Attachment #1: Type: text/plain, Size: 2271 bytes --]
On Tue, 2019-08-13 at 13:10 +0300, Leon Romanovsky wrote:
> On Mon, Aug 12, 2019 at 10:59:07AM -0400, Doug Ledford wrote:
> > On Thu, 2019-08-08 at 15:34 +0000, Jason Gunthorpe wrote:
> > > On Thu, Aug 08, 2019 at 06:32:00PM +0300, Yishai Hadas wrote:
> > > > > > diff --git a/drivers/infiniband/hw/mlx5/devx.c
> > > > > > b/drivers/infiniband/hw/mlx5/devx.c
> > > > > > index 2d1b3d9609d9..af5bbb35c058 100644
> > > > > > +++ b/drivers/infiniband/hw/mlx5/devx.c
> > > > > > @@ -2644,12 +2644,13 @@ static int
> > > > > > devx_async_event_close(struct
> > > > > > inode *inode, struct file *filp)
> > > > > > struct devx_async_event_file *ev_file = filp-
> > > > > > > private_data;
> > > > >
> > > > > This line is wrong, it should be
> > > > >
> > > > > struct devx_async_event_file *ev_file =
> > > > > container_of(struct
> > > > > devx_async_event_file, filp-
> > > > > > private_data, uobj);
> > > >
> > > > You suggested the below 2 lines instead of the above one line,
> > > > correct ? as
> > > > struct devx_async_event_file wraps uobj as its first field this
> > > > is
> > > > logically
> > > > equal, agree ?
> > >
> > > Yes, it is wrong only in the use of the type system, the
> > > private_data
> > > void * should only ever be cast to a ib_uobject.
> > >
> > > > struct ib_uobject *uobj = filp->private_data;
> > >
> > > This could be done with a cast
> > >
> > > > > It is also a bit redundant to store the mlx5_ib_dev in the
> > > > > devx_async_event_file as uobj->ucontext->dev is the same
> > > > > pointer.
> > > > >
> > > > Post hot unplug uobj->ucontext might not be accessible, isn't it
> > > > ?
> > > > Current code should be fine for that.
> > >
> > > That is the other kind of weird thing, all this destruction could
> > > be
> > > done on unplug..
> > >
> > > > Are we fine to take this patch ?
> > >
> > > Yes
> >
> > Thanks, applied to for-rc.
>
> Doug,
>
> I don't see it in WIP branch, did you push it?
Sorry, I did not. It's been pushed now, it should be visible within a
few moments.
--
Doug Ledford <dledford@redhat.com>
GPG KeyID: B826A3330E572FDD
Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-08-13 13:36 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-08 8:15 [PATCH rdma-rc] IB/mlx5: Fix use-after-free error while accessing ev_file pointer Leon Romanovsky
2019-08-08 12:26 ` Jason Gunthorpe
2019-08-08 15:32 ` Yishai Hadas
2019-08-08 15:34 ` Jason Gunthorpe
2019-08-12 14:59 ` Doug Ledford
2019-08-13 10:10 ` Leon Romanovsky
2019-08-13 13:35 ` Doug Ledford
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).