netdev - Re: [PATCH 1/3] io_uring: add support for async work inheriting files table

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG48ez0G2y0JS9=S2KmePO3xq-5DuzgovrLFiX4TJL-G897LCA@mail.gmail.com>
Date:   Fri, 18 Oct 2019 04:41:22 +0200
From:   Jann Horn <jannh@...gle.com>
To:     Jens Axboe <axboe@...nel.dk>
Cc:     linux-block@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH 1/3] io_uring: add support for async work inheriting files table

On Fri, Oct 18, 2019 at 4:01 AM Jens Axboe <axboe@...nel.dk> wrote:
> This is in preparation for adding opcodes that need to modify files
> in a process file table, either adding new ones or closing old ones.

Closing old ones would be tricky. Basically if you call
get_files_struct() while you're between an fdget()/fdput() pair (e.g.
from sys_io_uring_enter()), you're not allowed to use that
files_struct reference to replace or close existing FDs through that
reference. (Or more accurately, if you go through fdget() with
files_struct refcount 1, you must not replace/close FDs in there in
any way until you've passed the corresponding fdput().)

You can avoid that if you ensure that you never use fdget()/fdput() in
the relevant places, only fget()/fput().

> If an opcode needs this, it must set REQ_F_NEED_FILES in the request
> structure. If work that needs to get punted to async context have this
> set, they will grab a reference to the process file table. When the
> work is completed, the reference is dropped again.
[...]
> @@ -2220,6 +2223,10 @@ static void io_sq_wq_submit_work(struct work_struct *work)
>                                 set_fs(USER_DS);
>                         }
>                 }
> +               if (s->files && !old_files) {
> +                       old_files = current->files;
> +                       current->files = s->files;
> +               }

AFAIK e.g. stuff like proc_fd_link() in procfs can concurrently call
get_files_struct() even on kernel tasks, so you should take the
task_lock(current) while fiddling with the ->files pointer.

Also, maybe I'm too tired to read this correctly, but it seems like
when io_sq_wq_submit_work() is processing multiple elements with
->files pointers, this part will only consume a reference to the first
one?

>
>                 if (!ret) {
>                         s->has_user = cur_mm != NULL;
> @@ -2312,6 +2319,11 @@ static void io_sq_wq_submit_work(struct work_struct *work)
>                 unuse_mm(cur_mm);
>                 mmput(cur_mm);
>         }
> +       if (old_files) {
> +               struct files_struct *files = current->files;
> +               current->files = old_files;
> +               put_files_struct(files);
> +       }

And then here the first files_struct reference is dropped, and the
rest of them leak?

>  }
>
>  /*
> @@ -2413,6 +2425,8 @@ static int __io_queue_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
>
>                         s->sqe = sqe_copy;
>                         memcpy(&req->submit, s, sizeof(*s));
> +                       if (req->flags & REQ_F_NEED_FILES)
> +                               req->submit.files = get_files_struct(current);

Stupid question: How does this interact with sqpoll mode? In that
case, this function is running on a kernel thread that isn't sharing
the application's files_struct, right?