Re: [PATCH 2/3] io_uring: fix io_rsrc_ref_quiesce races

[Hao Xu <haoxu@linux.alibaba.com>]

在 2021/2/20 上午4:45, Pavel Begunkov 写道:
> There are different types of races in io_rsrc_ref_quiesce()  between
> ->release() of percpu_refs and reinit_completion(), fix them by always
> resurrecting between iterations. BTW, clean the function up, because
> DRY.
> 
> Fixes: 0ce4a72632317 ("io_uring: don't hold uring_lock when calling io_run_task_work*")
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>   fs/io_uring.c | 46 +++++++++++++---------------------------------
>   1 file changed, 13 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 50d4dba08f82..38ed52065a29 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -7316,19 +7316,6 @@ static void io_sqe_rsrc_set_node(struct io_ring_ctx *ctx,
>   	percpu_ref_get(&rsrc_data->refs);
>   }
>   
> -static int io_sqe_rsrc_add_node(struct io_ring_ctx *ctx, struct fixed_rsrc_data *data)
> -{
> -	struct fixed_rsrc_ref_node *backup_node;
> -
> -	backup_node = alloc_fixed_rsrc_ref_node(ctx);
> -	if (!backup_node)
> -		return -ENOMEM;
> -	init_fixed_file_ref_node(ctx, backup_node);
> -	io_sqe_rsrc_set_node(ctx, data, backup_node);
> -
> -	return 0;
> -}
> -
>   static void io_sqe_rsrc_kill_node(struct io_ring_ctx *ctx, struct fixed_rsrc_data *data)
>   {
>   	struct fixed_rsrc_ref_node *ref_node = NULL;
> @@ -7347,36 +7334,29 @@ static int io_rsrc_ref_quiesce(struct fixed_rsrc_data *data,
>   {
>   	int ret;
>   
> -	io_sqe_rsrc_kill_node(ctx, data);
> -	percpu_ref_kill(&data->refs);
> -
> -	/* wait for all refs nodes to complete */
> -	flush_delayed_work(&ctx->rsrc_put_work);
>   	do {
> +		io_sqe_rsrc_kill_node(ctx, data);
> +		percpu_ref_kill(&data->refs);
> +		flush_delayed_work(&ctx->rsrc_put_work);
> +
>   		ret = wait_for_completion_interruptible(&data->done);
>   		if (!ret)
>   			break;
>   
> -		ret = io_sqe_rsrc_add_node(ctx, data);
> -		if (ret < 0)
> -			break;
> -		/*
> -		 * There is small possibility that data->done is already completed
> -		 * So reinit it here
> -		 */
> +		percpu_ref_resurrect(&data->refs);
I've thought about this, if we resurrect data->refs, then we can't
avoid parallel files unregister by percpu_refs_is_dying.
> +		io_sqe_rsrc_set_node(ctx, data, backup_node);
>   		reinit_completion(&data->done);
>   		mutex_unlock(&ctx->uring_lock);
>   		ret = io_run_task_work_sig();
>   		mutex_lock(&ctx->uring_lock);
> -		io_sqe_rsrc_kill_node(ctx, data);
> -	} while (ret >= 0);
>   
> -	if (ret < 0) {
> -		percpu_ref_resurrect(&data->refs);
> -		reinit_completion(&data->done);
> -		io_sqe_rsrc_set_node(ctx, data, backup_node);
> -		return ret;
> -	}
> +		if (ret < 0)
> +			return ret;
> +		backup_node = alloc_fixed_rsrc_ref_node(ctx);
> +		if (!backup_node)
> +			return -ENOMEM;
Should we resurrect data->refs and reinit completion before
signal or error return?
> +		init_fixed_file_ref_node(ctx, backup_node);
> +	} while (1);
>   
>   	destroy_fixed_rsrc_ref_node(backup_node);
>   	return 0;
>