Re: [PATCH 13/18] io_uring: add file set registration

Re: [PATCH 13/18] io_uring: add file set registration

[Jann Horn]

On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
> We normally have to fget/fput for each IO we do on a file. Even with
> the batching we do, the cost of the atomic inc/dec of the file usage
> count adds up.
>
> This adds IORING_REGISTER_FILES, and IORING_UNREGISTER_FILES opcodes
> for the io_uring_register(2) system call. The arguments passed in must
> be an array of __s32 holding file descriptors, and nr_args should hold
> the number of file descriptors the application wishes to pin for the
> duration of the io_uring context (or until IORING_UNREGISTER_FILES is
> called).
>
> When used, the application must set IOSQE_FIXED_FILE in the sqe->flags
> member. Then, instead of setting sqe->fd to the real fd, it sets sqe->fd
> to the index in the array passed in to IORING_REGISTER_FILES.
>
> Files are automatically unregistered when the io_uring context is
> torn down. An application need only unregister if it wishes to
> register a new set of fds.

Crazy idea:

Taking a step back, at a high level, basically this patch creates sort
of the same difference that you get when you compare the following
scenarios for normal multithreaded I/O in userspace:

===========================================================
~/tests/fdget_perf$ cat fdget_perf.c
#define _GNU_SOURCE
#include <sys/wait.h>
#include <sched.h>
#include <unistd.h>
#include <stdbool.h>
#include <string.h>
#include <err.h>
#include <signal.h>
#include <sys/eventfd.h>
#include <stdio.h>

// two different physical processors on my machine
#define CORE_A 0
#define CORE_B 14

static void pin_to_core(int coreid) {
  cpu_set_t set;
  CPU_ZERO(&set);
  CPU_SET(coreid, &set);
  if (sched_setaffinity(0, sizeof(cpu_set_t), &set))
    err(1, "sched_setaffinity");
}

static int fd = -1;

static volatile int time_over = 0;
static void alarm_handler(int sig) { time_over = 1; }
static void run_stuff(void) {
  unsigned long long iterations = 0;
  if (signal(SIGALRM, alarm_handler) == SIG_ERR) err(1, "signal");
  alarm(10);
  while (1) {
    uint64_t val;
    read(fd, &val, sizeof(val));
    if (time_over) {
      printf("iterations = 0x%llx\n", iterations);
      return;
    }
    iterations++;
  }
}

static int child_fn(void *dummy) {
  pin_to_core(CORE_B);
  run_stuff();
  return 0;
}

static char child_stack[1024*1024];

int main(int argc, char **argv) {
  fd = eventfd(0, EFD_NONBLOCK);
  if (fd == -1) err(1, "eventfd");

  if (argc != 2) errx(1, "bad usage");
  int flags = SIGCHLD;
  if (strcmp(argv[1], "shared") == 0) {
    flags |= CLONE_FILES;
  } else if (strcmp(argv[1], "cloned") == 0) {
    /* nothing */
  } else {
    errx(1, "bad usage");
  }
  pid_t child = clone(child_fn, child_stack+sizeof(child_stack), flags, NULL);
  if (child == -1) err(1, "clone");

  pin_to_core(CORE_A);
  run_stuff();
  int status;
  if (wait(&status) != child) err(1, "wait");
  return 0;
}
~/tests/fdget_perf$ gcc -Wall -o fdget_perf fdget_perf.c
~/tests/fdget_perf$ ./fdget_perf shared
iterations = 0x8d3010
iterations = 0x92d894
~/tests/fdget_perf$ ./fdget_perf cloned
iterations = 0xad3bbd
iterations = 0xb08838
~/tests/fdget_perf$ ./fdget_perf shared
iterations = 0x8cc340
iterations = 0x8e4e64
~/tests/fdget_perf$ ./fdget_perf cloned
iterations = 0xada5f3
iterations = 0xb04b6f
===========================================================

This kinda makes me wonder whether this is really something that
should be implemented specifically for the io_uring API, or whether it
would make sense to somehow handle part of this in the generic VFS
code and give the user the ability to prepare a new files_struct that
can then be transferred to the worker thread, or something like
that... I'm not sure whether there's a particularly clean way to do
that though.

Or perhaps you could add a userspace API for marking file descriptor
table entries as "has percpu refcounting" somehow, with one percpu
refcount per files_struct and one bit per fd, allocated when percpu
refcounting is activated for the files_struct the first time, or
something like that...

> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  fs/io_uring.c                 | 138 +++++++++++++++++++++++++++++-----
>  include/uapi/linux/io_uring.h |   9 ++-
>  2 files changed, 127 insertions(+), 20 deletions(-)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 17c869f3ea2f..13c3f8212815 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -100,6 +100,14 @@ struct io_ring_ctx {
>                 struct fasync_struct    *cq_fasync;
>         } ____cacheline_aligned_in_smp;
>
> +       /*
> +        * If used, fixed file set. Writers must ensure that ->refs is dead,
> +        * readers must ensure that ->refs is alive as long as the file* is
> +        * used. Only updated through io_uring_register(2).
> +        */
> +       struct file             **user_files;
> +       unsigned                nr_user_files;
> +
>         /* if used, fixed mapped user buffers */
>         unsigned                nr_user_bufs;
>         struct io_mapped_ubuf   *user_bufs;
> @@ -136,6 +144,7 @@ struct io_kiocb {
>         unsigned int            flags;
>  #define REQ_F_FORCE_NONBLOCK   1       /* inline submission attempt */
>  #define REQ_F_IOPOLL_COMPLETED 2       /* polled IO has completed */
> +#define REQ_F_FIXED_FILE       4       /* ctx owns file */
>         u64                     user_data;
>         u64                     error;
>
> @@ -350,15 +359,17 @@ static void io_iopoll_complete(struct io_ring_ctx *ctx, unsigned int *nr_events,
>                  * Batched puts of the same file, to avoid dirtying the
>                  * file usage count multiple times, if avoidable.
>                  */
> -               if (!file) {
> -                       file = req->rw.ki_filp;
> -                       file_count = 1;
> -               } else if (file == req->rw.ki_filp) {
> -                       file_count++;
> -               } else {
> -                       fput_many(file, file_count);
> -                       file = req->rw.ki_filp;
> -                       file_count = 1;
> +               if (!(req->flags & REQ_F_FIXED_FILE)) {
> +                       if (!file) {
> +                               file = req->rw.ki_filp;
> +                               file_count = 1;
> +                       } else if (file == req->rw.ki_filp) {
> +                               file_count++;
> +                       } else {
> +                               fput_many(file, file_count);
> +                               file = req->rw.ki_filp;
> +                               file_count = 1;
> +                       }
>                 }
>
>                 if (to_free == ARRAY_SIZE(reqs))
> @@ -491,13 +502,19 @@ static void kiocb_end_write(struct kiocb *kiocb)
>         }
>  }
>
> +static void io_fput(struct io_kiocb *req)
> +{
> +       if (!(req->flags & REQ_F_FIXED_FILE))
> +               fput(req->rw.ki_filp);
> +}
> +
>  static void io_complete_rw(struct kiocb *kiocb, long res, long res2)
>  {
>         struct io_kiocb *req = container_of(kiocb, struct io_kiocb, rw);
>
>         kiocb_end_write(kiocb);
>
> -       fput(kiocb->ki_filp);
> +       io_fput(req);
>         io_cqring_add_event(req->ctx, req->user_data, res, 0);
>         io_free_req(req);
>  }
> @@ -596,11 +613,22 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>  {
>         struct io_ring_ctx *ctx = req->ctx;
>         struct kiocb *kiocb = &req->rw;
> -       unsigned ioprio;
> +       unsigned ioprio, flags;
>         int fd, ret;
>
> +       flags = READ_ONCE(sqe->flags);
>         fd = READ_ONCE(sqe->fd);
> -       kiocb->ki_filp = io_file_get(state, fd);
> +
> +       if (flags & IOSQE_FIXED_FILE) {
> +               if (unlikely(!ctx->user_files ||
> +                   (unsigned) fd >= ctx->nr_user_files))
> +                       return -EBADF;
> +               kiocb->ki_filp = ctx->user_files[fd];
> +               req->flags |= REQ_F_FIXED_FILE;
> +       } else {
> +               kiocb->ki_filp = io_file_get(state, fd);
> +       }
> +
>         if (unlikely(!kiocb->ki_filp))
>                 return -EBADF;
>         kiocb->ki_pos = READ_ONCE(sqe->off);
> @@ -641,7 +669,8 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>         }
>         return 0;
>  out_fput:
> -       io_file_put(state, kiocb->ki_filp);
> +       if (!(flags & IOSQE_FIXED_FILE))
> +               io_file_put(state, kiocb->ki_filp);
>         return ret;
>  }
>
> @@ -765,7 +794,7 @@ static ssize_t io_read(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>         kfree(iovec);
>  out_fput:
>         if (unlikely(ret))
> -               fput(file);
> +               io_fput(req);
>         return ret;
>  }
>
> @@ -820,7 +849,7 @@ static ssize_t io_write(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>         kfree(iovec);
>  out_fput:
>         if (unlikely(ret))
> -               fput(file);
> +               io_fput(req);
>         return ret;
>  }
>
> @@ -846,7 +875,7 @@ static int io_fsync(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>         loff_t sqe_off = READ_ONCE(sqe->off);
>         loff_t sqe_len = READ_ONCE(sqe->len);
>         loff_t end = sqe_off + sqe_len;
> -       unsigned fsync_flags;
> +       unsigned fsync_flags, flags;
>         struct file *file;
>         int ret, fd;
>
> @@ -864,14 +893,23 @@ static int io_fsync(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>                 return -EINVAL;
>
>         fd = READ_ONCE(sqe->fd);
> -       file = fget(fd);
> +       flags = READ_ONCE(sqe->flags);
> +
> +       if (flags & IOSQE_FIXED_FILE) {
> +               if (unlikely(!ctx->user_files || fd >= ctx->nr_user_files))
> +                       return -EBADF;
> +               file = ctx->user_files[fd];
> +       } else {
> +               file = fget(fd);
> +       }
>         if (unlikely(!file))
>                 return -EBADF;
>
>         ret = vfs_fsync_range(file, sqe_off, end > 0 ? end : LLONG_MAX,
>                                 fsync_flags & IORING_FSYNC_DATASYNC);
>
> -       fput(file);
> +       if (!(flags & IOSQE_FIXED_FILE))
> +               fput(file);
>         io_cqring_add_event(ctx, sqe->user_data, ret, 0);
>         io_free_req(req);
>         return 0;
> @@ -1002,7 +1040,7 @@ static int io_submit_sqe(struct io_ring_ctx *ctx, const struct sqe_submit *s,
>         ssize_t ret;
>
>         /* enforce forwards compatibility on users */
> -       if (unlikely(s->sqe->flags))
> +       if (unlikely(s->sqe->flags & ~IOSQE_FIXED_FILE))
>                 return -EINVAL;
>
>         req = io_get_req(ctx, state);
> @@ -1220,6 +1258,58 @@ static int __io_uring_enter(struct io_ring_ctx *ctx, unsigned to_submit,
>         return submitted ? submitted : ret;
>  }
>
> +static int io_sqe_files_unregister(struct io_ring_ctx *ctx)
> +{
> +       int i;
> +
> +       if (!ctx->user_files)
> +               return -ENXIO;
> +
> +       for (i = 0; i < ctx->nr_user_files; i++)
> +               fput(ctx->user_files[i]);
> +
> +       kfree(ctx->user_files);
> +       ctx->user_files = NULL;
> +       ctx->nr_user_files = 0;
> +       return 0;
> +}
> +
> +static int io_sqe_files_register(struct io_ring_ctx *ctx, void __user *arg,
> +                                unsigned nr_args)
> +{
> +       __s32 __user *fds = (__s32 __user *) arg;
> +       int fd, ret = 0;
> +       unsigned i;
> +
> +       if (ctx->user_files)
> +               return -EBUSY;
> +       if (!nr_args)
> +               return -EINVAL;
> +
> +       ctx->user_files = kcalloc(nr_args, sizeof(struct file *), GFP_KERNEL);
> +       if (!ctx->user_files)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < nr_args; i++) {
> +               ret = -EFAULT;
> +               if (copy_from_user(&fd, &fds[i], sizeof(fd)))
> +                       break;
> +
> +               ctx->user_files[i] = fget(fd);
> +
> +               ret = -EBADF;
> +               if (!ctx->user_files[i])
> +                       break;
> +               ctx->nr_user_files++;
> +               ret = 0;
> +       }
> +
> +       if (ret)
> +               io_sqe_files_unregister(ctx);
> +
> +       return ret;
> +}
> +
>  static int io_sq_offload_start(struct io_ring_ctx *ctx)
>  {
>         int ret;
> @@ -1509,6 +1599,7 @@ static void io_ring_ctx_free(struct io_ring_ctx *ctx)
>
>         io_iopoll_reap_events(ctx);
>         io_sqe_buffer_unregister(ctx);
> +       io_sqe_files_unregister(ctx);
>
>         io_mem_free(ctx->sq_ring);
>         io_mem_free(ctx->sq_sqes);
> @@ -1806,6 +1897,15 @@ static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
>                         break;
>                 ret = io_sqe_buffer_unregister(ctx);
>                 break;
> +       case IORING_REGISTER_FILES:
> +               ret = io_sqe_files_register(ctx, arg, nr_args);
> +               break;
> +       case IORING_UNREGISTER_FILES:
> +               ret = -EINVAL;
> +               if (arg || nr_args)
> +                       break;
> +               ret = io_sqe_files_unregister(ctx);
> +               break;
>         default:
>                 ret = -EINVAL;
>                 break;
> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> index 16c423d74f2e..3e79feb34a9c 100644
> --- a/include/uapi/linux/io_uring.h
> +++ b/include/uapi/linux/io_uring.h
> @@ -16,7 +16,7 @@
>   */
>  struct io_uring_sqe {
>         __u8    opcode;         /* type of operation for this sqe */
> -       __u8    flags;          /* as of now unused */
> +       __u8    flags;          /* IOSQE_ flags */
>         __u16   ioprio;         /* ioprio for the request */
>         __s32   fd;             /* file descriptor to do IO on */
>         __u64   off;            /* offset into file */
> @@ -33,6 +33,11 @@ struct io_uring_sqe {
>         };
>  };
>
> +/*
> + * sqe->flags
> + */
> +#define IOSQE_FIXED_FILE       (1U << 0)       /* use fixed fileset */
> +
>  /*
>   * io_uring_setup() flags
>   */
> @@ -112,5 +117,7 @@ struct io_uring_params {
>   */
>  #define IORING_REGISTER_BUFFERS                0
>  #define IORING_UNREGISTER_BUFFERS      1
> +#define IORING_REGISTER_FILES          2
> +#define IORING_UNREGISTER_FILES                3
>
>  #endif
> --
> 2.17.1
>

Re: [PATCH 13/18] io_uring: add file set registration

[Jens Axboe]

On 1/29/19 6:29 PM, Jann Horn wrote:
> On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
>> We normally have to fget/fput for each IO we do on a file. Even with
>> the batching we do, the cost of the atomic inc/dec of the file usage
>> count adds up.
>>
>> This adds IORING_REGISTER_FILES, and IORING_UNREGISTER_FILES opcodes
>> for the io_uring_register(2) system call. The arguments passed in must
>> be an array of __s32 holding file descriptors, and nr_args should hold
>> the number of file descriptors the application wishes to pin for the
>> duration of the io_uring context (or until IORING_UNREGISTER_FILES is
>> called).
>>
>> When used, the application must set IOSQE_FIXED_FILE in the sqe->flags
>> member. Then, instead of setting sqe->fd to the real fd, it sets sqe->fd
>> to the index in the array passed in to IORING_REGISTER_FILES.
>>
>> Files are automatically unregistered when the io_uring context is
>> torn down. An application need only unregister if it wishes to
>> register a new set of fds.
> 
> Crazy idea:
> 
> Taking a step back, at a high level, basically this patch creates sort
> of the same difference that you get when you compare the following
> scenarios for normal multithreaded I/O in userspace:
> 
> ===========================================================
> ~/tests/fdget_perf$ cat fdget_perf.c
> #define _GNU_SOURCE
> #include <sys/wait.h>
> #include <sched.h>
> #include <unistd.h>
> #include <stdbool.h>
> #include <string.h>
> #include <err.h>
> #include <signal.h>
> #include <sys/eventfd.h>
> #include <stdio.h>
> 
> // two different physical processors on my machine
> #define CORE_A 0
> #define CORE_B 14
> 
> static void pin_to_core(int coreid) {
>   cpu_set_t set;
>   CPU_ZERO(&set);
>   CPU_SET(coreid, &set);
>   if (sched_setaffinity(0, sizeof(cpu_set_t), &set))
>     err(1, "sched_setaffinity");
> }
> 
> static int fd = -1;
> 
> static volatile int time_over = 0;
> static void alarm_handler(int sig) { time_over = 1; }
> static void run_stuff(void) {
>   unsigned long long iterations = 0;
>   if (signal(SIGALRM, alarm_handler) == SIG_ERR) err(1, "signal");
>   alarm(10);
>   while (1) {
>     uint64_t val;
>     read(fd, &val, sizeof(val));
>     if (time_over) {
>       printf("iterations = 0x%llx\n", iterations);
>       return;
>     }
>     iterations++;
>   }
> }
> 
> static int child_fn(void *dummy) {
>   pin_to_core(CORE_B);
>   run_stuff();
>   return 0;
> }
> 
> static char child_stack[1024*1024];
> 
> int main(int argc, char **argv) {
>   fd = eventfd(0, EFD_NONBLOCK);
>   if (fd == -1) err(1, "eventfd");
> 
>   if (argc != 2) errx(1, "bad usage");
>   int flags = SIGCHLD;
>   if (strcmp(argv[1], "shared") == 0) {
>     flags |= CLONE_FILES;
>   } else if (strcmp(argv[1], "cloned") == 0) {
>     /* nothing */
>   } else {
>     errx(1, "bad usage");
>   }
>   pid_t child = clone(child_fn, child_stack+sizeof(child_stack), flags, NULL);
>   if (child == -1) err(1, "clone");
> 
>   pin_to_core(CORE_A);
>   run_stuff();
>   int status;
>   if (wait(&status) != child) err(1, "wait");
>   return 0;
> }
> ~/tests/fdget_perf$ gcc -Wall -o fdget_perf fdget_perf.c
> ~/tests/fdget_perf$ ./fdget_perf shared
> iterations = 0x8d3010
> iterations = 0x92d894
> ~/tests/fdget_perf$ ./fdget_perf cloned
> iterations = 0xad3bbd
> iterations = 0xb08838
> ~/tests/fdget_perf$ ./fdget_perf shared
> iterations = 0x8cc340
> iterations = 0x8e4e64
> ~/tests/fdget_perf$ ./fdget_perf cloned
> iterations = 0xada5f3
> iterations = 0xb04b6f
> ===========================================================
> 
> This kinda makes me wonder whether this is really something that
> should be implemented specifically for the io_uring API, or whether it
> would make sense to somehow handle part of this in the generic VFS
> code and give the user the ability to prepare a new files_struct that
> can then be transferred to the worker thread, or something like
> that... I'm not sure whether there's a particularly clean way to do
> that though.
> 
> Or perhaps you could add a userspace API for marking file descriptor
> table entries as "has percpu refcounting" somehow, with one percpu
> refcount per files_struct and one bit per fd, allocated when percpu
> refcounting is activated for the files_struct the first time, or
> something like that...

There's undoubtedly a win by NOT sharing, obviously. Not sure how to do
this in a generalized fashion, cleanly, it's easier (and a better fit)
to do it for specific cases, like io_uring here. If others want to go
down that path, io_uring could always be adapted to use that
infrastructure.

-- 
Jens Axboe

Re: [PATCH 13/18] io_uring: add file set registration

[Al Viro]

On Wed, Jan 30, 2019 at 02:29:05AM +0100, Jann Horn wrote:
> On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
> > We normally have to fget/fput for each IO we do on a file. Even with
> > the batching we do, the cost of the atomic inc/dec of the file usage
> > count adds up.
> >
> > This adds IORING_REGISTER_FILES, and IORING_UNREGISTER_FILES opcodes
> > for the io_uring_register(2) system call. The arguments passed in must
> > be an array of __s32 holding file descriptors, and nr_args should hold
> > the number of file descriptors the application wishes to pin for the
> > duration of the io_uring context (or until IORING_UNREGISTER_FILES is
> > called).
> >
> > When used, the application must set IOSQE_FIXED_FILE in the sqe->flags
> > member. Then, instead of setting sqe->fd to the real fd, it sets sqe->fd
> > to the index in the array passed in to IORING_REGISTER_FILES.
> >
> > Files are automatically unregistered when the io_uring context is
> > torn down. An application need only unregister if it wishes to
> > register a new set of fds.
> 
> Crazy idea:
> 
> Taking a step back, at a high level, basically this patch creates sort
> of the same difference that you get when you compare the following
> scenarios for normal multithreaded I/O in userspace:

> This kinda makes me wonder whether this is really something that
> should be implemented specifically for the io_uring API, or whether it
> would make sense to somehow handle part of this in the generic VFS
> code and give the user the ability to prepare a new files_struct that
> can then be transferred to the worker thread, or something like
> that... I'm not sure whether there's a particularly clean way to do
> that though.

Using files_struct for that opens a can of worms you really don't
want to touch.

Consider the following scenario with any variant of this interface:
	* create io_uring fd.
	* send an SCM_RIGHTS with that fd to AF_UNIX socket.
	* add the descriptor of that AF_UNIX socket to your fd
	* close AF_UNIX fd, close io_uring fd.
Voila - you've got a shiny leak.  No ->release() is called for
anyone (and you really don't want to do that on ->flush(), because
otherwise a library helper doing e.g. system("/bin/date") will tear
down all the io_uring in your process).  The socket is held by
the reference you've stashed into io_uring (whichever way you do
that).  io_uring is held by the reference you've stashed into
SCM_RIGHTS datagram in queue of the socket.

No matter what, you need net/unix/garbage.c to be aware of that stuff.
And getting files_struct lifetime mixed into that would be beyond
any reason.

The only reason for doing that as a descriptor table would be
avoiding the cost of fget() in whatever uses it, right?  Since
those are *not* the normal syscalls (and fdget() really should not
be used anywhere other than the very top of syscall's call chain -
that's another reason why tossing file_struct around like that
is insane) and since the benefit is all due to the fact that it's
*NOT* shared, *NOT* modified in parallel, etc., allowing us to
treat file references as stable... why the hell use the descriptor
tables at all?

All you need is an array of struct file *, explicitly populated.
With net/unix/garbage.c aware of such beasts.  Guess what?  We
do have such an object already.  The one net/unix/garbage.c is
working with.  SCM_RIGHTS datagrams, that is.

IOW, can't we give those io_uring descriptors associated struct
unix_sock?  No socket descriptors, no struct socket (probably),
just the AF_UNIX-specific part thereof.  Then teach
unix_inflight()/unix_notinflight() about getting unix_sock out
of these guys (incidentally, both would seem to benefit from
_not_ touching unix_gc_lock in case when there's no unix_sock
attached to file we are dealing with - I might be missing
something very subtle about barriers there, but it doesn't
look likely).

And make that (i.e. registering the descriptors) mandatory.
Hell, combine that with creating io_uring fd, if we really
care about the syscall count.  Benefits:
	* no file_struct refcount wanking
	* no fget()/fput() (conditional, at that) from kernel
threads
	* no CLOEXEC-dependent anything; just the teardown
on the final fput(), whichever way it comes.
	* no fun with duelling garbage collectors.

Re: [PATCH 13/18] io_uring: add file set registration

[Jens Axboe]

On 2/3/19 7:56 PM, Al Viro wrote:
> On Wed, Jan 30, 2019 at 02:29:05AM +0100, Jann Horn wrote:
>> On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
>>> We normally have to fget/fput for each IO we do on a file. Even with
>>> the batching we do, the cost of the atomic inc/dec of the file usage
>>> count adds up.
>>>
>>> This adds IORING_REGISTER_FILES, and IORING_UNREGISTER_FILES opcodes
>>> for the io_uring_register(2) system call. The arguments passed in must
>>> be an array of __s32 holding file descriptors, and nr_args should hold
>>> the number of file descriptors the application wishes to pin for the
>>> duration of the io_uring context (or until IORING_UNREGISTER_FILES is
>>> called).
>>>
>>> When used, the application must set IOSQE_FIXED_FILE in the sqe->flags
>>> member. Then, instead of setting sqe->fd to the real fd, it sets sqe->fd
>>> to the index in the array passed in to IORING_REGISTER_FILES.
>>>
>>> Files are automatically unregistered when the io_uring context is
>>> torn down. An application need only unregister if it wishes to
>>> register a new set of fds.
>>
>> Crazy idea:
>>
>> Taking a step back, at a high level, basically this patch creates sort
>> of the same difference that you get when you compare the following
>> scenarios for normal multithreaded I/O in userspace:
> 
>> This kinda makes me wonder whether this is really something that
>> should be implemented specifically for the io_uring API, or whether it
>> would make sense to somehow handle part of this in the generic VFS
>> code and give the user the ability to prepare a new files_struct that
>> can then be transferred to the worker thread, or something like
>> that... I'm not sure whether there's a particularly clean way to do
>> that though.
> 
> Using files_struct for that opens a can of worms you really don't
> want to touch.
> 
> Consider the following scenario with any variant of this interface:
> 	* create io_uring fd.
> 	* send an SCM_RIGHTS with that fd to AF_UNIX socket.
> 	* add the descriptor of that AF_UNIX socket to your fd
> 	* close AF_UNIX fd, close io_uring fd.
> Voila - you've got a shiny leak.  No ->release() is called for
> anyone (and you really don't want to do that on ->flush(), because
> otherwise a library helper doing e.g. system("/bin/date") will tear
> down all the io_uring in your process).  The socket is held by
> the reference you've stashed into io_uring (whichever way you do
> that).  io_uring is held by the reference you've stashed into
> SCM_RIGHTS datagram in queue of the socket.
> 
> No matter what, you need net/unix/garbage.c to be aware of that stuff.
> And getting files_struct lifetime mixed into that would be beyond
> any reason.
> 
> The only reason for doing that as a descriptor table would be
> avoiding the cost of fget() in whatever uses it, right?  Since

Right, the only purpose of this patch is to avoid doing fget/fput for
each IO.

> those are *not* the normal syscalls (and fdget() really should not
> be used anywhere other than the very top of syscall's call chain -
> that's another reason why tossing file_struct around like that
> is insane) and since the benefit is all due to the fact that it's
> *NOT* shared, *NOT* modified in parallel, etc., allowing us to
> treat file references as stable... why the hell use the descriptor
> tables at all?

This one is not a regular system call, since we don't do fget, then IO,
then fput. We hang on to it. But for the non-registered case, it's very
much just like a regular read/write system call, where we fget to do IO
on it, then fput when we are done.

> All you need is an array of struct file *, explicitly populated.
> With net/unix/garbage.c aware of such beasts.  Guess what?  We
> do have such an object already.  The one net/unix/garbage.c is
> working with.  SCM_RIGHTS datagrams, that is.
> 
> IOW, can't we give those io_uring descriptors associated struct
> unix_sock?  No socket descriptors, no struct socket (probably),
> just the AF_UNIX-specific part thereof.  Then teach
> unix_inflight()/unix_notinflight() about getting unix_sock out
> of these guys (incidentally, both would seem to benefit from
> _not_ touching unix_gc_lock in case when there's no unix_sock
> attached to file we are dealing with - I might be missing
> something very subtle about barriers there, but it doesn't
> look likely).

That might be workable, though I'm not sure we currently have helpers to
just explicitly create a unix_sock by itself. Not familiar with the
networking bits at all, I'll take a look.

> And make that (i.e. registering the descriptors) mandatory.

I don't want to make it mandatory, that's very inflexible for managing
tons of files. The registration is useful for specific cases where we
have high frequency of operations on a set of files. Besides, it'd make
the use of the API cumbersome as well for the basic case of just wanting
to do async IO.

> Hell, combine that with creating io_uring fd, if we really
> care about the syscall count.  Benefits:

We don't care about syscall count for setup as much. If you're doing
registration of a file set, you're expected to do a LOT of IO to those
files. Hence having an extra one for setup is not a concern. My concern
is just making it mandatory to do registration, I don't think that's a
workable alternative.

> 	* no file_struct refcount wanking
> 	* no fget()/fput() (conditional, at that) from kernel
> threads
> 	* no CLOEXEC-dependent anything; just the teardown
> on the final fput(), whichever way it comes.
> 	* no fun with duelling garbage collectors.

The fget/fput from a kernel thread can be solved by just hanging on to
the struct file * when we punt the IO. Right now we don't, which is a
little silly, that should be changed.

Getting rid of the files_struct{} is doable.

-- 
Jens Axboe

Re: [PATCH 13/18] io_uring: add file set registration

[Jens Axboe]

On 2/4/19 7:19 PM, Jens Axboe wrote:
> On 2/3/19 7:56 PM, Al Viro wrote:
>> On Wed, Jan 30, 2019 at 02:29:05AM +0100, Jann Horn wrote:
>>> On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>> We normally have to fget/fput for each IO we do on a file. Even with
>>>> the batching we do, the cost of the atomic inc/dec of the file usage
>>>> count adds up.
>>>>
>>>> This adds IORING_REGISTER_FILES, and IORING_UNREGISTER_FILES opcodes
>>>> for the io_uring_register(2) system call. The arguments passed in must
>>>> be an array of __s32 holding file descriptors, and nr_args should hold
>>>> the number of file descriptors the application wishes to pin for the
>>>> duration of the io_uring context (or until IORING_UNREGISTER_FILES is
>>>> called).
>>>>
>>>> When used, the application must set IOSQE_FIXED_FILE in the sqe->flags
>>>> member. Then, instead of setting sqe->fd to the real fd, it sets sqe->fd
>>>> to the index in the array passed in to IORING_REGISTER_FILES.
>>>>
>>>> Files are automatically unregistered when the io_uring context is
>>>> torn down. An application need only unregister if it wishes to
>>>> register a new set of fds.
>>>
>>> Crazy idea:
>>>
>>> Taking a step back, at a high level, basically this patch creates sort
>>> of the same difference that you get when you compare the following
>>> scenarios for normal multithreaded I/O in userspace:
>>
>>> This kinda makes me wonder whether this is really something that
>>> should be implemented specifically for the io_uring API, or whether it
>>> would make sense to somehow handle part of this in the generic VFS
>>> code and give the user the ability to prepare a new files_struct that
>>> can then be transferred to the worker thread, or something like
>>> that... I'm not sure whether there's a particularly clean way to do
>>> that though.
>>
>> Using files_struct for that opens a can of worms you really don't
>> want to touch.
>>
>> Consider the following scenario with any variant of this interface:
>> 	* create io_uring fd.
>> 	* send an SCM_RIGHTS with that fd to AF_UNIX socket.
>> 	* add the descriptor of that AF_UNIX socket to your fd
>> 	* close AF_UNIX fd, close io_uring fd.
>> Voila - you've got a shiny leak.  No ->release() is called for
>> anyone (and you really don't want to do that on ->flush(), because
>> otherwise a library helper doing e.g. system("/bin/date") will tear
>> down all the io_uring in your process).  The socket is held by
>> the reference you've stashed into io_uring (whichever way you do
>> that).  io_uring is held by the reference you've stashed into
>> SCM_RIGHTS datagram in queue of the socket.
>>
>> No matter what, you need net/unix/garbage.c to be aware of that stuff.
>> And getting files_struct lifetime mixed into that would be beyond
>> any reason.
>>
>> The only reason for doing that as a descriptor table would be
>> avoiding the cost of fget() in whatever uses it, right?  Since
> 
> Right, the only purpose of this patch is to avoid doing fget/fput for
> each IO.
> 
>> those are *not* the normal syscalls (and fdget() really should not
>> be used anywhere other than the very top of syscall's call chain -
>> that's another reason why tossing file_struct around like that
>> is insane) and since the benefit is all due to the fact that it's
>> *NOT* shared, *NOT* modified in parallel, etc., allowing us to
>> treat file references as stable... why the hell use the descriptor
>> tables at all?
> 
> This one is not a regular system call, since we don't do fget, then IO,
> then fput. We hang on to it. But for the non-registered case, it's very
> much just like a regular read/write system call, where we fget to do IO
> on it, then fput when we are done.
> 
>> All you need is an array of struct file *, explicitly populated.
>> With net/unix/garbage.c aware of such beasts.  Guess what?  We
>> do have such an object already.  The one net/unix/garbage.c is
>> working with.  SCM_RIGHTS datagrams, that is.
>>
>> IOW, can't we give those io_uring descriptors associated struct
>> unix_sock?  No socket descriptors, no struct socket (probably),
>> just the AF_UNIX-specific part thereof.  Then teach
>> unix_inflight()/unix_notinflight() about getting unix_sock out
>> of these guys (incidentally, both would seem to benefit from
>> _not_ touching unix_gc_lock in case when there's no unix_sock
>> attached to file we are dealing with - I might be missing
>> something very subtle about barriers there, but it doesn't
>> look likely).
> 
> That might be workable, though I'm not sure we currently have helpers to
> just explicitly create a unix_sock by itself. Not familiar with the
> networking bits at all, I'll take a look.
> 
>> And make that (i.e. registering the descriptors) mandatory.
> 
> I don't want to make it mandatory, that's very inflexible for managing
> tons of files. The registration is useful for specific cases where we
> have high frequency of operations on a set of files. Besides, it'd make
> the use of the API cumbersome as well for the basic case of just wanting
> to do async IO.
> 
>> Hell, combine that with creating io_uring fd, if we really
>> care about the syscall count.  Benefits:
> 
> We don't care about syscall count for setup as much. If you're doing
> registration of a file set, you're expected to do a LOT of IO to those
> files. Hence having an extra one for setup is not a concern. My concern
> is just making it mandatory to do registration, I don't think that's a
> workable alternative.
> 
>> 	* no file_struct refcount wanking
>> 	* no fget()/fput() (conditional, at that) from kernel
>> threads
>> 	* no CLOEXEC-dependent anything; just the teardown
>> on the final fput(), whichever way it comes.
>> 	* no fun with duelling garbage collectors.
> 
> The fget/fput from a kernel thread can be solved by just hanging on to
> the struct file * when we punt the IO. Right now we don't, which is a
> little silly, that should be changed.
> 
> Getting rid of the files_struct{} is doable.

OK, I've reworked the initial parts to wire up the io_uring fd to the
AF_UNIX garbage collection. As I made it to the file registration part,
I wanted to wire up that too. But I don't think there's a need for that
- if we have the io_uring fd appropriately protected, we'll be dropping
our struct file ** array index when the io_uring fd is released. That
should be adequate, we don't need the garbage collection to be aware of
those individually.

The only part I had to drop for now is the sq thread polling, as that
depends on us carrying the files_struct. I'm going to fold that in
shortly, but just make it be dependent on having registered files. That
avoids needing to fget/fput for that case, and needing registered files
for the sq side submission/polling is not a usability issue like it
would be for the "normal" use cases.

-- 
Jens Axboe

Re: [PATCH 13/18] io_uring: add file set registration

[Jens Axboe]

On 2/5/19 10:57 AM, Jens Axboe wrote:
> On 2/4/19 7:19 PM, Jens Axboe wrote:
>> On 2/3/19 7:56 PM, Al Viro wrote:
>>> On Wed, Jan 30, 2019 at 02:29:05AM +0100, Jann Horn wrote:
>>>> On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>> We normally have to fget/fput for each IO we do on a file. Even with
>>>>> the batching we do, the cost of the atomic inc/dec of the file usage
>>>>> count adds up.
>>>>>
>>>>> This adds IORING_REGISTER_FILES, and IORING_UNREGISTER_FILES opcodes
>>>>> for the io_uring_register(2) system call. The arguments passed in must
>>>>> be an array of __s32 holding file descriptors, and nr_args should hold
>>>>> the number of file descriptors the application wishes to pin for the
>>>>> duration of the io_uring context (or until IORING_UNREGISTER_FILES is
>>>>> called).
>>>>>
>>>>> When used, the application must set IOSQE_FIXED_FILE in the sqe->flags
>>>>> member. Then, instead of setting sqe->fd to the real fd, it sets sqe->fd
>>>>> to the index in the array passed in to IORING_REGISTER_FILES.
>>>>>
>>>>> Files are automatically unregistered when the io_uring context is
>>>>> torn down. An application need only unregister if it wishes to
>>>>> register a new set of fds.
>>>>
>>>> Crazy idea:
>>>>
>>>> Taking a step back, at a high level, basically this patch creates sort
>>>> of the same difference that you get when you compare the following
>>>> scenarios for normal multithreaded I/O in userspace:
>>>
>>>> This kinda makes me wonder whether this is really something that
>>>> should be implemented specifically for the io_uring API, or whether it
>>>> would make sense to somehow handle part of this in the generic VFS
>>>> code and give the user the ability to prepare a new files_struct that
>>>> can then be transferred to the worker thread, or something like
>>>> that... I'm not sure whether there's a particularly clean way to do
>>>> that though.
>>>
>>> Using files_struct for that opens a can of worms you really don't
>>> want to touch.
>>>
>>> Consider the following scenario with any variant of this interface:
>>> 	* create io_uring fd.
>>> 	* send an SCM_RIGHTS with that fd to AF_UNIX socket.
>>> 	* add the descriptor of that AF_UNIX socket to your fd
>>> 	* close AF_UNIX fd, close io_uring fd.
>>> Voila - you've got a shiny leak.  No ->release() is called for
>>> anyone (and you really don't want to do that on ->flush(), because
>>> otherwise a library helper doing e.g. system("/bin/date") will tear
>>> down all the io_uring in your process).  The socket is held by
>>> the reference you've stashed into io_uring (whichever way you do
>>> that).  io_uring is held by the reference you've stashed into
>>> SCM_RIGHTS datagram in queue of the socket.
>>>
>>> No matter what, you need net/unix/garbage.c to be aware of that stuff.
>>> And getting files_struct lifetime mixed into that would be beyond
>>> any reason.
>>>
>>> The only reason for doing that as a descriptor table would be
>>> avoiding the cost of fget() in whatever uses it, right?  Since
>>
>> Right, the only purpose of this patch is to avoid doing fget/fput for
>> each IO.
>>
>>> those are *not* the normal syscalls (and fdget() really should not
>>> be used anywhere other than the very top of syscall's call chain -
>>> that's another reason why tossing file_struct around like that
>>> is insane) and since the benefit is all due to the fact that it's
>>> *NOT* shared, *NOT* modified in parallel, etc., allowing us to
>>> treat file references as stable... why the hell use the descriptor
>>> tables at all?
>>
>> This one is not a regular system call, since we don't do fget, then IO,
>> then fput. We hang on to it. But for the non-registered case, it's very
>> much just like a regular read/write system call, where we fget to do IO
>> on it, then fput when we are done.
>>
>>> All you need is an array of struct file *, explicitly populated.
>>> With net/unix/garbage.c aware of such beasts.  Guess what?  We
>>> do have such an object already.  The one net/unix/garbage.c is
>>> working with.  SCM_RIGHTS datagrams, that is.
>>>
>>> IOW, can't we give those io_uring descriptors associated struct
>>> unix_sock?  No socket descriptors, no struct socket (probably),
>>> just the AF_UNIX-specific part thereof.  Then teach
>>> unix_inflight()/unix_notinflight() about getting unix_sock out
>>> of these guys (incidentally, both would seem to benefit from
>>> _not_ touching unix_gc_lock in case when there's no unix_sock
>>> attached to file we are dealing with - I might be missing
>>> something very subtle about barriers there, but it doesn't
>>> look likely).
>>
>> That might be workable, though I'm not sure we currently have helpers to
>> just explicitly create a unix_sock by itself. Not familiar with the
>> networking bits at all, I'll take a look.
>>
>>> And make that (i.e. registering the descriptors) mandatory.
>>
>> I don't want to make it mandatory, that's very inflexible for managing
>> tons of files. The registration is useful for specific cases where we
>> have high frequency of operations on a set of files. Besides, it'd make
>> the use of the API cumbersome as well for the basic case of just wanting
>> to do async IO.
>>
>>> Hell, combine that with creating io_uring fd, if we really
>>> care about the syscall count.  Benefits:
>>
>> We don't care about syscall count for setup as much. If you're doing
>> registration of a file set, you're expected to do a LOT of IO to those
>> files. Hence having an extra one for setup is not a concern. My concern
>> is just making it mandatory to do registration, I don't think that's a
>> workable alternative.
>>
>>> 	* no file_struct refcount wanking
>>> 	* no fget()/fput() (conditional, at that) from kernel
>>> threads
>>> 	* no CLOEXEC-dependent anything; just the teardown
>>> on the final fput(), whichever way it comes.
>>> 	* no fun with duelling garbage collectors.
>>
>> The fget/fput from a kernel thread can be solved by just hanging on to
>> the struct file * when we punt the IO. Right now we don't, which is a
>> little silly, that should be changed.
>>
>> Getting rid of the files_struct{} is doable.
> 
> OK, I've reworked the initial parts to wire up the io_uring fd to the
> AF_UNIX garbage collection. As I made it to the file registration part,
> I wanted to wire up that too. But I don't think there's a need for that
> - if we have the io_uring fd appropriately protected, we'll be dropping
> our struct file ** array index when the io_uring fd is released. That
> should be adequate, we don't need the garbage collection to be aware of
> those individually.
> 
> The only part I had to drop for now is the sq thread polling, as that
> depends on us carrying the files_struct. I'm going to fold that in
> shortly, but just make it be dependent on having registered files. That
> avoids needing to fget/fput for that case, and needing registered files
> for the sq side submission/polling is not a usability issue like it
> would be for the "normal" use cases.

