[PATCH 5.11] io_uring: NULL files dereference by SQPOLL

[PATCH 5.11] io_uring: NULL files dereference by SQPOLL

[Pavel Begunkov]

SQPOLL task may find sqo_task->files == NULL, so
__io_sq_thread_acquire_files() would left it unset and so all the
following fails, e.g. attempts to submit. Fail if sqo_task doesn't have
files.

[  118.962785] BUG: kernel NULL pointer dereference, address:
	0000000000000020
[  118.963812] #PF: supervisor read access in kernel mode
[  118.964534] #PF: error_code(0x0000) - not-present page
[  118.969029] RIP: 0010:__fget_files+0xb/0x80
[  119.005409] Call Trace:
[  119.005651]  fget_many+0x2b/0x30
[  119.005964]  io_file_get+0xcf/0x180
[  119.006315]  io_submit_sqes+0x3a4/0x950
[  119.006678]  ? io_double_put_req+0x43/0x70
[  119.007054]  ? io_async_task_func+0xc2/0x180
[  119.007481]  io_sq_thread+0x1de/0x6a0
[  119.007828]  kthread+0x114/0x150
[  119.008135]  ? __ia32_sys_io_uring_enter+0x3c0/0x3c0
[  119.008623]  ? kthread_park+0x90/0x90
[  119.008963]  ret_from_fork+0x22/0x30

Reported-by: Josef Grieb <josef.grieb@gmail.com>
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 8d721a652d61..9c035c5c4080 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1080,7 +1080,7 @@ static void io_sq_thread_drop_mm_files(void)
 	}
 }
 
-static void __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
+static int __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
 {
 	if (!current->files) {
 		struct files_struct *files;
@@ -1091,7 +1091,7 @@ static void __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
 		files = ctx->sqo_task->files;
 		if (!files) {
 			task_unlock(ctx->sqo_task);
-			return;
+			return -EFAULT;
 		}
 		atomic_inc(&files->count);
 		get_nsproxy(ctx->sqo_task->nsproxy);
@@ -1105,6 +1105,7 @@ static void __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
 		current->thread_pid = thread_pid;
 		task_unlock(current);
 	}
+	return 0;
 }
 
 static int __io_sq_thread_acquire_mm(struct io_ring_ctx *ctx)
@@ -1136,15 +1137,19 @@ static int io_sq_thread_acquire_mm_files(struct io_ring_ctx *ctx,
 					 struct io_kiocb *req)
 {
 	const struct io_op_def *def = &io_op_defs[req->opcode];
+	int ret;
 
 	if (def->work_flags & IO_WQ_WORK_MM) {
-		int ret = __io_sq_thread_acquire_mm(ctx);
+		ret = __io_sq_thread_acquire_mm(ctx);
 		if (unlikely(ret))
 			return ret;
 	}
 
-	if (def->needs_file || (def->work_flags & IO_WQ_WORK_FILES))
-		__io_sq_thread_acquire_files(ctx);
+	if (def->needs_file || (def->work_flags & IO_WQ_WORK_FILES)) {
+		ret = __io_sq_thread_acquire_files(ctx);
+		if (unlikely(ret))
+			return ret;
+	}
 
 	return 0;
 }
