io_uring: fix error clear of ->file_table in io_sqe_files_register()

PATCH In review Jens Axboe 3 participants 6 messages Nov 10, 2019 – Nov 11, 2019

Subsystems (2)

VFS io_uring

lore

Share & tools

lore

Jens Axboe

author maintainer

2019-11-10 15:47 UTC #0 lore raw

PATCH 1/1 io_uring: fix error clear of ->file_table in io_sqe_files_register()

syzbot reports that when using failslab and friends, we can get a double free in io_sqe_files_unregister():

BUG: KASAN: double-free or invalid-free in
io_sqe_files_unregister+0x20b/0x300 fs/io_uring.c:3185

CPU: 1 PID: 8819 Comm: syz-executor452 Not tainted 5.4.0-rc6-next-20191108
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x197/0x210 lib/dump_stack.c:118
  print_address_description.constprop.0.cold+0xd4/0x30b mm/kasan/report.c:374
  kasan_report_invalid_free+0x65/0xa0 mm/kasan/report.c:468
  __kasan_slab_free+0x13a/0x150 mm/kasan/common.c:450
  kasan_slab_free+0xe/0x10 mm/kasan/common.c:480
  __cache_free mm/slab.c:3426 [inline]
  kfree+0x10a/0x2c0 mm/slab.c:3757
  io_sqe_files_unregister+0x20b/0x300 fs/io_uring.c:3185
  io_ring_ctx_free fs/io_uring.c:3998 [inline]
  io_ring_ctx_wait_and_kill+0x348/0x700 fs/io_uring.c:4060
  io_uring_release+0x42/0x50 fs/io_uring.c:4068
  __fput+0x2ff/0x890 fs/file_table.c:280
  ____fput+0x16/0x20 fs/file_table.c:313
  task_work_run+0x145/0x1c0 kernel/task_work.c:113
  exit_task_work include/linux/task_work.h:22 [inline]
  do_exit+0x904/0x2e60 kernel/exit.c:817
  do_group_exit+0x135/0x360 kernel/exit.c:921
  __do_sys_exit_group kernel/exit.c:932 [inline]
  __se_sys_exit_group kernel/exit.c:930 [inline]
  __x64_sys_exit_group+0x44/0x50 kernel/exit.c:930
  do_syscall_64+0xfa/0x760 arch/x86/entry/common.c:290
  entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x43f2c8
Code: 31 b8 c5 f7 ff ff 48 8b 5c 24 28 48 8b 6c 24 30 4c 8b 64 24 38 4c 8b

6c 24 40 4c 8b 74 24 48 4c 8b 7c 24 50 48 83 c4 58 c3 66 <0f> 1f 84 00 00 00 00 00 48 8d 35 59 ca 00 00 0f b6 d2 48 89 fb 48
RSP: 002b:00007ffd5b976008 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 000000000043f2c8
RDX: 0000000000000000 RSI: 000000000000003c RDI: 0000000000000000
RBP: 00000000004bf0a8 R08: 00000000000000e7 R09: ffffffffffffffd0
R10: 0000000000000001 R11: 0000000000000246 R12: 0000000000000001
R13: 00000000006d1180 R14: 0000000000000000 R15: 0000000000000000

This happens if we fail allocating the file tables. For that case we do free the file table correctly, but we forget to set it to NULL. This means that ring teardown will see it as being non-NULL, and attempt to free it again.

Fix this by clearing the file_table pointer if we free the table.

Reported-by [email protected]

Fixes 65e19f54d29c ("io_uring: support for larger fixed file sets")

Signed-off-by Jens Axboe <[email protected]>

---

fs/io_uring.c +1 -0

