Re: [PATCH] io_uring: fix file leak on creating io ctx

Re: [PATCH] io_uring: fix file leak on creating io ctx

[Jens Axboe]

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

Re: [PATCH] io_uring: fix file leak on creating io ctx

[Jens Axboe]

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

Re: [PATCH] io_uring: fix file leak on creating io ctx

[Jens Axboe]

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

Re: [PATCH] io_uring: fix file leak on creating io ctx

[Jens Axboe]

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