Proof is in the pudding, here's the main commit introducing io_uring
and now wiring it up to the AF_UNIX garbage collection:

http://git.kernel.dk/cgit/linux-block/commit/?h=io_uring&id=158e6f42b67d0abe9ee84886b96ca8c4b3d3dfd5

How does that look? Outside of the inflight hookup, we simply retain
the file * for punting to the workqueue. This means that buffered
retry does NOT need to do fget/fput, so we don't need a files_struct
for that anymore.

In terms of the SQPOLL patch that's further down the series, it doesn't
allow that mode of operation without having fixed files enabled. That
eliminates the need for fget/fput from a kernel thread, and hence the
need to carry a files_struct around for that as well.

-- 
Jens Axboe

Re: [PATCH 13/18] io_uring: add file set registration

[Jens Axboe]

On 2/5/19 12:08 PM, Jens Axboe wrote:
> On 2/5/19 10:57 AM, Jens Axboe wrote:
>> On 2/4/19 7:19 PM, Jens Axboe wrote:
>>> On 2/3/19 7:56 PM, Al Viro wrote:
>>>> On Wed, Jan 30, 2019 at 02:29:05AM +0100, Jann Horn wrote:
>>>>> On Tue, Jan 29, 2019 at 8:27 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>> We normally have to fget/fput for each IO we do on a file. Even with
>>>>>> the batching we do, the cost of the atomic inc/dec of the file usage
>>>>>> count adds up.
>>>>>>
>>>>>> This adds IORING_REGISTER_FILES, and IORING_UNREGISTER_FILES opcodes
>>>>>> for the io_uring_register(2) system call. The arguments passed in must
>>>>>> be an array of __s32 holding file descriptors, and nr_args should hold
>>>>>> the number of file descriptors the application wishes to pin for the
>>>>>> duration of the io_uring context (or until IORING_UNREGISTER_FILES is
>>>>>> called).
>>>>>>
>>>>>> When used, the application must set IOSQE_FIXED_FILE in the sqe->flags
>>>>>> member. Then, instead of setting sqe->fd to the real fd, it sets sqe->fd
>>>>>> to the index in the array passed in to IORING_REGISTER_FILES.
>>>>>>
>>>>>> Files are automatically unregistered when the io_uring context is
>>>>>> torn down. An application need only unregister if it wishes to
>>>>>> register a new set of fds.
>>>>>
>>>>> Crazy idea:
>>>>>
>>>>> Taking a step back, at a high level, basically this patch creates sort
>>>>> of the same difference that you get when you compare the following
>>>>> scenarios for normal multithreaded I/O in userspace:
>>>>
>>>>> This kinda makes me wonder whether this is really something that
>>>>> should be implemented specifically for the io_uring API, or whether it
>>>>> would make sense to somehow handle part of this in the generic VFS
>>>>> code and give the user the ability to prepare a new files_struct that
>>>>> can then be transferred to the worker thread, or something like
>>>>> that... I'm not sure whether there's a particularly clean way to do
>>>>> that though.
>>>>
>>>> Using files_struct for that opens a can of worms you really don't
>>>> want to touch.
>>>>
>>>> Consider the following scenario with any variant of this interface:
>>>> 	* create io_uring fd.
>>>> 	* send an SCM_RIGHTS with that fd to AF_UNIX socket.
>>>> 	* add the descriptor of that AF_UNIX socket to your fd
>>>> 	* close AF_UNIX fd, close io_uring fd.
>>>> Voila - you've got a shiny leak.  No ->release() is called for
>>>> anyone (and you really don't want to do that on ->flush(), because
>>>> otherwise a library helper doing e.g. system("/bin/date") will tear
>>>> down all the io_uring in your process).  The socket is held by
>>>> the reference you've stashed into io_uring (whichever way you do
>>>> that).  io_uring is held by the reference you've stashed into
>>>> SCM_RIGHTS datagram in queue of the socket.
>>>>
>>>> No matter what, you need net/unix/garbage.c to be aware of that stuff.
>>>> And getting files_struct lifetime mixed into that would be beyond
>>>> any reason.
>>>>
>>>> The only reason for doing that as a descriptor table would be
>>>> avoiding the cost of fget() in whatever uses it, right?  Since
>>>
>>> Right, the only purpose of this patch is to avoid doing fget/fput for
>>> each IO.
>>>
>>>> those are *not* the normal syscalls (and fdget() really should not
>>>> be used anywhere other than the very top of syscall's call chain -
>>>> that's another reason why tossing file_struct around like that
>>>> is insane) and since the benefit is all due to the fact that it's
>>>> *NOT* shared, *NOT* modified in parallel, etc., allowing us to
>>>> treat file references as stable... why the hell use the descriptor
>>>> tables at all?
>>>
>>> This one is not a regular system call, since we don't do fget, then IO,
>>> then fput. We hang on to it. But for the non-registered case, it's very
>>> much just like a regular read/write system call, where we fget to do IO
>>> on it, then fput when we are done.
>>>
>>>> All you need is an array of struct file *, explicitly populated.
>>>> With net/unix/garbage.c aware of such beasts.  Guess what?  We
>>>> do have such an object already.  The one net/unix/garbage.c is
>>>> working with.  SCM_RIGHTS datagrams, that is.
>>>>
>>>> IOW, can't we give those io_uring descriptors associated struct
>>>> unix_sock?  No socket descriptors, no struct socket (probably),
>>>> just the AF_UNIX-specific part thereof.  Then teach
>>>> unix_inflight()/unix_notinflight() about getting unix_sock out
>>>> of these guys (incidentally, both would seem to benefit from
>>>> _not_ touching unix_gc_lock in case when there's no unix_sock
>>>> attached to file we are dealing with - I might be missing
>>>> something very subtle about barriers there, but it doesn't
>>>> look likely).
>>>
>>> That might be workable, though I'm not sure we currently have helpers to
>>> just explicitly create a unix_sock by itself. Not familiar with the
>>> networking bits at all, I'll take a look.
>>>
>>>> And make that (i.e. registering the descriptors) mandatory.
>>>
>>> I don't want to make it mandatory, that's very inflexible for managing
>>> tons of files. The registration is useful for specific cases where we
>>> have high frequency of operations on a set of files. Besides, it'd make
>>> the use of the API cumbersome as well for the basic case of just wanting
>>> to do async IO.
>>>
>>>> Hell, combine that with creating io_uring fd, if we really
>>>> care about the syscall count.  Benefits:
>>>
>>> We don't care about syscall count for setup as much. If you're doing
>>> registration of a file set, you're expected to do a LOT of IO to those
>>> files. Hence having an extra one for setup is not a concern. My concern
>>> is just making it mandatory to do registration, I don't think that's a
>>> workable alternative.
>>>
>>>> 	* no file_struct refcount wanking
>>>> 	* no fget()/fput() (conditional, at that) from kernel
>>>> threads
>>>> 	* no CLOEXEC-dependent anything; just the teardown
>>>> on the final fput(), whichever way it comes.
>>>> 	* no fun with duelling garbage collectors.
>>>
>>> The fget/fput from a kernel thread can be solved by just hanging on to
>>> the struct file * when we punt the IO. Right now we don't, which is a
>>> little silly, that should be changed.
>>>
>>> Getting rid of the files_struct{} is doable.
>>
>> OK, I've reworked the initial parts to wire up the io_uring fd to the
>> AF_UNIX garbage collection. As I made it to the file registration part,
>> I wanted to wire up that too. But I don't think there's a need for that
>> - if we have the io_uring fd appropriately protected, we'll be dropping
>> our struct file ** array index when the io_uring fd is released. That
>> should be adequate, we don't need the garbage collection to be aware of
>> those individually.
>>
>> The only part I had to drop for now is the sq thread polling, as that
>> depends on us carrying the files_struct. I'm going to fold that in
>> shortly, but just make it be dependent on having registered files. That
>> avoids needing to fget/fput for that case, and needing registered files
>> for the sq side submission/polling is not a usability issue like it
>> would be for the "normal" use cases.
> 
> Proof is in the pudding, here's the main commit introducing io_uring
> and now wiring it up to the AF_UNIX garbage collection:
> 
> http://git.kernel.dk/cgit/linux-block/commit/?h=io_uring&id=158e6f42b67d0abe9ee84886b96ca8c4b3d3dfd5
> 
> How does that look? Outside of the inflight hookup, we simply retain
> the file * for punting to the workqueue. This means that buffered
> retry does NOT need to do fget/fput, so we don't need a files_struct
> for that anymore.
> 
> In terms of the SQPOLL patch that's further down the series, it doesn't
> allow that mode of operation without having fixed files enabled. That
> eliminates the need for fget/fput from a kernel thread, and hence the
> need to carry a files_struct around for that as well.

