public inbox for [email protected]
 help / color / mirror / Atom feed
From: Stefan Metzmacher <[email protected]>
To: Jens Axboe <[email protected]>, [email protected]
Cc: [email protected], [email protected]
Subject: Re: [PATCH 3/6] io_uring: add support for IORING_OP_OPENAT
Date: Thu, 9 Jan 2020 11:40:57 +0100	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>


[-- Attachment #1.1: Type: text/plain, Size: 5724 bytes --]

>> I'm sorry, but I'm still unsure we're talking about the same thing
>> (or maybe I'm missing some basics here).
>>
>> My understanding of the io_uring_enter() is that it will execute as much
>> non-blocking calls as it can without switching to any other kernel thread.
> 
> Correct, any SQE that we can do without switching, we will.
> 
>> And my fear is that openat will use get_current_cred() instead of
>> ctx->creds.
> 
> OK, I think I follow your concern. So you'd like to setup the rings from
> a _different_ user, and then later on use it for submission for SQEs that
> a specific user. So sort of the same as our initial discussion, except
> the mapping would be static. The difference being that you might setup
> the ring from a different user than the user that would be submitting IO
> on it?

Our current (much simplified here) flow is this:

  # we start as root
  seteuid(0);setegid(0);setgroups()...
  ...
  # we become the user555 and
  # create our desired credential token
  seteuid(555); seteguid(555); setgroups()...
  # Start an openat2 on behalf of user555
  openat2()
  # we unbecome the user again and run as root
  seteuid(0);setegid(0); setgroups()...
  ...
  # we become the user444 and
  # create our desired credential token
  seteuid(444); seteguid(444); setgroups()...
  # Start an openat2 on behalf of user444
  openat2()
  # we unbecome the user again and run as root
  seteuid(0);setegid(0); setgroups()...
  ...
  # we become the user555 and
  # create our desired credential token
  seteuid(555); seteguid(555); setgroups()...
  # Start an openat2 on behalf of user555
  openat2()
  # we unbecome the user again and run as root
  seteuid(0);setegid(0); setgroups()...

It means we have to do about 7 syscalls in order
to open a file on behalf of a user.
(In reality we cache things and avoid set*id()
calls most of the time, but I want to demonstrate the
simplified design here)

With io_uring I'd like to use a flow like this:

  # we start as root
  seteuid(0);setegid(0);setgroups()...
  ...
  # we become the user444 and
  # create our desired credential token
  seteuid(444); seteguid(444); setgroups()...
  # we snapshot the credentials to the new ring for user444
  ring444 = io_uring_setup()
  # we unbecome the user again and run as root
  seteuid(0);setegid(0);setgroups()...
  ...
  # we become the user555 and
  # create our desired credential token
  seteuid(555); seteguid(555); setgroups()...
  # we snapshot the credentials to the new ring for user555
  ring555 = io_uring_setup()
  # we unbecome the user again and run as root
  seteuid(0);setegid(0);setgroups()...
  ...
  # Start an openat2 on behalf of user555
  io_uring_enter(ring555, OP_OPENAT2...)
  ...
  # Start an openat2 on behalf of user444
  io_uring_enter(ring444, OP_OPENAT2...)
  ...
  # Start an openat2 on behalf of user555
  io_uring_enter(ring555, OP_OPENAT2...)

So instead of constantly doing 7 syscalls per open,
we would be down to just at most one. And I would assume
that io_uring_enter() would do the temporary credential switch
for me also in the non-blocking case.

> If so, then we do need something to support that, probably an
> IORING_REGISTER_CREDS or similar. This would allow you to replace the
> creds you currently have in ctx->creds with whatever new one.

I don't want to change ctx->creds, but I want it to be used consistently.

What I think is missing is something like this:

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 32aee149f652..55dbb154915a 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -6359,10 +6359,27 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int,
fd, u32, to_submit,
                struct mm_struct *cur_mm;

                mutex_lock(&ctx->uring_lock);
+               if (current->mm != ctx->sqo_mm) {
+                       // TODO: somthing like this...
+                       restore_mm = current->mm;
+                       use_mm(ctx->sqo_mm);
+               }
                /* already have mm, so io_submit_sqes() won't try to
grab it */
                cur_mm = ctx->sqo_mm;
+               if (current_cred() != ctx->creds) {
+                       // TODO: somthing like this...
+                       restore_cred = override_creds(ctx->creds);
+               }
                submitted = io_submit_sqes(ctx, to_submit, f.file, fd,
                                           &cur_mm, false);
+               if (restore_cred != NULL) {
+                       revert_creds(restore_cred);
+               }
+               if (restore_mm != NULL) {
+                       // TODO: something like this...
+                       unuse_mm(ctx->sqo_mm);
+                       use_mm(restore_mm);
+               }
                mutex_unlock(&ctx->uring_lock);

                if (submitted != to_submit)

I'm not sure if current->mm is needed, I just added it for completeness
and as hint that io_op_defs[req->opcode].needs_mm is there and a
needs_creds could also be added (if it helps with performance)

Is it possible to trigger a change of current->mm from userspace?

An IORING_REGISTER_CREDS would only be useful if it's possible to
register a set of credentials and then use per io_uring_sqe credentials.
That would also be fine for me, but I'm not sure it's needed for now.

Apart from IORING_REGISTER_CREDS I think a change like the one
above is needed in order to avoid potential security problems.

>> I'm I missing something?
> 
> I think we're talking about the same thing, just different views of it :-)

I hope it's clear from my side now :-)

Thanks!
metze


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply	other threads:[~2020-01-09 10:41 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-07 17:00 [PATCHSET v2 0/6] io_uring: add support for open/close Jens Axboe
2020-01-07 17:00 ` [PATCH 1/6] fs: add namei support for doing a non-blocking path lookup Jens Axboe
2020-01-25 13:15   ` Jeff Layton
2020-01-07 17:00 ` [PATCH 2/6] fs: make build_open_flags() available internally Jens Axboe
2020-01-07 17:00 ` [PATCH 3/6] io_uring: add support for IORING_OP_OPENAT Jens Axboe
2020-01-08 13:05   ` Stefan Metzmacher
2020-01-08 16:20     ` Jens Axboe
2020-01-08 16:32       ` Stefan Metzmacher
2020-01-08 16:40         ` Jens Axboe
2020-01-08 17:04           ` Stefan Metzmacher
2020-01-08 22:53             ` Jens Axboe
2020-01-08 23:03               ` Stefan Metzmacher
2020-01-08 23:05                 ` Jens Axboe
2020-01-08 23:11                   ` Stefan Metzmacher
2020-01-08 23:22                     ` Jens Axboe
2020-01-09 10:40                       ` Stefan Metzmacher [this message]
2020-01-09 21:31                         ` Jens Axboe
2020-01-16 22:42                           ` Stefan Metzmacher
2020-01-17  0:16                             ` Jens Axboe
2020-01-07 17:00 ` [PATCH 4/6] fs: move filp_close() outside of __close_fd_get_file() Jens Axboe
2020-01-07 17:00 ` [PATCH 5/6] io-wq: add support for uncancellable work Jens Axboe
2020-01-07 17:00 ` [PATCH 6/6] io_uring: add support for IORING_OP_CLOSE Jens Axboe
2020-01-08 21:17 ` [PATCHSET v2 0/6] io_uring: add support for open/close Stefan Metzmacher
2020-01-08 22:57   ` Jens Axboe
2020-01-08 23:05     ` Stefan Metzmacher
2020-01-09  1:02       ` Jens Axboe
2020-01-09  2:03         ` Jens Axboe
2020-01-16 22:50           ` Stefan Metzmacher
2020-01-17  0:18             ` Jens Axboe
2020-01-20 12:15               ` Stefan Metzmacher
2020-01-20 13:04                 ` Pavel Begunkov
2020-01-17  0:44             ` Colin Walters
2020-01-17  0:51               ` Jens Axboe
2020-01-17  9:32               ` Pavel Begunkov
2020-01-17 15:21                 ` Jens Axboe
2020-01-17 22:27                   ` Pavel Begunkov
2020-01-17 22:36                     ` Jens Axboe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox