Re: [GIT PULL] io_uring fixes for 5.12-rc2

[Jens Axboe <axboe@kernel.dk>]

On 3/5/21 4:03 PM, Linus Torvalds wrote:
> On Fri, Mar 5, 2021 at 1:58 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> But it pertains to the problem in general, which is how do we handle
>> when the original task sets up a ring and then goes and does
>> unshare(FILES) or whatever. It then submits a new request that just
>> happens to go async, which is oblivious to this fact. Same thing that
>> would happen in userspace if you created a thread pool and then did
>> unshare, and then had your existing threads handle work for you. Except
>> here it just kind of happens implicitly, and I can see how that would be
>> confusing or even problematic.
> 
> Honestly, I'd aim for a "keep a cred per request".  And if that means
> that then the async worker thread has to check that it matches the
> cred it already has, then so be it.

Agree, which is actually not that bad and can be done without having
io-wq deal with it. See below.

> Otherwise, you'll always have odd situations where "synchronous
> completion gets different results than something that was kicked off
> to an async thread".

Exactly, that was my main concern. It violates the principle of least
surprise, and I didn't like it.

> But I don't think this has anything to do with unshare() per se. I
> think this is a generic "hey, the process can change creds between
> ring creation - and thread creation - and submission of an io_ring
> command".
> 
> No? Or am I misunderstanding what you're thinking of?

You're not, but creds is just one part of it. But I think we're OK
saying "If you do unshare(CLONE_FILES) or CLONE_FS, then it's not
going to impact async offload for your io_uring". IOW, you really
should do that before setting up your ring(s).


commit 6169c4a517317ff729553b66d55957bf03912dc4
Author: Jens Axboe <axboe@kernel.dk>
Date:   Sat Mar 6 09:22:27 2021 -0700

    io-wq: always track creds for async issue
    
    If we go async with a request, grab the creds that the task currently has
    assigned and make sure that the async side switches to them. This is
    handled in the same way that we do for registered personalities.
    
    Signed-off-by: Jens Axboe <axboe@kernel.dk>

diff --git a/fs/io-wq.h b/fs/io-wq.h
index 5fbf7997149e..1ac2f3248088 100644
--- a/fs/io-wq.h
+++ b/fs/io-wq.h
@@ -79,8 +79,8 @@ static inline void wq_list_del(struct io_wq_work_list *list,
 
 struct io_wq_work {
 	struct io_wq_work_node list;
+	const struct cred *creds;
 	unsigned flags;
-	unsigned short personality;
 };
 
 static inline struct io_wq_work *wq_next_work(struct io_wq_work *work)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 92c25b5f1349..d51c6ba9268b 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1183,6 +1183,9 @@ static void io_prep_async_work(struct io_kiocb *req)
 	const struct io_op_def *def = &io_op_defs[req->opcode];
 	struct io_ring_ctx *ctx = req->ctx;
 
+	if (!req->work.creds)
+		req->work.creds = get_current_cred();
+
 	if (req->flags & REQ_F_FORCE_ASYNC)
 		req->work.flags |= IO_WQ_WORK_CONCURRENT;
 
@@ -1648,6 +1651,10 @@ static void io_dismantle_req(struct io_kiocb *req)
 		io_put_file(req, req->file, (req->flags & REQ_F_FIXED_FILE));
 	if (req->fixed_rsrc_refs)
 		percpu_ref_put(req->fixed_rsrc_refs);
+	if (req->work.creds) {
+		put_cred(req->work.creds);
+		req->work.creds = NULL;
+	}
 
 	if (req->flags & REQ_F_INFLIGHT) {
 		struct io_ring_ctx *ctx = req->ctx;
@@ -5916,18 +5923,8 @@ static int io_issue_sqe(struct io_kiocb *req, unsigned int issue_flags)
 	const struct cred *creds = NULL;
 	int ret;
 
-	if (req->work.personality) {
-		const struct cred *new_creds;
-
-		if (!(issue_flags & IO_URING_F_NONBLOCK))
-			mutex_lock(&ctx->uring_lock);
-		new_creds = idr_find(&ctx->personality_idr, req->work.personality);
-		if (!(issue_flags & IO_URING_F_NONBLOCK))
-			mutex_unlock(&ctx->uring_lock);
-		if (!new_creds)
-			return -EINVAL;
-		creds = override_creds(new_creds);
-	}
+	if (req->work.creds && req->work.creds != current_cred())
+		creds = override_creds(req->work.creds);
 
 	switch (req->opcode) {
 	case IORING_OP_NOP:
@@ -6291,7 +6288,7 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
 {
 	struct io_submit_state *state;
 	unsigned int sqe_flags;
-	int ret = 0;
+	int personality, ret = 0;
 
 	req->opcode = READ_ONCE(sqe->opcode);
 	/* same numerical values with corresponding REQ_F_*, safe to copy */
@@ -6324,8 +6321,16 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
 		return -EOPNOTSUPP;
 
 	req->work.list.next = NULL;
+	personality = READ_ONCE(sqe->personality);
+	if (personality) {
+		req->work.creds = idr_find(&ctx->personality_idr, personality);
+		if (!req->work.creds)
+			return -EINVAL;
+		get_cred(req->work.creds);
+	} else {
+		req->work.creds = NULL;
+	}
 	req->work.flags = 0;
-	req->work.personality = READ_ONCE(sqe->personality);
 	state = &ctx->submit_state;
 
 	/*

-- 
Jens Axboe