public inbox for [email protected]
 help / color / mirror / Atom feed
* [PATCH 5.11] io_uring: NULL files dereference by SQPOLL
@ 2020-11-07 21:16 Pavel Begunkov
  2020-11-07 21:18 ` Pavel Begunkov
  2020-11-07 22:30 ` Jens Axboe
  0 siblings, 2 replies; 16+ messages in thread
From: Pavel Begunkov @ 2020-11-07 21:16 UTC (permalink / raw)
  To: Jens Axboe, io-uring; +Cc: Josef Grieb

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 <[email protected]>
Signed-off-by: Pavel Begunkov <[email protected]>
---
 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


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

* Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL
  2020-11-07 21:16 Pavel Begunkov
@ 2020-11-07 21:18 ` Pavel Begunkov
  2020-11-07 21:54   ` Pavel Begunkov
  2020-11-07 22:30 ` Jens Axboe
  1 sibling, 1 reply; 16+ messages in thread
From: Pavel Begunkov @ 2020-11-07 21:18 UTC (permalink / raw)
  To: Jens Axboe, io-uring, Josef Grieb

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 <[email protected]>
> Signed-off-by: Pavel Begunkov <[email protected]>
> ---
>  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

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

* Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL
  2020-11-07 21:18 ` Pavel Begunkov
@ 2020-11-07 21:54   ` Pavel Begunkov
  2020-11-07 22:28     ` Jens Axboe
  0 siblings, 1 reply; 16+ messages in thread
From: Pavel Begunkov @ 2020-11-07 21:54 UTC (permalink / raw)
  To: Jens Axboe, io-uring, Josef Grieb

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 <[email protected]>
>> Signed-off-by: Pavel Begunkov <[email protected]>
>> ---
>>  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

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

* Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL
  2020-11-07 21:54   ` Pavel Begunkov
@ 2020-11-07 22:28     ` Jens Axboe
  2020-11-07 22:47       ` Pavel Begunkov
  0 siblings, 1 reply; 16+ messages in thread
From: Jens Axboe @ 2020-11-07 22:28 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring, Josef Grieb

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


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

* Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL
  2020-11-07 21:16 Pavel Begunkov
  2020-11-07 21:18 ` Pavel Begunkov
@ 2020-11-07 22:30 ` Jens Axboe
  2020-11-07 22:49   ` Pavel Begunkov
  1 sibling, 1 reply; 16+ messages in thread
From: Jens Axboe @ 2020-11-07 22:30 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring; +Cc: Josef Grieb

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 <[email protected]>
> Signed-off-by: Pavel Begunkov <[email protected]>
> ---
>  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


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

* Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL
  2020-11-07 22:28     ` Jens Axboe
@ 2020-11-07 22:47       ` Pavel Begunkov
  2020-11-07 23:18         ` Jens Axboe
  0 siblings, 1 reply; 16+ messages in thread
From: Pavel Begunkov @ 2020-11-07 22:47 UTC (permalink / raw)
  To: Jens Axboe, io-uring, Josef Grieb

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

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

* Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL
  2020-11-07 22:30 ` Jens Axboe
@ 2020-11-07 22:49   ` Pavel Begunkov
  2020-11-07 23:17     ` Jens Axboe
  0 siblings, 1 reply; 16+ messages in thread
From: Pavel Begunkov @ 2020-11-07 22:49 UTC (permalink / raw)
  To: Jens Axboe, io-uring; +Cc: Josef Grieb

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 <[email protected]>
>> Signed-off-by: Pavel Begunkov <[email protected]>
>> ---
>>  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

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

* Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL
  2020-11-07 22:49   ` Pavel Begunkov
@ 2020-11-07 23:17     ` Jens Axboe
  0 siblings, 0 replies; 16+ messages in thread
From: Jens Axboe @ 2020-11-07 23:17 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring; +Cc: Josef Grieb

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 <[email protected]>
>>> Signed-off-by: Pavel Begunkov <[email protected]>
>>> ---
>>>  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


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

* Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL
  2020-11-07 22:47       ` Pavel Begunkov
@ 2020-11-07 23:18         ` Jens Axboe
  2020-11-08  2:09           ` Josef
  2020-11-08 11:24           ` Pavel Begunkov
  0 siblings, 2 replies; 16+ messages in thread
