[PATCH 1/3] io_uring: Fix use of XArray in __io_uring_files_cancel

[PATCH 1/3] io_uring: Fix use of XArray in __io_uring_files_cancel

[Matthew Wilcox (Oracle)]

We have to drop the lock during each iteration, so there's no advantage
to using the advanced API.  Convert this to a standard xa_for_each() loop.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/io_uring.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 299c530c66f9..2978cc78538a 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -8665,28 +8665,19 @@ static void io_uring_attempt_task_drop(struct file *file, bool exiting)
 void __io_uring_files_cancel(struct files_struct *files)
 {
 	struct io_uring_task *tctx = current->io_uring;
-	XA_STATE(xas, &tctx->xa, 0);
+	struct file *file;
+	unsigned long index;
 
 	/* make sure overflow events are dropped */
 	tctx->in_idle = true;
 
-	do {
-		struct io_ring_ctx *ctx;
-		struct file *file;
-
-		xas_lock(&xas);
-		file = xas_next_entry(&xas, ULONG_MAX);
-		xas_unlock(&xas);
-
-		if (!file)
-			break;
-
-		ctx = file->private_data;
+	xa_for_each(&tctx->xa, index, file) {
+		struct io_ring_ctx *ctx = file->private_data;
 
 		io_uring_cancel_task_requests(ctx, files);
 		if (files)
 			io_uring_del_task_file(file);
-	} while (1);
+	}
 }
 
 static inline bool io_uring_task_idle(struct io_uring_task *tctx)
-- 
2.28.0

[PATCH 2/3] io_uring: Fix XArray usage in io_uring_add_task_file

[Matthew Wilcox (Oracle)]

The xas_store() wasn't paired with an xas_nomem() loop, so if it couldn't
allocate memory using GFP_NOWAIT, it would leak the reference to the file
descriptor.  Also the node pointed to by the xas could be freed between
the call to xas_load() under the rcu_read_lock() and the acquisition of
the xa_lock.

It's easier to just use the normal xa_load/xa_store interface here.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/io_uring.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 2978cc78538a..bcef6210bf67 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -8586,27 +8586,24 @@ static void io_uring_cancel_task_requests(struct io_ring_ctx *ctx,
  */
 static int io_uring_add_task_file(struct file *file)
 {
-	if (unlikely(!current->io_uring)) {
+	struct io_uring_task *cur_uring = current->io_uring;
+
+	if (unlikely(!cur_uring)) {
 		int ret;
 
 		ret = io_uring_alloc_task_context(current);
 		if (unlikely(ret))
 			return ret;
 	}
-	if (current->io_uring->last != file) {
-		XA_STATE(xas, &current->io_uring->xa, (unsigned long) file);
-		void *old;
+	if (cur_uring->last != file) {
+		void *old = xa_load(&cur_uring->xa, (unsigned long)file);
 
-		rcu_read_lock();
-		old = xas_load(&xas);
-		if (old != file) {
+		if (!old) {
 			get_file(file);
-			xas_lock(&xas);
-			xas_store(&xas, file);
-			xas_unlock(&xas);
+			xa_store(&cur_uring->xa, (unsigned long)file, file,
+					GFP_KERNEL);
 		}
-		rcu_read_unlock();
-		current->io_uring->last = file;
+		cur_uring->last = file;
 	}
 
 	return 0;
-- 
2.28.0

[PATCH 3/3] io_uring: Convert advanced XArray uses to the normal API

[Matthew Wilcox (Oracle)]

There are no bugs here that I've spotted, it's just easier to use the
normal API and there are no performance advantages to using the more
verbose advanced API.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/io_uring.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index bcef6210bf67..1a894df526da 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -8615,27 +8615,17 @@ static int io_uring_add_task_file(struct file *file)
 static void io_uring_del_task_file(struct file *file)
 {
 	struct io_uring_task *tctx = current->io_uring;
-	XA_STATE(xas, &tctx->xa, (unsigned long) file);
 
 	if (tctx->last == file)
 		tctx->last = NULL;
-
-	xas_lock(&xas);
-	file = xas_store(&xas, NULL);
-	xas_unlock(&xas);
-
+	file = xa_erase(&tctx->xa, (unsigned long)file);
 	if (file)
 		fput(file);
 }
 
 static void __io_uring_attempt_task_drop(struct file *file)
 {
-	XA_STATE(xas, &current->io_uring->xa, (unsigned long) file);
-	struct file *old;
-
-	rcu_read_lock();
-	old = xas_load(&xas);
-	rcu_read_unlock();
+	struct file *old = xa_load(&current->io_uring->xa, (unsigned long)file);
 
 	if (old == file)
 		io_uring_del_task_file(file);
-- 
2.28.0

Re: [PATCH 1/3] io_uring: Fix use of XArray in __io_uring_files_cancel

[Pavel Begunkov]

On 09/10/2020 15:49, Matthew Wilcox (Oracle) wrote:
> We have to drop the lock during each iteration, so there's no advantage
> to using the advanced API.  Convert this to a standard xa_for_each() loop.

LGTM, but would be better to add

Reported-by: syzbot+27c12725d8ff0bfe1a13@syzkaller.appspotmail.com

> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/io_uring.c | 19 +++++--------------
>  1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 299c530c66f9..2978cc78538a 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -8665,28 +8665,19 @@ static void io_uring_attempt_task_drop(struct file *file, bool exiting)
>  void __io_uring_files_cancel(struct files_struct *files)
>  {
>  	struct io_uring_task *tctx = current->io_uring;
> -	XA_STATE(xas, &tctx->xa, 0);
> +	struct file *file;
> +	unsigned long index;
>  
>  	/* make sure overflow events are dropped */
>  	tctx->in_idle = true;
>  
> -	do {
> -		struct io_ring_ctx *ctx;
> -		struct file *file;
> -
> -		xas_lock(&xas);
> -		file = xas_next_entry(&xas, ULONG_MAX);
> -		xas_unlock(&xas);
> -
> -		if (!file)
> -			break;
> -
> -		ctx = file->private_data;
> +	xa_for_each(&tctx->xa, index, file) {
> +		struct io_ring_ctx *ctx = file->private_data;
>  
>  		io_uring_cancel_task_requests(ctx, files);
>  		if (files)
>  			io_uring_del_task_file(file);
> -	} while (1);
> +	}
>  }
>  
>  static inline bool io_uring_task_idle(struct io_uring_task *tctx)
> 

-- 
Pavel Begunkov

Re: [PATCH 2/3] io_uring: Fix XArray usage in io_uring_add_task_file

[Jens Axboe]

On 10/9/20 6:49 AM, Matthew Wilcox (Oracle) wrote:
> The xas_store() wasn't paired with an xas_nomem() loop, so if it couldn't
> allocate memory using GFP_NOWAIT, it would leak the reference to the file
> descriptor.  Also the node pointed to by the xas could be freed between
> the call to xas_load() under the rcu_read_lock() and the acquisition of
> the xa_lock.
> 
> It's easier to just use the normal xa_load/xa_store interface here.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/io_uring.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 2978cc78538a..bcef6210bf67 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -8586,27 +8586,24 @@ static void io_uring_cancel_task_requests(struct io_ring_ctx *ctx,
>   */
>  static int io_uring_add_task_file(struct file *file)
>  {
> -	if (unlikely(!current->io_uring)) {
> +	struct io_uring_task *cur_uring = current->io_uring;
> +
> +	if (unlikely(!cur_uring)) {
>  		int ret;
>  
>  		ret = io_uring_alloc_task_context(current);
>  		if (unlikely(ret))
>  			return ret;
>  	}

I think this is missing a:

	cur_uring = current->io_uring;

after the successful io_uring_alloc_task(). I'll also rename it to tctx
like what is used in other spots.

Apart from that, series looks good to me, thanks Matthew!

-- 
Jens Axboe

Re: [PATCH 2/3] io_uring: Fix XArray usage in io_uring_add_task_file

[Matthew Wilcox]

On Fri, Oct 09, 2020 at 08:57:55AM -0600, Jens Axboe wrote:
> > +	if (unlikely(!cur_uring)) {
> >  		int ret;
> >  
> >  		ret = io_uring_alloc_task_context(current);
> >  		if (unlikely(ret))
> >  			return ret;
> >  	}
> 
> I think this is missing a:
> 
> 	cur_uring = current->io_uring;
> 
> after the successful io_uring_alloc_task(). I'll also rename it to tctx
> like what is used in other spots.

Quite right!  I should have woken up a little bit more before writing code.

> Apart from that, series looks good to me, thanks Matthew!

NP.  At some point, I'd like to understand a bit better how you came
to write the code the way you did, so I can improve the documentation.
Maybe I just need to strengthen the warnings to stay away from the
advanced API unless you absolutely need it.