Re: [PATCH 1/2] io_uring: fix file leaks around io_fixed_fd_install()

[Jens Axboe <axboe@kernel.dk>]

On 5/27/22 10:53 AM, Xiaoguang Wang wrote:
> io_fixed_fd_install() may fail for many reasons, such as short of
> free fixed file bitmap, memory allocation failures, etc. When these
> errors happen, current code forgets to fput(file) correspondingly.
> 
> This patch will fix resource leaks around io_fixed_fd_install(),
> meanwhile io_fixed_fd_install() and io_install_fixed_file() are
> basically similar, fold them into one function.
> 
> Signed-off-by: Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>
> ---
>  fs/io_uring.c | 77 ++++++++++++++++++++++++++---------------------------------
>  1 file changed, 34 insertions(+), 43 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index d50bbf8de4fb..ff50e5f1753d 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1364,8 +1364,8 @@ static void io_req_task_queue(struct io_kiocb *req);
>  static void __io_submit_flush_completions(struct io_ring_ctx *ctx);
>  static int io_req_prep_async(struct io_kiocb *req);
>  
> -static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
> -				 unsigned int issue_flags, u32 slot_index);
> +static int io_install_fixed_file(struct io_kiocb *req, unsigned int issue_flags,
> +				 struct file *file, u32 slot);
>  static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags);
>  
>  static enum hrtimer_restart io_link_timeout_fn(struct hrtimer *timer);
> @@ -5438,36 +5438,6 @@ static int io_file_bitmap_get(struct io_ring_ctx *ctx)
>  	return -ENFILE;
>  }
>  
> -static int io_fixed_fd_install(struct io_kiocb *req, unsigned int issue_flags,
> -			       struct file *file, unsigned int file_slot)
> -{
> -	bool alloc_slot = file_slot == IORING_FILE_INDEX_ALLOC;
> -	struct io_ring_ctx *ctx = req->ctx;
> -	int ret;
> -
> -	if (alloc_slot) {
> -		io_ring_submit_lock(ctx, issue_flags);
> -		ret = io_file_bitmap_get(ctx);
> -		if (unlikely(ret < 0)) {
> -			io_ring_submit_unlock(ctx, issue_flags);
> -			return ret;
> -		}
> -
> -		file_slot = ret;
> -	} else {
> -		file_slot--;
> -	}
> -
> -	ret = io_install_fixed_file(req, file, issue_flags, file_slot);
> -	if (alloc_slot) {
> -		io_ring_submit_unlock(ctx, issue_flags);
> -		if (!ret)
> -			return file_slot;
> -	}
> -
> -	return ret;
> -}
> -
>  static int io_openat2(struct io_kiocb *req, unsigned int issue_flags)
>  {
>  	struct open_flags op;
> @@ -5520,11 +5490,14 @@ static int io_openat2(struct io_kiocb *req, unsigned int issue_flags)
>  		file->f_flags &= ~O_NONBLOCK;
>  	fsnotify_open(file);
>  
> -	if (!fixed)
> +	if (!fixed) {
>  		fd_install(ret, file);
> -	else
> -		ret = io_fixed_fd_install(req, issue_flags, file,
> -						req->open.file_slot);
> +	} else {
> +		ret = io_install_fixed_file(req, issue_flags, file,
> +					    req->open.file_slot);
> +		if (ret < 0)
> +			fput(file);
> +	}
>  err:
>  	putname(req->open.filename);
>  	req->flags &= ~REQ_F_NEED_CLEANUP;
> @@ -6603,8 +6576,10 @@ static int io_accept(struct io_kiocb *req, unsigned int issue_flags)
>  		fd_install(fd, file);
>  		ret = fd;
>  	} else {
> -		ret = io_fixed_fd_install(req, issue_flags, file,
> -						accept->file_slot);
> +		ret = io_install_fixed_file(req, issue_flags, file,
> +					    accept->file_slot);
> +		if (ret < 0)
> +			fput(file);
>  	}
>  
>  	if (!(req->flags & REQ_F_APOLL_MULTISHOT)) {
> @@ -6676,8 +6651,10 @@ static int io_socket(struct io_kiocb *req, unsigned int issue_flags)
>  		fd_install(fd, file);
>  		ret = fd;
>  	} else {
> -		ret = io_fixed_fd_install(req, issue_flags, file,
> +		ret = io_install_fixed_file(req, issue_flags, file,
>  					    sock->file_slot);
> +		if (ret < 0)
> +			fput(file);
>  	}
>  	__io_req_complete(req, issue_flags, ret, 0);
>  	return 0;
> @@ -10130,15 +10107,27 @@ static int io_queue_rsrc_removal(struct io_rsrc_data *data, unsigned idx,
>  	return 0;
>  }
>  
> -static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
> -				 unsigned int issue_flags, u32 slot_index)
> +static int io_install_fixed_file(struct io_kiocb *req, unsigned int issue_flags,
> +				 struct file *file, u32 slot)
>  {
>  	struct io_ring_ctx *ctx = req->ctx;
>  	bool needs_switch = false;
>  	struct io_fixed_file *file_slot;
>  	int ret = -EBADF;
> +	bool alloc_slot = slot == IORING_FILE_INDEX_ALLOC;
> +	int slot_index;
>  
>  	io_ring_submit_lock(ctx, issue_flags);
> +	if (alloc_slot) {
> +		slot_index = io_file_bitmap_get(ctx);
> +		if (unlikely(slot_index < 0)) {
> +			io_ring_submit_unlock(ctx, issue_flags);
> +			return slot_index;
> +		}
> +	} else {
> +		slot_index = slot - 1;
> +	}
> +
>  	if (file->f_op == &io_uring_fops)
>  		goto err;
>  	ret = -ENXIO;
> @@ -10178,8 +10167,10 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
>  	if (needs_switch)
>  		io_rsrc_node_switch(ctx, ctx->file_data);
>  	io_ring_submit_unlock(ctx, issue_flags);
> -	if (ret)
> -		fput(file);
> +	if (alloc_slot) {
> +		if (!ret)
> +			return slot_index;
> +	}
>  	return ret;
>  }
>  


-- 
Jens Axboe