This should be better, passes some basic testing, too:

http://git.kernel.dk/cgit/linux-block/commit/?h=io_uring&id=01a93aa784319a02ccfa6523371b93401c9e0073

Verified that we're grabbing the right refs, and don't hold any
ourselves. For the file registration, forbid registration of the
io_uring fd, as that is pointless and will introduce a loop regardless
of fd passing.

-- 
Jens Axboe

Re: [PATCH 13/18] io_uring: add file set registration

[Al Viro]

On Tue, Feb 05, 2019 at 12:08:25PM -0700, Jens Axboe wrote:
> Proof is in the pudding, here's the main commit introducing io_uring
> and now wiring it up to the AF_UNIX garbage collection:
> 
> http://git.kernel.dk/cgit/linux-block/commit/?h=io_uring&id=158e6f42b67d0abe9ee84886b96ca8c4b3d3dfd5
> 
> How does that look?

In a word - wrong.  Some theory: garbage collector assumes that there is
a subset of file references such that
	* for all files with such references there's an associated unix_sock.
	* all such references are stored in SCM_RIGHTS datagrams that can be
found by the garbage collector (currently: for data-bearing AF_UNIX sockets -
queued SCM_RIGHTS datagrams, for listeners - SCM_RIGHTS datagrams sent via
yet-to-be-accepted connections).
	* there is an efficient way to count those references for given file
(->inflight of the corresponding unix_sock).
	* removal of those references would render the graph acyclic.
	* file can _NOT_ be subject to syscalls unless there are references
to it outside of that subset.

unix_inflight() moves a reference into the subset
unix_notinflight() moves a reference out of the subset
activity that might add such references ought to call wait_for_unix_gc() first
(basically, to stall the massive insertions when gc is running).

Note that unix_gc() does *NOT* work in terms of dropping file references -
the primary effect is locating the SCM_RIGHTS datagrams that can be disposed
of and taking them out.  It simply won't do anything to your file references,
no matter what.  Add a printk into your ->release() and try to register io_uring
descriptor into itself, then close it.  And observe ->release() not being
called for that object.  Ever.

PS: The algorithm used by unix_gc() is basically this -

	grab unix_gc_lock (giving exclusion with unix_inflight/unix_notinflight
			   and stabilizing ->inflight counters)

	Candidates = {}
	for all unix_sock u such that u->inflight > 0
		if file corresponding to u has no other references
			Candidates += u

	/* everything else already is reachable; due to unix_gc_lock these
	   can't die or get syscall-visible references under us */
	Might_Die = Candidates

	/* invariant to maintain: for u in Candidates u->inflight will be equal
	   to the number of references from SCM_RIGHTS datagrams *except*
	   those immediately reachable from elements of Might_Die */

	for all u in Candidates
		for each file reference v in SCM_RIGHTS datagrams
					immediately reachable from u
			if v in Candidates
				v->inflight--

	To_Scan = ()	// stuff reachable from those must live
	for all u in Might_Die
		if u->inflight > 0
			queue u into To_Scan

	while To_Scan is non-empty
		u = dequeue(To_Scan)
		Might_Die -= u
		for each file reference v in SCM_RIGHTS datagrams
					immediately reachable from u
			if v in Candidates
				v->inflight++	// maintain the invariant
				if v in Might_Die
					queue v into To_Scan

	/* at that point nothing in Might_Die is reachable from the outside */

	/* restore the original values of ->inflight */
	for all u in Might_Die
		for each file reference v in SCM_RIGHTS datagrams
					immediately reachable from u
			if v in Candidates
				v->inflight++

	hitlist = ()
	for all u in Might_Die
		for each SCM_RIGHTS datagram D immediately reachable from u
			if D contains references to something in Candidates
				move D to hitlist
	/* all those datagrams would've never become reachable */

	drop unix_gc_lock

	discard all datagrams in hitlist.

Re: [PATCH 13/18] io_uring: add file set registration

[Al Viro]

On Tue, Feb 05, 2019 at 05:27:29PM -0700, Jens Axboe wrote:

> This should be better, passes some basic testing, too:
> 
> http://git.kernel.dk/cgit/linux-block/commit/?h=io_uring&id=01a93aa784319a02ccfa6523371b93401c9e0073
> 
> Verified that we're grabbing the right refs, and don't hold any
> ourselves. For the file registration, forbid registration of the
> io_uring fd, as that is pointless and will introduce a loop regardless
> of fd passing.

*shrug*

So pass it to AF_UNIX socket and register _that_ - does't change the
underlying problem.

Re: [PATCH 13/18] io_uring: add file set registration

[Jens Axboe]

On 2/5/19 5:56 PM, Al Viro wrote:
> On Tue, Feb 05, 2019 at 12:08:25PM -0700, Jens Axboe wrote:
>> Proof is in the pudding, here's the main commit introducing io_uring
>> and now wiring it up to the AF_UNIX garbage collection:
>>
>> http://git.kernel.dk/cgit/linux-block/commit/?h=io_uring&id=158e6f42b67d0abe9ee84886b96ca8c4b3d3dfd5
>>
>> How does that look?
> 
> In a word - wrong.  Some theory: garbage collector assumes that there is
> a subset of file references such that
> 	* for all files with such references there's an associated unix_sock.
> 	* all such references are stored in SCM_RIGHTS datagrams that can be
> found by the garbage collector (currently: for data-bearing AF_UNIX sockets -
> queued SCM_RIGHTS datagrams, for listeners - SCM_RIGHTS datagrams sent via
> yet-to-be-accepted connections).
> 	* there is an efficient way to count those references for given file
> (->inflight of the corresponding unix_sock).
> 	* removal of those references would render the graph acyclic.
> 	* file can _NOT_ be subject to syscalls unless there are references
> to it outside of that subset.

IOW, we cannot use fget() for registering files, and we still need fget/fput
in the fast path to retain safe use of the file. If I'm understanding you
correctly?

Just trying to ensure that I understand what you're saying here, as it seems
to refer to the file registration part, not the main patch (which did get
reworked, though).

-- 
Jens Axboe

Re: [PATCH 13/18] io_uring: add file set registration

[Jens Axboe]

On 2/5/19 6:01 PM, Al Viro wrote:
> On Tue, Feb 05, 2019 at 05:27:29PM -0700, Jens Axboe wrote:
> 
>> This should be better, passes some basic testing, too:
>>
>> http://git.kernel.dk/cgit/linux-block/commit/?h=io_uring&id=01a93aa784319a02ccfa6523371b93401c9e0073
>>
>> Verified that we're grabbing the right refs, and don't hold any
>> ourselves. For the file registration, forbid registration of the
>> io_uring fd, as that is pointless and will introduce a loop regardless
>> of fd passing.
> 
> *shrug*
> 
> So pass it to AF_UNIX socket and register _that_ - does't change the
> underlying problem.

Maybe I'm being dense here, but it's an f_op match. Should catch a
passed fd as well, correct?

With that, how can there be a loop?

-- 
Jens Axboe

Re: [PATCH 13/18] io_uring: add file set registration

[Al Viro]

On Wed, Feb 06, 2019 at 06:41:00AM -0700, Jens Axboe wrote:
> On 2/5/19 5:56 PM, Al Viro wrote:
> > On Tue, Feb 05, 2019 at 12:08:25PM -0700, Jens Axboe wrote:
> >> Proof is in the pudding, here's the main commit introducing io_uring
> >> and now wiring it up to the AF_UNIX garbage collection:
> >>
> >> http://git.kernel.dk/cgit/linux-block/commit/?h=io_uring&id=158e6f42b67d0abe9ee84886b96ca8c4b3d3dfd5
> >>
> >> How does that look?
> > 
> > In a word - wrong.  Some theory: garbage collector assumes that there is
> > a subset of file references such that
> > 	* for all files with such references there's an associated unix_sock.
> > 	* all such references are stored in SCM_RIGHTS datagrams that can be
> > found by the garbage collector (currently: for data-bearing AF_UNIX sockets -
> > queued SCM_RIGHTS datagrams, for listeners - SCM_RIGHTS datagrams sent via
> > yet-to-be-accepted connections).
> > 	* there is an efficient way to count those references for given file
> > (->inflight of the corresponding unix_sock).
> > 	* removal of those references would render the graph acyclic.
> > 	* file can _NOT_ be subject to syscalls unless there are references
> > to it outside of that subset.
> 
> IOW, we cannot use fget() for registering files, and we still need fget/fput
> in the fast path to retain safe use of the file. If I'm understanding you
> correctly?

No.  *ALL* references (inflight and not) are the same for file->f_count.
unix_inflight() does not grab a new reference to file; it only says that
reference passed to it by the caller is now an in-flight one.

OK, braindump time:

Lifetime for struct file is controlled by a simple refcount.  Destructor
(__fput() and ->release() called from it) is called once the counter hits
zero.  Which is fine, except for the situations when some struct file
references are pinned down in structures reachable only via our struct file
instance.

Each file descriptor counts as a reference.  IOW, dup() will increment
the refcount by 1, close() will decrement it, fork() will increment it
by the number of descriptors in your descriptor table refering to this
struct file, desctruction of descriptor table on exit() will decrement
by the same amount, etc.

Syscalls like read() and friends turn descriptor(s) into struct file
references.  If descriptor table is shared, that counts as a new reference
that must be dropped in the end of syscall.  If it's not shared, we are
guaranteed that the reference in descriptor table will stay around until
the end of syscall, so we may use it without bumping the file refcount.
That's the difference between fget() and fdget() - the former will
bump the refcount, the latter will try to avoid that.  Of course, if
we do not intend to drop the reference we'd acquired by the end of
syscall, we want fget() - fdget() is for transient references only.