@@ -2117,8 +2122,8 @@ static void __io_req_task_submit(struct io_kiocb *req)
 {
 	struct io_ring_ctx *ctx = req->ctx;
 
-	if (!__io_sq_thread_acquire_mm(ctx)) {
-		__io_sq_thread_acquire_files(ctx);
+	if (!__io_sq_thread_acquire_mm(ctx) &&
+	    !__io_sq_thread_acquire_files(ctx)) {
 		mutex_lock(&ctx->uring_lock);
 		__io_queue_sqe(req, NULL);
 		mutex_unlock(&ctx->uring_lock);
-- 
2.24.0

Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL

[Pavel Begunkov]

On 07/11/2020 21:16, Pavel Begunkov wrote:
> SQPOLL task may find sqo_task->files == NULL, so
> __io_sq_thread_acquire_files() would left it unset and so all the
> following fails, e.g. attempts to submit. Fail if sqo_task doesn't have
> files.

Josef, could you try this one?

> 
> [  118.962785] BUG: kernel NULL pointer dereference, address:
> 	0000000000000020
> [  118.963812] #PF: supervisor read access in kernel mode
> [  118.964534] #PF: error_code(0x0000) - not-present page
> [  118.969029] RIP: 0010:__fget_files+0xb/0x80
> [  119.005409] Call Trace:
> [  119.005651]  fget_many+0x2b/0x30
> [  119.005964]  io_file_get+0xcf/0x180
> [  119.006315]  io_submit_sqes+0x3a4/0x950
> [  119.006678]  ? io_double_put_req+0x43/0x70
> [  119.007054]  ? io_async_task_func+0xc2/0x180
> [  119.007481]  io_sq_thread+0x1de/0x6a0
> [  119.007828]  kthread+0x114/0x150
> [  119.008135]  ? __ia32_sys_io_uring_enter+0x3c0/0x3c0
> [  119.008623]  ? kthread_park+0x90/0x90
> [  119.008963]  ret_from_fork+0x22/0x30
> 
> Reported-by: Josef Grieb <josef.grieb@gmail.com>
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  fs/io_uring.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 8d721a652d61..9c035c5c4080 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1080,7 +1080,7 @@ static void io_sq_thread_drop_mm_files(void)
>  	}
>  }
>  
> -static void __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
> +static int __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
>  {
>  	if (!current->files) {
>  		struct files_struct *files;
> @@ -1091,7 +1091,7 @@ static void __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
>  		files = ctx->sqo_task->files;
>  		if (!files) {
>  			task_unlock(ctx->sqo_task);
> -			return;
> +			return -EFAULT;
>  		}
>  		atomic_inc(&files->count);
>  		get_nsproxy(ctx->sqo_task->nsproxy);
> @@ -1105,6 +1105,7 @@ static void __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
>  		current->thread_pid = thread_pid;
>  		task_unlock(current);
>  	}
> +	return 0;
>  }
>  
>  static int __io_sq_thread_acquire_mm(struct io_ring_ctx *ctx)
> @@ -1136,15 +1137,19 @@ static int io_sq_thread_acquire_mm_files(struct io_ring_ctx *ctx,
>  					 struct io_kiocb *req)
>  {
>  	const struct io_op_def *def = &io_op_defs[req->opcode];
> +	int ret;
>  
>  	if (def->work_flags & IO_WQ_WORK_MM) {
> -		int ret = __io_sq_thread_acquire_mm(ctx);
> +		ret = __io_sq_thread_acquire_mm(ctx);
>  		if (unlikely(ret))
>  			return ret;
>  	}
>  
> -	if (def->needs_file || (def->work_flags & IO_WQ_WORK_FILES))
> -		__io_sq_thread_acquire_files(ctx);
> +	if (def->needs_file || (def->work_flags & IO_WQ_WORK_FILES)) {
> +		ret = __io_sq_thread_acquire_files(ctx);
> +		if (unlikely(ret))
> +			return ret;
> +	}
>  
>  	return 0;
>  }
> @@ -2117,8 +2122,8 @@ static void __io_req_task_submit(struct io_kiocb *req)
>  {
>  	struct io_ring_ctx *ctx = req->ctx;
>  
> -	if (!__io_sq_thread_acquire_mm(ctx)) {
> -		__io_sq_thread_acquire_files(ctx);
> +	if (!__io_sq_thread_acquire_mm(ctx) &&
> +	    !__io_sq_thread_acquire_files(ctx)) {
>  		mutex_lock(&ctx->uring_lock);
>  		__io_queue_sqe(req, NULL);
>  		mutex_unlock(&ctx->uring_lock);
> 

-- 
Pavel Begunkov

Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL

[Pavel Begunkov]

On 07/11/2020 21:18, Pavel Begunkov wrote:
> On 07/11/2020 21:16, Pavel Begunkov wrote:
>> SQPOLL task may find sqo_task->files == NULL, so
>> __io_sq_thread_acquire_files() would left it unset and so all the
>> following fails, e.g. attempts to submit. Fail if sqo_task doesn't have
>> files.
> 
> Josef, could you try this one?

Hmm, as you said it happens often... IIUC there is a drawback with
SQPOLL -- after the creator process/thread exits most of subsequent
requests will start failing.
I'd say from application correctness POV such tasks should exit
only after their SQPOLL io_urings got killed.

> 
>>
>> [  118.962785] BUG: kernel NULL pointer dereference, address:
>> 	0000000000000020
>> [  118.963812] #PF: supervisor read access in kernel mode
>> [  118.964534] #PF: error_code(0x0000) - not-present page
>> [  118.969029] RIP: 0010:__fget_files+0xb/0x80
>> [  119.005409] Call Trace:
>> [  119.005651]  fget_many+0x2b/0x30
>> [  119.005964]  io_file_get+0xcf/0x180
>> [  119.006315]  io_submit_sqes+0x3a4/0x950
>> [  119.006678]  ? io_double_put_req+0x43/0x70
>> [  119.007054]  ? io_async_task_func+0xc2/0x180
>> [  119.007481]  io_sq_thread+0x1de/0x6a0
>> [  119.007828]  kthread+0x114/0x150
>> [  119.008135]  ? __ia32_sys_io_uring_enter+0x3c0/0x3c0
>> [  119.008623]  ? kthread_park+0x90/0x90
>> [  119.008963]  ret_from_fork+0x22/0x30
>>
>> Reported-by: Josef Grieb <josef.grieb@gmail.com>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>  fs/io_uring.c | 19 ++++++++++++-------
>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 8d721a652d61..9c035c5c4080 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -1080,7 +1080,7 @@ static void io_sq_thread_drop_mm_files(void)
>>  	}
>>  }
>>  
>> -static void __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
>> +static int __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
>>  {
>>  	if (!current->files) {
>>  		struct files_struct *files;
>> @@ -1091,7 +1091,7 @@ static void __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
>>  		files = ctx->sqo_task->files;
>>  		if (!files) {
>>  			task_unlock(ctx->sqo_task);
>> -			return;
>> +			return -EFAULT;
>>  		}
>>  		atomic_inc(&files->count);
>>  		get_nsproxy(ctx->sqo_task->nsproxy);
>> @@ -1105,6 +1105,7 @@ static void __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
>>  		current->thread_pid = thread_pid;
>>  		task_unlock(current);
>>  	}
>> +	return 0;
>>  }
>>  
>>  static int __io_sq_thread_acquire_mm(struct io_ring_ctx *ctx)
>> @@ -1136,15 +1137,19 @@ static int io_sq_thread_acquire_mm_files(struct io_ring_ctx *ctx,
>>  					 struct io_kiocb *req)
>>  {
>>  	const struct io_op_def *def = &io_op_defs[req->opcode];
>> +	int ret;
>>  
>>  	if (def->work_flags & IO_WQ_WORK_MM) {
>> -		int ret = __io_sq_thread_acquire_mm(ctx);
>> +		ret = __io_sq_thread_acquire_mm(ctx);
>>  		if (unlikely(ret))
>>  			return ret;
>>  	}
>>  
>> -	if (def->needs_file || (def->work_flags & IO_WQ_WORK_FILES))
>> -		__io_sq_thread_acquire_files(ctx);
>> +	if (def->needs_file || (def->work_flags & IO_WQ_WORK_FILES)) {
>> +		ret = __io_sq_thread_acquire_files(ctx);
>> +		if (unlikely(ret))
>> +			return ret;
>> +	}
>>  
>>  	return 0;
>>  }
>> @@ -2117,8 +2122,8 @@ static void __io_req_task_submit(struct io_kiocb *req)
>>  {
>>  	struct io_ring_ctx *ctx = req->ctx;
>>  
>> -	if (!__io_sq_thread_acquire_mm(ctx)) {
>> -		__io_sq_thread_acquire_files(ctx);
>> +	if (!__io_sq_thread_acquire_mm(ctx) &&
>> +	    !__io_sq_thread_acquire_files(ctx)) {
>>  		mutex_lock(&ctx->uring_lock);
>>  		__io_queue_sqe(req, NULL);
>>  		mutex_unlock(&ctx->uring_lock);
>>
> 

-- 
Pavel Begunkov

Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL

[Jens Axboe]

On 11/7/20 2:54 PM, Pavel Begunkov wrote:
> On 07/11/2020 21:18, Pavel Begunkov wrote:
>> On 07/11/2020 21:16, Pavel Begunkov wrote:
>>> SQPOLL task may find sqo_task->files == NULL, so
>>> __io_sq_thread_acquire_files() would left it unset and so all the
>>> following fails, e.g. attempts to submit. Fail if sqo_task doesn't have
>>> files.
>>
>> Josef, could you try this one?
> 
> Hmm, as you said it happens often... IIUC there is a drawback with
> SQPOLL -- after the creator process/thread exits most of subsequent
> requests will start failing.
> I'd say from application correctness POV such tasks should exit
> only after their SQPOLL io_urings got killed.

I don't think there's anything wrong with that - if you submit requests
and exit before they have completed, then you by definition are not
caring about the result of them.

-- 
Jens Axboe

Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL

[Jens Axboe]

On 11/7/20 2:16 PM, Pavel Begunkov wrote:
> SQPOLL task may find sqo_task->files == NULL, so
> __io_sq_thread_acquire_files() would left it unset and so all the
> following fails, e.g. attempts to submit. Fail if sqo_task doesn't have
> files.
> 
> [  118.962785] BUG: kernel NULL pointer dereference, address:
> 	0000000000000020
> [  118.963812] #PF: supervisor read access in kernel mode
> [  118.964534] #PF: error_code(0x0000) - not-present page
> [  118.969029] RIP: 0010:__fget_files+0xb/0x80
> [  119.005409] Call Trace:
> [  119.005651]  fget_many+0x2b/0x30
> [  119.005964]  io_file_get+0xcf/0x180
> [  119.006315]  io_submit_sqes+0x3a4/0x950
> [  119.006678]  ? io_double_put_req+0x43/0x70
> [  119.007054]  ? io_async_task_func+0xc2/0x180
> [  119.007481]  io_sq_thread+0x1de/0x6a0
> [  119.007828]  kthread+0x114/0x150
> [  119.008135]  ? __ia32_sys_io_uring_enter+0x3c0/0x3c0
> [  119.008623]  ? kthread_park+0x90/0x90
> [  119.008963]  ret_from_fork+0x22/0x30
> 
> Reported-by: Josef Grieb <josef.grieb@gmail.com>
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  fs/io_uring.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 8d721a652d61..9c035c5c4080 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1080,7 +1080,7 @@ static void io_sq_thread_drop_mm_files(void)
>  	}
>  }
>  
> -static void __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
> +static int __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
>  {
>  	if (!current->files) {
>  		struct files_struct *files;
> @@ -1091,7 +1091,7 @@ static void __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
>  		files = ctx->sqo_task->files;
>  		if (!files) {
>  			task_unlock(ctx->sqo_task);
> -			return;
> +			return -EFAULT;

I don't think we should use -EFAULT here, it's generally used for trying
to copy in/out of invalid regions. Probably -ECANCELED is better here,
in lieu of something super appropriate. Maybe -EBADF would be fine too.

-- 
Jens Axboe

Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL

[Pavel Begunkov]

On 07/11/2020 22:28, Jens Axboe wrote:
> On 11/7/20 2:54 PM, Pavel Begunkov wrote:
>> On 07/11/2020 21:18, Pavel Begunkov wrote:
>>> On 07/11/2020 21:16, Pavel Begunkov wrote:
>>>> SQPOLL task may find sqo_task->files == NULL, so
>>>> __io_sq_thread_acquire_files() would left it unset and so all the
>>>> following fails, e.g. attempts to submit. Fail if sqo_task doesn't have
>>>> files.
>>>
>>> Josef, could you try this one?
>>
>> Hmm, as you said it happens often... IIUC there is a drawback with
>> SQPOLL -- after the creator process/thread exits most of subsequent
>> requests will start failing.
>> I'd say from application correctness POV such tasks should exit
>> only after their SQPOLL io_urings got killed.
> 
> I don't think there's anything wrong with that - if you submit requests
> and exit before they have completed, then you by definition are not
> caring about the result of them.

Other threads may use it as well thinking that this is fine as
they share mm, files, etc.

1. task1 create io_uring
2. clone(CLONE_FILES|CLONE_VM|...) -> task2
3. task1 exits
4. task2 continues to use io_uring

-- 
Pavel Begunkov

Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL

[Pavel Begunkov]

On 07/11/2020 22:30, Jens Axboe wrote:
> On 11/7/20 2:16 PM, Pavel Begunkov wrote:
>> SQPOLL task may find sqo_task->files == NULL, so
>> __io_sq_thread_acquire_files() would left it unset and so all the
>> following fails, e.g. attempts to submit. Fail if sqo_task doesn't have
>> files.
>>
>> [  118.962785] BUG: kernel NULL pointer dereference, address:
>> 	0000000000000020
>> [  118.963812] #PF: supervisor read access in kernel mode
>> [  118.964534] #PF: error_code(0x0000) - not-present page
>> [  118.969029] RIP: 0010:__fget_files+0xb/0x80
>> [  119.005409] Call Trace:
>> [  119.005651]  fget_many+0x2b/0x30
>> [  119.005964]  io_file_get+0xcf/0x180
>> [  119.006315]  io_submit_sqes+0x3a4/0x950
>> [  119.006678]  ? io_double_put_req+0x43/0x70
>> [  119.007054]  ? io_async_task_func+0xc2/0x180
>> [  119.007481]  io_sq_thread+0x1de/0x6a0
>> [  119.007828]  kthread+0x114/0x150
>> [  119.008135]  ? __ia32_sys_io_uring_enter+0x3c0/0x3c0
>> [  119.008623]  ? kthread_park+0x90/0x90
>> [  119.008963]  ret_from_fork+0x22/0x30
>>
>> Reported-by: Josef Grieb <josef.grieb@gmail.com>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>  fs/io_uring.c | 19 ++++++++++++-------
>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 8d721a652d61..9c035c5c4080 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -1080,7 +1080,7 @@ static void io_sq_thread_drop_mm_files(void)
>>  	}
>>  }
>>  
>> -static void __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
>> +static int __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
>>  {
>>  	if (!current->files) {
>>  		struct files_struct *files;
>> @@ -1091,7 +1091,7 @@ static void __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
>>  		files = ctx->sqo_task->files;
>>  		if (!files) {
>>  			task_unlock(ctx->sqo_task);
>> -			return;
>> +			return -EFAULT;
> 
> I don't think we should use -EFAULT here, it's generally used for trying
> to copy in/out of invalid regions. Probably -ECANCELED is better here,

Noted, I'll resend after Josef tests this.

> in lieu of something super appropriate. Maybe -EBADF would be fine too.

Yeah, something along OWNER_TASK_DEAD would make more sense.

-- 
Pavel Begunkov

Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL

[Jens Axboe]

On 11/7/20 3:49 PM, Pavel Begunkov wrote:
> On 07/11/2020 22:30, Jens Axboe wrote:
>> On 11/7/20 2:16 PM, Pavel Begunkov wrote:
>>> SQPOLL task may find sqo_task->files == NULL, so
>>> __io_sq_thread_acquire_files() would left it unset and so all the
>>> following fails, e.g. attempts to submit. Fail if sqo_task doesn't have
>>> files.
>>>
>>> [  118.962785] BUG: kernel NULL pointer dereference, address:
>>> 	0000000000000020
>>> [  118.963812] #PF: supervisor read access in kernel mode
>>> [  118.964534] #PF: error_code(0x0000) - not-present page
>>> [  118.969029] RIP: 0010:__fget_files+0xb/0x80
>>> [  119.005409] Call Trace:
>>> [  119.005651]  fget_many+0x2b/0x30
>>> [  119.005964]  io_file_get+0xcf/0x180
>>> [  119.006315]  io_submit_sqes+0x3a4/0x950
>>> [  119.006678]  ? io_double_put_req+0x43/0x70
>>> [  119.007054]  ? io_async_task_func+0xc2/0x180
>>> [  119.007481]  io_sq_thread+0x1de/0x6a0
>>> [  119.007828]  kthread+0x114/0x150
>>> [  119.008135]  ? __ia32_sys_io_uring_enter+0x3c0/0x3c0
>>> [  119.008623]  ? kthread_park+0x90/0x90
>>> [  119.008963]  ret_from_fork+0x22/0x30
>>>
>>> Reported-by: Josef Grieb <josef.grieb@gmail.com>
>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>> ---
>>>  fs/io_uring.c | 19 ++++++++++++-------
>>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>> index 8d721a652d61..9c035c5c4080 100644
>>> --- a/fs/io_uring.c
>>> +++ b/fs/io_uring.c
>>> @@ -1080,7 +1080,7 @@ static void io_sq_thread_drop_mm_files(void)
>>>  	}
>>>  }
>>>  
>>> -static void __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
>>> +static int __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
>>>  {
>>>  	if (!current->files) {
>>>  		struct files_struct *files;
>>> @@ -1091,7 +1091,7 @@ static void __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
>>>  		files = ctx->sqo_task->files;
>>>  		if (!files) {
>>>  			task_unlock(ctx->sqo_task);
>>> -			return;
>>> +			return -EFAULT;
>>
>> I don't think we should use -EFAULT here, it's generally used for trying
>> to copy in/out of invalid regions. Probably -ECANCELED is better here,
> 
> Noted, I'll resend after Josef tests this.
> 
>> in lieu of something super appropriate. Maybe -EBADF would be fine too.
> 
> Yeah, something along OWNER_TASK_DEAD would make more sense.

You could try and commandeer -EOWNERDEAD for this use case, it does
make sense.

-- 
Jens Axboe

Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL

[Jens Axboe]

On 11/7/20 3:47 PM, Pavel Begunkov wrote:
> On 07/11/2020 22:28, Jens Axboe wrote:
>> On 11/7/20 2:54 PM, Pavel Begunkov wrote:
>>> On 07/11/2020 21:18, Pavel Begunkov wrote:
>>>> On 07/11/2020 21:16, Pavel Begunkov wrote:
>>>>> SQPOLL task may find sqo_task->files == NULL, so
>>>>> __io_sq_thread_acquire_files() would left it unset and so all the
>>>>> following fails, e.g. attempts to submit. Fail if sqo_task doesn't have
>>>>> files.
>>>>
>>>> Josef, could you try this one?
>>>
>>> Hmm, as you said it happens often... IIUC there is a drawback with
>>> SQPOLL -- after the creator process/thread exits most of subsequent
>>> requests will start failing.
>>> I'd say from application correctness POV such tasks should exit
>>> only after their SQPOLL io_urings got killed.
>>
>> I don't think there's anything wrong with that - if you submit requests
>> and exit before they have completed, then you by definition are not
>> caring about the result of them.
> 
> Other threads may use it as well thinking that this is fine as
> they share mm, files, etc.
> 
> 1. task1 create io_uring
> 2. clone(CLONE_FILES|CLONE_VM|...) -> task2
> 3. task1 exits
> 4. task2 continues to use io_uring

Sure, but I think this is getting pretty contrived. Yes, if the task
that created the ring (and whose credentials are being used) exits,
then the ring is effectively dead if you're using SQPOLL. If you're
using threads, the threads go away too.

-- 
Jens Axboe

Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL

[Josef]

> Josef, could you try this one?

it's weird I couldn't apply this patch..did you pull
for-5.11/io_uring? I'm gonna try manually

---
Josef Grieb

Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL

[Josef]

> Josef, could you try this one?

the null files dereference error no longer occurs, but after a certain
point in time the OP_READ operation always returns -EFAULT

BTW forgot to mention that the NULL files dereference error only
occurs when OP_READ returns a -EFAULT

On Sun, 8 Nov 2020 at 03:09, Josef <josef.grieb@gmail.com> wrote:
>
> > Josef, could you try this one?
>
> it's weird I couldn't apply this patch..did you pull
> for-5.11/io_uring? I'm gonna try manually
>
> ---
> Josef Grieb

Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL

[Pavel Begunkov]

On 07/11/2020 23:18, Jens Axboe wrote:
> On 11/7/20 3:47 PM, Pavel Begunkov wrote:
>> On 07/11/2020 22:28, Jens Axboe wrote:
>>> On 11/7/20 2:54 PM, Pavel Begunkov wrote:
>>>> On 07/11/2020 21:18, Pavel Begunkov wrote:
>>>>> On 07/11/2020 21:16, Pavel Begunkov wrote:
>>>>>> SQPOLL task may find sqo_task->files == NULL, so
>>>>>> __io_sq_thread_acquire_files() would left it unset and so all the
>>>>>> following fails, e.g. attempts to submit. Fail if sqo_task doesn't have
>>>>>> files.
>>>>>
>>>>> Josef, could you try this one?
>>>>
>>>> Hmm, as you said it happens often... IIUC there is a drawback with
>>>> SQPOLL -- after the creator process/thread exits most of subsequent
>>>> requests will start failing.
>>>> I'd say from application correctness POV such tasks should exit
>>>> only after their SQPOLL io_urings got killed.
>>>
>>> I don't think there's anything wrong with that - if you submit requests
>>> and exit before they have completed, then you by definition are not
>>> caring about the result of them.
>>
>> Other threads may use it as well thinking that this is fine as
>> they share mm, files, etc.
>>
>> 1. task1 create io_uring
>> 2. clone(CLONE_FILES|CLONE_VM|...) -> task2
>> 3. task1 exits
>> 4. task2 continues to use io_uring
> 
> Sure, but I think this is getting pretty contrived. Yes, if the task
> that created the ring (and whose credentials are being used) exits,
> then the ring is effectively dead if you're using SQPOLL. If you're
> using threads, the threads go away too.

Sibling threads are not necessarily go away, isn't it? And pre-5.11
that wasn't a problem as it didn't have files and mm was taken directly. 


-- 
Pavel Begunkov

Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL

[Pavel Begunkov]

On 08/11/2020 02:09, Josef wrote:
>> Josef, could you try this one?
> 
> it's weird I couldn't apply this patch..did you pull
> for-5.11/io_uring? I'm gonna try manually

I'm a bit ahead of it, but nothing that would hinder. Just tried git
apply, works well. Anyway, thanks for testing by hand

-- 
Pavel Begunkov

Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL

[Pavel Begunkov]

On 08/11/2020 06:50, Josef wrote:
>> Josef, could you try this one?
> 
> the null files dereference error no longer occurs, but after a certain
> point in time the OP_READ operation always returns -EFAULT

That confirms the issue, and for efaults, as describes, I expect them to
start falling out after the task-creator exits or do exec. Just to note
that the patch only propagates errors and doesn't change the semantics.

> 
> BTW forgot to mention that the NULL files dereference error only
> occurs when OP_READ returns a -EFAULT
-- 
Pavel Begunkov

[PATCH 5.11] io_uring: NULL files dereference by SQPOLL

[Pavel Begunkov]

SQPOLL task may find sqo_task->files == NULL and
__io_sq_thread_acquire_files() would leave it unset, so following
fget_many() and others try to dereference NULL and fault. Propagate
an error files are missing.

[  118.962785] BUG: kernel NULL pointer dereference, address:
	0000000000000020
[  118.963812] #PF: supervisor read access in kernel mode
[  118.964534] #PF: error_code(0x0000) - not-present page
[  118.969029] RIP: 0010:__fget_files+0xb/0x80
[  119.005409] Call Trace:
[  119.005651]  fget_many+0x2b/0x30
[  119.005964]  io_file_get+0xcf/0x180
[  119.006315]  io_submit_sqes+0x3a4/0x950
[  119.007481]  io_sq_thread+0x1de/0x6a0
[  119.007828]  kthread+0x114/0x150
[  119.008963]  ret_from_fork+0x22/0x30

Reported-by: Josef Grieb <josef.grieb@gmail.com>
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
v2: -EFAULT -> -EOWNERDEAD (Jens Axboe)

 fs/io_uring.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index b5aaa6e097f9..aa8aa4945a06 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1062,7 +1062,7 @@ static void io_sq_thread_drop_mm_files(void)
 	}
 }
 
-static void __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
+static int __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
 {
 	if (!current->files) {
 		struct files_struct *files;
@@ -1073,7 +1073,7 @@ static void __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
 		files = ctx->sqo_task->files;
 		if (!files) {
 			task_unlock(ctx->sqo_task);
-			return;
+			return -EOWNERDEAD;
 		}
 		atomic_inc(&files->count);
 		get_nsproxy(ctx->sqo_task->nsproxy);
@@ -1087,6 +1087,7 @@ static void __io_sq_thread_acquire_files(struct io_ring_ctx *ctx)
 		current->thread_pid = thread_pid;
 		task_unlock(current);
 	}
+	return 0;
 }
 
 static int __io_sq_thread_acquire_mm(struct io_ring_ctx *ctx)
@@ -1118,15 +1119,19 @@ static int io_sq_thread_acquire_mm_files(struct io_ring_ctx *ctx,
 					 struct io_kiocb *req)
 {
 	const struct io_op_def *def = &io_op_defs[req->opcode];
+	int ret;
 
 	if (def->work_flags & IO_WQ_WORK_MM) {
-		int ret = __io_sq_thread_acquire_mm(ctx);
+		ret = __io_sq_thread_acquire_mm(ctx);
 		if (unlikely(ret))
 			return ret;
 	}
 
-	if (def->needs_file || (def->work_flags & IO_WQ_WORK_FILES))
-		__io_sq_thread_acquire_files(ctx);
+	if (def->needs_file || (def->work_flags & IO_WQ_WORK_FILES)) {
+		ret = __io_sq_thread_acquire_files(ctx);
+		if (unlikely(ret))
+			return ret;
+	}
 
 	return 0;
 }
@@ -2134,8 +2139,8 @@ static void __io_req_task_submit(struct io_kiocb *req)
 {
 	struct io_ring_ctx *ctx = req->ctx;
 
-	if (!__io_sq_thread_acquire_mm(ctx)) {
-		__io_sq_thread_acquire_files(ctx);
+	if (!__io_sq_thread_acquire_mm(ctx) &&
+	    !__io_sq_thread_acquire_files(ctx)) {
 		mutex_lock(&ctx->uring_lock);
 		__io_queue_sqe(req, NULL);
 		mutex_unlock(&ctx->uring_lock);
-- 
2.24.0

Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL

[Jens Axboe]

On 11/8/20 5:55 AM, Pavel Begunkov wrote:
> SQPOLL task may find sqo_task->files == NULL and
> __io_sq_thread_acquire_files() would leave it unset, so following
> fget_many() and others try to dereference NULL and fault. Propagate
> an error files are missing.

Applied, thanks.

-- 
Jens Axboe