From: Jens Axboe @ 2020-11-07 23:18 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring, Josef Grieb

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


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

* Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL
  2020-11-07 23:18         ` Jens Axboe
@ 2020-11-08  2:09           ` Josef
  2020-11-08  6:50             ` Josef
  2020-11-08 11:31             ` Pavel Begunkov
  2020-11-08 11:24           ` Pavel Begunkov
  1 sibling, 2 replies; 16+ messages in thread
From: Josef @ 2020-11-08  2:09 UTC (permalink / raw)
  To: Jens Axboe, io-uring

> 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

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

* Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL
  2020-11-08  2:09           ` Josef
@ 2020-11-08  6:50             ` Josef
  2020-11-08 11:39               ` Pavel Begunkov
  2020-11-08 11:31             ` Pavel Begunkov
  1 sibling, 1 reply; 16+ messages in thread
From: Josef @ 2020-11-08  6:50 UTC (permalink / raw)
  To: Jens Axboe, io-uring, Pavel Begunkov; +Cc: norman

> 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 <[email protected]> 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

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

* Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL
  2020-11-07 23:18         ` Jens Axboe
  2020-11-08  2:09           ` Josef
@ 2020-11-08 11:24           ` Pavel Begunkov
  1 sibling, 0 replies; 16+ messages in thread
From: Pavel Begunkov @ 2020-11-08 11:24 UTC (permalink / raw)
  To: Jens Axboe, io-uring, Josef Grieb

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

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

* Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL
  2020-11-08  2:09           ` Josef
  2020-11-08  6:50             ` Josef
@ 2020-11-08 11:31             ` Pavel Begunkov
  1 sibling, 0 replies; 16+ messages in thread
From: Pavel Begunkov @ 2020-11-08 11:31 UTC (permalink / raw)
  To: Josef, Jens Axboe, io-uring

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

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

* Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL
  2020-11-08  6:50             ` Josef
@ 2020-11-08 11:39               ` Pavel Begunkov
  0 siblings, 0 replies; 16+ messages in thread
From: Pavel Begunkov @ 2020-11-08 11:39 UTC (permalink / raw)
  To: Josef, Jens Axboe, io-uring; +Cc: norman

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

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

* [PATCH 5.11] io_uring: NULL files dereference by SQPOLL
@ 2020-11-08 12:55 Pavel Begunkov
  2020-11-09 14:21 ` Jens Axboe
  0 siblings, 1 reply; 16+ messages in thread
From: Pavel Begunkov @ 2020-11-08 12:55 UTC (permalink / raw)
  To: Jens Axboe, io-uring; +Cc: Josef Grieb

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 <[email protected]>
Signed-off-by: Pavel Begunkov <[email protected]>
---
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


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

* Re: [PATCH 5.11] io_uring: NULL files dereference by SQPOLL
  2020-11-08 12:55 [PATCH 5.11] io_uring: NULL files dereference by SQPOLL Pavel Begunkov
@ 2020-11-09 14:21 ` Jens Axboe
  0 siblings, 0 replies; 16+ messages in thread
From: Jens Axboe @ 2020-11-09 14:21 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring; +Cc: Josef Grieb

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


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

end of thread, other threads:[~2020-11-09 14:21 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-08 12:55 [PATCH 5.11] io_uring: NULL files dereference by SQPOLL Pavel Begunkov
2020-11-09 14:21 ` Jens Axboe
  -- strict thread matches above, loose matches on Subject: below --
2020-11-07 21:16 Pavel Begunkov
2020-11-07 21:18 ` Pavel Begunkov
2020-11-07 21:54   ` Pavel Begunkov
2020-11-07 22:28     ` Jens Axboe
2020-11-07 22:47       ` Pavel Begunkov
2020-11-07 23:18         ` Jens Axboe
2020-11-08  2:09           ` Josef
2020-11-08  6:50             ` Josef
2020-11-08 11:39               ` Pavel Begunkov
2020-11-08 11:31             ` Pavel Begunkov
2020-11-08 11:24           ` Pavel Begunkov
2020-11-07 22:30 ` Jens Axboe
2020-11-07 22:49   ` Pavel Begunkov
2020-11-07 23:17     ` Jens Axboe

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