Re: [PATCH 1/3] io_uring: don't use req->work.creds for inline requests

[Xiaoguang Wang <xiaoguang.wang@xxxxxxxxxxxxxxxxx>]

hi,

> On 26/05/2020 09:43, Xiaoguang Wang wrote:
> > In io_init_req(), if uers requires a new credentials, currently we'll
> > save it in req->work.creds, but indeed io_wq_work is designed to describe
> > needed running environment for requests that will go to io-wq, if one
> > request is going to be submitted inline, we'd better not touch io_wq_work.
> > Here add a new 'const struct cred *creds' in io_kiocb, if uers requires a
> > new credentials, inline requests can use it.
> >
> > This patch is also a preparation for later patch.
>
> What's the difference from keeping only one creds field in io_kiocb (i.e.
> req->work.creds), but handling it specially (i.e. always initialising)? It will
> be a lot easier than tossing it around.
>
> Also, the patch doubles {get,put}_creds() for sqe->personality case, and that's
> extra atomics without a good reason.
You're right, thanks.
The original motivation for this patch is that it's just a preparation later patch
"io_uring: avoid whole io_wq_work copy for inline requests", I can use io_wq_work.func
to determine whether to drop io_wq_work in io_req_work_drop_env(), so if io_wq_work.func
is NULL, I don't want io_wq_work has a valid creds.
I'll look into whether we can just assign req->creds's pointer to io_wq_work.creds to
reduce the atomic operations.

Regards,
Xiaoguang Wang

> > Signed-off-by: Xiaoguang Wang <xiaoguang.wang@xxxxxxxxxxxxxxxxx>
> > ---
> >   fs/io_uring.c | 23 +++++++++++++++--------
> >   1 file changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/io_uring.c b/fs/io_uring.c
> > index 2af87f73848e..788d960abc69 100644
> > --- a/fs/io_uring.c
> > +++ b/fs/io_uring.c
> > @@ -635,6 +635,7 @@ struct io_kiocb {
> >   	unsigned int		flags;
> >   	refcount_t		refs;
> >   	struct task_struct	*task;
> > +	const struct cred	*creds;
> >   	unsigned long		fsize;
> >   	u64			user_data;
> >   	u32			result;
> > @@ -1035,8 +1036,10 @@ static inline void io_req_work_grab_env(struct io_kiocb *req,
> >   		mmgrab(current->mm);
> >   		req->work.mm = current->mm;
> >   	}
> > -	if (!req->work.creds)
> > +	if (!req->creds)
> >   		req->work.creds = get_current_cred();
> > +	else
> > +		req->work.creds = get_cred(req->creds);
> >   	if (!req->work.fs && def->needs_fs) {
> >   		spin_lock(&current->fs->lock);
> >   		if (!current->fs->in_exec) {
> > @@ -1368,6 +1371,9 @@ static void __io_req_aux_free(struct io_kiocb *req)
> >   	if (req->flags & REQ_F_NEED_CLEANUP)
> >   		io_cleanup_req(req);
> >   
> > +	if (req->creds)
> > +		put_cred(req->creds);
> > +
> >   	kfree(req->io);
> >   	if (req->file)
> >   		io_put_file(req, req->file, (req->flags & REQ_F_FIXED_FILE));
> > @@ -5673,13 +5679,13 @@ static void __io_queue_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe)
> >   again:
> >   	linked_timeout = io_prep_linked_timeout(req);
> >   
> > -	if (req->work.creds && req->work.creds != current_cred()) {
> > +	if (req->creds && req->creds != current_cred()) {
> >   		if (old_creds)
> >   			revert_creds(old_creds);
> > -		if (old_creds == req->work.creds)
> > +		if (old_creds == req->creds)
> >   			old_creds = NULL; /* restored original creds */
> >   		else
> > -			old_creds = override_creds(req->work.creds);
> > +			old_creds = override_creds(req->creds);
> >   	}
> >   
> >   	ret = io_issue_sqe(req, sqe, true);
> > @@ -5970,11 +5976,12 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
> >   
> >   	id = READ_ONCE(sqe->personality);
> >   	if (id) {
> > -		req->work.creds = idr_find(&ctx->personality_idr, id);
> > -		if (unlikely(!req->work.creds))
> > +		req->creds = idr_find(&ctx->personality_idr, id);
> > +		if (unlikely(!req->creds))
> >   			return -EINVAL;
> > -		get_cred(req->work.creds);
> > -	}
> > +		get_cred(req->creds);
> > +	} else
> > +		req->creds = NULL;
> >   
> >   	/* same numerical values with corresponding REQ_F_*, safe to copy */
> >   	req->flags |= sqe_flags;