Descriptor tables (struct files) *can* be shared; several processes
(usually - threads that share VM as well, but that's not necessary)
may be working with the same instance of struct files, so e.g. open()
in one of them is seen by the others.  The same goes for close(),
dup(), dup2(), etc.

That makes for an interesting corner case - what if two threads happen
to share a descriptor table and we have close(fd) in one of them
in the middle of read(fd, ..., ...) in another?  That's one aread where
Unices differ - one variant is to abort read(), another - to have close()
wait for read() to finish, etc.  What we do is
	* close() succeeds immediately; the reference is removed from
descriptor table and dropped.
	* if close(fd) has happened before read(fd, ...) has converted
fd to struce file reference, read() will get -EBADF.
	* otherwise, read() proceeds unmolested; the reference it has
acquired is dropped in the end of syscall.  If that's the last reference
to struct file, struct file will get shut down at that point.

clone(2) will have the child sharing descriptor table of parent if
CLONE_FILES is in the flags.  Note that in this case struct file
refcounts are not modified at all - no new references to files are
created.  Without CLONE_FILES it's the same as fork() - an independent
copy of descriptor table is created and populated by copies of references
to files, each bumping file's refcount.

unshare(2) with CLONE_FILES in flags will get a copy of descriptor table
(same as done on fork(), etc.) and switch to using it; the old reference
is dropped (note: it'll only bother with that if descriptor table used
to be shared in the first place - if we hold the only reference to
descriptor table, we'll just keep using it).

execve(2) does almost the same - if descriptor table used to be shared,
it will switch to a new copy first; in case of success the reference
to original is dropped, in case of failure we revert to original and
drop the copy.  Note that handling of close-on-exec is done in the _copy_
- the original is unaffected, so failing execve() does not disrupt the
descriptor table.

exit(2) will drop the reference to descriptor table.  When the last
reference is dropped, all file references are removed from it (and dropped).

The thread's pointer to descriptor table (current->files) is never
modified by other thread; something like ls /proc/<pid>/fd will fetch
it, so stores need to be protected (by task_lock(current)), but
the only the thread itself can do them.  

Note that while extra references to descriptor table can appear at any
time (/proc/<pid>/fd accesses, for example), such references may not
be used for modifications.  In particular, you can't switch to another
thread's descriptor table, unless it had been yours at some earlier
point _and_ you've kept a reference to it.

That's about it for descriptor tables; that, by far, is the main case
of persistently held struct file references.  Transient references
are grabbed by syscalls when they resolve descriptor to struct file *,
which ought to be done once per syscall _and_ reasonably early in
it.  Unfortunately, that's not all - there are other persistent struct
file references.

The things like "LOOP_SET_FD grabs a reference to struct file and
stashes it in ->lo_backing_file" are reasonably simple - the reference
will be dropped later, either directly by LOOP_CLR_FD (if nothing else
held the damn thing open at the time) or later in lo_release().
Note that in the latter case it's possible to get "close() of
/dev/loop descriptor drops the last reference to it, triggering
bdput(), which happens to by the last thing that held block device
opened, which triggers lo_release(), which drops the reference to
underlying struct file (almost certainly the last one by that point)".
It's still not a problem - while we have the underlying struct file
pinned by something held by another struct file, the dependencies'
graph is acyclic, so plain refcounts we are using work fine.

The same goes for the things like e.g. ecryptfs opening an underlying
(encrypted) file on open() and dropping it when the last reference
to ecryptfs file is dropped - the only difference here is that the
underlying struct file is never appearing in _anyone's_ descriptor
tables.

However, in a couple of cases we do have something trickier.

Case 1: SCM_RIGHTS datagram can be sent to an AF_UNIX socket.  That
converts the caller-supplied array of descriptors into an array of
struct file references, which gets attached to the packet we queue.
When the datagram is received, the struct file references are
moved into the descriptor table of recepient or, in case of error,
dropped.  Note that sending some descriptors in an SCM_RIGHTS datagram
and closing them is perfectly legitimate - as soon as sendmsg(2)
returns you can go ahead and close the descriptors you've sent;
the references are already acquired and you don't need to wait for
the packet to be received.

That would still be simple, if not for the fact that there's nothing
to stop you from passing AF_UNIX sockets around the same way.  In fact,
that has legitimate uses and, most of the time, doesn't cause any
complications at all.  However, it is possible to get the situation
when
	* struct file instances A and B are both AF_UNIX sockets.
	* the only reference to A is in the SCM_RIGHTS packet that
sits in the receiving queue of B.
	* the only reference to B is in the SCM_RIGHTS packet that
sits in the receiving queue of A.
That, of course, is where the pure refcounting of any kind will break.

SCM_RIGHTS datagram that contains the sole reference to A can't be
received without the recepient getting hold of a reference to B.
Which cannot happen until somebody manages to receive the SCM_RIGHTS
datagram containing the sole reference to B.  Which cannot happen
until that somebody manages to get hold of a reference to A,
which cannot happen until the first SCM_RIGHTS datagram is
received.

Dropping the last reference to A would've discarded everything in
its receiving queue, including the SCM_RIGHTS that contains the
reference to B; however, that can't happen either - the other
SCM_RIGHTS datagram would have to be either received or discarded
first, etc.

Case 2: similar, with a bit of a twist.  AF_UNIX socket used for
descriptor passing is normally set up by socket(), followed by
connect().  As soon as connect() returns, one can start sending.
Note that connect() does *NOT* wait for the recepient to call
accept() - it creates the object that will serve as low-level
part of the other end of connection (complete with received
packet queue) and stashes that object into the queue of *listener*
socket.  Subsequent accept() fetches it from there and attaches
it to a new socket, completing the setup; in the meanwhile,
sending packets works fine.  Once accept() is done, it'll see
the stuff you'd sent already in the queue of the new socket and
everything works fine.

If the listening socket gets closed without accept() having
been called, its queue is flushed, discarding all pending
connection attempts, complete with _their_ queues.  Which is
the same effect as accept() + close(), so again, normally
everything just works.

However, consider the case when we have
	* struct file instances A and B being AF_UNIX sockets.
	* A is a listener
	* B is an established connection, with the other end
yet to be accepted on A
	* the only references to A and B are in an SCM_RIGHTS
datagram sent over by A.

That SCM_RIGHTS datagram could've been received, if somebody
had managed to call accept(2) on A and recvmsg(2) on the
socket created by that accept(2).  But that can't happen
without that somebody getting hold of a reference to A in
the first place, which can't happen without having received
that SCM_RIGHTS datagram.  It can't be discarded either,
since that can't happen without dropping the last reference
to A, which sits right in it.

The difference from the previous case is that there we had
	A holds unix_sock of A
	unix_sock of A holds SCM_RIGHTS with reference to B
	B holds unix_sock of B
	unix_sock of B holds SCM_RIGHTS with reference to A
and here we have
	A holds unix_sock of A
	unix_sock of A holds the packet with reference to embryonic
unix_sock created by connect()
	that embryionic unix_sock holds SCM_RIGHTS with references
to A and B.

Dependency graph is different, but the problem is the same -
unreachable loops in it.  Note that neither class of situations
would occur normally - in the best case it's "somebody had been
doing rather convoluted descriptor passing, but everyone involved
got hit with kill -9 at the wrong time; please, make sure nothing
leaks".  That can happen, but a userland race (e.g. botched protocol
handling of some sort) or a deliberate abuse are much more likely.

Catching the loop creation is hard and paying for that every time
we do descriptor-passing would be a bad idea.  Besides, the loop
per se is not fatal - e.g if in the second case the descriptor for
A had been kept around, close(accept()) would've cleaned everything
up.  Which means that we need a garbage collector to deal with
the (rare) leaks.

Note that in both cases the leaks are caused by loops passing through
some SCM_RIGHTS datagrams that could never be received.  So locating
those, removing them from the queues they sit in and then discarding
the suckers is enough to resolve the situation.

Furthermore, in both cases the loop passes through unix_sock of
something that got sent over in an SCM_RIGHTS datagram.  So we
can do the following:
	1) keep the count of references to struct file of AF_UNIX
socket held by SCM_RIGHTS (kept in unix_sock->inflight).  Any
struct unix_sock instance without such references is not a part of
unreachable loop.  Maintain the set of unix_sock that are not excluded
by that (i.e. the ones that have some of references from SCM_RIGHTS
instances).  Note that we don't need to maintain those counts in
struct file - we care only about unix_sock here.
	2) struct file of AF_UNIX socket with some references
*NOT* from SCM_RIGHTS is also not a part of unreachable loop.
	3) for each unix_sock consider the following set of
SCM_RIGHTS: everything in queue of that unix_sock if it's
a non-listener and everything in queues of all embryonic unix_sock
in queue of a listener.  Let's call those SCM_RIGHTS associated
with our unix_sock.
	4) all SCM_RIGHTS associated with a reachable unix_sock
are reachable.
	5) if some references to struct file of a unix_sock
are in reachable SCM_RIGHTS, it is reachable.

Garbage collector starts with calculating the set of potentially
unreachable unix_sock - the ones not excluded by (1,2).
No unix_sock instances outside of that set need to be considered.

If some unix_sock in that set has counter _not_ entirely covered
by SCM_RIGHTS associated with the elements of the set, we can
conclude that there are references to it in SCM_RIGHTS associated
with something outside of our set and therefore it is reachable
and can be removed from the set.

If that process converges to a non-empty set, we know that
everything left in that set is unreachable - all references
to their struct file come from _some_ SCM_RIGHTS datagrams
and all those SCM_RIGHTS datagrams are among those that can't
be received or discarded without getting hold of a reference
to struct file of something in our set.

Everything outside of that set is reachable, so taking the
SCM_RIGHTS with references to stuff in our set (all of
them to be found among those associated with elements of
our set) out of the queues they are in will break all
unreachable loops.  Discarding the collected datagrams
will do the rest - file references in those will be
dropped, etc.

One thing to keep in mind here is the locking.  What the garbage
collector relies upon is
	* changes of ->inflight are serialized with respect to
it (on unix_gc_lock; increment done by unix_inflight(),
decrement - by unix_notinflight()).
	* references cannot be extracted from SCM_RIGHTS datagrams
while the garbage collector is running (achieved by having
unix_notinflight() done before references out of SCM_RIGHTS)
	* removal of SCM_RIGHTS associated with a socket can't
be done without a reference to that socket _outside_ of any
SCM_RIGHTS (automatically true).
	* adding SCM_RIGHTS in the middle of garbage collection
is possible, but in that case it will contain no references to
anything in the initial candidate set.

The last one is delicate.  SCM_RIGHTS creation has unix_inflight()
called for each reference we put there, so it's serialized wrt
unix_gc(); however, insertion into queue is *NOT* covered by that -
queue rescans are, but each queue has a lock of its own and they
are definitely not going to be held throughout the whole thing.

So in theory it would be possible to have
	* thread A: sendmsg() has SCM_RIGHTS created and populated,
complete with file refcount and ->inflight increments implied,
at which point it gets preempted and loses the timeslice.
	* thread B: gets to run and removes all references
from descriptor table it shares with thread A.
	* on another CPU we have garbage collector triggered;
it determines the set of potentially unreachable unix_sock and
everything in our SCM_RIGHTS _is_ in that set, now that no
other references remain.
	* on the first CPU, thread A regains the timeslice
and inserts its SCM_RIGHTS into queue.  And it does contain
references to sockets from the candidate set of running
garbage collector, confusing the hell out of it.


That is avoided by a convoluted dance around the SCM_RIGHTS creation
and insertion - we use fget() to obtain struct file references,
then _duplicate_ them in SCM_RIGHTS (bumping a refcount for each, so
we are holding *two* references), do unix_inflight() on them, then
queue the damn thing, then drop each reference we got from fget().

That way everything refered to in that SCM_RIGHTS is going to have
extra struct file references (and thus be excluded from the initial
candidate set) until after it gets inserted into queue.  In other
words, if it does appear in a queue between two passes, it's
guaranteed to contain no references to anything in the initial
canidate set.

End of braindump.
===================================================================

What you are about to add is *ANOTHER* kind of loops - references
to files in the "registered" set are pinned down by owning io_uring.

That would invalidate just about every assumption made the garbage
collector - even if you forbid to register io_uring itself, you
still can register both ends of AF_UNIX socket pair, then pass
io_uring in SCM_RIGHTS over that, then close all descriptors involved.
From the garbage collector point of view all sockets have external
references, so there's nothing to collect.  In fact those external
references are only reachable if you have a reachable reference
to io_uring, so we get a leak.

To make it work:
	* have unix_sock created for each io_uring (as your code does)
	* do *NOT* have unix_inflight() done at that point - it's
completely wrong there.
	* file set registration becomes
		* create and populate SCM_RIGHTS, with the same
fget()+grab an extra reference + unix_inflight() sequence.
Don't forget to have skb->destructor set to unix_destruct_scm
or equivalent thereof.
		* remember UNIXCB(skb).fp - that'll give you your
array of struct file *, to use in lookups.
		* queue it into your unix_sock
		* do _one_ fput() for everything you've grabbed,
dropping one of two references you've taken.
	* unregistering is simply skb_dequeue() + kfree_skb().
	* in ->release() you do sock_release(); it'll do
everything you need (including unregistering the set, etc.)

The hairiest part is the file set registration, of course -
there's almost certainly a helper or two buried in that thing;
simply exposing all the required net/unix/af_unix.c bits is
ucking fugly.

I'm not sure what you propose for non-registered descriptors -
_any_ struct file reference that outlives the return from syscall
stored in some io_uring-attached data structure is has exact same
loop (and leak) problem.  And if you mean to have it dropped before
return from syscall, I'm afraid I don't follow you.  How would
that be done?

Again, "io_uring descriptor can't be used in those requests" does
not help at all - use a socket instead, pass the io_uring fd over
it in SCM_RIGHTS and you are back to square 1.

Re: [PATCH 13/18] io_uring: add file set registration

[Al Viro]

On Wed, Feb 06, 2019 at 10:56:41AM -0700, Jens Axboe wrote:
> On 2/5/19 6:01 PM, Al Viro wrote:
> > On Tue, Feb 05, 2019 at 05:27:29PM -0700, Jens Axboe wrote:
> > 
> >> This should be better, passes some basic testing, too:
> >>
> >> http://git.kernel.dk/cgit/linux-block/commit/?h=io_uring&id=01a93aa784319a02ccfa6523371b93401c9e0073
> >>
> >> Verified that we're grabbing the right refs, and don't hold any
> >> ourselves. For the file registration, forbid registration of the
> >> io_uring fd, as that is pointless and will introduce a loop regardless
> >> of fd passing.
> > 
> > *shrug*
> > 
> > So pass it to AF_UNIX socket and register _that_ - does't change the
> > underlying problem.
> 
> Maybe I'm being dense here, but it's an f_op match. Should catch a
> passed fd as well, correct?

f_op match on _what_?

> With that, how can there be a loop?

	io_uring_fd = ....
	socketpair(PF_UNIX, SOCK_STREAM, 0, sock_fds);
	register sock_fds[0] and sock_fds[1] to io_uring_fd
	send SCM_RIGHTS datagram with io_uring_fd to sock_fds[0]
	close sock_fds[0], sock_fds[1] and io_uring_fd

And there's your unreachable loop.

Re: [PATCH 13/18] io_uring: add file set registration

[Miklos Szeredi]

On Thu, Feb 07, 2019 at 04:00:59AM +0000, Al Viro wrote:

> So in theory it would be possible to have
> 	* thread A: sendmsg() has SCM_RIGHTS created and populated,
> complete with file refcount and ->inflight increments implied,
> at which point it gets preempted and loses the timeslice.
> 	* thread B: gets to run and removes all references
> from descriptor table it shares with thread A.
> 	* on another CPU we have garbage collector triggered;
> it determines the set of potentially unreachable unix_sock and
> everything in our SCM_RIGHTS _is_ in that set, now that no
> other references remain.
> 	* on the first CPU, thread A regains the timeslice
> and inserts its SCM_RIGHTS into queue.  And it does contain
> references to sockets from the candidate set of running
> garbage collector, confusing the hell out of it.

Reminds me: long time ago there was a bug report, and based on that I found a
bug in MSG_PEEK handling (not confirmed to have fixed the reported bug).  This
fix, although pretty simple, got lost somehow.  While unix gc code is in your
head, can you please review and I'll resend through davem?

Thanks,
Miklos
---

From: Miklos Szeredi <mszeredi@redhat.com>
Subject: af_unix: fix garbage collect vs. MSG_PEEK

Gc assumes that in-flight sockets that don't have an external ref can't
gain one while unix_gc_lock is held.  That is true because
unix_notinflight() will be called before detaching fds, which takes
unix_gc_lock.

Only MSG_PEEK was somehow overlooked.  That one also clones the fds, also
keeping them in the skb.  But through MSG_PEEK an external reference can
definitely be gained without ever touching unix_gc_lock.

This patch adds unix_gc_barrier() that waits for a garbage collect run to
finish (if there is one), before actually installing the peeked in-flight
files to file descriptors.  This prevents problems from a pure in-flight
socket having its buffers modified while the garbage collect is taking
place.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Cc: <stable@vger.kernel.org>
---
 include/net/af_unix.h |    1 +
 net/unix/af_unix.c    |   15 +++++++++++++--
 net/unix/garbage.c    |    6 ++++++
 3 files changed, 20 insertions(+), 2 deletions(-)