index 7fc3a72e1e1e..5c10b34ceb24 100644
@@ -3488,6 +3488,7 @@ static int io_sqe_files_register(struct io_ring_ctx *ctx, void __user *arg,
 
 	if (io_sqe_alloc_file_tables(ctx, nr_tables, nr_args)) {
 		kfree(ctx->file_table);
+		ctx->file_table = NULL;
 		return -ENOMEM;
 	}

Signature · 1 line

Jens Axboe

Bob Liu

Reviewed-by on v1 1/1

2019-11-10 23:44 UTC #1 lore raw

Quoted 49 lines · Jens Axboe

syzbot reports that when using failslab and friends, we can get a double free in io_sqe_files_unregister():
BUG: KASAN: double-free or invalid-free in
io_sqe_files_unregister+0x20b/0x300 fs/io_uring.c:3185

CPU: 1 PID: 8819 Comm: syz-executor452 Not tainted 5.4.0-rc6-next-20191108
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x197/0x210 lib/dump_stack.c:118
  print_address_description.constprop.0.cold+0xd4/0x30b mm/kasan/report.c:374
  kasan_report_invalid_free+0x65/0xa0 mm/kasan/report.c:468
  __kasan_slab_free+0x13a/0x150 mm/kasan/common.c:450
  kasan_slab_free+0xe/0x10 mm/kasan/common.c:480
  __cache_free mm/slab.c:3426 [inline]
  kfree+0x10a/0x2c0 mm/slab.c:3757
  io_sqe_files_unregister+0x20b/0x300 fs/io_uring.c:3185
  io_ring_ctx_free fs/io_uring.c:3998 [inline]
  io_ring_ctx_wait_and_kill+0x348/0x700 fs/io_uring.c:4060
  io_uring_release+0x42/0x50 fs/io_uring.c:4068
  __fput+0x2ff/0x890 fs/file_table.c:280
  ____fput+0x16/0x20 fs/file_table.c:313
  task_work_run+0x145/0x1c0 kernel/task_work.c:113
  exit_task_work include/linux/task_work.h:22 [inline]
  do_exit+0x904/0x2e60 kernel/exit.c:817
  do_group_exit+0x135/0x360 kernel/exit.c:921
  __do_sys_exit_group kernel/exit.c:932 [inline]
  __se_sys_exit_group kernel/exit.c:930 [inline]
  __x64_sys_exit_group+0x44/0x50 kernel/exit.c:930
  do_syscall_64+0xfa/0x760 arch/x86/entry/common.c:290
  entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x43f2c8
Code: 31 b8 c5 f7 ff ff 48 8b 5c 24 28 48 8b 6c 24 30 4c 8b 64 24 38 4c 8b
6c 24 40 4c 8b 74 24 48 4c 8b 7c 24 50 48 83 c4 58 c3 66 <0f> 1f 84 00 00 00 00 00 48 8d 35 59 ca 00 00 0f b6 d2 48 89 fb 48
RSP: 002b:00007ffd5b976008 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 000000000043f2c8
RDX: 0000000000000000 RSI: 000000000000003c RDI: 0000000000000000
RBP: 00000000004bf0a8 R08: 00000000000000e7 R09: ffffffffffffffd0
R10: 0000000000000001 R11: 0000000000000246 R12: 0000000000000001
R13: 00000000006d1180 R14: 0000000000000000 R15: 0000000000000000
This happens if we fail allocating the file tables. For that case we do free the file table correctly, but we forget to set it to NULL. This means that ring teardown will see it as being non-NULL, and attempt to free it again.

Quoted 6 lines

Fix this by clearing the file_table pointer if we free the table.
Reported-by [email protected]
Fixes 65e19f54d29c ("io_uring: support for larger fixed file sets")
Signed-off-by Jens Axboe <[email protected]>

Reviewed-by Bob Liu <[email protected]>

By the way, there are many place(besides io_uring.c) which need to set pointer to NULL after free. I saw similar fix from time to time.

Do you think a safe_free() is worth? e.g #define SAFE_FREE(p) { if (p) { free(p); (p)=NULL; } }

Quoted 15 lines

---

fs/io_uring.c +1 -0

index 7fc3a72e1e1e..5c10b34ceb24 100644
@@ -3488,6 +3488,7 @@ static int io_sqe_files_register(struct io_ring_ctx *ctx, void __user *arg,
 
 	if (io_sqe_alloc_file_tables(ctx, nr_tables, nr_args)) {
 		kfree(ctx->file_table);
+		ctx->file_table = NULL;
 		return -ENOMEM;
 	}

Jens Axboe

author maintainer

2019-11-11 03:55 UTC #2 lore raw

Quoted 50 lines · Bob Liu

Quoted 49 lines · Jens Axboe
syzbot reports that when using failslab and friends, we can get a double free in io_sqe_files_unregister():
BUG: KASAN: double-free or invalid-free in
io_sqe_files_unregister+0x20b/0x300 fs/io_uring.c:3185

CPU: 1 PID: 8819 Comm: syz-executor452 Not tainted 5.4.0-rc6-next-20191108
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
   __dump_stack lib/dump_stack.c:77 [inline]
   dump_stack+0x197/0x210 lib/dump_stack.c:118
   print_address_description.constprop.0.cold+0xd4/0x30b mm/kasan/report.c:374
   kasan_report_invalid_free+0x65/0xa0 mm/kasan/report.c:468
   __kasan_slab_free+0x13a/0x150 mm/kasan/common.c:450
   kasan_slab_free+0xe/0x10 mm/kasan/common.c:480
   __cache_free mm/slab.c:3426 [inline]
   kfree+0x10a/0x2c0 mm/slab.c:3757
   io_sqe_files_unregister+0x20b/0x300 fs/io_uring.c:3185
   io_ring_ctx_free fs/io_uring.c:3998 [inline]
   io_ring_ctx_wait_and_kill+0x348/0x700 fs/io_uring.c:4060
   io_uring_release+0x42/0x50 fs/io_uring.c:4068
   __fput+0x2ff/0x890 fs/file_table.c:280
   ____fput+0x16/0x20 fs/file_table.c:313
   task_work_run+0x145/0x1c0 kernel/task_work.c:113
   exit_task_work include/linux/task_work.h:22 [inline]
   do_exit+0x904/0x2e60 kernel/exit.c:817
   do_group_exit+0x135/0x360 kernel/exit.c:921
   __do_sys_exit_group kernel/exit.c:932 [inline]
   __se_sys_exit_group kernel/exit.c:930 [inline]
   __x64_sys_exit_group+0x44/0x50 kernel/exit.c:930
   do_syscall_64+0xfa/0x760 arch/x86/entry/common.c:290
   entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x43f2c8
Code: 31 b8 c5 f7 ff ff 48 8b 5c 24 28 48 8b 6c 24 30 4c 8b 64 24 38 4c 8b
6c 24 40 4c 8b 74 24 48 4c 8b 7c 24 50 48 83 c4 58 c3 66 <0f> 1f 84 00 00 00 00 00 48 8d 35 59 ca 00 00 0f b6 d2 48 89 fb 48
RSP: 002b:00007ffd5b976008 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 000000000043f2c8
RDX: 0000000000000000 RSI: 000000000000003c RDI: 0000000000000000
RBP: 00000000004bf0a8 R08: 00000000000000e7 R09: ffffffffffffffd0
R10: 0000000000000001 R11: 0000000000000246 R12: 0000000000000001
R13: 00000000006d1180 R14: 0000000000000000 R15: 0000000000000000
This happens if we fail allocating the file tables. For that case we do free the file table correctly, but we forget to set it to NULL. This means that ring teardown will see it as being non-NULL, and attempt to free it again.

Quoted 8 lines

Quoted 6 lines
Fix this by clearing the file_table pointer if we free the table.
Reported-by [email protected]
Fixes 65e19f54d29c ("io_uring: support for larger fixed file sets")
Signed-off-by Jens Axboe <[email protected]>
Reviewed-by Bob Liu <[email protected]>

Thanks, added.

Quoted 5 lines

By the way, there are many place(besides io_uring.c) which need to set pointer to NULL after free. I saw similar fix from time to time.
Do you think a safe_free() is worth? e.g #define SAFE_FREE(p) { if (p) { free(p); (p)=NULL; } }

Hmm not sure, and would probably be better as:

kfree_safe(&ptr);

or something instead. I seem to recall discussions about that ages ago, probably worth while to try and search and see if you can find those. I suspect Linus hates it, reasons not remembered ;-)

Signature · 1 line

Jens Axboe

Jackie Liu

2019-11-11 04:02 UTC #3 lore raw

Quoted 72 lines

2019年11月11日 11:54，Jens Axboe <[email protected]> 写道：
Quoted 50 lines · Bob Liu
Quoted 49 lines · Jens Axboe
syzbot reports that when using failslab and friends, we can get a double free in io_sqe_files_unregister():
BUG: KASAN: double-free or invalid-free in
io_sqe_files_unregister+0x20b/0x300 fs/io_uring.c:3185

CPU: 1 PID: 8819 Comm: syz-executor452 Not tainted 5.4.0-rc6-next-20191108
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x197/0x210 lib/dump_stack.c:118
  print_address_description.constprop.0.cold+0xd4/0x30b mm/kasan/report.c:374
  kasan_report_invalid_free+0x65/0xa0 mm/kasan/report.c:468
  __kasan_slab_free+0x13a/0x150 mm/kasan/common.c:450
  kasan_slab_free+0xe/0x10 mm/kasan/common.c:480
  __cache_free mm/slab.c:3426 [inline]
  kfree+0x10a/0x2c0 mm/slab.c:3757
  io_sqe_files_unregister+0x20b/0x300 fs/io_uring.c:3185
  io_ring_ctx_free fs/io_uring.c:3998 [inline]
  io_ring_ctx_wait_and_kill+0x348/0x700 fs/io_uring.c:4060
  io_uring_release+0x42/0x50 fs/io_uring.c:4068
  __fput+0x2ff/0x890 fs/file_table.c:280
  ____fput+0x16/0x20 fs/file_table.c:313
  task_work_run+0x145/0x1c0 kernel/task_work.c:113
  exit_task_work include/linux/task_work.h:22 [inline]
  do_exit+0x904/0x2e60 kernel/exit.c:817
  do_group_exit+0x135/0x360 kernel/exit.c:921
  __do_sys_exit_group kernel/exit.c:932 [inline]
  __se_sys_exit_group kernel/exit.c:930 [inline]
  __x64_sys_exit_group+0x44/0x50 kernel/exit.c:930
  do_syscall_64+0xfa/0x760 arch/x86/entry/common.c:290
  entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x43f2c8
Code: 31 b8 c5 f7 ff ff 48 8b 5c 24 28 48 8b 6c 24 30 4c 8b 64 24 38 4c 8b
6c 24 40 4c 8b 74 24 48 4c 8b 7c 24 50 48 83 c4 58 c3 66 <0f> 1f 84 00 00 00 00 00 48 8d 35 59 ca 00 00 0f b6 d2 48 89 fb 48
RSP: 002b:00007ffd5b976008 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 000000000043f2c8
RDX: 0000000000000000 RSI: 000000000000003c RDI: 0000000000000000
RBP: 00000000004bf0a8 R08: 00000000000000e7 R09: ffffffffffffffd0
R10: 0000000000000001 R11: 0000000000000246 R12: 0000000000000001
R13: 00000000006d1180 R14: 0000000000000000 R15: 0000000000000000
This happens if we fail allocating the file tables. For that case we do free the file table correctly, but we forget to set it to NULL. This means that ring teardown will see it as being non-NULL, and attempt to free it again.
Quoted 8 lines
Quoted 6 lines
Fix this by clearing the file_table pointer if we free the table.
Reported-by [email protected]
Fixes 65e19f54d29c ("io_uring: support for larger fixed file sets")
Signed-off-by Jens Axboe <[email protected]>
Reviewed-by Bob Liu <[email protected]>
Thanks, added.
Quoted 5 lines
By the way, there are many place(besides io_uring.c) which need to set pointer to NULL after free. I saw similar fix from time to time.
Do you think a safe_free() is worth? e.g #define SAFE_FREE(p) { if (p) { free(p); (p)=NULL; } }

Quoted 8 lines

Hmm not sure, and would probably be better as:
kfree_safe(&ptr);
or something instead. I seem to recall discussions about that ages ago, probably worth while to try and search and see if you can find those. I suspect Linus hates it, reasons not remembered ;-)

I think this may be a worthwhile solution, but kfree can handle NULL, we can set it to NULL directly after free is finished.

void kfree_safe(const void *ptr)

{
	kfree(ptr);
	ptr = NULL;
}

--
BR, Jackie Liu

Jens Axboe

author maintainer

2019-11-11 04:09 UTC #4 lore raw

Quoted 81 lines · Jackie Liu

Quoted 72 lines
2019年11月11日 11:54，Jens Axboe <[email protected]> 写道：
Quoted 50 lines · Bob Liu
Quoted 49 lines · Jens Axboe
syzbot reports that when using failslab and friends, we can get a double free in io_sqe_files_unregister():
BUG: KASAN: double-free or invalid-free in
io_sqe_files_unregister+0x20b/0x300 fs/io_uring.c:3185

CPU: 1 PID: 8819 Comm: syz-executor452 Not tainted 5.4.0-rc6-next-20191108
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
   __dump_stack lib/dump_stack.c:77 [inline]
   dump_stack+0x197/0x210 lib/dump_stack.c:118
   print_address_description.constprop.0.cold+0xd4/0x30b mm/kasan/report.c:374
   kasan_report_invalid_free+0x65/0xa0 mm/kasan/report.c:468
   __kasan_slab_free+0x13a/0x150 mm/kasan/common.c:450
   kasan_slab_free+0xe/0x10 mm/kasan/common.c:480
   __cache_free mm/slab.c:3426 [inline]
   kfree+0x10a/0x2c0 mm/slab.c:3757
   io_sqe_files_unregister+0x20b/0x300 fs/io_uring.c:3185
   io_ring_ctx_free fs/io_uring.c:3998 [inline]
   io_ring_ctx_wait_and_kill+0x348/0x700 fs/io_uring.c:4060
   io_uring_release+0x42/0x50 fs/io_uring.c:4068
   __fput+0x2ff/0x890 fs/file_table.c:280
   ____fput+0x16/0x20 fs/file_table.c:313
   task_work_run+0x145/0x1c0 kernel/task_work.c:113
   exit_task_work include/linux/task_work.h:22 [inline]
   do_exit+0x904/0x2e60 kernel/exit.c:817
   do_group_exit+0x135/0x360 kernel/exit.c:921
   __do_sys_exit_group kernel/exit.c:932 [inline]
   __se_sys_exit_group kernel/exit.c:930 [inline]
   __x64_sys_exit_group+0x44/0x50 kernel/exit.c:930
   do_syscall_64+0xfa/0x760 arch/x86/entry/common.c:290
   entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x43f2c8
