* Re: [PATCH 05/18] Add io_uring IO interface
From: Jann Horn @ 2019-01-29 15:35 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-aio, linux-block, Linux API, hch, jmoyer, Avi Kivity
In-Reply-To: <20190128213538.13486-6-axboe@kernel.dk>
On Mon, Jan 28, 2019 at 10:35 PM Jens Axboe <axboe@kernel.dk> wrote:
> The submission queue (SQ) and completion queue (CQ) rings are shared
> between the application and the kernel. This eliminates the need to
> copy data back and forth to submit and complete IO.
[...]
> +static int io_import_iovec(struct io_ring_ctx *ctx, int rw,
> + const struct io_uring_sqe *sqe,
> + struct iovec **iovec, struct iov_iter *iter)
> +{
> + void __user *buf = u64_to_user_ptr(sqe->addr);
> +
> +#ifdef CONFIG_COMPAT
> + if (in_compat_syscall())
> + return compat_import_iovec(rw, buf, sqe->len, UIO_FASTIOV,
> + iovec, iter);
> +#endif
> +
> + return import_iovec(rw, buf, sqe->len, UIO_FASTIOV, iovec, iter);
> +}
This code can run in kthread context, right? I think
in_compat_syscall() might not work if this is a kthread launched by a
compat task; I don't see anything that propagates the compat flag to
the kthread.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jens Axboe @ 2019-01-29 15:20 UTC (permalink / raw)
To: Arnd Bergmann, Christoph Hellwig
Cc: Linux FS-devel Mailing List, linux-aio, linux-block, Jeff Moyer,
Avi Kivity, Linux API, linux-man, Deepa Dinamani
In-Reply-To: <CAK8P3a1x4BsFfacxOmaDXKOKmmQ2j0ypX2+mCNCbjH3E7nC4DQ@mail.gmail.com>
On 1/29/19 4:58 AM, Arnd Bergmann wrote:
> On Tue, Jan 29, 2019 at 7:30 AM Christoph Hellwig <hch@lst.de> wrote:
>>
>> On Mon, Jan 28, 2019 at 11:25:12AM -0700, Jens Axboe wrote:
>>>> Especially with poll support now in the series, don't we need a ѕigmask
>>>> argument similar to pselect/ppoll/io_pgetevents now to deal with signal
>>>> blocking during waiting for events?
>>>
>>> Is there any way to avoid passing in the sigset_t size? If it's just a
>>> 32-bit/64-bit thing, surely the in_compat_syscall() could cover it? Or
>>> are there other cases that need to be catered to?
>>
>> As far as I can tell we never look at it, never looked at it and don't
>> have any plans to look at it anytime soon. But when I tried to omit
>> it for io_pgetevents I got stong pushback and thus had to add the
>> crazy double indirection calling convention.
>
> Deepa has recently reworked the handling for the sigset_t handling
> to be more consistent. As I understand it, we only ever check the
> size argument to ensure that user and kernel space agree on
> the size, as it there had been some historic differences.
>
> If you pass a signal mask to a syscall, you should now just use the
> set_user_sigmask()/set_compat_user_sigmask()/restore_user_sigmask()
> helpers. The compat version is required for the incompatible bit order
> between 32-bit and 64-bit big-endian architectures (on little-endian, compat
> and native signal masks are compatible), and to deal with the one
> architecture that has an _NSIG defined to something other than 64:
> MIPS uses 128 because of a historic accident.
>
> I think Deepa originally suggested combining set_user_sigmask()
> and set_compat_user_sigmask(), using an in_compat_syscall()
> check. This would let us simplify a number of compat syscalls
> (ppoll, ppoll_time32, pselect6, pselect6_time32, epoll_pwait()
> io_pgetevents_time64, io_pgetevents_time32). I advised against
> changing it at the time for consistency with other compat syscalls,
> but it's something we can still do now.
That's good info. I am currently using set_user_sigmask() for it.
I'd really like to avoid having to pass in a sigset_t size for the
system call, however. What's the best way of achieving that? Can I get
away with doing something like this:
if (in_compat_syscall()) {
const compat_sigset_t __user *compat_sig;
compat_sig = (const compat_sigset_t __user *) sig;
ret = set_compat_user_sigmask(compat_sig, &ksigmask,
&sigsaved, _NSIG_WORDS);
} else {
ret = set_user_sigmask(sig, &ksigmask, &sigsaved,
_NSIG_WORDS);
}
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jens Axboe @ 2019-01-29 13:35 UTC (permalink / raw)
To: Florian Weimer
Cc: linux-aio, linux-block, linux-man, linux-api, hch, jmoyer, avi
In-Reply-To: <87tvhrpsqg.fsf@oldenburg2.str.redhat.com>
On 1/29/19 5:12 AM, Florian Weimer wrote:
> * Jens Axboe:
>
>> +#define IORING_MAX_ENTRIES 4096
>
> Where does this constant come from? Should it really be exposed to
> userspace?
Seems pretty handy for the application to know what the limit is?
>> +struct io_uring_params {
>> + __u32 sq_entries;
>> + __u32 cq_entries;
>> + __u32 flags;
>> + __u16 resv[10];
>> + struct io_sqring_offsets sq_off;
>> + struct io_cqring_offsets cq_off;
>> +};
>
>> +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];
>> +};
>
> Should the reserved fields include a __u64 member, to increase struct
> alignment on architectures that might need it in the future?
Sure, I can do that.
>> +#define IORING_ENTER_GETEVENTS (1 << 0)
>
> Should this be unsigned, to match the u32 flags argument? (Otherwise
> using 32 flags can be difficult).
Good point, I've changed them all to be unsigned.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Florian Weimer @ 2019-01-29 12:12 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-aio, linux-block, linux-man, linux-api, hch, jmoyer, avi
In-Reply-To: <20190128213538.13486-6-axboe@kernel.dk>
* Jens Axboe:
> +#define IORING_MAX_ENTRIES 4096
Where does this constant come from? Should it really be exposed to
userspace?
> +struct io_uring_params {
> + __u32 sq_entries;
> + __u32 cq_entries;
> + __u32 flags;
> + __u16 resv[10];
> + struct io_sqring_offsets sq_off;
> + struct io_cqring_offsets cq_off;
> +};
> +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];
> +};
Should the reserved fields include a __u64 member, to increase struct
alignment on architectures that might need it in the future?
> +#define IORING_ENTER_GETEVENTS (1 << 0)
Should this be unsigned, to match the u32 flags argument? (Otherwise
using 32 flags can be difficult).
Thanks,
Florian
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Arnd Bergmann @ 2019-01-29 12:05 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Andy Lutomirski, Linux FS Devel, linux-aio,
linux-block, Jeff Moyer, Avi Kivity, Linux API, linux-man
In-Reply-To: <20190129064548.GA3280@lst.de>
On Tue, Jan 29, 2019 at 7:45 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, Jan 28, 2019 at 06:20:08PM -0700, Jens Axboe wrote:
> > Sure, that would be straight forward. Is there a strong reason to do
> > so outside of "that would be nice"? It's not like it's a huge amount
> > of code.
>
> And it would be really painful for userspace. Because now you
> can't pass struct iovec through from a higher level, but will instead
> of to copy the iovec to a different type in the submission path.
Agreed. However, if we decide to add the in_compat_syscall() check
to set_user_sigmask()/set_compat_user_sigmask(), we probably want
to do the same thing in import_iovec()/compat_import_iovec() and
rw_copy_check_uvector()/compat_rw_copy_check_uvector().
Arnd
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Arnd Bergmann @ 2019-01-29 11:58 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Linux FS-devel Mailing List, linux-aio, linux-block,
Jeff Moyer, Avi Kivity, Linux API, linux-man, Deepa Dinamani
In-Reply-To: <20190129063043.GC2996@lst.de>
On Tue, Jan 29, 2019 at 7:30 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, Jan 28, 2019 at 11:25:12AM -0700, Jens Axboe wrote:
> > > Especially with poll support now in the series, don't we need a ѕigmask
> > > argument similar to pselect/ppoll/io_pgetevents now to deal with signal
> > > blocking during waiting for events?
> >
> > Is there any way to avoid passing in the sigset_t size? If it's just a
> > 32-bit/64-bit thing, surely the in_compat_syscall() could cover it? Or
> > are there other cases that need to be catered to?
>
> As far as I can tell we never look at it, never looked at it and don't
> have any plans to look at it anytime soon. But when I tried to omit
> it for io_pgetevents I got stong pushback and thus had to add the
> crazy double indirection calling convention.
Deepa has recently reworked the handling for the sigset_t handling
to be more consistent. As I understand it, we only ever check the
size argument to ensure that user and kernel space agree on
the size, as it there had been some historic differences.
If you pass a signal mask to a syscall, you should now just use the
set_user_sigmask()/set_compat_user_sigmask()/restore_user_sigmask()
helpers. The compat version is required for the incompatible bit order
between 32-bit and 64-bit big-endian architectures (on little-endian, compat
and native signal masks are compatible), and to deal with the one
architecture that has an _NSIG defined to something other than 64:
MIPS uses 128 because of a historic accident.
I think Deepa originally suggested combining set_user_sigmask()
and set_compat_user_sigmask(), using an in_compat_syscall()
check. This would let us simplify a number of compat syscalls
(ppoll, ppoll_time32, pselect6, pselect6_time32, epoll_pwait()
io_pgetevents_time64, io_pgetevents_time32). I advised against
changing it at the time for consistency with other compat syscalls,
but it's something we can still do now.
There was a recent discussion about the size of sigset_t in glibc,
which is 1024 bits there instead of 64 bits in the kernel, the idea
being that the kernel might eventually grow more signals at
some point in the future, as we did when we extended from 32
to 64 a long time ago with the addition of the rt_sig* signals;
see the thread around
https://www.spinics.net/lists/linux-snps-arc/msg04860.html
Arnd
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Bert Wesarg @ 2019-01-29 7:12 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-aio, linux-block, linux-man, linux-api, hch, jmoyer, avi
In-Reply-To: <20190128213538.13486-6-axboe@kernel.dk>
On Mon, Jan 28, 2019 at 10:35 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> The submission queue (SQ) and completion queue (CQ) rings are shared
> between the application and the kernel. This eliminates the need to
> copy data back and forth to submit and complete IO.
>
> IO submissions use the io_uring_sqe data structure, and completions
> are generated in the form of io_uring_sqe data structures. The SQ
> ring is an index into the io_uring_sqe array, which makes it possible
> to submit a batch of IOs without them being contiguous in the ring.
> The CQ ring is always contiguous, as completion events are inherently
> unordered, and hence any io_uring_cqe entry can point back to an
> arbitrary submission.
>
> Two new system calls are added for this:
>
> io_uring_setup(entries, params)
> Sets up a context for doing async IO. On success, returns a file
> descriptor that the application can mmap to gain access to the
> SQ ring, CQ ring, and io_uring_sqes.
>
> io_uring_enter(fd, to_submit, min_complete, flags, sigset, sigsetsize)
> Initiates IO against the rings mapped to this fd, or waits for
> them to complete, or both. The behavior is controlled by the
> parameters passed in. If 'to_submit' is non-zero, then we'll
> try and submit new IO. If IORING_ENTER_GETEVENTS is set, the
> kernel will wait for 'min_complete' events, if they aren't
> already available. It's valid to set IORING_ENTER_GETEVENTS
> and 'min_complete' == 0 at the same time, this allows the
> kernel to return already completed events without waiting
> for them. This is useful only for polling, as for IRQ
> driven IO, the application can just check the CQ ring
> without entering the kernel.
>
> With this setup, it's possible to do async IO with a single system
> call. Future developments will enable polled IO with this interface,
> and polled submission as well. The latter will enable an application
> to do IO without doing ANY system calls at all.
>
> For IRQ driven IO, an application only needs to enter the kernel for
> completions if it wants to wait for them to occur.
>
> Each io_uring is backed by a workqueue, to support buffered async IO
> as well. We will only punt to an async context if the command would
> need to wait for IO on the device side. Any data that can be accessed
> directly in the page cache is done inline. This avoids the slowness
> issue of usual threadpools, since cached data is accessed as quickly
> as a sync interface.
>
> Sample application: http://git.kernel.dk/cgit/fio/plain/t/io_uring.c
>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
> arch/x86/entry/syscalls/syscall_32.tbl | 2 +
> arch/x86/entry/syscalls/syscall_64.tbl | 2 +
> fs/Makefile | 1 +
> fs/io_uring.c | 1090 ++++++++++++++++++++++++
> include/linux/syscalls.h | 6 +
> include/uapi/asm-generic/unistd.h | 6 +-
> include/uapi/linux/io_uring.h | 96 +++
> init/Kconfig | 9 +
> kernel/sys_ni.c | 2 +
> 9 files changed, 1213 insertions(+), 1 deletion(-)
> create mode 100644 fs/io_uring.c
> create mode 100644 include/uapi/linux/io_uring.h
>
> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> new file mode 100644
> index 000000000000..ce65db9269a8
> --- /dev/null
> +++ b/include/uapi/linux/io_uring.h
> @@ -0,0 +1,96 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Header file for the io_uring interface.
> + *
> + * Copyright (C) 2019 Jens Axboe
> + * Copyright (C) 2019 Christoph Hellwig
> + */
> +#ifndef LINUX_IO_URING_H
> +#define LINUX_IO_URING_H
> +
> +#include <linux/fs.h>
> +#include <linux/types.h>
> +
> +#define IORING_MAX_ENTRIES 4096
> +
> +/*
> + * IO submission data structure (Submission Queue Entry)
> + */
> +struct io_uring_sqe {
> + __u8 opcode; /* type of operation for this sqe */
> + __u8 flags; /* as of now unused */
> + __u16 ioprio; /* ioprio for the request */
> + __s32 fd; /* file descriptor to do IO on */
> + __u64 off; /* offset into file */
> + __u64 addr; /* pointer to buffer or iovecs */
> + __u32 len; /* buffer size or number of iovecs */
> + union {
> + __kernel_rwf_t rw_flags;
> + __u32 __resv;
> + };
> + __u64 user_data; /* data to be passed back at completion time */
> + __u64 __pad2[3];
> +};
> +
> +#define IORING_OP_NOP 0
> +#define IORING_OP_READV 1
> +#define IORING_OP_WRITEV 2
> +
> +/*
> + * IO completion data structure (Completion Queue Entry)
> + */
> +struct io_uring_cqe {
> + __u64 user_data; /* sqe->data submission passed back */
> + __s32 res; /* result code for this event */
> + __u32 flags;
> +};
> +
> +/*
> + * Magic offsets for the application to mmap the data it needs
> + */
> +#define IORING_OFF_SQ_RING 0ULL
> +#define IORING_OFF_CQ_RING 0x8000000ULL
> +#define IORING_OFF_SQES 0x10000000ULL
> +
> +/*
> + * Filled with the offset for mmap(2)
> + */
> +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];
> +};
> +
> +/*
> + * io_uring_enter(2) flags
> + */
> +#define IORING_ENTER_GETEVENTS (1 << 0)
> +
> +/*
> + * Passed in for io_uring_setup(2). Copied back with updated info on success
> + */
> +struct io_uring_params {
> + __u32 sq_entries;
> + __u32 cq_entries;
> + __u32 flags;
> + __u16 resv[10];
> + struct io_sqring_offsets sq_off;
> + struct io_cqring_offsets cq_off;
> +};
> +
> +#endif
from a user perspective, it should always be easier if all exported
symbols and macros have a common prefix. Here it seems particular
worrisome, because of the missing 'u' in in the defines.
Best,
Bert
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Christoph Hellwig @ 2019-01-29 6:45 UTC (permalink / raw)
To: Jens Axboe
Cc: Andy Lutomirski, Christoph Hellwig, Linux FS Devel, linux-aio,
linux-block, Jeff Moyer, Avi Kivity, Linux API, linux-man
In-Reply-To: <dab3fa32-3887-3264-9fe1-bc3b7abc58b3@kernel.dk>
On Mon, Jan 28, 2019 at 06:20:08PM -0700, Jens Axboe wrote:
> Sure, that would be straight forward. Is there a strong reason to do
> so outside of "that would be nice"? It's not like it's a huge amount
> of code.
And it would be really painful for userspace. Because now you
can't pass struct iovec through from a higher level, but will instead
of to copy the iovec to a different type in the submission path.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Christoph Hellwig @ 2019-01-29 6:30 UTC (permalink / raw)
To: Jens Axboe
Cc: Christoph Hellwig, linux-fsdevel, linux-aio, linux-block, jmoyer,
avi, linux-api, linux-man
In-Reply-To: <2729ab43-b2bf-44b0-d41d-dbb495ddffbf@kernel.dk>
On Mon, Jan 28, 2019 at 11:25:12AM -0700, Jens Axboe wrote:
> > Especially with poll support now in the series, don't we need a ѕigmask
> > argument similar to pselect/ppoll/io_pgetevents now to deal with signal
> > blocking during waiting for events?
>
> Is there any way to avoid passing in the sigset_t size? If it's just a
> 32-bit/64-bit thing, surely the in_compat_syscall() could cover it? Or
> are there other cases that need to be catered to?
As far as I can tell we never look at it, never looked at it and don't
have any plans to look at it anytime soon. But when I tried to omit
it for io_pgetevents I got stong pushback and thus had to add the
crazy double indirection calling convention.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jens Axboe @ 2019-01-29 3:46 UTC (permalink / raw)
To: Jann Horn
Cc: linux-aio, linux-block, linux-man, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <CAG48ez1Ms+NPTXPj_UiQyv=2aMaMR3akCdp5SdDL3x7x7gd_ig@mail.gmail.com>
On 1/28/19 7:21 PM, Jann Horn wrote:
> Please create a local copy of the request before parsing it to keep
> the data from changing under you. Additionally, it might make sense to
> annotate every pointer to shared memory with a comment, or something
> like that, to ensure that anyone looking at the code can immediately
> see for which pointers special caution is required on access.
I took a look at the viability of NOT having to local copy the data, and
I don't think it's too bad. Local copy has a noticeable impact on the
performance, hence I'd really (REALLY) like to avoid it.
Here's something on top of the current git branch. I think I even went a
bit too far in some areas, but it should hopefully catch the cases where
we might end up double evaluating the parts of the sqe that we depend
on. For most of the sqe reading we don't really care too much. For
instance, the sqe->user_data. If the app changes this field, then it
just gets whatever passed back in cqe->user_data. That's not a kernel
issue.
For cases like addr/len etc validation, it should be sound. I'll double
check this in the morning as well, and obviously would need to be folded
in along the way.
I'd appreciate your opinion on this part, if you see any major issues
with it, or if I missed something.
diff --git a/fs/io_uring.c b/fs/io_uring.c
index e8760ad02e82..64d090300990 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -668,31 +668,35 @@ 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;
- int ret;
+ unsigned flags, ioprio;
+ int fd, ret;
- if (sqe->flags & IOSQE_FIXED_FILE) {
- if (unlikely(!ctx->user_files || sqe->fd >= ctx->nr_user_files))
+ flags = READ_ONCE(sqe->flags);
+ fd = READ_ONCE(sqe->fd);
+ if (flags & IOSQE_FIXED_FILE) {
+ if (unlikely(!ctx->user_files || fd >= ctx->nr_user_files))
return -EBADF;
- kiocb->ki_filp = ctx->user_files[sqe->fd];
+ kiocb->ki_filp = ctx->user_files[fd];
req->flags |= REQ_F_FIXED_FILE;
} else {
- kiocb->ki_filp = io_file_get(state, sqe->fd);
+ kiocb->ki_filp = io_file_get(state, fd);
}
if (unlikely(!kiocb->ki_filp))
return -EBADF;
- kiocb->ki_pos = sqe->off;
+ kiocb->ki_pos = READ_ONCE(sqe->off);
kiocb->ki_flags = iocb_flags(kiocb->ki_filp);
kiocb->ki_hint = ki_hint_validate(file_write_hint(kiocb->ki_filp));
- if (sqe->ioprio) {
- ret = ioprio_check_cap(sqe->ioprio);
+ ioprio = READ_ONCE(sqe->ioprio);
+ if (ioprio) {
+ ret = ioprio_check_cap(ioprio);
if (ret)
goto out_fput;
- kiocb->ki_ioprio = sqe->ioprio;
+ kiocb->ki_ioprio = ioprio;
} else
kiocb->ki_ioprio = get_current_ioprio();
- ret = kiocb_set_rw_flags(kiocb, sqe->rw_flags);
+ ret = kiocb_set_rw_flags(kiocb, READ_ONCE(sqe->rw_flags));
if (unlikely(ret))
goto out_fput;
if (force_nonblock) {
@@ -716,7 +720,7 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
}
return 0;
out_fput:
- if (!(sqe->flags & IOSQE_FIXED_FILE))
+ if (!(flags & IOSQE_FIXED_FILE))
io_file_put(state, kiocb->ki_filp);
return ret;
}
@@ -746,28 +750,31 @@ static int io_import_fixed(struct io_ring_ctx *ctx, int rw,
const struct io_uring_sqe *sqe,
struct iov_iter *iter)
{
+ size_t len = READ_ONCE(sqe->len);
struct io_mapped_ubuf *imu;
- size_t len = sqe->len;
+ int buf_index, index;
size_t offset;
- int index;
+ u64 buf_addr;
/* attempt to use fixed buffers without having provided iovecs */
if (unlikely(!ctx->user_bufs))
return -EFAULT;
- if (unlikely(sqe->buf_index >= ctx->nr_user_bufs))
+
+ buf_index = READ_ONCE(sqe->buf_index);
+ if (unlikely(buf_index >= ctx->nr_user_bufs))
return -EFAULT;
- index = array_index_nospec(sqe->buf_index, ctx->sq_entries);
+ index = array_index_nospec(buf_index, ctx->sq_entries);
imu = &ctx->user_bufs[index];
- if ((unsigned long) sqe->addr < imu->ubuf ||
- (unsigned long) sqe->addr + len > imu->ubuf + imu->len)
+ buf_addr = READ_ONCE(sqe->addr);
+ if (buf_addr < imu->ubuf || buf_addr + len > imu->ubuf + imu->len)
return -EFAULT;
/*
* May not be a start of buffer, set size appropriately
* and advance us to the beginning.
*/
- offset = (unsigned long) sqe->addr - imu->ubuf;
+ offset = buf_addr - imu->ubuf;
iov_iter_bvec(iter, rw, imu->bvec, imu->nr_bvecs, offset + len);
if (offset)
iov_iter_advance(iter, offset);
@@ -778,10 +785,12 @@ static int io_import_iovec(struct io_ring_ctx *ctx, int rw,
const struct io_uring_sqe *sqe,
struct iovec **iovec, struct iov_iter *iter)
{
- void __user *buf = u64_to_user_ptr(sqe->addr);
+ void __user *buf = u64_to_user_ptr(READ_ONCE(sqe->addr));
+ int opcode;
- if (sqe->opcode == IORING_OP_READ_FIXED ||
- sqe->opcode == IORING_OP_WRITE_FIXED) {
+ opcode = READ_ONCE(sqe->opcode);
+ if (opcode == IORING_OP_READ_FIXED ||
+ opcode == IORING_OP_WRITE_FIXED) {
ssize_t ret = io_import_fixed(ctx, rw, sqe, iter);
*iovec = NULL;
return ret;
@@ -789,11 +798,12 @@ static int io_import_iovec(struct io_ring_ctx *ctx, int rw,
#ifdef CONFIG_COMPAT
if (in_compat_syscall())
- return compat_import_iovec(rw, buf, sqe->len, UIO_FASTIOV,
- iovec, iter);
+ return compat_import_iovec(rw, buf, READ_ONCE(sqe->len),
+ UIO_FASTIOV, iovec, iter);
#endif
- return import_iovec(rw, buf, sqe->len, UIO_FASTIOV, iovec, iter);
+ return import_iovec(rw, buf, READ_ONCE(sqe->len), UIO_FASTIOV, iovec,
+ iter);
}
static void io_async_list_note(int rw, struct io_kiocb *req, size_t len)
@@ -939,14 +949,14 @@ static ssize_t io_write(struct io_kiocb *req, const struct io_uring_sqe *sqe,
/*
* IORING_OP_NOP just posts a completion event, nothing else.
*/
-static int io_nop(struct io_kiocb *req, const struct io_uring_sqe *sqe)
+static int io_nop(struct io_kiocb *req, u64 user_data)
{
struct io_ring_ctx *ctx = req->ctx;
if (unlikely(ctx->flags & IORING_SETUP_IOPOLL))
return -EINVAL;
- io_cqring_add_event(ctx, sqe->user_data, 0, 0);
+ io_cqring_add_event(ctx, user_data, 0, 0);
io_free_req(req);
return 0;
}
@@ -955,9 +965,12 @@ static int io_fsync(struct io_kiocb *req, const struct io_uring_sqe *sqe,
bool force_nonblock)
{
struct io_ring_ctx *ctx = req->ctx;
- loff_t end = sqe->off + sqe->len;
+ loff_t sqe_off = READ_ONCE(sqe->off);
+ loff_t sqe_len = READ_ONCE(sqe->len);
+ loff_t end = sqe_off + sqe_len;
struct file *file;
- int ret;
+ unsigned flags;
+ int ret, fd;
/* fsync always requires a blocking context */
if (force_nonblock)
@@ -970,21 +983,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;
- if (sqe->flags & IOSQE_FIXED_FILE) {
- if (unlikely(!ctx->user_files || sqe->fd >= ctx->nr_user_files))
+ fd = READ_ONCE(sqe->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[sqe->fd];
+ file = ctx->user_files[fd];
} else {
- file = fget(sqe->fd);
+ file = fget(fd);
}
if (unlikely(!file))
return -EBADF;
- ret = vfs_fsync_range(file, sqe->off, end > 0 ? end : LLONG_MAX,
+ ret = vfs_fsync_range(file, sqe_off, end > 0 ? end : LLONG_MAX,
sqe->fsync_flags & IORING_FSYNC_DATASYNC);
- if (!(sqe->flags & IOSQE_FIXED_FILE))
+ if (!(flags & IOSQE_FIXED_FILE))
fput(file);
io_cqring_add_event(ctx, sqe->user_data, ret, 0);
@@ -1037,7 +1052,7 @@ static int io_poll_remove(struct io_kiocb *req, const struct io_uring_sqe *sqe)
spin_lock_irq(&ctx->completion_lock);
list_for_each_entry_safe(poll_req, next, &ctx->cancel_list, list) {
- if (sqe->addr == poll_req->user_data) {
+ if (READ_ONCE(sqe->addr) == poll_req->user_data) {
io_poll_remove_one(poll_req);
ret = 0;
break;
@@ -1145,7 +1160,10 @@ static int io_poll_add(struct io_kiocb *req, const struct io_uring_sqe *sqe)
struct io_poll_iocb *poll = &req->poll;
struct io_ring_ctx *ctx = req->ctx;
struct io_poll_table ipt;
+ unsigned flags;
__poll_t mask;
+ u16 events;
+ int fd;
if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
return -EINVAL;
@@ -1153,15 +1171,18 @@ static int io_poll_add(struct io_kiocb *req, const struct io_uring_sqe *sqe)
return -EINVAL;
INIT_WORK(&req->work, io_poll_complete_work);
- poll->events = demangle_poll(sqe->poll_events) | EPOLLERR | EPOLLHUP;
+ events = READ_ONCE(sqe->poll_events);
+ poll->events = demangle_poll(events) | EPOLLERR | EPOLLHUP;
- if (sqe->flags & IOSQE_FIXED_FILE) {
- if (unlikely(!ctx->user_files || sqe->fd >= ctx->nr_user_files))
+ flags = READ_ONCE(sqe->flags);
+ fd = READ_ONCE(sqe->fd);
+ if (flags & IOSQE_FIXED_FILE) {
+ if (unlikely(!ctx->user_files || fd >= ctx->nr_user_files))
return -EBADF;
- poll->file = ctx->user_files[sqe->fd];
+ poll->file = ctx->user_files[fd];
req->flags |= REQ_F_FIXED_FILE;
} else {
- poll->file = fget(sqe->fd);
+ poll->file = fget(fd);
}
if (unlikely(!poll->file))
return -EBADF;
@@ -1207,7 +1228,7 @@ static int io_poll_add(struct io_kiocb *req, const struct io_uring_sqe *sqe)
out:
if (unlikely(ipt.error)) {
- if (!(sqe->flags & IOSQE_FIXED_FILE))
+ if (!(flags & IOSQE_FIXED_FILE))
fput(poll->file);
return ipt.error;
}
@@ -1224,15 +1245,17 @@ static int __io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
{
const struct io_uring_sqe *sqe = s->sqe;
ssize_t ret;
+ int opcode;
if (unlikely(s->index >= ctx->sq_entries))
return -EINVAL;
- req->user_data = sqe->user_data;
+ req->user_data = READ_ONCE(sqe->user_data);
ret = -EINVAL;
- switch (sqe->opcode) {
+ opcode = READ_ONCE(sqe->opcode);
+ switch (opcode) {
case IORING_OP_NOP:
- ret = io_nop(req, sqe);
+ ret = io_nop(req, req->user_data);
break;
case IORING_OP_READV:
if (unlikely(sqe->buf_index))
@@ -1317,7 +1340,7 @@ static void io_sq_wq_submit_work(struct work_struct *work)
restart:
do {
struct sqe_submit *s = &req->submit;
- u64 user_data = s->sqe->user_data;
+ u64 user_data = READ_ONCE(s->sqe->user_data);
/* Ensure we clear previously set forced non-block flag */
req->flags &= ~REQ_F_FORCE_NONBLOCK;
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jens Axboe @ 2019-01-29 2:54 UTC (permalink / raw)
To: Jann Horn
Cc: linux-aio, linux-block, linux-man, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <CAG48ez1Ms+NPTXPj_UiQyv=2aMaMR3akCdp5SdDL3x7x7gd_ig@mail.gmail.com>
On 1/28/19 7:21 PM, Jann Horn wrote:
> On Tue, Jan 29, 2019 at 2:07 AM Jann Horn <jannh@google.com> wrote:
>> On Mon, Jan 28, 2019 at 10:35 PM Jens Axboe <axboe@kernel.dk> wrote:
>>> The submission queue (SQ) and completion queue (CQ) rings are shared
>>> between the application and the kernel. This eliminates the need to
>>> copy data back and forth to submit and complete IO.
>> [...]
>>> +static bool io_get_sqring(struct io_ring_ctx *ctx, struct sqe_submit *s)
>>> +{
>>> + struct io_sq_ring *ring = ctx->sq_ring;
>>> + unsigned head;
>>> +
>>> + /*
>>> + * The cached sq head (or cq tail) serves two purposes:
>>> + *
>>> + * 1) allows us to batch the cost of updating the user visible
>>> + * head updates.
>>> + * 2) allows the kernel side to track the head on its own, even
>>> + * though the application is the one updating it.
>>> + */
>>> + head = ctx->cached_sq_head;
>>> + smp_rmb();
>>> + if (head == READ_ONCE(ring->r.tail))
>>> + return false;
>>> +
>>> + head = ring->array[head & ctx->sq_mask];
>>> + if (head < ctx->sq_entries) {
>>> + s->index = head;
>>> + s->sqe = &ctx->sq_sqes[head];
>>
>> ring->array can be mapped writable into userspace, right? If so: This
>> looks like a double-read issue; the compiler might assume that
>> ring->array is not modified concurrently and perform separate memory
>> accesses for the "if (head < ctx->sq_entries)" check and the
>> "&ctx->sq_sqes[head]" computation. Please use READ_ONCE()/WRITE_ONCE()
>> for all accesses to memory that userspace could concurrently modify in
>> a malicious way.
>>
>> There have been some pretty severe security bugs caused by missing
>> READ_ONCE() annotations around accesses to shared memory; see, for
>> example, https://www.blackhat.com/docs/us-16/materials/us-16-Wilhelm-Xenpwn-Breaking-Paravirtualized-Devices.pdf
>> . Slides 35-48 show how the code "switch (op->cmd)", where "op" is a
>> pointer to shared memory, allowed an attacker to break out of a Xen
>> virtual machine because the compiler generated multiple memory
>> accesses.
>
> Oh, actually, it's even worse (comments with "//" added by me):
>
> io_sq_thread() does this:
>
> do {
> // sqes[i].sqe is pointer to shared memory, result of
> // io_sqe_needs_user() is unreliable
> if (all_fixed && io_sqe_needs_user(sqes[i].sqe))
> all_fixed = false;
>
> i++;
> if (i == ARRAY_SIZE(sqes))
> break;
> } while (io_get_sqring(ctx, &sqes[i]));
> // sqes[...].sqe are pointers to shared memory
>
> io_commit_sqring(ctx);
>
> /* Unless all new commands are FIXED regions, grab mm */
> if (!all_fixed && !cur_mm) {
> mm_fault = !mmget_not_zero(ctx->sqo_mm);
> if (!mm_fault) {
> use_mm(ctx->sqo_mm);
> cur_mm = ctx->sqo_mm;
> }
> }
>
> inflight += io_submit_sqes(ctx, sqes, i, mm_fault);
>
> Then the shared memory pointers go into io_submit_sqes():
>
> static int io_submit_sqes(struct io_ring_ctx *ctx, struct sqe_submit *sqes,
> unsigned int nr, bool mm_fault)
> {
> struct io_submit_state state, *statep = NULL;
> int ret, i, submitted = 0;
> // sqes[...].sqe are pointers to shared memory
> [...]
> for (i = 0; i < nr; i++) {
> if (unlikely(mm_fault))
> ret = -EFAULT;
> else
> ret = io_submit_sqe(ctx, &sqes[i], statep);
> [...]
> }
> [...]
> }
>
> And on into io_submit_sqe():
>
> static int io_submit_sqe(struct io_ring_ctx *ctx, struct sqe_submit *s,
> struct io_submit_state *state)
> {
> [...]
> ret = __io_submit_sqe(ctx, req, s, true, state);
> [...]
> }
>
> And there it gets interesting:
>
> static int __io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
> struct sqe_submit *s, bool force_nonblock,
> struct io_submit_state *state)
> {
> // s->sqe is a pointer to shared memory
> const struct io_uring_sqe *sqe = s->sqe;
> // sqe is a pointer to shared memory
> ssize_t ret;
>
> if (unlikely(s->index >= ctx->sq_entries))
> return -EINVAL;
> req->user_data = sqe->user_data;
>
> ret = -EINVAL;
> // switch() on read from shared memory, potential instruction pointer
> // control
> switch (sqe->opcode) {
> [...]
> case IORING_OP_READV:
> if (unlikely(sqe->buf_index))
> return -EINVAL;
> ret = io_read(req, sqe, force_nonblock, state);
> break;
> [...]
> case IORING_OP_READ_FIXED:
> ret = io_read(req, sqe, force_nonblock, state);
> break;
> [...]
> }
> [...]
> }
>
> On into io_read():
>
> static ssize_t io_read(struct io_kiocb *req, const struct io_uring_sqe *sqe,
> bool force_nonblock, struct io_submit_state *state)
> {
> [...]
> // sqe is a pointer to shared memory
> ret = io_prep_rw(req, sqe, force_nonblock, state);
> [...]
> }
>
> And then io_prep_rw() does multiple reads even in the source code:
>
> static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
> bool force_nonblock, struct io_submit_state *state)
> {
> struct io_ring_ctx *ctx = req->ctx;
> struct kiocb *kiocb = &req->rw;
> int ret;
>
> // sqe is a pointer to shared memory
>
> // double-read of sqe->flags, see end of function
> if (sqe->flags & IOSQE_FIXED_FILE) {
> // double-read of sqe->fd for the bounds check and the
> array access, potential OOB pointer read
> 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;
> kiocb->ki_flags = iocb_flags(kiocb->ki_filp);
> kiocb->ki_hint = ki_hint_validate(file_write_hint(kiocb->ki_filp));
> // three reads of sqe->ioprio, bypassable capability check
> if (sqe->ioprio) {
> ret = ioprio_check_cap(sqe->ioprio);
> if (ret)
> goto out_fput;
>
> kiocb->ki_ioprio = sqe->ioprio;
> } else
> kiocb->ki_ioprio = get_current_ioprio();
> [...]
> return 0;
> out_fput:
> // double-read of sqe->flags, changed value can lead to
> unbalanced refcount
> if (!(sqe->flags & IOSQE_FIXED_FILE))
> io_file_put(state, kiocb->ki_filp);
> return ret;
> }
>
> Please create a local copy of the request before parsing it to keep
> the data from changing under you. Additionally, it might make sense to
> annotate every pointer to shared memory with a comment, or something
> like that, to ensure that anyone looking at the code can immediately
> see for which pointers special caution is required on access.
Ugh, that's pretty dire. But good catch, I'll fix that up so the
application changing sqe malicously won't affect the kernel. I hope we
can get away with NOT copying the whole sqe, but we'll do that if we
have to.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jens Axboe @ 2019-01-29 2:23 UTC (permalink / raw)
To: Jann Horn
Cc: linux-aio, linux-block, linux-man, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <CAG48ez1NoSVeb19acpi4g2hP3Ft02E1_rOFir_qpsCtgXiSv9A@mail.gmail.com>
On 1/28/19 6:32 PM, Jann Horn wrote:
> On Tue, Jan 29, 2019 at 2:31 AM Jens Axboe <axboe@kernel.dk> wrote:
>> On 1/28/19 6:29 PM, Jann Horn wrote:
>>> On Mon, Jan 28, 2019 at 10:35 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>> The submission queue (SQ) and completion queue (CQ) rings are shared
>>>> between the application and the kernel. This eliminates the need to
>>>> copy data back and forth to submit and complete IO.
>>> [...]
>>>> +static struct io_kiocb *io_get_req(struct io_ring_ctx *ctx)
>>>> +{
>>>> + struct io_kiocb *req;
>>>> +
>>>> + /* safe to use the non tryget, as we're inside ring ref already */
>>>> + percpu_ref_get(&ctx->refs);
>>>
>>> Is that true? In the path io_sq_thread() -> io_submit_sqes() ->
>>> io_submit_sqe() -> io_get_req(), I don't see anything that's already
>>> holding a reference for you. Is the worker thread holding a reference
>>> somewhere that I'm missing?
>>
>> If the thread is alive, then the ctx is alive. Before we drop the last
>> ref to the ctx (and kill it), we wait for the thread to exit.
>
> Where in __io_uring_register() are you waiting for the thread to exit
> before killing it? As far as I can tell, you come straight in from
> syscall context, take a mutex, and do percpu_ref_kill().
I was just referring to the regular shutdown path. You are right, for
the later io_uring_register() more care needs to be taken. I'll just
switch to the tryget, it's not like the non-try is a big cycle saver
by any stretch.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jens Axboe @ 2019-01-29 2:21 UTC (permalink / raw)
To: Jann Horn
Cc: linux-aio, linux-block, linux-man, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <CAG48ez1qxZLfJzK95kjh0RiQ6kvZLbQhr6Dr0EjBNQB6Fr7NXQ@mail.gmail.com>
On 1/28/19 6:07 PM, Jann Horn wrote:
> On Mon, Jan 28, 2019 at 10:35 PM Jens Axboe <axboe@kernel.dk> wrote:
>> The submission queue (SQ) and completion queue (CQ) rings are shared
>> between the application and the kernel. This eliminates the need to
>> copy data back and forth to submit and complete IO.
> [...]
>> +static bool io_get_sqring(struct io_ring_ctx *ctx, struct sqe_submit *s)
>> +{
>> + struct io_sq_ring *ring = ctx->sq_ring;
>> + unsigned head;
>> +
>> + /*
>> + * The cached sq head (or cq tail) serves two purposes:
>> + *
>> + * 1) allows us to batch the cost of updating the user visible
>> + * head updates.
>> + * 2) allows the kernel side to track the head on its own, even
>> + * though the application is the one updating it.
>> + */
>> + head = ctx->cached_sq_head;
>> + smp_rmb();
>> + if (head == READ_ONCE(ring->r.tail))
>> + return false;
>> +
>> + head = ring->array[head & ctx->sq_mask];
>> + if (head < ctx->sq_entries) {
>> + s->index = head;
>> + s->sqe = &ctx->sq_sqes[head];
>
> ring->array can be mapped writable into userspace, right? If so: This
> looks like a double-read issue; the compiler might assume that
> ring->array is not modified concurrently and perform separate memory
> accesses for the "if (head < ctx->sq_entries)" check and the
> "&ctx->sq_sqes[head]" computation. Please use READ_ONCE()/WRITE_ONCE()
> for all accesses to memory that userspace could concurrently modify in
> a malicious way.
>
> There have been some pretty severe security bugs caused by missing
> READ_ONCE() annotations around accesses to shared memory; see, for
> example, https://www.blackhat.com/docs/us-16/materials/us-16-Wilhelm-Xenpwn-Breaking-Paravirtualized-Devices.pdf
> . Slides 35-48 show how the code "switch (op->cmd)", where "op" is a
> pointer to shared memory, allowed an attacker to break out of a Xen
> virtual machine because the compiler generated multiple memory
> accesses.
Thanks, I'll update these to use READ/WRITE_ONCE.
>> + ctx->cached_sq_head++;
>> + return true;
>> + }
>> +
>> + /* drop invalid entries */
>> + ctx->cached_sq_head++;
>> + ring->dropped++;
>> + smp_wmb();
>> + return false;
>> +}
> [...]
>> +SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
>> + u32, min_complete, u32, flags, const sigset_t __user *, sig,
>> + size_t, sigsz)
>> +{
>> + struct io_ring_ctx *ctx;
>> + long ret = -EBADF;
>> + struct fd f;
>> +
>> + f = fdget(fd);
>> + if (!f.file)
>> + return -EBADF;
>> +
>> + ret = -EOPNOTSUPP;
>> + if (f.file->f_op != &io_uring_fops)
>> + goto out_fput;
>
> Oh, by the way: If you feel like it, maybe you could add a helper
> fdget_typed(int fd, struct file_operations *f_op), or something like
> that, so that there is less boilerplate code for first doing fdget(),
> then checking ->f_op, and then coding an extra bailout path for that?
> But that doesn't really have much to do with your patchset, feel free
> to ignore this comment.
That's not a bad idea, I think this is a fairly common code pattern.
I'll look into it.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jann Horn @ 2019-01-29 2:21 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-aio, linux-block, linux-man, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <CAG48ez1qxZLfJzK95kjh0RiQ6kvZLbQhr6Dr0EjBNQB6Fr7NXQ@mail.gmail.com>
On Tue, Jan 29, 2019 at 2:07 AM Jann Horn <jannh@google.com> wrote:
> On Mon, Jan 28, 2019 at 10:35 PM Jens Axboe <axboe@kernel.dk> wrote:
> > The submission queue (SQ) and completion queue (CQ) rings are shared
> > between the application and the kernel. This eliminates the need to
> > copy data back and forth to submit and complete IO.
> [...]
> > +static bool io_get_sqring(struct io_ring_ctx *ctx, struct sqe_submit *s)
> > +{
> > + struct io_sq_ring *ring = ctx->sq_ring;
> > + unsigned head;
> > +
> > + /*
> > + * The cached sq head (or cq tail) serves two purposes:
> > + *
> > + * 1) allows us to batch the cost of updating the user visible
> > + * head updates.
> > + * 2) allows the kernel side to track the head on its own, even
> > + * though the application is the one updating it.
> > + */
> > + head = ctx->cached_sq_head;
> > + smp_rmb();
> > + if (head == READ_ONCE(ring->r.tail))
> > + return false;
> > +
> > + head = ring->array[head & ctx->sq_mask];
> > + if (head < ctx->sq_entries) {
> > + s->index = head;
> > + s->sqe = &ctx->sq_sqes[head];
>
> ring->array can be mapped writable into userspace, right? If so: This
> looks like a double-read issue; the compiler might assume that
> ring->array is not modified concurrently and perform separate memory
> accesses for the "if (head < ctx->sq_entries)" check and the
> "&ctx->sq_sqes[head]" computation. Please use READ_ONCE()/WRITE_ONCE()
> for all accesses to memory that userspace could concurrently modify in
> a malicious way.
>
> There have been some pretty severe security bugs caused by missing
> READ_ONCE() annotations around accesses to shared memory; see, for
> example, https://www.blackhat.com/docs/us-16/materials/us-16-Wilhelm-Xenpwn-Breaking-Paravirtualized-Devices.pdf
> . Slides 35-48 show how the code "switch (op->cmd)", where "op" is a
> pointer to shared memory, allowed an attacker to break out of a Xen
> virtual machine because the compiler generated multiple memory
> accesses.
Oh, actually, it's even worse (comments with "//" added by me):
io_sq_thread() does this:
do {
// sqes[i].sqe is pointer to shared memory, result of
// io_sqe_needs_user() is unreliable
if (all_fixed && io_sqe_needs_user(sqes[i].sqe))
all_fixed = false;
i++;
if (i == ARRAY_SIZE(sqes))
break;
} while (io_get_sqring(ctx, &sqes[i]));
// sqes[...].sqe are pointers to shared memory
io_commit_sqring(ctx);
/* Unless all new commands are FIXED regions, grab mm */
if (!all_fixed && !cur_mm) {
mm_fault = !mmget_not_zero(ctx->sqo_mm);
if (!mm_fault) {
use_mm(ctx->sqo_mm);
cur_mm = ctx->sqo_mm;
}
}
inflight += io_submit_sqes(ctx, sqes, i, mm_fault);
Then the shared memory pointers go into io_submit_sqes():
static int io_submit_sqes(struct io_ring_ctx *ctx, struct sqe_submit *sqes,
unsigned int nr, bool mm_fault)
{
struct io_submit_state state, *statep = NULL;
int ret, i, submitted = 0;
// sqes[...].sqe are pointers to shared memory
[...]
for (i = 0; i < nr; i++) {
if (unlikely(mm_fault))
ret = -EFAULT;
else
ret = io_submit_sqe(ctx, &sqes[i], statep);
[...]
}
[...]
}
And on into io_submit_sqe():
static int io_submit_sqe(struct io_ring_ctx *ctx, struct sqe_submit *s,
struct io_submit_state *state)
{
[...]
ret = __io_submit_sqe(ctx, req, s, true, state);
[...]
}
And there it gets interesting:
static int __io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
struct sqe_submit *s, bool force_nonblock,
struct io_submit_state *state)
{
// s->sqe is a pointer to shared memory
const struct io_uring_sqe *sqe = s->sqe;
// sqe is a pointer to shared memory
ssize_t ret;
if (unlikely(s->index >= ctx->sq_entries))
return -EINVAL;
req->user_data = sqe->user_data;
ret = -EINVAL;
// switch() on read from shared memory, potential instruction pointer
// control
switch (sqe->opcode) {
[...]
case IORING_OP_READV:
if (unlikely(sqe->buf_index))
return -EINVAL;
ret = io_read(req, sqe, force_nonblock, state);
break;
[...]
case IORING_OP_READ_FIXED:
ret = io_read(req, sqe, force_nonblock, state);
break;
[...]
}
[...]
}
On into io_read():
static ssize_t io_read(struct io_kiocb *req, const struct io_uring_sqe *sqe,
bool force_nonblock, struct io_submit_state *state)
{
[...]
// sqe is a pointer to shared memory
ret = io_prep_rw(req, sqe, force_nonblock, state);
[...]
}
And then io_prep_rw() does multiple reads even in the source code:
static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
bool force_nonblock, struct io_submit_state *state)
{
struct io_ring_ctx *ctx = req->ctx;
struct kiocb *kiocb = &req->rw;
int ret;
// sqe is a pointer to shared memory
// double-read of sqe->flags, see end of function
if (sqe->flags & IOSQE_FIXED_FILE) {
// double-read of sqe->fd for the bounds check and the
array access, potential OOB pointer read
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;
kiocb->ki_flags = iocb_flags(kiocb->ki_filp);
kiocb->ki_hint = ki_hint_validate(file_write_hint(kiocb->ki_filp));
// three reads of sqe->ioprio, bypassable capability check
if (sqe->ioprio) {
ret = ioprio_check_cap(sqe->ioprio);
if (ret)
goto out_fput;
kiocb->ki_ioprio = sqe->ioprio;
} else
kiocb->ki_ioprio = get_current_ioprio();
[...]
return 0;
out_fput:
// double-read of sqe->flags, changed value can lead to
unbalanced refcount
if (!(sqe->flags & IOSQE_FIXED_FILE))
io_file_put(state, kiocb->ki_filp);
return ret;
}
Please create a local copy of the request before parsing it to keep
the data from changing under you. Additionally, it might make sense to
annotate every pointer to shared memory with a comment, or something
like that, to ensure that anyone looking at the code can immediately
see for which pointers special caution is required on access.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jann Horn @ 2019-01-29 1:32 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-aio, linux-block, linux-man, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <9170f97f-bea3-04b9-71db-a0f250d1f610@kernel.dk>
On Tue, Jan 29, 2019 at 2:31 AM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/28/19 6:29 PM, Jann Horn wrote:
> > On Mon, Jan 28, 2019 at 10:35 PM Jens Axboe <axboe@kernel.dk> wrote:
> >> The submission queue (SQ) and completion queue (CQ) rings are shared
> >> between the application and the kernel. This eliminates the need to
> >> copy data back and forth to submit and complete IO.
> > [...]
> >> +static struct io_kiocb *io_get_req(struct io_ring_ctx *ctx)
> >> +{
> >> + struct io_kiocb *req;
> >> +
> >> + /* safe to use the non tryget, as we're inside ring ref already */
> >> + percpu_ref_get(&ctx->refs);
> >
> > Is that true? In the path io_sq_thread() -> io_submit_sqes() ->
> > io_submit_sqe() -> io_get_req(), I don't see anything that's already
> > holding a reference for you. Is the worker thread holding a reference
> > somewhere that I'm missing?
>
> If the thread is alive, then the ctx is alive. Before we drop the last
> ref to the ctx (and kill it), we wait for the thread to exit.
Where in __io_uring_register() are you waiting for the thread to exit
before killing it? As far as I can tell, you come straight in from
syscall context, take a mutex, and do percpu_ref_kill().
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jens Axboe @ 2019-01-29 1:31 UTC (permalink / raw)
To: Jann Horn
Cc: linux-aio, linux-block, linux-man, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <CAG48ez2gvgBUKNihnjEWeBv1WZiXWwfxcA2Wf90n8GHLket2Mg@mail.gmail.com>
On 1/28/19 6:29 PM, Jann Horn wrote:
> On Mon, Jan 28, 2019 at 10:35 PM Jens Axboe <axboe@kernel.dk> wrote:
>> The submission queue (SQ) and completion queue (CQ) rings are shared
>> between the application and the kernel. This eliminates the need to
>> copy data back and forth to submit and complete IO.
> [...]
>> +static struct io_kiocb *io_get_req(struct io_ring_ctx *ctx)
>> +{
>> + struct io_kiocb *req;
>> +
>> + /* safe to use the non tryget, as we're inside ring ref already */
>> + percpu_ref_get(&ctx->refs);
>
> Is that true? In the path io_sq_thread() -> io_submit_sqes() ->
> io_submit_sqe() -> io_get_req(), I don't see anything that's already
> holding a reference for you. Is the worker thread holding a reference
> somewhere that I'm missing?
If the thread is alive, then the ctx is alive. Before we drop the last
ref to the ctx (and kill it), we wait for the thread to exit.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jann Horn @ 2019-01-29 1:29 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-aio, linux-block, linux-man, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <20190128213538.13486-6-axboe@kernel.dk>
On Mon, Jan 28, 2019 at 10:35 PM Jens Axboe <axboe@kernel.dk> wrote:
> The submission queue (SQ) and completion queue (CQ) rings are shared
> between the application and the kernel. This eliminates the need to
> copy data back and forth to submit and complete IO.
[...]
> +static struct io_kiocb *io_get_req(struct io_ring_ctx *ctx)
> +{
> + struct io_kiocb *req;
> +
> + /* safe to use the non tryget, as we're inside ring ref already */
> + percpu_ref_get(&ctx->refs);
Is that true? In the path io_sq_thread() -> io_submit_sqes() ->
io_submit_sqe() -> io_get_req(), I don't see anything that's already
holding a reference for you. Is the worker thread holding a reference
somewhere that I'm missing?
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 12/18] io_uring: add support for pre-mapped user IO buffers
From: Jens Axboe @ 2019-01-29 1:25 UTC (permalink / raw)
To: Jann Horn
Cc: linux-aio, linux-block, linux-man, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <CAG48ez0JjCDtAwbYRKcEOjsMDWmEWKUYHo4nLEDbXmQ9N9Ca=w@mail.gmail.com>
On 1/28/19 5:36 PM, Jann Horn wrote:
> On Tue, Jan 29, 2019 at 12:50 AM Jens Axboe <axboe@kernel.dk> wrote:
>> On 1/28/19 4:35 PM, Jann Horn wrote:
>>> On Mon, Jan 28, 2019 at 10:36 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>> If we have fixed user buffers, we can map them into the kernel when we
>>>> setup the io_context. That avoids the need to do get_user_pages() for
>>>> each and every IO.
>>> [...]
>>>> +static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
>>>> + void __user *arg, unsigned nr_args)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + /* Drop our initial ref and wait for the ctx to be fully idle */
>>>> + percpu_ref_put(&ctx->refs);
>>>
>>> The line above drops a reference that you just got in the caller...
>>
>> Right
>>
>>>> + percpu_ref_kill(&ctx->refs);
>>>> + wait_for_completion(&ctx->ctx_done);
>>>> +
>>>> + switch (opcode) {
>>>> + case IORING_REGISTER_BUFFERS:
>>>> + ret = io_sqe_buffer_register(ctx, arg, nr_args);
>>>> + break;
>>>> + case IORING_UNREGISTER_BUFFERS:
>>>> + ret = -EINVAL;
>>>> + if (arg || nr_args)
>>>> + break;
>>>> + ret = io_sqe_buffer_unregister(ctx);
>>>> + break;
>>>> + default:
>>>> + ret = -EINVAL;
>>>> + break;
>>>> + }
>>>> +
>>>> + /* bring the ctx back to life */
>>>> + reinit_completion(&ctx->ctx_done);
>>>> + percpu_ref_resurrect(&ctx->refs);
>>>> + percpu_ref_get(&ctx->refs);
>>>
>>> And then this line takes a reference that the caller will immediately
>>> drop again? Why?
>>
>> Just want to keep it symmetric and avoid having weird "this function drops
>> a reference" use cases.
>>
>>>
>>>> + return ret;
>>>> +}
>>>> +
>>>> +SYSCALL_DEFINE4(io_uring_register, unsigned int, fd, unsigned int, opcode,
>>>> + void __user *, arg, unsigned int, nr_args)
>>>> +{
>>>> + struct io_ring_ctx *ctx;
>>>> + long ret = -EBADF;
>>>> + struct fd f;
>>>> +
>>>> + f = fdget(fd);
>>>> + if (!f.file)
>>>> + return -EBADF;
>>>> +
>>>> + ret = -EOPNOTSUPP;
>>>> + if (f.file->f_op != &io_uring_fops)
>>>> + goto out_fput;
>>>> +
>>>> + ret = -ENXIO;
>>>> + ctx = f.file->private_data;
>>>> + if (!percpu_ref_tryget(&ctx->refs))
>>>> + goto out_fput;
>>>
>>> If you are holding the uring_lock of a ctx that can be accessed
>>> through a file descriptor (which you do just after this point), you
>>> know that the percpu_ref isn't zero, right? Why are you doing the
>>> tryget here?
>>
>> Not sure I follow... We don't hold the lock at this point. I guess your
>> point is that since the descriptor is open (or we'd fail the above
>> check), then there's no point doing the tryget variant here? That's
>> strictly true, that could just be a get().
>
> As far as I can tell, you could do the following without breaking anything:
>
> ========================
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 6916dc3222cf..c2d82765eefe 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -2485,7 +2485,6 @@ static int __io_uring_register(struct
> io_ring_ctx *ctx, unsigned opcode,
> int ret;
>
> /* Drop our initial ref and wait for the ctx to be fully idle */
> - percpu_ref_put(&ctx->refs);
> percpu_ref_kill(&ctx->refs);
> wait_for_completion(&ctx->ctx_done);
>
> @@ -2516,7 +2515,6 @@ static int __io_uring_register(struct
> io_ring_ctx *ctx, unsigned opcode,
> /* bring the ctx back to life */
> reinit_completion(&ctx->ctx_done);
> percpu_ref_resurrect(&ctx->refs);
> - percpu_ref_get(&ctx->refs);
> return ret;
> }
>
> @@ -2535,17 +2533,13 @@ SYSCALL_DEFINE4(io_uring_register, unsigned
> int, fd, unsigned int, opcode,
> if (f.file->f_op != &io_uring_fops)
> goto out_fput;
>
> - ret = -ENXIO;
> ctx = f.file->private_data;
> - if (!percpu_ref_tryget(&ctx->refs))
> - goto out_fput;
>
> ret = -EBUSY;
> if (mutex_trylock(&ctx->uring_lock)) {
> ret = __io_uring_register(ctx, opcode, arg, nr_args);
> mutex_unlock(&ctx->uring_lock);
> }
> - io_ring_drop_ctx_refs(ctx, 1);
> out_fput:
> fdput(f);
> return ret;
> ========================
>
> The two functions that can drop the initial ref of the percpu refcount are:
>
> 1. io_ring_ctx_wait_and_kill(); this is only used on ->release() or on
> setup failure, meaning that as long as you have a reference to the
> file from fget()/fdget(), io_ring_ctx_wait_and_kill() can't have been
> called on your context
> 2. __io_uring_register(); this temporarily kills the percpu refcount
> and resurrects it, all under ctx->uring_lock, meaning that as long as
> you're holding ctx->uring_lock, __io_uring_register() can't have
> killed the percpu refcount
>
> Therefore, I think that as long as you're in sys_io_uring_register and
> holding the ctx->uring_lock, you know that the percpu refcount is
> alive, and bumping and dropping non-initial references has no effect.
>
> Perhaps this makes more sense when you view the percpu refcount as a
> read/write lock - percpu_ref_tryget() takes a read lock, the
> percpu_ref_kill() dance takes a write lock.
This looks good, I'll fold it in. Thanks!
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [RESEND PATCH v2 1/1] prctl: add PR_{GET,SET}_KILL_DESCENDANTS_ON_EXIT
From: Andrew Morton @ 2019-01-29 1:23 UTC (permalink / raw)
To: Jürg Billeter
Cc: Oleg Nesterov, Thomas Gleixner, Eric Biederman, Kees Cook,
Andy Lutomirski, linux-api, linux-kernel
In-Reply-To: <20190118131130.42209-2-j@bitron.ch>
On Fri, 18 Jan 2019 14:11:30 +0100 Jürg Billeter <j@bitron.ch> wrote:
> This introduces a new thread group flag that can be set by calling
>
> prctl(PR_SET_KILL_DESCENDANTS_ON_EXIT, 1, 0, 0, 0)
>
> When a thread group exits with this flag set, it will send SIGKILL to
> all descendant processes. This can be used to prevent stray child
> processes.
>
> This flag is cleared on privilege gaining execve(2) to ensure an
> unprivileged process cannot get a privileged process to send SIGKILL.
>
> Descendants that are orphaned and reparented to an ancestor of the
> current process before the current process exits, will not be killed.
> PR_SET_CHILD_SUBREAPER can be used to contain orphaned processes.
>
> If a descendant gained privileges, the current process may not be
> allowed to kill it, and the descendant process will survive.
> PR_SET_NO_NEW_PRIVS can be used to prevent descendant processes from
> gaining privileges.
I don't feel that I'm able to judge the usefulness of this. It would
help to have a lot more words right here in this changelog which
communicate the value of this change to our users. References are
useful, but please don't send people off to chase down mailing list and
bugzilla discussions as a substitute for properly describing the feature
and its justification.
Some test code in tools/testing/selftests/ would be helpful.
We'll need to update the prctl(2) manpage if we proceed with this.
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jens Axboe @ 2019-01-29 1:20 UTC (permalink / raw)
To: Andy Lutomirski, Christoph Hellwig
Cc: Linux FS Devel, linux-aio, linux-block, Jeff Moyer, Avi Kivity,
Linux API, linux-man
In-Reply-To: <CALCETrV30uN2qOUEf+kNGWdphBtps7K3rEk8_8+VAwdtPomRkQ@mail.gmail.com>
On 1/28/19 5:47 PM, Andy Lutomirski wrote:
> On Mon, Jan 28, 2019 at 6:57 AM Christoph Hellwig <hch@lst.de> wrote:
>>
>> [please make sure linux-api and linux-man are CCed on new syscalls
>> so that we get API experts to review them]
>
>>> +static int io_import_iovec(struct io_ring_ctx *ctx, int rw,
>>> + const struct io_uring_sqe *sqe,
>>> + struct iovec **iovec, struct iov_iter *iter)
>>> +{
>>> + void __user *buf = u64_to_user_ptr(sqe->addr);
>>> +
>>> +#ifdef CONFIG_COMPAT
>>> + if (ctx->compat)
>>> + return compat_import_iovec(rw, buf, sqe->len, UIO_FASTIOV,
>>> + iovec, iter);
>>> +#endif
>>
>> I think we can just check in_compat_syscall() here, which means we
>> can kill the ->compat member, and the separate compat version of the
>> setup syscall.
>>
>
> Since this whole API is new, I don't suppose you could introduce a
> struct iovec64 or similar and just make the ABI be identical for
> 64-bit and 32-bit code?
Sure, that would be straight forward. Is there a strong reason to do
so outside of "that would be nice"? It's not like it's a huge amount
of code.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jann Horn @ 2019-01-29 1:07 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-aio, linux-block, linux-man, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <20190128213538.13486-6-axboe@kernel.dk>
On Mon, Jan 28, 2019 at 10:35 PM Jens Axboe <axboe@kernel.dk> wrote:
> The submission queue (SQ) and completion queue (CQ) rings are shared
> between the application and the kernel. This eliminates the need to
> copy data back and forth to submit and complete IO.
[...]
> +static bool io_get_sqring(struct io_ring_ctx *ctx, struct sqe_submit *s)
> +{
> + struct io_sq_ring *ring = ctx->sq_ring;
> + unsigned head;
> +
> + /*
> + * The cached sq head (or cq tail) serves two purposes:
> + *
> + * 1) allows us to batch the cost of updating the user visible
> + * head updates.
> + * 2) allows the kernel side to track the head on its own, even
> + * though the application is the one updating it.
> + */
> + head = ctx->cached_sq_head;
> + smp_rmb();
> + if (head == READ_ONCE(ring->r.tail))
> + return false;
> +
> + head = ring->array[head & ctx->sq_mask];
> + if (head < ctx->sq_entries) {
> + s->index = head;
> + s->sqe = &ctx->sq_sqes[head];
ring->array can be mapped writable into userspace, right? If so: This
looks like a double-read issue; the compiler might assume that
ring->array is not modified concurrently and perform separate memory
accesses for the "if (head < ctx->sq_entries)" check and the
"&ctx->sq_sqes[head]" computation. Please use READ_ONCE()/WRITE_ONCE()
for all accesses to memory that userspace could concurrently modify in
a malicious way.
There have been some pretty severe security bugs caused by missing
READ_ONCE() annotations around accesses to shared memory; see, for
example, https://www.blackhat.com/docs/us-16/materials/us-16-Wilhelm-Xenpwn-Breaking-Paravirtualized-Devices.pdf
. Slides 35-48 show how the code "switch (op->cmd)", where "op" is a
pointer to shared memory, allowed an attacker to break out of a Xen
virtual machine because the compiler generated multiple memory
accesses.
> + ctx->cached_sq_head++;
> + return true;
> + }
> +
> + /* drop invalid entries */
> + ctx->cached_sq_head++;
> + ring->dropped++;
> + smp_wmb();
> + return false;
> +}
[...]
> +SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
> + u32, min_complete, u32, flags, const sigset_t __user *, sig,
> + size_t, sigsz)
> +{
> + struct io_ring_ctx *ctx;
> + long ret = -EBADF;
> + struct fd f;
> +
> + f = fdget(fd);
> + if (!f.file)
> + return -EBADF;
> +
> + ret = -EOPNOTSUPP;
> + if (f.file->f_op != &io_uring_fops)
> + goto out_fput;
Oh, by the way: If you feel like it, maybe you could add a helper
fdget_typed(int fd, struct file_operations *f_op), or something like
that, so that there is less boilerplate code for first doing fdget(),
then checking ->f_op, and then coding an extra bailout path for that?
But that doesn't really have much to do with your patchset, feel free
to ignore this comment.
[...]
> +out_fput:
> + fdput(f);
> + return ret;
> +}
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jens Axboe @ 2019-01-29 1:01 UTC (permalink / raw)
To: Jann Horn
Cc: Al Viro, linux-aio, linux-block, linux-man, Linux API, hch,
jmoyer, Avi Kivity
In-Reply-To: <CAG48ez2dESJAg80N3+vgSWy_Pb+Est0S-jshhmZD2nnwgpuR3w@mail.gmail.com>
On 1/28/19 5:58 PM, Jann Horn wrote:
> On Tue, Jan 29, 2019 at 1:55 AM Jens Axboe <axboe@kernel.dk> wrote:
>> On 1/28/19 5:34 PM, Jann Horn wrote:
>>> On Tue, Jan 29, 2019 at 1:32 AM Jens Axboe <axboe@kernel.dk> wrote:
>>>> On 1/28/19 5:03 PM, Jens Axboe wrote:
>>>>>> But you only do that teardown on ->release, right? And ->release
>>>>>> doesn't have much to do with the process lifetime.
>>>>>
>>>>> Yes, only on ->relase().
>>>>
>>>> OK, so I reworked the files struct to just grab it, then we ensure that
>>>> doesn't go away. For mm, it's a bit more tricky. I think the best
>>>> solution here is to add a fops->flush() and check for the process
>>>> exiting its files. If it does, we quiesce the async contexts and prevent
>>>> further use of that mm. We can't just keep holding a reference to the mm
>>>> like we do with the files.
>>>>
>>>> That should solve both cases.
>>>
>>> You still have to hold a reference on the mm though, I think (for
>>> example, because two tasks might be sharing the fd table without
>>> sharing the mm).
>>
>> Yes good point, except we can't hold a reference to it.
>
> Why not? kvm_create_vm() does it, too:
>
> mmgrab(current->mm);
> kvm->mm = current->mm;
I missed that helper, was only looking at mmget(). But yeah, that'll do it!
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jann Horn @ 2019-01-29 0:58 UTC (permalink / raw)
To: Jens Axboe
Cc: Al Viro, linux-aio, linux-block, linux-man, Linux API, hch,
jmoyer, Avi Kivity
In-Reply-To: <a89d28dc-d0c4-d84e-b3f2-20d10c6f673e@kernel.dk>
On Tue, Jan 29, 2019 at 1:55 AM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/28/19 5:34 PM, Jann Horn wrote:
> > On Tue, Jan 29, 2019 at 1:32 AM Jens Axboe <axboe@kernel.dk> wrote:
> >> On 1/28/19 5:03 PM, Jens Axboe wrote:
> >>>> But you only do that teardown on ->release, right? And ->release
> >>>> doesn't have much to do with the process lifetime.
> >>>
> >>> Yes, only on ->relase().
> >>
> >> OK, so I reworked the files struct to just grab it, then we ensure that
> >> doesn't go away. For mm, it's a bit more tricky. I think the best
> >> solution here is to add a fops->flush() and check for the process
> >> exiting its files. If it does, we quiesce the async contexts and prevent
> >> further use of that mm. We can't just keep holding a reference to the mm
> >> like we do with the files.
> >>
> >> That should solve both cases.
> >
> > You still have to hold a reference on the mm though, I think (for
> > example, because two tasks might be sharing the fd table without
> > sharing the mm).
>
> Yes good point, except we can't hold a reference to it.
Why not? kvm_create_vm() does it, too:
mmgrab(current->mm);
kvm->mm = current->mm;
> But I think
> we can get around this by using an mmu notifier instead. That eliminates
> the need for ->flush() as well.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jens Axboe @ 2019-01-29 0:55 UTC (permalink / raw)
To: Jann Horn
Cc: Al Viro, linux-aio, linux-block, linux-man, Linux API, hch,
jmoyer, Avi Kivity
In-Reply-To: <CAG48ez3XGjFZbPiof4u4QGtoumBXXfCsFxOQ_2pGt=MnqqY7ZQ@mail.gmail.com>
On 1/28/19 5:34 PM, Jann Horn wrote:
> On Tue, Jan 29, 2019 at 1:32 AM Jens Axboe <axboe@kernel.dk> wrote:
>> On 1/28/19 5:03 PM, Jens Axboe wrote:
>>>> But you only do that teardown on ->release, right? And ->release
>>>> doesn't have much to do with the process lifetime.
>>>
>>> Yes, only on ->relase().
>>
>> OK, so I reworked the files struct to just grab it, then we ensure that
>> doesn't go away. For mm, it's a bit more tricky. I think the best
>> solution here is to add a fops->flush() and check for the process
>> exiting its files. If it does, we quiesce the async contexts and prevent
>> further use of that mm. We can't just keep holding a reference to the mm
>> like we do with the files.
>>
>> That should solve both cases.
>
> You still have to hold a reference on the mm though, I think (for
> example, because two tasks might be sharing the fd table without
> sharing the mm).
Yes good point, except we can't hold a reference to it. But I think
we can get around this by using an mmu notifier instead. That eliminates
the need for ->flush() as well.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Andy Lutomirski @ 2019-01-29 0:47 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Linux FS Devel, linux-aio, linux-block, Jeff Moyer,
Avi Kivity, Linux API, linux-man
In-Reply-To: <20190128145700.GA9795@lst.de>
On Mon, Jan 28, 2019 at 6:57 AM Christoph Hellwig <hch@lst.de> wrote:
>
> [please make sure linux-api and linux-man are CCed on new syscalls
> so that we get API experts to review them]
> > +static int io_import_iovec(struct io_ring_ctx *ctx, int rw,
> > + const struct io_uring_sqe *sqe,
> > + struct iovec **iovec, struct iov_iter *iter)
> > +{
> > + void __user *buf = u64_to_user_ptr(sqe->addr);
> > +
> > +#ifdef CONFIG_COMPAT
> > + if (ctx->compat)
> > + return compat_import_iovec(rw, buf, sqe->len, UIO_FASTIOV,
> > + iovec, iter);
> > +#endif
>
> I think we can just check in_compat_syscall() here, which means we
> can kill the ->compat member, and the separate compat version of the
> setup syscall.
>
Since this whole API is new, I don't suppose you could introduce a
struct iovec64 or similar and just make the ABI be identical for
64-bit and 32-bit code?
--Andy
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox