Re: [PATCH 7/9] io_uring: add per-task callback handler

[Pavel Begunkov <asml.silence@gmail.com>]

On 23/02/2020 09:26, Jens Axboe wrote:
> On 2/22/20 11:00 PM, Jens Axboe wrote:
>> On 2/21/20 12:10 PM, Jens Axboe wrote:
>>>> Got it. Then, it may happen in the future after returning from
>>>> __io_arm_poll_handler() and io_uring_enter(). And by that time io_submit_sqes()
>>>> should have already restored creds (i.e. personality stuff) on the way back.
>>>> This might be a problem.
>>>
>>> Not sure I follow, can you elaborate? Just to be sure, the requests that
>>> go through the poll handler will go through __io_queue_sqe() again. Oh I
>>> guess your point is that that is one level below where we normally
>>> assign the creds.
>>
>> Fixed this one.

Looking at

io_async_task_func() {
	...
	/* ensure req->work.creds is valid for __io_queue_sqe() */
	req->work.creds = apoll->work.creds;
}

It copies creds, but doesn't touch the rest req->work fields. And if you have
one, you most probably got all of them in *grab_env(). Are you sure it doesn't
leak, e.g. mmgrab()'ed mm?


>>
>>>> BTW, Is it by design, that all requests of a link use personality creds
>>>> specified in the head's sqe?
>>>
>>> No, I think that's more by accident. We should make sure they use the
>>> specified creds, regardless of the issue time. Care to clean that up?
>>> Would probably help get it right for the poll case, too.
>>
>> Took a look at this, and I think you're wrong. Every iteration of
>> io_submit_sqe() will lookup the right creds, and assign them to the
>> current task in case we're going to issue it. In the case of a link
>> where we already have the head, then we grab the current work
>> environment. This means assigning req->work.creds from
>> get_current_cred(), if not set, and these are the credentials we looked
>> up already.

Yeah, I've spotted that there something wrong, but never looked up properly.

> 
> What does look wrong is that we don't restore the right credentials for
> queuing the head, so basically the opposite problem. Something like the
> below should fix that.
> index de650df9ac53..59024e4757d6 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -4705,11 +4705,18 @@ static void __io_queue_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>  {
>  	struct io_kiocb *linked_timeout;
>  	struct io_kiocb *nxt = NULL;
> +	const struct cred *old_creds = NULL;
>  	int ret;
>  
>  again:
>  	linked_timeout = io_prep_linked_timeout(req);
>  
> +	if (req->work.creds && req->work.creds != get_current_cred()) {

get_current_cred() gets a ref.
See my attempt below, it fixes miscount, and should work better for cases
changing back to initial creds (i.e. personality 0)

Anyway, creds handling is too scattered across the code, and this do a lot of
useless refcounting and bouncing. It's better to find it a better place in the
near future.

> +		if (old_creds)
> +			revert_creds(old_creds);
> +		old_creds = override_creds(req->work.creds);
> +	}
> +
>  	ret = io_issue_sqe(req, sqe, &nxt, true);
>  
>  	/*
> @@ -4759,6 +4766,8 @@ static void __io_queue_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>  			goto punt;
>  		goto again;
>  	}
> +	if (old_creds)
> +		revert_creds(old_creds);
>  }
>  
>  static void io_queue_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe)
> 

diff --git a/fs/io_uring.c b/fs/io_uring.c
index de650df9ac53..dc06298abb37 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4705,11 +4705,21 @@ static void __io_queue_sqe(struct io_kiocb *req, const
struct io_uring_sqe *sqe)
 {
 	struct io_kiocb *linked_timeout;
 	struct io_kiocb *nxt = NULL;
+	const struct cred *old_creds = NULL;
 	int ret;

 again:
 	linked_timeout = io_prep_linked_timeout(req);

+	if (req->work.creds && req->work.creds != current_cred()) {
+		if (old_creds)
+			revert_creds(old_creds);
+		if (old_creds == req->work.creds)
+			old_creds = NULL; /* restored original creds */
+		else
+			old_creds = override_creds(req->work.creds);
+	}
+
 	ret = io_issue_sqe(req, sqe, &nxt, true);

 	/*
@@ -4759,6 +4769,8 @@ static void __io_queue_sqe(struct io_kiocb *req, const
struct io_uring_sqe *sqe)
 			goto punt;
 		goto again;
 	}
+	if (old_creds)
+		revert_creds(old_creds);
 }

 static void io_queue_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe)

-- 
Pavel Begunkov