Code: 31 b8 c5 f7 ff ff 48 8b 5c 24 28 48 8b 6c 24 30 4c 8b 64 24 38 4c 8b
6c 24 40 4c 8b 74 24 48 4c 8b 7c 24 50 48 83 c4 58 c3 66 <0f> 1f 84 00 00 00 00 00 48 8d 35 59 ca 00 00 0f b6 d2 48 89 fb 48
RSP: 002b:00007ffd5b976008 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 000000000043f2c8
RDX: 0000000000000000 RSI: 000000000000003c RDI: 0000000000000000
RBP: 00000000004bf0a8 R08: 00000000000000e7 R09: ffffffffffffffd0
R10: 0000000000000001 R11: 0000000000000246 R12: 0000000000000001
R13: 00000000006d1180 R14: 0000000000000000 R15: 0000000000000000
This happens if we fail allocating the file tables. For that case we do free the file table correctly, but we forget to set it to NULL. This means that ring teardown will see it as being non-NULL, and attempt to free it again.
Quoted 8 lines
Quoted 6 lines
Fix this by clearing the file_table pointer if we free the table.
Reported-by [email protected]
Fixes 65e19f54d29c ("io_uring: support for larger fixed file sets")
Signed-off-by Jens Axboe <[email protected]>
Reviewed-by Bob Liu <[email protected]>
Thanks, added.
Quoted 5 lines
By the way, there are many place(besides io_uring.c) which need to set pointer to NULL after free. I saw similar fix from time to time.
Do you think a safe_free() is worth? e.g #define SAFE_FREE(p) { if (p) { free(p); (p)=NULL; } }
Quoted 8 lines
Hmm not sure, and would probably be better as:
kfree_safe(&ptr);
or something instead. I seem to recall discussions about that ages ago, probably worth while to try and search and see if you can find those. I suspect Linus hates it, reasons not remembered ;-)

