[PATCH] io_uring: cancel all requests on task exit

[PATCH] io_uring: cancel all requests on task exit

[Jens Axboe]

We used to have task exit tied to canceling files_struct ownership, but we
really should just simplify this and cancel any request that the task has
pending when it exits. Instead of handling files ownership specially, we
do the same regardless of request type.

This can be further simplified in the next major kernel release, unifying
how we cancel across exec and exit.

Cc: stable@vger.kernel.org # 5.9+
Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 383ff6ed3734..1190296fc95f 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -9029,7 +9029,7 @@ static void io_uring_remove_task_files(struct io_uring_task *tctx)
 		io_uring_del_task_file(file);
 }
 
-void __io_uring_files_cancel(struct files_struct *files)
+static void __io_uring_files_cancel(void)
 {
 	struct io_uring_task *tctx = current->io_uring;
 	struct file *file;
@@ -9038,11 +9038,10 @@ void __io_uring_files_cancel(struct files_struct *files)
 	/* make sure overflow events are dropped */
 	atomic_inc(&tctx->in_idle);
 	xa_for_each(&tctx->xa, index, file)
-		io_uring_cancel_task_requests(file->private_data, files);
+		io_uring_cancel_task_requests(file->private_data, NULL);
 	atomic_dec(&tctx->in_idle);
 
-	if (files)
-		io_uring_remove_task_files(tctx);
+	io_uring_remove_task_files(tctx);
 }
 
 static s64 tctx_inflight(struct io_uring_task *tctx)
@@ -9087,14 +9086,14 @@ void __io_uring_task_cancel(void)
 
 	/* trigger io_disable_sqo_submit() */
 	if (tctx->sqpoll)
-		__io_uring_files_cancel(NULL);
+		__io_uring_files_cancel();
 
 	do {
 		/* read completions before cancelations */
 		inflight = tctx_inflight(tctx);
 		if (!inflight)
 			break;
-		__io_uring_files_cancel(NULL);
+		__io_uring_files_cancel();
 
 		prepare_to_wait(&tctx->wait, &wait, TASK_UNINTERRUPTIBLE);
 
diff --git a/include/linux/io_uring.h b/include/linux/io_uring.h
index 35b2d845704d..378662dcfa02 100644
--- a/include/linux/io_uring.h
+++ b/include/linux/io_uring.h
@@ -37,7 +37,6 @@ struct io_uring_task {
 #if defined(CONFIG_IO_URING)
 struct sock *io_uring_get_socket(struct file *file);
 void __io_uring_task_cancel(void);
-void __io_uring_files_cancel(struct files_struct *files);
 void __io_uring_free(struct task_struct *tsk);
 
 static inline void io_uring_task_cancel(void)
@@ -45,11 +44,6 @@ static inline void io_uring_task_cancel(void)
 	if (current->io_uring && !xa_empty(&current->io_uring->xa))
 		__io_uring_task_cancel();
 }
-static inline void io_uring_files_cancel(struct files_struct *files)
-{
-	if (current->io_uring && !xa_empty(&current->io_uring->xa))
-		__io_uring_files_cancel(files);
-}
 static inline void io_uring_free(struct task_struct *tsk)
 {
 	if (tsk->io_uring)
diff --git a/kernel/exit.c b/kernel/exit.c
index 04029e35e69a..81523e8d8893 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -777,7 +777,7 @@ void __noreturn do_exit(long code)
 		schedule();
 	}
 
-	io_uring_files_cancel(tsk->files);
+	io_uring_task_cancel();
 	exit_signals(tsk);  /* sets PF_EXITING */
 
 	/* sync mm's RSS info before statistics gathering */
-- 
Jens Axboe

Re: [PATCH] io_uring: cancel all requests on task exit

[Pavel Begunkov]

On 17/01/2021 04:04, Jens Axboe wrote:
> We used to have task exit tied to canceling files_struct ownership, but we
> really should just simplify this and cancel any request that the task has
> pending when it exits. Instead of handling files ownership specially, we
> do the same regardless of request type.
> 
> This can be further simplified in the next major kernel release, unifying
> how we cancel across exec and exit.

Looks good in general. See a comment below, but otherwise
Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>

btw, I wonder if we can incite syzbot to try to break it.

> 
> Cc: stable@vger.kernel.org # 5.9+
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> ---
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 383ff6ed3734..1190296fc95f 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -9029,7 +9029,7 @@ static void io_uring_remove_task_files(struct io_uring_task *tctx)
>  		io_uring_del_task_file(file);
>  }
>  
> -void __io_uring_files_cancel(struct files_struct *files)
> +static void __io_uring_files_cancel(void)
>  {
>  	struct io_uring_task *tctx = current->io_uring;
>  	struct file *file;
> @@ -9038,11 +9038,10 @@ void __io_uring_files_cancel(struct files_struct *files)
>  	/* make sure overflow events are dropped */
>  	atomic_inc(&tctx->in_idle);
>  	xa_for_each(&tctx->xa, index, file)
> -		io_uring_cancel_task_requests(file->private_data, files);
> +		io_uring_cancel_task_requests(file->private_data, NULL);
>  	atomic_dec(&tctx->in_idle);
>  
> -	if (files)
> -		io_uring_remove_task_files(tctx);
> +	io_uring_remove_task_files(tctx);

This restricts cancellations to only one iteration. Just delete it,
__io_uring_task_cancel() calls it already.

-- 
Pavel Begunkov

Re: [PATCH] io_uring: cancel all requests on task exit

[Josef]

thanks :) I ran several tests, I can confirm this fixed the issue.
Tested-by: Josef Grieb <josef.grieb@gmail.com>

On Sun, 17 Jan 2021 at 12:00, Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> On 17/01/2021 04:04, Jens Axboe wrote:
> > We used to have task exit tied to canceling files_struct ownership, but we
> > really should just simplify this and cancel any request that the task has
> > pending when it exits. Instead of handling files ownership specially, we
> > do the same regardless of request type.
> >
> > This can be further simplified in the next major kernel release, unifying
> > how we cancel across exec and exit.
>
> Looks good in general. See a comment below, but otherwise
> Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>
>
> btw, I wonder if we can incite syzbot to try to break it.
>
> >
> > Cc: stable@vger.kernel.org # 5.9+
> > Signed-off-by: Jens Axboe <axboe@kernel.dk>
> >
> > ---
> >
> > diff --git a/fs/io_uring.c b/fs/io_uring.c
> > index 383ff6ed3734..1190296fc95f 100644
> > --- a/fs/io_uring.c
> > +++ b/fs/io_uring.c
> > @@ -9029,7 +9029,7 @@ static void io_uring_remove_task_files(struct io_uring_task *tctx)
> >               io_uring_del_task_file(file);
> >  }
> >
> > -void __io_uring_files_cancel(struct files_struct *files)
> > +static void __io_uring_files_cancel(void)
> >  {
> >       struct io_uring_task *tctx = current->io_uring;
> >       struct file *file;
> > @@ -9038,11 +9038,10 @@ void __io_uring_files_cancel(struct files_struct *files)
> >       /* make sure overflow events are dropped */
> >       atomic_inc(&tctx->in_idle);
> >       xa_for_each(&tctx->xa, index, file)
> > -             io_uring_cancel_task_requests(file->private_data, files);
> > +             io_uring_cancel_task_requests(file->private_data, NULL);
> >       atomic_dec(&tctx->in_idle);
> >
> > -     if (files)
> > -             io_uring_remove_task_files(tctx);
> > +     io_uring_remove_task_files(tctx);
>
> This restricts cancellations to only one iteration. Just delete it,
> __io_uring_task_cancel() calls it already.
>
> --
> Pavel Begunkov



-- 
Josef

Re: [PATCH] io_uring: cancel all requests on task exit

[Jens Axboe]

On 1/17/21 5:51 AM, Josef wrote:
> thanks :) I ran several tests, I can confirm this fixed the issue.
> Tested-by: Josef Grieb <josef.grieb@gmail.com>

Awesome, thanks!

-- 
Jens Axboe

Re: [PATCH] io_uring: cancel all requests on task exit

[Jens Axboe]

On 1/17/21 3:52 AM, Pavel Begunkov wrote:
> On 17/01/2021 04:04, Jens Axboe wrote:
>> We used to have task exit tied to canceling files_struct ownership, but we
>> really should just simplify this and cancel any request that the task has
>> pending when it exits. Instead of handling files ownership specially, we
>> do the same regardless of request type.
>>
>> This can be further simplified in the next major kernel release, unifying
>> how we cancel across exec and exit.
> 
> Looks good in general. See a comment below, but otherwise
> Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>
> 
> btw, I wonder if we can incite syzbot to try to break it.
> 
>>
>> Cc: stable@vger.kernel.org # 5.9+
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>
>> ---
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 383ff6ed3734..1190296fc95f 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -9029,7 +9029,7 @@ static void io_uring_remove_task_files(struct io_uring_task *tctx)
>>  		io_uring_del_task_file(file);
>>  }
>>  
>> -void __io_uring_files_cancel(struct files_struct *files)
>> +static void __io_uring_files_cancel(void)
>>  {
>>  	struct io_uring_task *tctx = current->io_uring;
>>  	struct file *file;
>> @@ -9038,11 +9038,10 @@ void __io_uring_files_cancel(struct files_struct *files)
>>  	/* make sure overflow events are dropped */
>>  	atomic_inc(&tctx->in_idle);
>>  	xa_for_each(&tctx->xa, index, file)
>> -		io_uring_cancel_task_requests(file->private_data, files);
>> +		io_uring_cancel_task_requests(file->private_data, NULL);
>>  	atomic_dec(&tctx->in_idle);
>>  
>> -	if (files)
>> -		io_uring_remove_task_files(tctx);
>> +	io_uring_remove_task_files(tctx);
> 
> This restricts cancellations to only one iteration. Just delete it,
> __io_uring_task_cancel() calls it already.

Ah indeed, makes it even better. I removed it, thanks.

-- 
Jens Axboe