Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 11 Aug 2020 16:00:10 +0200
From:   Stefano Garzarella <sgarzare@redhat.com>
To:     Jens Axboe <axboe@kernel.dk>
Cc:     syzbot <syzbot+996f91b6ec3812c48042@syzkaller.appspotmail.com>,
        io-uring@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com,
        viro@zeniv.linux.org.uk
Subject: Re: possible deadlock in __io_queue_deferred
Message-ID: <20200811140010.gigc2amchytqmrkk@steredhat>
References: <00000000000035fdf505ac87b7f9@google.com>
 <76cc7c43-2ebb-180d-c2c8-912972a3f258@kernel.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <76cc7c43-2ebb-180d-c2c8-912972a3f258@kernel.dk>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Aug 10, 2020 at 09:55:17AM -0600, Jens Axboe wrote:
> On 8/10/20 9:36 AM, syzbot wrote:
> > Hello,
> > 
> > syzbot found the following issue on:
> > 
> > HEAD commit:    449dc8c9 Merge tag 'for-v5.9' of git://git.kernel.org/pub/..
> > git tree:       upstream
> > console output: https://syzkaller.appspot.com/x/log.txt?x=14d41e02900000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=9d25235bf0162fbc
> > dashboard link: https://syzkaller.appspot.com/bug?extid=996f91b6ec3812c48042
> > compiler:       clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
> > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=133c9006900000
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1191cb1a900000
> > 
> > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > Reported-by: syzbot+996f91b6ec3812c48042@syzkaller.appspotmail.com
> 
> Thanks, the below should fix this one.

Yeah, it seems right to me, since only __io_queue_deferred() (invoked by
io_commit_cqring()) can be called with 'completion_lock' held.

Just out of curiosity, while exploring the code I noticed that we call
io_commit_cqring() always with the 'completion_lock' held, except in the
io_poll_* functions.

That's because then there can't be any concurrency?

Thanks,
Stefano

> 
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 443eecdfeda9..f9be665d1c5e 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -898,6 +898,7 @@ static void io_put_req(struct io_kiocb *req);
>  static void io_double_put_req(struct io_kiocb *req);
>  static void __io_double_put_req(struct io_kiocb *req);
>  static struct io_kiocb *io_prep_linked_timeout(struct io_kiocb *req);
> +static void __io_queue_linked_timeout(struct io_kiocb *req);
>  static void io_queue_linked_timeout(struct io_kiocb *req);
>  static int __io_sqe_files_update(struct io_ring_ctx *ctx,
>  				 struct io_uring_files_update *ip,
> @@ -1179,7 +1180,7 @@ static void io_prep_async_link(struct io_kiocb *req)
>  			io_prep_async_work(cur);
>  }
>  
> -static void __io_queue_async_work(struct io_kiocb *req)
> +static struct io_kiocb *__io_queue_async_work(struct io_kiocb *req)
>  {
>  	struct io_ring_ctx *ctx = req->ctx;
>  	struct io_kiocb *link = io_prep_linked_timeout(req);
> @@ -1187,16 +1188,19 @@ static void __io_queue_async_work(struct io_kiocb *req)
>  	trace_io_uring_queue_async_work(ctx, io_wq_is_hashed(&req->work), req,
>  					&req->work, req->flags);
>  	io_wq_enqueue(ctx->io_wq, &req->work);
> -
> -	if (link)
> -		io_queue_linked_timeout(link);
> +	return link;
>  }
>  
>  static void io_queue_async_work(struct io_kiocb *req)
>  {
> +	struct io_kiocb *link;
> +
>  	/* init ->work of the whole link before punting */
>  	io_prep_async_link(req);
> -	__io_queue_async_work(req);
> +	link = __io_queue_async_work(req);
> +
> +	if (link)
> +		io_queue_linked_timeout(link);
>  }
>  
>  static void io_kill_timeout(struct io_kiocb *req)
> @@ -1229,12 +1233,19 @@ static void __io_queue_deferred(struct io_ring_ctx *ctx)
>  	do {
>  		struct io_defer_entry *de = list_first_entry(&ctx->defer_list,
>  						struct io_defer_entry, list);
> +		struct io_kiocb *link;
>  
>  		if (req_need_defer(de->req, de->seq))
>  			break;
>  		list_del_init(&de->list);
>  		/* punt-init is done before queueing for defer */
> -		__io_queue_async_work(de->req);
> +		link = __io_queue_async_work(de->req);
> +		if (link) {
> +			__io_queue_linked_timeout(link);
> +			/* drop submission reference */
> +			link->flags |= REQ_F_COMP_LOCKED;
> +			io_put_req(link);
> +		}
>  		kfree(de);
>  	} while (!list_empty(&ctx->defer_list));
>  }
> @@ -5945,15 +5956,12 @@ static enum hrtimer_restart io_link_timeout_fn(struct hrtimer *timer)
>  	return HRTIMER_NORESTART;
>  }
>  
> -static void io_queue_linked_timeout(struct io_kiocb *req)
> +static void __io_queue_linked_timeout(struct io_kiocb *req)
>  {
> -	struct io_ring_ctx *ctx = req->ctx;
> -
>  	/*
>  	 * If the list is now empty, then our linked request finished before
>  	 * we got a chance to setup the timer
>  	 */
> -	spin_lock_irq(&ctx->completion_lock);
>  	if (!list_empty(&req->link_list)) {
>  		struct io_timeout_data *data = &req->io->timeout;
>  
> @@ -5961,6 +5969,14 @@ static void io_queue_linked_timeout(struct io_kiocb *req)
>  		hrtimer_start(&data->timer, timespec64_to_ktime(data->ts),
>  				data->mode);
>  	}
> +}
> +
> +static void io_queue_linked_timeout(struct io_kiocb *req)
> +{
> +	struct io_ring_ctx *ctx = req->ctx;
> +
> +	spin_lock_irq(&ctx->completion_lock);
> +	__io_queue_linked_timeout(req);
>  	spin_unlock_irq(&ctx->completion_lock);
>  
>  	/* drop submission reference */
> 
> -- 
> Jens Axboe
>