Quoted 8 lines

I think this may be a worthwhile solution, but kfree can handle NULL, we can set it to NULL directly after free is finished.
void kfree_safe(const void *ptr)
{
	kfree(ptr);
	ptr = NULL;
}

Sure, but that doesn't change the ptr in the caller, which was my point. You need to pass in a pointer to the pointer for that, otherwise clearing it in kfree_safe() is pointless:

void kfree_safe(const void **ptr)

{
	kfree(*ptr);
	*ptr = NULL;
}

and then you run into all sorts of fun, since void ** isn't the same as 'struct foo **'.

Signature · 1 line

Jens Axboe

Jackie Liu

2019-11-11 04:19 UTC #5 lore raw

Quoted 99 lines

2019年11月11日 12:09，Jens Axboe <[email protected]> 写道：
Quoted 81 lines · Jackie Liu
Quoted 72 lines
2019年11月11日 11:54，Jens Axboe <[email protected]> 写道：
Quoted 50 lines · Bob Liu
Quoted 49 lines · Jens Axboe
syzbot reports that when using failslab and friends, we can get a double free in io_sqe_files_unregister():
BUG: KASAN: double-free or invalid-free in
io_sqe_files_unregister+0x20b/0x300 fs/io_uring.c:3185

CPU: 1 PID: 8819 Comm: syz-executor452 Not tainted 5.4.0-rc6-next-20191108
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x197/0x210 lib/dump_stack.c:118
  print_address_description.constprop.0.cold+0xd4/0x30b mm/kasan/report.c:374
  kasan_report_invalid_free+0x65/0xa0 mm/kasan/report.c:468
  __kasan_slab_free+0x13a/0x150 mm/kasan/common.c:450
  kasan_slab_free+0xe/0x10 mm/kasan/common.c:480
  __cache_free mm/slab.c:3426 [inline]
  kfree+0x10a/0x2c0 mm/slab.c:3757
  io_sqe_files_unregister+0x20b/0x300 fs/io_uring.c:3185
  io_ring_ctx_free fs/io_uring.c:3998 [inline]
  io_ring_ctx_wait_and_kill+0x348/0x700 fs/io_uring.c:4060
  io_uring_release+0x42/0x50 fs/io_uring.c:4068
  __fput+0x2ff/0x890 fs/file_table.c:280
  ____fput+0x16/0x20 fs/file_table.c:313
  task_work_run+0x145/0x1c0 kernel/task_work.c:113
  exit_task_work include/linux/task_work.h:22 [inline]
  do_exit+0x904/0x2e60 kernel/exit.c:817
  do_group_exit+0x135/0x360 kernel/exit.c:921
  __do_sys_exit_group kernel/exit.c:932 [inline]
  __se_sys_exit_group kernel/exit.c:930 [inline]
  __x64_sys_exit_group+0x44/0x50 kernel/exit.c:930
  do_syscall_64+0xfa/0x760 arch/x86/entry/common.c:290
  entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x43f2c8
Code: 31 b8 c5 f7 ff ff 48 8b 5c 24 28 48 8b 6c 24 30 4c 8b 64 24 38 4c 8b
6c 24 40 4c 8b 74 24 48 4c 8b 7c 24 50 48 83 c4 58 c3 66 <0f> 1f 84 00 00 00 00 00 48 8d 35 59 ca 00 00 0f b6 d2 48 89 fb 48
RSP: 002b:00007ffd5b976008 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 000000000043f2c8
RDX: 0000000000000000 RSI: 000000000000003c RDI: 0000000000000000
RBP: 00000000004bf0a8 R08: 00000000000000e7 R09: ffffffffffffffd0
R10: 0000000000000001 R11: 0000000000000246 R12: 0000000000000001
R13: 00000000006d1180 R14: 0000000000000000 R15: 0000000000000000
This happens if we fail allocating the file tables. For that case we do free the file table correctly, but we forget to set it to NULL. This means that ring teardown will see it as being non-NULL, and attempt to free it again.
Quoted 8 lines
Quoted 6 lines
Fix this by clearing the file_table pointer if we free the table.
Reported-by [email protected]
Fixes 65e19f54d29c ("io_uring: support for larger fixed file sets")
Signed-off-by Jens Axboe <[email protected]>
Reviewed-by Bob Liu <[email protected]>
Thanks, added.
Quoted 5 lines
By the way, there are many place(besides io_uring.c) which need to set pointer to NULL after free. I saw similar fix from time to time.
Do you think a safe_free() is worth? e.g #define SAFE_FREE(p) { if (p) { free(p); (p)=NULL; } }
Quoted 8 lines
Hmm not sure, and would probably be better as:
kfree_safe(&ptr);
or something instead. I seem to recall discussions about that ages ago, probably worth while to try and search and see if you can find those. I suspect Linus hates it, reasons not remembered ;-)
Quoted 8 lines
I think this may be a worthwhile solution, but kfree can handle NULL, we can set it to NULL directly after free is finished.
void kfree_safe(const void *ptr)
{
	kfree(ptr);
	ptr = NULL;
}
Sure, but that doesn't change the ptr in the caller, which was my point. You need to pass in a pointer to the pointer for that, otherwise clearing it in kfree_safe() is pointless:

Quoted 5 lines

void kfree_safe(const void **ptr)
{
	kfree(*ptr);
	*ptr = NULL;
}

Yes, you are right. If it is set to NULL directly, if it is wrong, it can be panic immediately, which is convenient for debugging.

Quoted 4 lines

and then you run into all sorts of fun, since void ** isn't the same as 'struct foo **'.

--
BR, Jackie Liu