--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -12,6 +12,7 @@ void unix_inflight(struct user_struct *u
 void unix_notinflight(struct user_struct *user, struct file *fp);
 void unix_gc(void);
 void wait_for_unix_gc(void);
+void unix_gc_barrier(void);
 struct sock *unix_get_socket(struct file *filp);
 struct sock *unix_peer_get(struct sock *sk);
 
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1547,6 +1547,17 @@ static int unix_attach_fds(struct scm_co
 	return 0;
 }
 
+static void unix_peek_fds(struct scm_cookie *scm, struct sk_buff *skb)
+{
+	scm->fp = scm_fp_dup(UNIXCB(skb).fp);
+	/*
+	 * During garbage collection it is assumed that in-flight sockets don't
+	 * get a new external reference.  So we need to wait until current run
+	 * finishes.
+	 */
+	unix_gc_barrier();
+}
+
 static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb, bool send_fds)
 {
 	int err = 0;
@@ -2171,7 +2182,7 @@ static int unix_dgram_recvmsg(struct soc
 		sk_peek_offset_fwd(sk, size);
 
 		if (UNIXCB(skb).fp)
-			scm.fp = scm_fp_dup(UNIXCB(skb).fp);
+			unix_peek_fds(&scm, skb);
 	}
 	err = (flags & MSG_TRUNC) ? skb->len - skip : size;
 
@@ -2412,7 +2423,7 @@ static int unix_stream_read_generic(stru
 			/* It is questionable, see note in unix_dgram_recvmsg.
 			 */
 			if (UNIXCB(skb).fp)
-				scm.fp = scm_fp_dup(UNIXCB(skb).fp);
+				unix_peek_fds(&scm, skb);
 
 			sk_peek_offset_fwd(sk, chunk);
 
--- a/net/unix/garbage.c
+++ b/net/unix/garbage.c
@@ -267,6 +267,12 @@ void wait_for_unix_gc(void)
 	wait_event(unix_gc_wait, gc_in_progress == false);
 }
 
+void unix_gc_barrier(void)
+{
+	spin_lock(&unix_gc_lock);
+	spin_unlock(&unix_gc_lock);
+}
+
 /* The external entry point: unix_gc() */
 void unix_gc(void)
 {

Re: [PATCH 13/18] io_uring: add file set registration

[Al Viro]

On Thu, Feb 07, 2019 at 10:22:53AM +0100, Miklos Szeredi wrote:
> On Thu, Feb 07, 2019 at 04:00:59AM +0000, Al Viro wrote:
> 
> > So in theory it would be possible to have
> > 	* thread A: sendmsg() has SCM_RIGHTS created and populated,
> > complete with file refcount and ->inflight increments implied,
> > at which point it gets preempted and loses the timeslice.
> > 	* thread B: gets to run and removes all references
> > from descriptor table it shares with thread A.
> > 	* on another CPU we have garbage collector triggered;
> > it determines the set of potentially unreachable unix_sock and
> > everything in our SCM_RIGHTS _is_ in that set, now that no
> > other references remain.
> > 	* on the first CPU, thread A regains the timeslice
> > and inserts its SCM_RIGHTS into queue.  And it does contain
> > references to sockets from the candidate set of running
> > garbage collector, confusing the hell out of it.
> 
> Reminds me: long time ago there was a bug report, and based on that I found a
> bug in MSG_PEEK handling (not confirmed to have fixed the reported bug).  This
> fix, although pretty simple, got lost somehow.  While unix gc code is in your
> head, can you please review and I'll resend through davem?

Umm...  I think the bug is real (something that looks like an eviction
candidate, but actually is referenced from the reachable queue might
get peeked via that queue, then have _its_ queue modified via new
external reference, all between two passes over that queue, confusing the
fuck out of unix_gc()), but I think the fix is an overkill...

Am I right assuming that this queue-modifying operation is accept(), removing
an embryo unix_sock from the queue of listener and thus hiding SCM_RIGHTS in
_its_ queue from scan_children()?

Let me think of it a bit, OK?  While we are at it, some questions from digging
through the current net/unix/garbage.c:
	1) is there any need for ->inflight to be atomic?  All accesses are under
unix_gc_lock, after all...
	2) pumping unix_gc on each sodding reference in SCM_RIGHTS (within
unix_notinflight()/unix_inflight()) looks atrocious... wouldn't it be better to
hold it over that loop?
	3) unix_get_socket() probably ought to be static nowadays...
	4) I wonder if in scan_inflight()/scan_children() we would be better
off with explicit switch (by enum argument) instead of an indirect call.
	5) do we really need UNIX_GC_MAYBE_CYCLE?  Looks like the only
real use is in inc_inflight_move_tail(), and AFAICS it could bloody well
have been
	u->inflight++;
	if (u->inflight == 1)	// just got from zero to non-zero
		list_move_tail(&u->link, &gc_candidates);
The logics there is "we'd found a reference to something that still was
a candidate for eviction in a reachable SCM_RIGHTS, so it's actually
reachable and needs to be scanned (and removed from the set of candidates);
move to the end of list, so that the main loop gets around to it".
If it *was* past the cursor in the list, there's no need to move it; if
we got past it, it must've had zero ->inflight (or we would've removed
it from the set back when we got past it).  Note that it's only called if
UNIX_GC_CANDIDATE is set (i.e. if it's in the initial candidate set),
so for this one ->inflight is guaranteed to mean the number of SCM_RIGHTS
refs from outside of the current candidate set...
	6) unix_get_socket() looks like it might benefit from another
FMODE bit; not sure where it's best set, though - the obvious way would
be SOCK_GC_CARES in sock->flags, set by e.g. unix_create1(), with
sock_alloc_file() propagating it into file->f_mode.  Then unix_get_socket()
would be able to bugger off with NULL for most of the references in
SCM_RIGHTS, without looking into inode...

Comments?  IIRC, you'd done the last serious round of rewriting unix_gc()
and friends...

Re: [PATCH 13/18] io_uring: add file set registration

[Miklos Szeredi]

On Thu, Feb 7, 2019 at 2:31 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Thu, Feb 07, 2019 at 10:22:53AM +0100, Miklos Szeredi wrote:
> > On Thu, Feb 07, 2019 at 04:00:59AM +0000, Al Viro wrote:
> >
> > > So in theory it would be possible to have
> > >     * thread A: sendmsg() has SCM_RIGHTS created and populated,
> > > complete with file refcount and ->inflight increments implied,
> > > at which point it gets preempted and loses the timeslice.
> > >     * thread B: gets to run and removes all references
> > > from descriptor table it shares with thread A.
> > >     * on another CPU we have garbage collector triggered;
> > > it determines the set of potentially unreachable unix_sock and
> > > everything in our SCM_RIGHTS _is_ in that set, now that no
> > > other references remain.
> > >     * on the first CPU, thread A regains the timeslice
> > > and inserts its SCM_RIGHTS into queue.  And it does contain
> > > references to sockets from the candidate set of running
> > > garbage collector, confusing the hell out of it.
> >
> > Reminds me: long time ago there was a bug report, and based on that I found a
> > bug in MSG_PEEK handling (not confirmed to have fixed the reported bug).  This
> > fix, although pretty simple, got lost somehow.  While unix gc code is in your
> > head, can you please review and I'll resend through davem?
>
> Umm...  I think the bug is real (something that looks like an eviction
> candidate, but actually is referenced from the reachable queue might
> get peeked via that queue, then have _its_ queue modified via new
> external reference, all between two passes over that queue, confusing the
> fuck out of unix_gc()), but I think the fix is an overkill...
>
> Am I right assuming that this queue-modifying operation is accept(), removing
> an embryo unix_sock from the queue of listener and thus hiding SCM_RIGHTS in
> _its_ queue from scan_children()?

Hmm... How about just receiving an SCM_RIGHTS socket (which was a
candidate) from the queue of the peeked socket?

> Let me think of it a bit, OK?  While we are at it, some questions from digging
> through the current net/unix/garbage.c:
>         1) is there any need for ->inflight to be atomic?  All accesses are under
> unix_gc_lock, after all...

Seems so.  Probably historic.

>         2) pumping unix_gc on each sodding reference in SCM_RIGHTS (within
> unix_notinflight()/unix_inflight()) looks atrocious... wouldn't it be better to
> hold it over that loop?

Sure.  I guess SCM_RIGHTS are not too performance sensitive, but
that's a trivial cleanup...


>         3) unix_get_socket() probably ought to be static nowadays...
>         4) I wonder if in scan_inflight()/scan_children() we would be better
> off with explicit switch (by enum argument) instead of an indirect call.

Right.

>         5) do we really need UNIX_GC_MAYBE_CYCLE?  Looks like the only
> real use is in inc_inflight_move_tail(), and AFAICS it could bloody well
> have been
>         u->inflight++;
>         if (u->inflight == 1)   // just got from zero to non-zero
>                 list_move_tail(&u->link, &gc_candidates);
> The logics there is "we'd found a reference to something that still was
> a candidate for eviction in a reachable SCM_RIGHTS, so it's actually
> reachable and needs to be scanned (and removed from the set of candidates);
> move to the end of list, so that the main loop gets around to it".
> If it *was* past the cursor in the list, there's no need to move it; if
> we got past it, it must've had zero ->inflight (or we would've removed
> it from the set back when we got past it).  Note that it's only called if
> UNIX_GC_CANDIDATE is set (i.e. if it's in the initial candidate set),
> so for this one ->inflight is guaranteed to mean the number of SCM_RIGHTS
> refs from outside of the current candidate set...

Right, makes sense.

>         6) unix_get_socket() looks like it might benefit from another
> FMODE bit; not sure where it's best set, though - the obvious way would
> be SOCK_GC_CARES in sock->flags, set by e.g. unix_create1(), with
> sock_alloc_file() propagating it into file->f_mode.  Then unix_get_socket()
> would be able to bugger off with NULL for most of the references in
> SCM_RIGHTS, without looking into inode...

Yep, sounds good.

Thanks,
Miklos

Re: [PATCH 13/18] io_uring: add file set registration

[Al Viro]

On Thu, Feb 07, 2019 at 03:20:06PM +0100, Miklos Szeredi wrote:

> > Am I right assuming that this queue-modifying operation is accept(), removing
> > an embryo unix_sock from the queue of listener and thus hiding SCM_RIGHTS in
> > _its_ queue from scan_children()?
> 
> Hmm... How about just receiving an SCM_RIGHTS socket (which was a
> candidate) from the queue of the peeked socket?

Right, skb unlinked before unix_detach_fds().  I was actually thinking of a stream
case, where unlink is done after that...

*grumble*

The entire thing is far too brittle for my taste ;-/

Re: [PATCH 13/18] io_uring: add file set registration

[Miklos Szeredi]

On Thu, Feb 7, 2019 at 4:20 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Thu, Feb 07, 2019 at 03:20:06PM +0100, Miklos Szeredi wrote:
>
> > > Am I right assuming that this queue-modifying operation is accept(), removing
> > > an embryo unix_sock from the queue of listener and thus hiding SCM_RIGHTS in
> > > _its_ queue from scan_children()?
> >
> > Hmm... How about just receiving an SCM_RIGHTS socket (which was a
> > candidate) from the queue of the peeked socket?
>
> Right, skb unlinked before unix_detach_fds().  I was actually thinking of a stream
> case, where unlink is done after that...
>
> *grumble*
>
> The entire thing is far too brittle for my taste ;-/

If it gets used as part of io_uring, I guess it's worth a fresh look.
I wrote it without basically any experience with either networking or
garbage collecting, so no wonder it has rough edges.

Thanks,
Miklos

Re: [PATCH 13/18] io_uring: add file set registration

[Jens Axboe]

