Re: [PATCH 1/6] io_uring: enhance flush completion logic

[Pavel Begunkov <asml.silence@gmail.com>]

On 9/3/21 12:00 PM, Hao Xu wrote:
> Though currently refcount of a req is always one when we flush inline

It can be refcounted and != 1. E.g. poll requests, or consider
that tw also flushes, and you may have a read that goes to apoll
and then get tw resubmitted from io_async_task_func(). And other
cases.

> completions, but still a chance there will be exception in the future.
> Enhance the flush logic to make sure we maintain compl_nr correctly.

See below

> 
> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
> ---
> 
> we need to either removing the if check to claim clearly that the req's
> refcount is 1 or adding this patch's logic.
> 
>  fs/io_uring.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 2bde732a1183..c48d43207f57 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -2291,7 +2291,7 @@ static void io_submit_flush_completions(struct io_ring_ctx *ctx)
>  	__must_hold(&ctx->uring_lock)
>  {
>  	struct io_submit_state *state = &ctx->submit_state;
> -	int i, nr = state->compl_nr;
> +	int i, nr = state->compl_nr, remain = 0;
>  	struct req_batch rb;
>  
>  	spin_lock(&ctx->completion_lock);
> @@ -2311,10 +2311,12 @@ static void io_submit_flush_completions(struct io_ring_ctx *ctx)
>  
>  		if (req_ref_put_and_test(req))
>  			io_req_free_batch(&rb, req, &ctx->submit_state);
> +		else
> +			state->compl_reqs[remain++] = state->compl_reqs[i];

Our ref is dropped, and something else holds another reference. That
"something else" is responsible to free the request once it put the last
reference. This chunk would make the following io_submit_flush_completions()
to underflow refcount and double free.

>  	}
>  
>  	io_req_free_batch_finish(ctx, &rb);
> -	state->compl_nr = 0;
> +	state->compl_nr = remain;
>  }
>  
>  /*
> 

-- 
Pavel Begunkov