Re: [PATCH 3/3] io_uring: refactor io_sq_offload_create()

From: Al Viro <viro@zeniv.linux.org.uk>
To: Jens Axboe <axboe@kernel.dk>
Cc: Pavel Begunkov <asml.silence@gmail.com>,
	io-uring@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH 3/3] io_uring: refactor io_sq_offload_create()
Date: Fri, 23 Jul 2021 17:11:21 +0000	[thread overview]
Message-ID: <YPr4OaHv0iv0KTOc@zeniv-ca.linux.org.uk> (raw)
In-Reply-To: <8fb39022-ba21-2c1f-3df5-29be002014d8@kernel.dk>

On Fri, Jul 23, 2021 at 10:17:27AM -0600, Jens Axboe wrote:
> On 7/22/21 6:10 PM, Al Viro wrote:
> > On Thu, Jul 22, 2021 at 05:42:55PM -0600, Jens Axboe wrote:
> > 
> >>> So how can we possibly get there with tsk->files == NULL and what does it
> >>> have to do with files, anyway?
> >>
> >> It's not the clearest, but the files check is just to distinguish between
> >> exec vs normal cancel. For exec, we pass in files == NULL. It's not
> >> related to task->files being NULL or not, we explicitly pass NULL for
> >> exec.
> > 
> > Er...  So turn that argument into bool cancel_all, and pass false on exit and
> > true on exec? 
> 
> Yes
> 
> > While we are at it, what happens if you pass io_uring descriptor
> > to another process, close yours and then have the recepient close the one it
> > has gotten?  AFAICS, io_ring_ctx_wait_and_kill(ctx) will be called in context
> > of a process that has never done anything io_uring-related.  Can it end up
> > trying to resubmit some requests?> 
> > I rather hope it can't happen, but I don't see what would prevent it...
> 
> No, the pending request would either have gone to a created thread of
> the original task on submission, or it would be sitting in a
> ready-to-retry state. The retry would attempt to queue to original task,
> and either succeed (if still alive) or get failed with -ECANCELED. Any
> given request is tied to the original task.

Hmm...  Sure, you'll be pushing it to the same io_wqe it went through originally,
but you are still in context of io_uring_release() caller, aren't you?

So you call io_wqe_wake_worker(), and it decides that all threads are busy,
but ->nr_workers is still below ->max_workers.  And proceeds to
	create_io_worker(wqe->wq, wqe, acct->index);
which will create a new io-worker thread, but do that in the thread group of
current, i.e. the caller of io_uring_release().  Looks like we'd get
an io-worker thread with the wrong parent...

What am I missing here?