[-- Attachment #1: Type: text/plain, Size: 1524 bytes --]

On 2/6/19 9:05 PM, Al Viro wrote:
> On Wed, Feb 06, 2019 at 10:56:41AM -0700, Jens Axboe wrote:
>> On 2/5/19 6:01 PM, Al Viro wrote:
>>> On Tue, Feb 05, 2019 at 05:27:29PM -0700, Jens Axboe wrote:
>>>
>>>> This should be better, passes some basic testing, too:
>>>>
>>>> http://git.kernel.dk/cgit/linux-block/commit/?h=io_uring&id=01a93aa784319a02ccfa6523371b93401c9e0073
>>>>
>>>> Verified that we're grabbing the right refs, and don't hold any
>>>> ourselves. For the file registration, forbid registration of the
>>>> io_uring fd, as that is pointless and will introduce a loop regardless
>>>> of fd passing.
>>>
>>> *shrug*
>>>
>>> So pass it to AF_UNIX socket and register _that_ - does't change the
>>> underlying problem.
>>
>> Maybe I'm being dense here, but it's an f_op match. Should catch a
>> passed fd as well, correct?
> 
> f_op match on _what_?
> 
>> With that, how can there be a loop?
> 
> 	io_uring_fd = ....
> 	socketpair(PF_UNIX, SOCK_STREAM, 0, sock_fds);
> 	register sock_fds[0] and sock_fds[1] to io_uring_fd
> 	send SCM_RIGHTS datagram with io_uring_fd to sock_fds[0]
> 	close sock_fds[0], sock_fds[1] and io_uring_fd
> 
> And there's your unreachable loop.

I created a small app to do just that, and ran it and verified that
->release() is called and the io_uring is released as expected. This
is run on the current -git branch, which has a socket backing for
the io_uring fd itself, but not for the registered files.

What am I missing here? Attaching the program as a reference.

-- 
Jens Axboe


[-- Attachment #2: viro.c --]
[-- Type: text/x-csrc, Size: 2969 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <stddef.h>
#include <signal.h>
#include <inttypes.h>
#include <sys/types.h>
#include <sys/syscall.h>
#include <sys/socket.h>
#include <sys/wait.h>
#include <fcntl.h>
#include <unistd.h>
#include <string.h>
#include <linux/fs.h>

struct io_sqring_offsets {
	__u32 head;
	__u32 tail;
	__u32 ring_mask;
	__u32 ring_entries;
	__u32 flags;
	__u32 dropped;
	__u32 array;
	__u32 resv[3];
};

struct io_cqring_offsets {
	__u32 head;
	__u32 tail;
	__u32 ring_mask;
	__u32 ring_entries;
	__u32 overflow;
	__u32 cqes;
	__u32 resv[4];
};

struct io_uring_params {
	__u32 sq_entries;
	__u32 cq_entries;
	__u32 flags;
	__u32 sq_thread_cpu;
	__u32 sq_thread_idle;
	__u32 resv[5];
	struct io_sqring_offsets sq_off;
	struct io_cqring_offsets cq_off;
};

#define IORING_REGISTER_FILES		2

#define __NR_sys_io_uring_setup		425
#define __NR_sys_io_uring_register	427

static int io_uring_register_files(int ring_fd, int fd1, int fd2)
{
	__s32 *fds;

	fds = calloc(2, sizeof(__s32));
	fds[0] = fd1;
	fds[1] = fd2;

	return syscall(__NR_sys_io_uring_register, ring_fd,
			IORING_REGISTER_FILES, fds, 2);
}

static int io_uring_setup(unsigned entries, struct io_uring_params *p)
{
	return syscall(__NR_sys_io_uring_setup, entries, p);
}

static int get_ring_fd(void)
{
	struct io_uring_params p;
	int fd;

	memset(&p, 0, sizeof(p));

	fd = io_uring_setup(2, &p);
	if (fd < 0) {
		perror("io_uring_setup");
		return -1;
	}

	return fd;
}

static void send_fd(int socket, int fd)
{
	char buf[CMSG_SPACE(sizeof(fd))];
	struct cmsghdr *cmsg;
	struct msghdr msg;

	memset(buf, 0, sizeof(buf));
	memset(&msg, 0, sizeof(msg));

	msg.msg_control = buf;
	msg.msg_controllen = sizeof(buf);

	cmsg = CMSG_FIRSTHDR(&msg);
	cmsg->cmsg_level = SOL_SOCKET;
	cmsg->cmsg_type = SCM_RIGHTS;
	cmsg->cmsg_len = CMSG_LEN(sizeof(fd));

	memmove(CMSG_DATA(cmsg), &fd, sizeof(fd));

	msg.msg_controllen = CMSG_SPACE(sizeof(fd));

	if (sendmsg(socket, &msg, 0) < 0)
		perror("sendmsg");
}

static int recv_fd(int socket)
{
	struct msghdr msg;
	char c_buffer[256];
	struct cmsghdr *cmsg;
	int fd;

	memset(&msg, 0, sizeof(msg));

	msg.msg_control = c_buffer;
	msg.msg_controllen = sizeof(c_buffer);

	if (recvmsg(socket, &msg, 0) < 0)
		perror("recvmsg\n");

	cmsg = CMSG_FIRSTHDR(&msg);
	memmove(&fd, CMSG_DATA(cmsg), sizeof(fd));
	return fd;
}

int main(int argc, char *argv[])
{
	int sp[2], pid, ring_fd, ret;

	if (socketpair(AF_UNIX, SOCK_DGRAM, 0, sp) != 0) {
		perror("Failed to create Unix-domain socket pair\n");
		return 1;
	}

	ring_fd = get_ring_fd();
	if (ring_fd < 0)
		return 1;

	ret = io_uring_register_files(ring_fd, sp[0], sp[1]);
	if (ret < 0) {
		perror("register files");
		return 1;
	}

	pid = fork();
	if (pid) {
		printf("Sending fd %d\n", ring_fd);

		send_fd(sp[0], ring_fd);
	} else {
		int fd;

		fd = recv_fd(sp[1]);
		printf("Got fd %d\n", fd);
		close(fd);
	}

	usleep(500000);
	close(ring_fd);
	close(sp[0]);
	close(sp[1]);
	return 0;
}

Re: [PATCH 13/18] io_uring: add file set registration

[Al Viro]

On Thu, Feb 07, 2019 at 04:27:13PM +0100, Miklos Szeredi wrote:
> On Thu, Feb 7, 2019 at 4:20 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Thu, Feb 07, 2019 at 03:20:06PM +0100, Miklos Szeredi wrote:
> >
> > > > Am I right assuming that this queue-modifying operation is accept(), removing
> > > > an embryo unix_sock from the queue of listener and thus hiding SCM_RIGHTS in
> > > > _its_ queue from scan_children()?
> > >
> > > Hmm... How about just receiving an SCM_RIGHTS socket (which was a
> > > candidate) from the queue of the peeked socket?
> >
> > Right, skb unlinked before unix_detach_fds().  I was actually thinking of a stream
> > case, where unlink is done after that...
> >
> > *grumble*
> >
> > The entire thing is far too brittle for my taste ;-/
> 
> If it gets used as part of io_uring, I guess it's worth a fresh look.
> I wrote it without basically any experience with either networking or
> garbage collecting, so no wonder it has rough edges.

It had a plenty of those edges before your changes as well - I'm not blaming you
for that mess, in case that's not obvious from what I'd written.

I'm trying to put together some formal description of what's going on in there.
Another question, BTW: updates of user->unix_inflight would seem to be movable
into the callers of unix_{not,}inflight().  Any objections against lifting
it into unix_{attach,detach}_fds()?  We do, after all, have fp->count right
there, so what's the point incrementing/decrementing the sucker one-by-one?
_And_ we are checking it right there (in too_many_unix_fds() called from
unix_attach_fds())...

Re: [PATCH 13/18] io_uring: add file set registration

[Al Viro]

On Thu, Feb 07, 2019 at 09:14:41AM -0700, Jens Axboe wrote:

> I created a small app to do just that, and ran it and verified that
> ->release() is called and the io_uring is released as expected. This
> is run on the current -git branch, which has a socket backing for
> the io_uring fd itself, but not for the registered files.
> 
> What am I missing here? Attaching the program as a reference.

> int main(int argc, char *argv[])
> {
> 	int sp[2], pid, ring_fd, ret;
> 
> 	if (socketpair(AF_UNIX, SOCK_DGRAM, 0, sp) != 0) {
> 		perror("Failed to create Unix-domain socket pair\n");
> 		return 1;
> 	}
> 
> 	ring_fd = get_ring_fd();
> 	if (ring_fd < 0)
> 		return 1;
> 
> 	ret = io_uring_register_files(ring_fd, sp[0], sp[1]);
> 	if (ret < 0) {
> 		perror("register files");
> 		return 1;
> 	}
> 
> 	pid = fork();
> 	if (pid) {
> 		printf("Sending fd %d\n", ring_fd);
> 
> 		send_fd(sp[0], ring_fd);
> 	} else {
> 		int fd;
> 
> 		fd = recv_fd(sp[1]);

Well, yes - once you receive it, you obviously have no references
sitting in SCM_RIGHTS anymore.

Get rid of recv_fd() there (along with fork(), while we are at it - what's
it for?) and just do send_fd + these 3 close (or just exit, for that matter).

Re: [PATCH 13/18] io_uring: add file set registration

[Jens Axboe]

On 2/7/19 9:30 AM, Al Viro wrote:
> On Thu, Feb 07, 2019 at 09:14:41AM -0700, Jens Axboe wrote:
> 
>> I created a small app to do just that, and ran it and verified that
>> ->release() is called and the io_uring is released as expected. This
>> is run on the current -git branch, which has a socket backing for
>> the io_uring fd itself, but not for the registered files.
>>
>> What am I missing here? Attaching the program as a reference.
> 
>> int main(int argc, char *argv[])
>> {
>> 	int sp[2], pid, ring_fd, ret;
>>
>> 	if (socketpair(AF_UNIX, SOCK_DGRAM, 0, sp) != 0) {
>> 		perror("Failed to create Unix-domain socket pair\n");
>> 		return 1;
>> 	}
>>
>> 	ring_fd = get_ring_fd();
>> 	if (ring_fd < 0)
>> 		return 1;
>>
>> 	ret = io_uring_register_files(ring_fd, sp[0], sp[1]);
>> 	if (ret < 0) {
>> 		perror("register files");
>> 		return 1;
>> 	}
>>
>> 	pid = fork();
>> 	if (pid) {
>> 		printf("Sending fd %d\n", ring_fd);
>>
>> 		send_fd(sp[0], ring_fd);
>> 	} else {
>> 		int fd;
>>
>> 		fd = recv_fd(sp[1]);
> 
> Well, yes - once you receive it, you obviously have no references
> sitting in SCM_RIGHTS anymore.
> 
> Get rid of recv_fd() there (along with fork(), while we are at it - what's
> it for?) and just do send_fd + these 3 close (or just exit, for that matter).

Ah got it, yes you are right, that does leak.

Thanks for the other (very) detailed note, I'll add a socket backing for the
registered files. I'll respond to the other details in there a bit later.

-- 
Jens Axboe

Re: [PATCH 13/18] io_uring: add file set registration

[Al Viro]

On Thu, Feb 07, 2019 at 04:30:47PM +0000, Al Viro wrote:
> Well, yes - once you receive it, you obviously have no references
> sitting in SCM_RIGHTS anymore.
> 
> Get rid of recv_fd() there (along with fork(), while we are at it - what's
> it for?) and just do send_fd + these 3 close (or just exit, for that matter).

If you pardon a bad ASCII graphics,

ring_fd            sv[0]             sv[1]                 (descriptors)
   |                |                  |
   V                V                  V
[io_uring]        [socket1]          [socket2]             (struct file)
 ^ |               ^  |                ^  |
 | |               |  |                |  |
 | \_______________/__|________________/  |                (after registering)
 |                    |                   |
 |                    V                   V
 |                 [unix_sock1]<---->[unix_sock2]          (struct unix_sock)
 |                                        |
 |                                        V
 \----------------------------------[SCM_RIGHTS]           (queue contents)

References from io_uring to other two struct file are added when you
register these suckers.  Reference from SCM_RIGHTS appears when you
do send_fd().  Now, each file has two references to it.  And
if you close all 3 descriptors (either explicitly, or by exiting)
you will be left with this graph:

[io_uring]------------\-------------------\
 ^                    |                   |
 |                    V                   V
 |                [socket1]          [socket2]
 |                    |                   |
 |                    V                   V
 |                 [unix_sock1]<---->[unix_sock2]
 |                                        |
 |                                        V
 \----------------------------------[SCM_RIGHTS]

All struct file still have references, so they are all still alive,
->release() isn't called on any of them.  And the entire thing
is obviously unreachable from the rest of data structures.

Of course recvmsg() would've removed the loop.  The point is, with
that situation you *can't* get it called - you'd need to reach
socket2 to do that and you can't do that anymore.

Re: [PATCH 13/18] io_uring: add file set registration

[Jens Axboe]

On 2/6/19 9:00 PM, Al Viro wrote:
> On Wed, Feb 06, 2019 at 06:41:00AM -0700, Jens Axboe wrote:
>> On 2/5/19 5:56 PM, Al Viro wrote:
>>> On Tue, Feb 05, 2019 at 12:08:25PM -0700, Jens Axboe wrote:
>>>> Proof is in the pudding, here's the main commit introducing io_uring
>>>> and now wiring it up to the AF_UNIX garbage collection:
>>>>
>>>> http://git.kernel.dk/cgit/linux-block/commit/?h=io_uring&id=158e6f42b67d0abe9ee84886b96ca8c4b3d3dfd5
>>>>
>>>> How does that look?
>>>
>>> In a word - wrong.  Some theory: garbage collector assumes that there is
>>> a subset of file references such that
>>> 	* for all files with such references there's an associated unix_sock.
>>> 	* all such references are stored in SCM_RIGHTS datagrams that can be
>>> found by the garbage collector (currently: for data-bearing AF_UNIX sockets -
>>> queued SCM_RIGHTS datagrams, for listeners - SCM_RIGHTS datagrams sent via
>>> yet-to-be-accepted connections).
>>> 	* there is an efficient way to count those references for given file
>>> (->inflight of the corresponding unix_sock).
>>> 	* removal of those references would render the graph acyclic.
>>> 	* file can _NOT_ be subject to syscalls unless there are references
>>> to it outside of that subset.
>>
>> IOW, we cannot use fget() for registering files, and we still need fget/fput
>> in the fast path to retain safe use of the file. If I'm understanding you
>> correctly?
> 
> No.  *ALL* references (inflight and not) are the same for file->f_count.
> unix_inflight() does not grab a new reference to file; it only says that
> reference passed to it by the caller is now an in-flight one.
> 
> OK, braindump time:

[snip]

This is great info, and I think it belongs in Documentation/ somewhere.
Not sure I've ever seen such a good and detailed dump of this before.

> What you are about to add is *ANOTHER* kind of loops - references
> to files in the "registered" set are pinned down by owning io_uring.
> 
> That would invalidate just about every assumption made the garbage
> collector - even if you forbid to register io_uring itself, you
> still can register both ends of AF_UNIX socket pair, then pass
> io_uring in SCM_RIGHTS over that, then close all descriptors involved.
> From the garbage collector point of view all sockets have external
> references, so there's nothing to collect.  In fact those external
> references are only reachable if you have a reachable reference
> to io_uring, so we get a leak.
> 
> To make it work:
> 	* have unix_sock created for each io_uring (as your code does)
> 	* do *NOT* have unix_inflight() done at that point - it's
> completely wrong there.
> 	* file set registration becomes
> 		* create and populate SCM_RIGHTS, with the same
> fget()+grab an extra reference + unix_inflight() sequence.
> Don't forget to have skb->destructor set to unix_destruct_scm
> or equivalent thereof.
> 		* remember UNIXCB(skb).fp - that'll give you your
> array of struct file *, to use in lookups.
> 		* queue it into your unix_sock
> 		* do _one_ fput() for everything you've grabbed,
> dropping one of two references you've taken.
> 	* unregistering is simply skb_dequeue() + kfree_skb().
> 	* in ->release() you do sock_release(); it'll do
> everything you need (including unregistering the set, etc.)

This is genius! I implemented this and it works. I've verified that the
previous test app failed to release due to the loop, and with this in
place, once the GC kicks in, the io_uring is released appropriately.

> The hairiest part is the file set registration, of course -
> there's almost certainly a helper or two buried in that thing;
> simply exposing all the required net/unix/af_unix.c bits is
> ucking fugly.

Outside of the modification to unix_get_socket(), the only change I had
to make was to ensure that unix_destruct_scm() is available to io_uring.
No other changes needed.

> I'm not sure what you propose for non-registered descriptors -
> _any_ struct file reference that outlives the return from syscall
> stored in some io_uring-attached data structure is has exact same
> loop (and leak) problem.  And if you mean to have it dropped before
> return from syscall, I'm afraid I don't follow you.  How would
> that be done?
> 
> Again, "io_uring descriptor can't be used in those requests" does
> not help at all - use a socket instead, pass the io_uring fd over
> it in SCM_RIGHTS and you are back to square 1.

I wasn't proposing to fput() before return, otherwise I can't hang on to
that file *.

Right now for async punt, we don't release the reference, and then we
fput() when IO completes. According to what you're saying here, that's
not good enough. Correct me if I'm wrong, but what if we:

1) For non-sock/io_uring fds, the current approach is sufficient
2) Disallow io_uring fd, doesn't make sense anyway

That leaves the socket fd, which is problematic. Should be solvable by
allocating an skb and marking that file inflight?

-- 
Jens Axboe

Re: [PATCH 13/18] io_uring: add file set registration

[Jens Axboe]

On 2/7/19 11:45 AM, Jens Axboe wrote:
> On 2/6/19 9:00 PM, Al Viro wrote:
>> On Wed, Feb 06, 2019 at 06:41:00AM -0700, Jens Axboe wrote:
>>> On 2/5/19 5:56 PM, Al Viro wrote:
>>>> On Tue, Feb 05, 2019 at 12:08:25PM -0700, Jens Axboe wrote:
>>>>> Proof is in the pudding, here's the main commit introducing io_uring
>>>>> and now wiring it up to the AF_UNIX garbage collection:
>>>>>
>>>>> http://git.kernel.dk/cgit/linux-block/commit/?h=io_uring&id=158e6f42b67d0abe9ee84886b96ca8c4b3d3dfd5
>>>>>
>>>>> How does that look?
>>>>
>>>> In a word - wrong.  Some theory: garbage collector assumes that there is
>>>> a subset of file references such that
>>>> 	* for all files with such references there's an associated unix_sock.
>>>> 	* all such references are stored in SCM_RIGHTS datagrams that can be
>>>> found by the garbage collector (currently: for data-bearing AF_UNIX sockets -
>>>> queued SCM_RIGHTS datagrams, for listeners - SCM_RIGHTS datagrams sent via
>>>> yet-to-be-accepted connections).
>>>> 	* there is an efficient way to count those references for given file
>>>> (->inflight of the corresponding unix_sock).
>>>> 	* removal of those references would render the graph acyclic.
>>>> 	* file can _NOT_ be subject to syscalls unless there are references
>>>> to it outside of that subset.
>>>
>>> IOW, we cannot use fget() for registering files, and we still need fget/fput
>>> in the fast path to retain safe use of the file. If I'm understanding you
>>> correctly?
>>
>> No.  *ALL* references (inflight and not) are the same for file->f_count.
>> unix_inflight() does not grab a new reference to file; it only says that
>> reference passed to it by the caller is now an in-flight one.
>>
>> OK, braindump time:
> 
> [snip]
> 
> This is great info, and I think it belongs in Documentation/ somewhere.
> Not sure I've ever seen such a good and detailed dump of this before.
> 
>> What you are about to add is *ANOTHER* kind of loops - references
>> to files in the "registered" set are pinned down by owning io_uring.
>>
>> That would invalidate just about every assumption made the garbage
>> collector - even if you forbid to register io_uring itself, you
>> still can register both ends of AF_UNIX socket pair, then pass
>> io_uring in SCM_RIGHTS over that, then close all descriptors involved.
>> From the garbage collector point of view all sockets have external
>> references, so there's nothing to collect.  In fact those external
>> references are only reachable if you have a reachable reference
>> to io_uring, so we get a leak.
>>
>> To make it work:
>> 	* have unix_sock created for each io_uring (as your code does)
>> 	* do *NOT* have unix_inflight() done at that point - it's
>> completely wrong there.
>> 	* file set registration becomes
>> 		* create and populate SCM_RIGHTS, with the same
>> fget()+grab an extra reference + unix_inflight() sequence.
>> Don't forget to have skb->destructor set to unix_destruct_scm
>> or equivalent thereof.
>> 		* remember UNIXCB(skb).fp - that'll give you your
>> array of struct file *, to use in lookups.
>> 		* queue it into your unix_sock
>> 		* do _one_ fput() for everything you've grabbed,
>> dropping one of two references you've taken.
>> 	* unregistering is simply skb_dequeue() + kfree_skb().
>> 	* in ->release() you do sock_release(); it'll do
>> everything you need (including unregistering the set, etc.)
> 
> This is genius! I implemented this and it works. I've verified that the
> previous test app failed to release due to the loop, and with this in
> place, once the GC kicks in, the io_uring is released appropriately.
> 
>> The hairiest part is the file set registration, of course -
>> there's almost certainly a helper or two buried in that thing;
>> simply exposing all the required net/unix/af_unix.c bits is
>> ucking fugly.
> 
> Outside of the modification to unix_get_socket(), the only change I had
> to make was to ensure that unix_destruct_scm() is available to io_uring.
> No other changes needed.
> 
>> I'm not sure what you propose for non-registered descriptors -
>> _any_ struct file reference that outlives the return from syscall
>> stored in some io_uring-attached data structure is has exact same
>> loop (and leak) problem.  And if you mean to have it dropped before
>> return from syscall, I'm afraid I don't follow you.  How would
>> that be done?
>>
>> Again, "io_uring descriptor can't be used in those requests" does
>> not help at all - use a socket instead, pass the io_uring fd over
>> it in SCM_RIGHTS and you are back to square 1.
> 
> I wasn't proposing to fput() before return, otherwise I can't hang on to
> that file *.
> 
> Right now for async punt, we don't release the reference, and then we
> fput() when IO completes. According to what you're saying here, that's
> not good enough. Correct me if I'm wrong, but what if we:
> 
> 1) For non-sock/io_uring fds, the current approach is sufficient
> 2) Disallow io_uring fd, doesn't make sense anyway
> 
> That leaves the socket fd, which is problematic. Should be solvable by
> allocating an skb and marking that file inflight?

Actually, we can just NOT set NOWAIT for types we don't support. That
means we'll never punt to async context for those.

-- 
Jens Axboe

Re: [PATCH 13/18] io_uring: add file set registration

[Miklos Szeredi]

On Thu, Feb 7, 2019 at 5:26 PM Al Viro <viro@zeniv.linux.org.uk> wrote:

> I'm trying to put together some formal description of what's going on in there.
> Another question, BTW: updates of user->unix_inflight would seem to be movable
> into the callers of unix_{not,}inflight().  Any objections against lifting
> it into unix_{attach,detach}_fds()?  We do, after all, have fp->count right
> there, so what's the point incrementing/decrementing the sucker one-by-one?
> _And_ we are checking it right there (in too_many_unix_fds() called from
> unix_attach_fds())...

I see no issues with that.

Also shouldn't the rlimit check be made against user->unix_inflight +
fp->count?  Althought I'm not quite following if fp->user can end up
different from current_user() and what should happen in that case...

Thanks,
Miklos

Re: [PATCH 13/18] io_uring: add file set registration

[Jonathan Corbet]

On Thu, 7 Feb 2019 11:45:40 -0700
Jens Axboe <axboe@kernel.dk> wrote:

> > OK, braindump time:  
> 
> [snip]
> 
> This is great info, and I think it belongs in Documentation/ somewhere.
> Not sure I've ever seen such a good and detailed dump of this before.

I suspect I might be able to make something like that happen :)  Stay
tuned.

Thanks,

jon

Re: [PATCH 13/18] io_uring: add file set registration

[Al Viro]

On Mon, Feb 11, 2019 at 08:55:33AM -0700, Jonathan Corbet wrote:
> On Thu, 7 Feb 2019 11:45:40 -0700
> Jens Axboe <axboe@kernel.dk> wrote:
> 
> > > OK, braindump time:  
> > 
> > [snip]
> > 
> > This is great info, and I think it belongs in Documentation/ somewhere.
> > Not sure I've ever seen such a good and detailed dump of this before.
> 
> I suspect I might be able to make something like that happen :)  Stay
> tuned.

There are several typos I see in there (e.g. in

        * struct file instances A and B being AF_UNIX sockets.
        * A is a listener
        * B is an established connection, with the other end
yet to be accepted on A
        * the only references to A and B are in an SCM_RIGHTS
datagram sent over by A.

the last line should be "datagram sent over by B", of course - you can't
send anything over a listener socket, to start with).

Another thing is this:

        * references cannot be extracted from SCM_RIGHTS datagrams
while the garbage collector is running (achieved by having
unix_notinflight() done before references out of SCM_RIGHTS)
        * removal of SCM_RIGHTS associated with a socket can't
be done without a reference to that socket _outside_ of any
SCM_RIGHTS (automatically true).

That's worse than a typo - that's an actual bug (see the subthread
with Miklos).  Correct version would be
	* any references extracted from SCM_RIGHTS during the
garbage collector run will not be actually used until the end
of garbage collection.  For normal recvmsg() it is guaranteed
by having unix_notinflight() called between the extraction of
scm_fp_list from the packet and doing anything else with the
references extracted.  For MSG_PEEK recvmsg() it's actually
broken and lacks synchronization; Miklos has proposed to grab
and release unix_gc_lock in those, between scm_fp_dup() and
doing anything else with the references copied.

If you turn that thing into a coherent text, I'd appreciate a chance
to take a look at the result and see if anything else needs to be
corrected...

Re: [PATCH 13/18] io_uring: add file set registration

[Jonathan Corbet]

On Mon, 11 Feb 2019 17:35:21 +0000
Al Viro <viro@zeniv.linux.org.uk> wrote:

> If you turn that thing into a coherent text, I'd appreciate a chance
> to take a look at the result and see if anything else needs to be
> corrected...

Thanks for the updated info!

I'm likely to do a watered-down version for some disreputable web site
first; after that, I'll try to do it properly for Documentation/.  You'll
certainly see the result in your inbox once it's ready to be looked at.

Thanks,

jon

[PATCH 13/18] io_uring: add file set registration

[Jens Axboe]

We normally have to fget/fput for each IO we do on a file. Even with
the batching we do, the cost of the atomic inc/dec of the file usage
count adds up.

This adds IORING_REGISTER_FILES, and IORING_UNREGISTER_FILES opcodes
for the io_uring_register(2) system call. The arguments passed in must
be an array of __s32 holding file descriptors, and nr_args should hold
the number of file descriptors the application wishes to pin for the
duration of the io_uring context (or until IORING_UNREGISTER_FILES is
called).

When used, the application must set IOSQE_FIXED_FILE in the sqe->flags
member. Then, instead of setting sqe->fd to the real fd, it sets sqe->fd
to the index in the array passed in to IORING_REGISTER_FILES.

Files are automatically unregistered when the io_uring context is
torn down. An application need only unregister if it wishes to
register a few set of fds.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/io_uring.c                 | 125 +++++++++++++++++++++++++++++-----
 include/uapi/linux/io_uring.h |   9 ++-
 2 files changed, 116 insertions(+), 18 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 63ad09e7cdc7..86add82e1008 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -100,6 +100,10 @@ struct io_ring_ctx {
 		struct fasync_struct	*cq_fasync;
 	} ____cacheline_aligned_in_smp;
 
+	/* if used, fixed file set */
+	struct file		**user_files;
+	unsigned		nr_user_files;
+
 	/* if used, fixed mapped user buffers */
 	unsigned		nr_user_bufs;
 	struct io_mapped_ubuf	*user_bufs;
@@ -137,6 +141,7 @@ struct io_kiocb {
 #define REQ_F_FORCE_NONBLOCK	1	/* inline submission attempt */
 #define REQ_F_IOPOLL_COMPLETED	2	/* polled IO has completed */
 #define REQ_F_IOPOLL_EAGAIN	4	/* submission got EAGAIN */
+#define REQ_F_FIXED_FILE	8	/* ctx owns file */
 	u64			user_data;
 	u64			res;
 
@@ -359,15 +364,17 @@ static void io_iopoll_complete(struct io_ring_ctx *ctx, unsigned int *nr_events,
 		 * Batched puts of the same file, to avoid dirtying the
 		 * file usage count multiple times, if avoidable.
 		 */
-		if (!file) {
-			file = req->rw.ki_filp;
-			file_count = 1;
-		} else if (file == req->rw.ki_filp) {
-			file_count++;
-		} else {
-			fput_many(file, file_count);
-			file = req->rw.ki_filp;
-			file_count = 1;
+		if (!(req->flags & REQ_F_FIXED_FILE)) {
+			if (!file) {
+				file = req->rw.ki_filp;
+				file_count = 1;
+			} else if (file == req->rw.ki_filp) {
+				file_count++;
+			} else {
+				fput_many(file, file_count);
+				file = req->rw.ki_filp;
+				file_count = 1;
+			}
 		}
 
 		if (to_free == ARRAY_SIZE(reqs))
@@ -504,13 +511,19 @@ static void kiocb_end_write(struct kiocb *kiocb)
 	}
 }
 
+static void io_fput(struct io_kiocb *req)
+{
+	if (!(req->flags & REQ_F_FIXED_FILE))
+		fput(req->rw.ki_filp);
+}
+
 static void io_complete_rw(struct kiocb *kiocb, long res, long res2)
 {
 	struct io_kiocb *req = container_of(kiocb, struct io_kiocb, rw);
 
 	kiocb_end_write(kiocb);
 
-	fput(kiocb->ki_filp);
+	io_fput(req);
 	io_cqring_add_event(req->ctx, req->user_data, res, 0);
 	io_free_req(req);
 }
@@ -614,7 +627,14 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	struct kiocb *kiocb = &req->rw;
 	int ret;
 
-	kiocb->ki_filp = io_file_get(state, sqe->fd);
+	if (sqe->flags & IOSQE_FIXED_FILE) {
+		if (unlikely(!ctx->user_files || sqe->fd >= ctx->nr_user_files))
+			return -EBADF;
+		kiocb->ki_filp = ctx->user_files[sqe->fd];
+		req->flags |= REQ_F_FIXED_FILE;
+	} else {
+		kiocb->ki_filp = io_file_get(state, sqe->fd);
+	}
 	if (unlikely(!kiocb->ki_filp))
 		return -EBADF;
 	kiocb->ki_pos = sqe->off;
@@ -653,7 +673,8 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	}
 	return 0;
 out_fput:
-	io_file_put(state, kiocb->ki_filp);
+	if (!(sqe->flags & IOSQE_FIXED_FILE))
+		io_file_put(state, kiocb->ki_filp);
 	return ret;
 }
 
@@ -770,7 +791,7 @@ static ssize_t io_read(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	kfree(iovec);
 out_fput:
 	if (unlikely(ret))
-		fput(file);
+		io_fput(req);
 	return ret;
 }
 
@@ -825,7 +846,7 @@ static ssize_t io_write(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	kfree(iovec);
 out_fput:
 	if (unlikely(ret))
-		fput(file);
+		io_fput(req);
 	return ret;
 }
 
@@ -863,14 +884,23 @@ static int io_fsync(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	if (unlikely(sqe->fsync_flags & ~IORING_FSYNC_DATASYNC))
 		return -EINVAL;
 
-	file = fget(sqe->fd);
+	if (sqe->flags & IOSQE_FIXED_FILE) {
+		if (unlikely(!ctx->user_files || sqe->fd >= ctx->nr_user_files))
+			return -EBADF;
+		file = ctx->user_files[sqe->fd];
+	} else {
+		file = fget(sqe->fd);
+	}
+
 	if (unlikely(!file))
 		return -EBADF;
 
 	ret = vfs_fsync_range(file, sqe->off, end > 0 ? end : LLONG_MAX,
 			sqe->fsync_flags & IORING_FSYNC_DATASYNC);
 
-	fput(file);
+	if (!(sqe->flags & IOSQE_FIXED_FILE))
+		fput(file);
+
 	io_cqring_add_event(ctx, sqe->user_data, ret, 0);
 	io_free_req(req);
 	return 0;
@@ -988,7 +1018,7 @@ static int io_submit_sqe(struct io_ring_ctx *ctx, struct sqe_submit *s,
 	ssize_t ret;
 
 	/* enforce forwards compatibility on users */
-	if (unlikely(s->sqe->flags))
+	if (unlikely(s->sqe->flags & ~IOSQE_FIXED_FILE))
 		return -EINVAL;
 
 	req = io_get_req(ctx, state);
@@ -1173,6 +1203,57 @@ static int __io_uring_enter(struct io_ring_ctx *ctx, unsigned to_submit,
 	return ret;
 }
 
+static int io_sqe_files_unregister(struct io_ring_ctx *ctx)
+{
+	int i;
+
+	if (!ctx->user_files)
+		return -ENXIO;
+
+	for (i = 0; i < ctx->nr_user_files; i++)
+		fput(ctx->user_files[i]);
+
+	kfree(ctx->user_files);
+	ctx->user_files = NULL;
+	ctx->nr_user_files = 0;
+	return 0;
+}
+
+static int io_sqe_files_register(struct io_ring_ctx *ctx, void __user *arg,
+				 unsigned nr_args)
+{
+	__s32 __user *fds = (__s32 __user *) arg;
+	int fd, i, ret = 0;
+
+	if (ctx->user_files)
+		return -EBUSY;
+	if (!nr_args)
+		return -EINVAL;
+
+	ctx->user_files = kcalloc(nr_args, sizeof(struct file *), GFP_KERNEL);
+	if (!ctx->user_files)
+		return -ENOMEM;
+
+	for (i = 0; i < nr_args; i++) {
+		ret = -EFAULT;
+		if (copy_from_user(&fd, &fds[i], sizeof(fd)))
+			break;
+
+		ctx->user_files[i] = fget(fd);
+
+		ret = -EBADF;
+		if (!ctx->user_files[i])
+			break;
+		ctx->nr_user_files++;
+		ret = 0;
+	}
+
+	if (ret)
+		io_sqe_files_unregister(ctx);
+
+	return ret;
+}
+
 static int io_sq_offload_start(struct io_ring_ctx *ctx)
 {
 	int ret;
@@ -1468,6 +1549,7 @@ static void io_ring_ctx_free(struct io_ring_ctx *ctx)
 	io_sq_offload_stop(ctx);
 	io_iopoll_reap_events(ctx);
 	io_free_scq_urings(ctx);
+	io_sqe_files_unregister(ctx);
 	io_sqe_buffer_unregister(ctx);
 	percpu_ref_exit(&ctx->refs);
 	io_unaccount_mem(ctx, ring_pages(ctx->sq_entries, ctx->cq_entries));
@@ -1780,6 +1862,15 @@ static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
 			break;
 		ret = io_sqe_buffer_unregister(ctx);
 		break;
+	case IORING_REGISTER_FILES:
+		ret = io_sqe_files_register(ctx, arg, nr_args);
+		break;
+	case IORING_UNREGISTER_FILES:
+		ret = -EINVAL;
+		if (arg || nr_args)
+			break;
+		ret = io_sqe_files_unregister(ctx);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 03ce7133c3b2..8323320077ec 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -18,7 +18,7 @@
  */
 struct io_uring_sqe {
 	__u8	opcode;		/* type of operation for this sqe */
-	__u8	flags;		/* as of now unused */
+	__u8	flags;		/* IOSQE_ flags */
 	__u16	ioprio;		/* ioprio for the request */
 	__s32	fd;		/* file descriptor to do IO on */
 	__u64	off;		/* offset into file */
@@ -35,6 +35,11 @@ struct io_uring_sqe {
 	};
 };
 
+/*
+ * sqe->flags
+ */
+#define IOSQE_FIXED_FILE	(1 << 0)	/* use fixed fileset */
+
 /*
  * io_uring_setup() flags
  */
@@ -114,5 +119,7 @@ struct io_uring_params {
  */
 #define IORING_REGISTER_BUFFERS		0
 #define IORING_UNREGISTER_BUFFERS	1
+#define IORING_REGISTER_FILES		2
+#define IORING_UNREGISTER_FILES		3
 
 #endif
-- 
2.17.1