public inbox for [email protected]
 help / color / mirror / Atom feed
* Re: [PATCH] io_uring: fix file leak on creating io ctx
       [not found] <[email protected]>
@ 2020-12-07 15:04 ` Jens Axboe
  2020-12-07 15:42   ` Jens Axboe
  2020-12-07 16:42 ` Jens Axboe
       [not found] ` <[email protected]>
  2 siblings, 1 reply; 4+ messages in thread
From: Jens Axboe @ 2020-12-07 15:04 UTC (permalink / raw)
  To: Hillf Danton, LKML
  Cc: io-uring, syzkaller-bugs, syzbot+71c4697e27c99fddcf17,
	Pavel Begunkov

On 12/7/20 1:15 AM, Hillf Danton wrote:
> Put file as part of error handling when setting up io ctx to fix
> memory leak like the following one.
> 
>    BUG: memory leak
>    unreferenced object 0xffff888101ea2200 (size 256):
>      comm "syz-executor355", pid 8470, jiffies 4294953658 (age 32.400s)
>      hex dump (first 32 bytes):
>        00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>        20 59 03 01 81 88 ff ff 80 87 a8 10 81 88 ff ff   Y..............
>      backtrace:
>        [<000000002e0a7c5f>] kmem_cache_zalloc include/linux/slab.h:654 [inline]
>        [<000000002e0a7c5f>] __alloc_file+0x1f/0x130 fs/file_table.c:101
>        [<000000001a55b73a>] alloc_empty_file+0x69/0x120 fs/file_table.c:151
>        [<00000000fb22349e>] alloc_file+0x33/0x1b0 fs/file_table.c:193
>        [<000000006e1465bb>] alloc_file_pseudo+0xb2/0x140 fs/file_table.c:233
>        [<000000007118092a>] anon_inode_getfile fs/anon_inodes.c:91 [inline]
>        [<000000007118092a>] anon_inode_getfile+0xaa/0x120 fs/anon_inodes.c:74
>        [<000000002ae99012>] io_uring_get_fd fs/io_uring.c:9198 [inline]
>        [<000000002ae99012>] io_uring_create fs/io_uring.c:9377 [inline]
>        [<000000002ae99012>] io_uring_setup+0x1125/0x1630 fs/io_uring.c:9411
>        [<000000008280baad>] do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
>        [<00000000685d8cf0>] entry_SYSCALL_64_after_hwframe+0x44/0xa9

Applied for 5.10, thanks.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] io_uring: fix file leak on creating io ctx
  2020-12-07 15:04 ` [PATCH] io_uring: fix file leak on creating io ctx Jens Axboe
@ 2020-12-07 15:42   ` Jens Axboe
  0 siblings, 0 replies; 4+ messages in thread
From: Jens Axboe @ 2020-12-07 15:42 UTC (permalink / raw)
  To: Hillf Danton, LKML
  Cc: io-uring, syzkaller-bugs, syzbot+71c4697e27c99fddcf17,
	Pavel Begunkov

On 12/7/20 8:04 AM, Jens Axboe wrote:
> On 12/7/20 1:15 AM, Hillf Danton wrote:
>> Put file as part of error handling when setting up io ctx to fix
>> memory leak like the following one.
>>
>>    BUG: memory leak
>>    unreferenced object 0xffff888101ea2200 (size 256):
>>      comm "syz-executor355", pid 8470, jiffies 4294953658 (age 32.400s)
>>      hex dump (first 32 bytes):
>>        00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>>        20 59 03 01 81 88 ff ff 80 87 a8 10 81 88 ff ff   Y..............
>>      backtrace:
>>        [<000000002e0a7c5f>] kmem_cache_zalloc include/linux/slab.h:654 [inline]
>>        [<000000002e0a7c5f>] __alloc_file+0x1f/0x130 fs/file_table.c:101
>>        [<000000001a55b73a>] alloc_empty_file+0x69/0x120 fs/file_table.c:151
>>        [<00000000fb22349e>] alloc_file+0x33/0x1b0 fs/file_table.c:193
>>        [<000000006e1465bb>] alloc_file_pseudo+0xb2/0x140 fs/file_table.c:233
>>        [<000000007118092a>] anon_inode_getfile fs/anon_inodes.c:91 [inline]
>>        [<000000007118092a>] anon_inode_getfile+0xaa/0x120 fs/anon_inodes.c:74
>>        [<000000002ae99012>] io_uring_get_fd fs/io_uring.c:9198 [inline]
>>        [<000000002ae99012>] io_uring_create fs/io_uring.c:9377 [inline]
>>        [<000000002ae99012>] io_uring_setup+0x1125/0x1630 fs/io_uring.c:9411
>>        [<000000008280baad>] do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
>>        [<00000000685d8cf0>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> Applied for 5.10, thanks.

I take that back, this patch is totally broken. Please test your patches
before sending them out, this cannot have been even put through the most
basic of tests.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] io_uring: fix file leak on creating io ctx
       [not found] <[email protected]>
  2020-12-07 15:04 ` [PATCH] io_uring: fix file leak on creating io ctx Jens Axboe
@ 2020-12-07 16:42 ` Jens Axboe
       [not found] ` <[email protected]>
  2 siblings, 0 replies; 4+ messages in thread
From: Jens Axboe @ 2020-12-07 16:42 UTC (permalink / raw)
  To: Hillf Danton, LKML
  Cc: io-uring, syzkaller-bugs, syzbot+71c4697e27c99fddcf17,
	Pavel Begunkov

On 12/7/20 1:15 AM, Hillf Danton wrote:
> @@ -9207,12 +9208,14 @@ err_fd:
>  #if defined(CONFIG_UNIX)
>  	ctx->ring_sock->file = file;
>  #endif
> -	if (unlikely(io_uring_add_task_file(ctx, file))) {
> -		file = ERR_PTR(-ENOMEM);
> -		goto err_fd;
> +	ret = io_uring_add_task_file(ctx, file);
> +	if (ret) {
> +		fput(file);
> +		put_unused_fd(fd);
> +		goto err;
>  	}
>  	fd_install(ret, file);
> -	return ret;
> +	return 0;

You're installing the return value from io_uring_add_task_file() in the
fd table, and then returning '0' for the fd...

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] io_uring: fix file leak on creating io ctx
       [not found] ` <[email protected]>
@ 2020-12-08 15:50   ` Jens Axboe
  0 siblings, 0 replies; 4+ messages in thread
From: Jens Axboe @ 2020-12-08 15:50 UTC (permalink / raw)
  To: Hillf Danton, LKML
  Cc: io-uring, syzkaller-bugs, syzbot+71c4697e27c99fddcf17,
	Pavel Begunkov

On 12/8/20 3:28 AM, Hillf Danton wrote:
> On Mon, 7 Dec 2020 09:42:21 -0700 Jens Axboe wrote:
>> On 12/7/20 1:15 AM, Hillf Danton wrote:
>>> @@ -9207,12 +9208,14 @@ err_fd:
>>>  #if defined(CONFIG_UNIX)
>>>  	ctx->ring_sock->file = file;
>>>  #endif
>>> -	if (unlikely(io_uring_add_task_file(ctx, file))) {
>>> -		file = ERR_PTR(-ENOMEM);
>>> -		goto err_fd;
>>> +	ret = io_uring_add_task_file(ctx, file);
>>> +	if (ret) {
>>> +		fput(file);
>>> +		put_unused_fd(fd);
>>> +		goto err;
>>>  	}
>>>  	fd_install(ret, file);
>>> -	return ret;
>>> +	return 0;
>>
>> You're installing the return value from io_uring_add_task_file() in the
>> fd table, and then returning '0' for the fd...
> 
> I canot find phrases to describe the stupid mistake in my patch.
> Thank you so much for pointing it out.

This one is still utterly broken, and (again) cannot have been even
tested in the most basic way. So let's focus on not how things are
phrased, but proper patch etiquette:

- Always (ALWAYS) test your patches. There's no excuse for not doing
  so, and you are blacklisting yourself and ruining your reputation
  by sending garbage that doesn't even pass basic functionality.

- If something isn't tested at all, make it VERY clear that this is
  the case. Generally that's done by putting RFC in there and also
  stating that this is just for discussion, it's not a patch that
  is proposed for inclusion.

- Slow down! I see you sent a patch 10 min after this one, with no
  extra notice in there why that was the case. It's clearly because
  you figured out that this hasty send was bad.

I'd really like to get this in for 5.10, but I'd almost feel better
just redoing the patch myself to ensure it doesn't have other silly
errors in there. Don't put yourself in that position.


> @@ -9207,12 +9208,14 @@ err_fd:
>  #if defined(CONFIG_UNIX)
>  	ctx->ring_sock->file = file;
>  #endif
> -	if (unlikely(io_uring_add_task_file(ctx, file))) {
> -		file = ERR_PTR(-ENOMEM);
> -		goto err_fd;
> +	ret = io_uring_add_task_file(ctx, file);
> +	if (ret) {
> +		fput(file);
> +		put_unused_fd(fd);
> +		goto err;
>  	}
> -	fd_install(ret, file);
> -	return ret;
> +	fd_install(fd, file);
> +	return 0;

You're still returning '0' for the fd. 


-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-12-08 15:51 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <[email protected]>
2020-12-07 15:04 ` [PATCH] io_uring: fix file leak on creating io ctx Jens Axboe
2020-12-07 15:42   ` Jens Axboe
2020-12-07 16:42 ` Jens Axboe
     [not found] ` <[email protected]>
2020-12-08 15:50   ` Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox