Add support for shared io-wq backends

fs/io-wq.c +13 -0

+/*
+ * Find and return io_wq with given id and grab a reference to it.
+ */
+struct io_wq *io_wq_create_id(unsigned bounded, struct io_wq_data *data,
+			      unsigned int id)
+{
+	struct io_wq *wq, *ret = NULL;
+
+	mutex_lock(&wq_lock);
+	list_for_each_entry(wq, &wq_list, wq_list) {
+		if (id != wq->id)
+			continue;
+		if (data->creds != wq->creds || data->user != wq->user)

Quoted 18 lines

fs/io-wq.c +18 -0

+/*
+ * Find and return io_wq with given id and grab a reference to it.
+ */
+struct io_wq *io_wq_create_id(unsigned bounded, struct io_wq_data *data,
+			      unsigned int id)
+{
+	struct io_wq *wq, *ret = NULL;
+
+	mutex_lock(&wq_lock);
+	list_for_each_entry(wq, &wq_list, wq_list) {
+		if (id != wq->id)
+			continue;
+		if (data->creds != wq->creds || data->user != wq->user)
+			continue;
+		if (data->get_work != wq->get_work ||
+		    data->put_work != wq->put_work)
+			continue;
+		if (!refcount_inc_not_zero(&wq->use_refs))

fs/io-wq.c struct io_wq *io_wq_create(unsigned bounded, struct io_wq_data *data) · line 1137

Quoted 5 lines

fs/io-wq.c +5 -0

+			continue;
+		ret = wq;
+		break;
+	}
+	mutex_unlock(&wq_lock);

Isn't there a more efficient ida_find function in order to avoid the loop, which won't really scale in the long run.

Sometimes an applications wants to use multiple smaller rings, because it's more efficient than sharing a ring. The downside of that is that we'll create the io-wq backend separately for all of them, while they would be perfectly happy just sharing that.

This patchset adds support for that. io_uring_params grows an 'id' field, which denotes an identifier for the async backend. If an application wants to utilize sharing, it'll simply grab the id from the first ring created, and pass it in to the next one and set IORING_SETUP_SHARED. This allows efficient sharing of backend resources, while allowing multiple rings in the application or library.

Not a huge fan of the IORING_SETUP_SHARED name, we should probably make that better (I'm taking suggestions).

Hi Jens,

Quoted 5 lines

Sometimes an applications wants to use multiple smaller rings, because it's more efficient than sharing a ring. The downside of that is that we'll create the io-wq backend separately for all of them, while they would be perfectly happy just sharing that.

Quoted 6 lines

This patchset adds support for that. io_uring_params grows an 'id' field, which denotes an identifier for the async backend. If an application wants to utilize sharing, it'll simply grab the id from the first ring created, and pass it in to the next one and set IORING_SETUP_SHARED. This allows efficient sharing of backend resources, while allowing multiple rings in the application or library.

But still all rings need to use the same creds, correct?

Quoted 2 lines

Not a huge fan of the IORING_SETUP_SHARED name, we should probably make that better (I'm taking suggestions).

The flag is supposed to be used for the new ring that attached to the existing workqueue, correct?

What about IORING_ATTACH_TO_WORKQUEUE?

Trying to avoid something that is too tied to the internals, but workqueue (or wq) is probably generic enough that it can be used. IORING_SETUP_ATTACH_WQ?

Hi Jens,

Quoted 3 lines

Trying to avoid something that is too tied to the internals, but workqueue (or wq) is probably generic enough that it can be used. IORING_SETUP_ATTACH_WQ?

Ok :-)

Sometimes an applications wants to use multiple smaller rings, because it's more efficient than sharing a ring. The downside of that is that we'll create the io-wq backend separately for all of them, while they would be perfectly happy just sharing that.

This patchset adds support for that. io_uring_params grows an 'id' field, which denotes an identifier for the async backend. If an application wants to utilize sharing, it'll simply grab the id from the first ring created, and pass it in to the next one and set IORING_SETUP_SHARED. This allows efficient sharing of backend resources, while allowing multiple rings in the application or library.

Not a huge fan of the IORING_SETUP_SHARED name, we should probably make that better (I'm taking suggestions).

Quoted 12 lines · Jens Axboe

Sometimes an applications wants to use multiple smaller rings, because it's more efficient than sharing a ring. The downside of that is that we'll create the io-wq backend separately for all of them, while they would be perfectly happy just sharing that.

This patchset adds support for that. io_uring_params grows an 'id' field, which denotes an identifier for the async backend. If an application wants to utilize sharing, it'll simply grab the id from the first ring created, and pass it in to the next one and set IORING_SETUP_SHARED. This allows efficient sharing of backend resources, while allowing multiple rings in the application or library.

Quoted 3 lines

Not a huge fan of the IORING_SETUP_SHARED name, we should probably make that better (I'm taking suggestions).

Took a look at the latest version (b942f31ee0 at io_uring-vfs-shared-wq). There is an outdated commit message for the last patch mentioning renamed IORING_SETUP_SHARED, but the code looks good to me.

Reviewed-by Pavel Begunkov <[email protected]>

Sometimes an applications wants to use multiple smaller rings, because it's more efficient than sharing a ring. The downside of that is that we'll create the io-wq backend separately for all of them, while they would be perfectly happy just sharing that.

This patchset adds support for that. io_uring_params grows an 'id' field, which denotes an identifier for the async backend. If an application wants to utilize sharing, it'll simply grab the id from the first ring created, and pass it in to the next one and set IORING_SETUP_SHARED. This allows efficient sharing of backend resources, while allowing multiple rings in the application or library.

Not a huge fan of the IORING_SETUP_SHARED name, we should probably make that better (I'm taking suggestions).

On Fri, 24 Jan 2020 at 10:16, Jens Axboe <[email protected]> wrote:

I don't love the idea of some new type of magic user<>kernel identifier. It would be nice if the id itself was e.g. a file descriptor

What if when creating an io_uring you could pass in an existing io_uring file descriptor, and the new one would share the io-wq backend?

Quoted 10 lines · Daurnimator

On Fri, 24 Jan 2020 at 10:16, Jens Axboe <[email protected]> wrote:

I don't love the idea of some new type of magic user<>kernel identifier. It would be nice if the id itself was e.g. a file descriptor

What if when creating an io_uring you could pass in an existing io_uring file descriptor, and the new one would share the io-wq backend?

Good idea! It can solve potential problems with jails, isolation, etc in the future.

May we need having other shared resources and want fine-grained control over them at some moment? It can prove helpful for the BPF plans. E.g.

io_uring_setup(share_io-wq=ring_fd1,

               share_fds=ring_fd2,
               share_ebpf=ring_fd3, ...);

If so, it's better to have more flexible API. E.g. as follows or a pointer to a struct with @size field.

struct io_shared_resource {
    int type;
    int fd;
};

struct io_uring_params {
    ...
    struct io_shared_resource shared[];
};

params = {
    ...
    .shared = {{ATTACH_IO_WQ, fd1}, ..., SANTINEL_ENTRY};
};

Quoted 35 lines · Pavel Begunkov

Quoted 10 lines · Daurnimator

On Fri, 24 Jan 2020 at 10:16, Jens Axboe <[email protected]> wrote:

I don't love the idea of some new type of magic user<>kernel identifier. It would be nice if the id itself was e.g. a file descriptor

What if when creating an io_uring you could pass in an existing io_uring file descriptor, and the new one would share the io-wq backend?

Good idea! It can solve potential problems with jails, isolation, etc in the future.

May we need having other shared resources and want fine-grained control over them at some moment? It can prove helpful for the BPF plans. E.g.

io_uring_setup(share_io-wq=ring_fd1,

               share_fds=ring_fd2,
               share_ebpf=ring_fd3, ...);

If so, it's better to have more flexible API. E.g. as follows or a pointer to a struct with @size field.

struct io_shared_resource {
    int type;
    int fd;
};

struct io_uring_params {
    ...
    struct io_shared_resource shared[];
};

Quoted 4 lines

params = {
    ...
    .shared = {{ATTACH_IO_WQ, fd1}, ..., SANTINEL_ENTRY};
};

I'm fine with changing/extending the sharing API, please send a patch!

Quoted 41 lines · Jens Axboe

Quoted 39 lines · Pavel Begunkov

Quoted 10 lines · Daurnimator

On Fri, 24 Jan 2020 at 10:16, Jens Axboe <[email protected]> wrote:

I don't love the idea of some new type of magic user<>kernel identifier. It would be nice if the id itself was e.g. a file descriptor

What if when creating an io_uring you could pass in an existing io_uring file descriptor, and the new one would share the io-wq backend?

Good idea! It can solve potential problems with jails, isolation, etc in the future.

May we need having other shared resources and want fine-grained control over them at some moment? It can prove helpful for the BPF plans. E.g.

io_uring_setup(share_io-wq=ring_fd1,

               share_fds=ring_fd2,
               share_ebpf=ring_fd3, ...);

If so, it's better to have more flexible API. E.g. as follows or a pointer to a struct with @size field.

struct io_shared_resource {
    int type;
    int fd;
};

struct io_uring_params {
    ...
    struct io_shared_resource shared[];
};

params = {
    ...
    .shared = {{ATTACH_IO_WQ, fd1}, ..., SANTINEL_ENTRY};
};

Quoted 3 lines

I'm fine with changing/extending the sharing API, please send a patch!

Ok. I can't promise it'll play handy for sharing. Though, you'll be out of space in struct io_uring_params soon anyway.

Quoted 6 lines · Pavel Begunkov

Quoted 3 lines · Jens Axboe

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Daurnimator

On Fri, 24 Jan 2020 at 10:16, Jens Axboe <[email protected]> wrote:

Ok. I can't promise it'll play handy for sharing. Though, you'll be out of space in struct io_uring_params soon anyway.

I'm going to keep what we have for now, as I'm really not imagining a lot more sharing - what else would we share? So let's not over-design anything.

Quoted 12 lines · Jens Axboe

Quoted 6 lines · Pavel Begunkov

Quoted 3 lines · Jens Axboe

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Daurnimator

On Fri, 24 Jan 2020 at 10:16, Jens Axboe <[email protected]> wrote:

Ok. I can't promise it'll play handy for sharing. Though, you'll be out of space in struct io_uring_params soon anyway.

I'm going to keep what we have for now, as I'm really not imagining a lot more sharing - what else would we share? So let's not over-design anything.

Fair enough. I prefer a ptr to an extendable struct, that will take the last u64, when needed.

However, it's still better to share through file descriptors. It's just not secure enough the way it's now.

Quoted 16 lines · Pavel Begunkov

Quoted 12 lines · Jens Axboe

Quoted 6 lines · Pavel Begunkov

Quoted 3 lines · Jens Axboe

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Daurnimator

On Fri, 24 Jan 2020 at 10:16, Jens Axboe <[email protected]> wrote:

Ok. I can't promise it'll play handy for sharing. Though, you'll be out of space in struct io_uring_params soon anyway.

I'm going to keep what we have for now, as I'm really not imagining a lot more sharing - what else would we share? So let's not over-design anything.

Fair enough. I prefer a ptr to an extendable struct, that will take the last u64, when needed.

Quoted 3 lines

However, it's still better to share through file descriptors. It's just not secure enough the way it's now.

I'll send a patch with fd-approach shortly, just if you want to squeeze them into 5.6-rc

Quoted 12 lines · Jens Axboe

Quoted 6 lines · Pavel Begunkov

Quoted 3 lines · Jens Axboe

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Daurnimator

On Fri, 24 Jan 2020 at 10:16, Jens Axboe <[email protected]> wrote:

Ok. I can't promise it'll play handy for sharing. Though, you'll be out of space in struct io_uring_params soon anyway.

I'm going to keep what we have for now, as I'm really not imagining a lot more sharing - what else would we share? So let's not over-design anything.

Fair enough. I prefer a ptr to an extendable struct, that will take the last u64, when needed.

However, it's still better to share through file descriptors. It's just not secure enough the way it's now.

Quoted 16 lines · Pavel Begunkov

Quoted 12 lines · Jens Axboe

Quoted 6 lines · Pavel Begunkov

Quoted 3 lines · Jens Axboe

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Daurnimator

On Fri, 24 Jan 2020 at 10:16, Jens Axboe <[email protected]> wrote:

Ok. I can't promise it'll play handy for sharing. Though, you'll be out of space in struct io_uring_params soon anyway.

I'm going to keep what we have for now, as I'm really not imagining a lot more sharing - what else would we share? So let's not over-design anything.

Fair enough. I prefer a ptr to an extendable struct, that will take the last u64, when needed.

Quoted 2 lines

However, it's still better to share through file descriptors. It's just not secure enough the way it's now.

Is the file descriptor value really a good choice? We just had some confusion on ring sharing across forks. Not sure using an fd value is a sane "key" to use across processes.

Quoted 17 lines · Jens Axboe

Quoted 16 lines · Pavel Begunkov

Quoted 12 lines · Jens Axboe

Quoted 6 lines · Pavel Begunkov

Quoted 3 lines · Jens Axboe

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Daurnimator

On Fri, 24 Jan 2020 at 10:16, Jens Axboe <[email protected]> wrote:

Ok. I can't promise it'll play handy for sharing. Though, you'll be out of space in struct io_uring_params soon anyway.

I'm going to keep what we have for now, as I'm really not imagining a lot more sharing - what else would we share? So let's not over-design anything.

Fair enough. I prefer a ptr to an extendable struct, that will take the last u64, when needed.

Quoted 7 lines

Quoted 2 lines

However, it's still better to share through file descriptors. It's just not secure enough the way it's now.

Is the file descriptor value really a good choice? We just had some confusion on ring sharing across forks. Not sure using an fd value is a sane "key" to use across processes.

As I see it, the problem with @mm is that uring is dead-bound to it. For example, a process can create and send uring (e.g. via socket), and then be killed. And that basically means
1. @mm of the process is locked just because of the sent uring instance.
2. a process may have an io_uring, which bound to @mm of another process, even though the layouts may be completely different.

File descriptors are different here, because io_uring doesn't know about them, They are controlled by the userspace (send, dup, fork, etc), and don't sabotage all isolation work done in the kernel. A dire example here is stealing io-wq from within a container, which is trivial with global self-made id. I would love to hear, if I am mistaken somewhere.

Is there some better option?

Quoted 33 lines · Pavel Begunkov

Quoted 17 lines · Jens Axboe

Quoted 16 lines · Pavel Begunkov

Quoted 12 lines · Jens Axboe

Quoted 6 lines · Pavel Begunkov

Quoted 3 lines · Jens Axboe

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Daurnimator

On Fri, 24 Jan 2020 at 10:16, Jens Axboe <[email protected]> wrote:

Ok. I can't promise it'll play handy for sharing. Though, you'll be out of space in struct io_uring_params soon anyway.

I'm going to keep what we have for now, as I'm really not imagining a lot more sharing - what else would we share? So let's not over-design anything.

Fair enough. I prefer a ptr to an extendable struct, that will take the last u64, when needed.

Quoted 7 lines

Quoted 2 lines

However, it's still better to share through file descriptors. It's just not secure enough the way it's now.

Is the file descriptor value really a good choice? We just had some confusion on ring sharing across forks. Not sure using an fd value is a sane "key" to use across processes.

As I see it, the problem with @mm is that uring is dead-bound to it. For example, a process can create and send uring (e.g. via socket), and then be killed. And that basically means
1. @mm of the process is locked just because of the sent uring instance.
2. a process may have an io_uring, which bound to @mm of another process, even though the layouts may be completely different.

Quoted 8 lines

File descriptors are different here, because io_uring doesn't know about them, They are controlled by the userspace (send, dup, fork, etc), and don't sabotage all isolation work done in the kernel. A dire example here is stealing io-wq from within a container, which is trivial with global self-made id. I would love to hear, if I am mistaken somewhere.

Is there some better option?

OK, so how about this:

- We use the 'fd' as the lookup key. This makes it easy since we can just check if it's a io_uring instance or not, we don't need to do any tracking on the side. It also means that the application asking for sharing must already have some relationship to the process that created the ring.

- mm/creds must be transferred through the work item. Any SQE done on behalf of io_uring_enter() directly already has that, if punted we must pass the creds and mm. This means we break the static setup of io_wq->mm/creds. It also means that we probably have to add that to io_wq_work, which kind of sucks, but...

I think with that we have a decent setup, that's also safe. I've dropped the sharing patches for now, from the 5.6 tree.

Quoted 43 lines · Jens Axboe

Quoted 41 lines · Pavel Begunkov

Quoted 17 lines · Jens Axboe

Quoted 16 lines · Pavel Begunkov

Quoted 12 lines · Jens Axboe

Quoted 6 lines · Pavel Begunkov

Quoted 3 lines · Jens Axboe

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Daurnimator

On Fri, 24 Jan 2020 at 10:16, Jens Axboe <[email protected]> wrote:

Ok. I can't promise it'll play handy for sharing. Though, you'll be out of space in struct io_uring_params soon anyway.

I'm going to keep what we have for now, as I'm really not imagining a lot more sharing - what else would we share? So let's not over-design anything.

Fair enough. I prefer a ptr to an extendable struct, that will take the last u64, when needed.

Quoted 7 lines

Quoted 2 lines

However, it's still better to share through file descriptors. It's just not secure enough the way it's now.

Is the file descriptor value really a good choice? We just had some confusion on ring sharing across forks. Not sure using an fd value is a sane "key" to use across processes.

As I see it, the problem with @mm is that uring is dead-bound to it. For example, a process can create and send uring (e.g. via socket), and then be killed. And that basically means
1. @mm of the process is locked just because of the sent uring instance.
2. a process may have an io_uring, which bound to @mm of another process, even though the layouts may be completely different.

File descriptors are different here, because io_uring doesn't know about them, They are controlled by the userspace (send, dup, fork, etc), and don't sabotage all isolation work done in the kernel. A dire example here is stealing io-wq from within a container, which is trivial with global self-made id. I would love to hear, if I am mistaken somewhere.

Is there some better option?

Quoted 7 lines

OK, so how about this:

- We use the 'fd' as the lookup key. This makes it easy since we can just check if it's a io_uring instance or not, we don't need to do any tracking on the side. It also means that the application asking for sharing must already have some relationship to the process that created the ring.

Quoted 6 lines

- mm/creds must be transferred through the work item. Any SQE done on behalf of io_uring_enter() directly already has that, if punted we must pass the creds and mm. This means we break the static setup of io_wq->mm/creds. It also means that we probably have to add that to io_wq_work, which kind of sucks, but...

It'd fix Stefan's worry too.

Quoted 2 lines

I think with that we have a decent setup, that's also safe. I've dropped the sharing patches for now, from the 5.6 tree.

So one concern might be SQPOLL, it'll have to use the ctx creds and mm as usual. I guess that is ok.

Quoted 44 lines · Jens Axboe

Quoted 43 lines · Jens Axboe

Quoted 41 lines · Pavel Begunkov

Quoted 17 lines · Jens Axboe

Quoted 16 lines · Pavel Begunkov

Quoted 12 lines · Jens Axboe

Quoted 6 lines · Pavel Begunkov

Quoted 3 lines · Jens Axboe

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Daurnimator

On Fri, 24 Jan 2020 at 10:16, Jens Axboe <[email protected]> wrote:

Ok. I can't promise it'll play handy for sharing. Though, you'll be out of space in struct io_uring_params soon anyway.

I'm going to keep what we have for now, as I'm really not imagining a lot more sharing - what else would we share? So let's not over-design anything.

Fair enough. I prefer a ptr to an extendable struct, that will take the last u64, when needed.

Quoted 7 lines

Quoted 2 lines

However, it's still better to share through file descriptors. It's just not secure enough the way it's now.

Is the file descriptor value really a good choice? We just had some confusion on ring sharing across forks. Not sure using an fd value is a sane "key" to use across processes.

As I see it, the problem with @mm is that uring is dead-bound to it. For example, a process can create and send uring (e.g. via socket), and then be killed. And that basically means
1. @mm of the process is locked just because of the sent uring instance.
2. a process may have an io_uring, which bound to @mm of another process, even though the layouts may be completely different.

File descriptors are different here, because io_uring doesn't know about them, They are controlled by the userspace (send, dup, fork, etc), and don't sabotage all isolation work done in the kernel. A dire example here is stealing io-wq from within a container, which is trivial with global self-made id. I would love to hear, if I am mistaken somewhere.

Is there some better option?

Quoted 7 lines

Quoted 7 lines

OK, so how about this:

- We use the 'fd' as the lookup key. This makes it easy since we can just check if it's a io_uring instance or not, we don't need to do any tracking on the side. It also means that the application asking for sharing must already have some relationship to the process that created the ring.

Yeah, that's exactly the point.

Quoted 1 line

Quoted 1 line

Quoted 5 lines

Quoted 5 lines

- mm/creds must be transferred through the work item. Any SQE done on behalf of io_uring_enter() directly already has that, if punted we must pass the creds and mm. This means we break the static setup of io_wq->mm/creds. It also means that we probably have to add that to io_wq_work, which kind of sucks, but...

ehh, juggling mm's... But don't have anything nicer myself.

Quoted 8 lines

It'd fix Stefan's worry too.

Quoted 2 lines

I think with that we have a decent setup, that's also safe. I've dropped the sharing patches for now, from the 5.6 tree.

So one concern might be SQPOLL, it'll have to use the ctx creds and mm as usual. I guess that is ok.

OK. I'll send the patches for the first part now, and take a look at the second one a bit latter if isn't done until then.

Quoted 56 lines · Pavel Begunkov

Quoted 44 lines · Jens Axboe

Quoted 43 lines · Jens Axboe

Quoted 41 lines · Pavel Begunkov

Quoted 17 lines · Jens Axboe

Quoted 16 lines · Pavel Begunkov

Quoted 12 lines · Jens Axboe

Quoted 6 lines · Pavel Begunkov

Quoted 3 lines · Jens Axboe

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Daurnimator

On Fri, 24 Jan 2020 at 10:16, Jens Axboe <[email protected]> wrote:

Ok. I can't promise it'll play handy for sharing. Though, you'll be out of space in struct io_uring_params soon anyway.

I'm going to keep what we have for now, as I'm really not imagining a lot more sharing - what else would we share? So let's not over-design anything.

Fair enough. I prefer a ptr to an extendable struct, that will take the last u64, when needed.

Quoted 7 lines

Quoted 2 lines

However, it's still better to share through file descriptors. It's just not secure enough the way it's now.

Is the file descriptor value really a good choice? We just had some confusion on ring sharing across forks. Not sure using an fd value is a sane "key" to use across processes.

As I see it, the problem with @mm is that uring is dead-bound to it. For example, a process can create and send uring (e.g. via socket), and then be killed. And that basically means
1. @mm of the process is locked just because of the sent uring instance.
2. a process may have an io_uring, which bound to @mm of another process, even though the layouts may be completely different.

File descriptors are different here, because io_uring doesn't know about them, They are controlled by the userspace (send, dup, fork, etc), and don't sabotage all isolation work done in the kernel. A dire example here is stealing io-wq from within a container, which is trivial with global self-made id. I would love to hear, if I am mistaken somewhere.

Is there some better option?

Quoted 7 lines

Quoted 7 lines

OK, so how about this:

- We use the 'fd' as the lookup key. This makes it easy since we can just check if it's a io_uring instance or not, we don't need to do any tracking on the side. It also means that the application asking for sharing must already have some relationship to the process that created the ring.

Yeah, that's exactly the point.

Quoted 1 line

Quoted 1 line

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

- mm/creds must be transferred through the work item. Any SQE done on behalf of io_uring_enter() directly already has that, if punted we must pass the creds and mm. This means we break the static setup of io_wq->mm/creds. It also means that we probably have to add that to io_wq_work, which kind of sucks, but...

ehh, juggling mm's... But don't have anything nicer myself.

We already do juggle mm's, this is no different. A worker potentially retain the mm across works if they are the same.

Quoted 9 lines

Quoted 8 lines

It'd fix Stefan's worry too.

Quoted 2 lines

I think with that we have a decent setup, that's also safe. I've dropped the sharing patches for now, from the 5.6 tree.

So one concern might be SQPOLL, it'll have to use the ctx creds and mm as usual. I guess that is ok.

Quoted 2 lines

OK. I'll send the patches for the first part now, and take a look at the second one a bit latter if isn't done until then.

Hang on a second, I'm doing the mm and creds bits right now. I'll push that to a branch, if you want to do the actual fd stuff on top of that, that would be great.

Quoted 77 lines · Jens Axboe

Quoted 56 lines · Pavel Begunkov

Quoted 44 lines · Jens Axboe

Quoted 43 lines · Jens Axboe

Quoted 41 lines · Pavel Begunkov

Quoted 17 lines · Jens Axboe

Quoted 16 lines · Pavel Begunkov

Quoted 12 lines · Jens Axboe

Quoted 6 lines · Pavel Begunkov

Quoted 3 lines · Jens Axboe

Quoted 2 lines · Pavel Begunkov

Quoted 1 line · Daurnimator

On Fri, 24 Jan 2020 at 10:16, Jens Axboe <[email protected]> wrote:

Ok. I can't promise it'll play handy for sharing. Though, you'll be out of space in struct io_uring_params soon anyway.

I'm going to keep what we have for now, as I'm really not imagining a lot more sharing - what else would we share? So let's not over-design anything.

Fair enough. I prefer a ptr to an extendable struct, that will take the last u64, when needed.

Quoted 7 lines

Quoted 2 lines

However, it's still better to share through file descriptors. It's just not secure enough the way it's now.

Is the file descriptor value really a good choice? We just had some confusion on ring sharing across forks. Not sure using an fd value is a sane "key" to use across processes.

As I see it, the problem with @mm is that uring is dead-bound to it. For example, a process can create and send uring (e.g. via socket), and then be killed. And that basically means
1. @mm of the process is locked just because of the sent uring instance.
2. a process may have an io_uring, which bound to @mm of another process, even though the layouts may be completely different.

File descriptors are different here, because io_uring doesn't know about them, They are controlled by the userspace (send, dup, fork, etc), and don't sabotage all isolation work done in the kernel. A dire example here is stealing io-wq from within a container, which is trivial with global self-made id. I would love to hear, if I am mistaken somewhere.

Is there some better option?

Quoted 7 lines

Quoted 7 lines

OK, so how about this:

- We use the 'fd' as the lookup key. This makes it easy since we can just check if it's a io_uring instance or not, we don't need to do any tracking on the side. It also means that the application asking for sharing must already have some relationship to the process that created the ring.

Yeah, that's exactly the point.

Quoted 1 line

Quoted 1 line

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

- mm/creds must be transferred through the work item. Any SQE done on behalf of io_uring_enter() directly already has that, if punted we must pass the creds and mm. This means we break the static setup of io_wq->mm/creds. It also means that we probably have to add that to io_wq_work, which kind of sucks, but...

ehh, juggling mm's... But don't have anything nicer myself.

We already do juggle mm's, this is no different. A worker potentially retain the mm across works if they are the same.

Quoted 9 lines

Quoted 8 lines

It'd fix Stefan's worry too.

Quoted 2 lines

I think with that we have a decent setup, that's also safe. I've dropped the sharing patches for now, from the 5.6 tree.

So one concern might be SQPOLL, it'll have to use the ctx creds and mm as usual. I guess that is ok.

Quoted 7 lines

Quoted 2 lines

OK. I'll send the patches for the first part now, and take a look at the second one a bit latter if isn't done until then.

Hang on a second, I'm doing the mm and creds bits right now. I'll push that to a branch, if you want to do the actual fd stuff on top of that, that would be great.

Sure, should be trivially mergeable.

Quoted 85 lines · Jens Axboe

Quoted 84 lines · Pavel Begunkov

Quoted 56 lines · Jens Axboe

Quoted 44 lines · Jens Axboe

Quoted 43 lines · Pavel Begunkov

Quoted 41 lines · Jens Axboe

Quoted 17 lines · Pavel Begunkov

Quoted 16 lines · Jens Axboe

Quoted 12 lines · Pavel Begunkov

Quoted 6 lines · Jens Axboe

Quoted 3 lines · Pavel Begunkov

Quoted 2 lines · Daurnimator

On Fri, 24 Jan 2020 at 10:16, Jens Axboe <[email protected]> wrote:

Ok. I can't promise it'll play handy for sharing. Though, you'll be out of space in struct io_uring_params soon anyway.

I'm going to keep what we have for now, as I'm really not imagining a lot more sharing - what else would we share? So let's not over-design anything.

Fair enough. I prefer a ptr to an extendable struct, that will take the last u64, when needed.

Quoted 7 lines

Quoted 2 lines

However, it's still better to share through file descriptors. It's just not secure enough the way it's now.

Is the file descriptor value really a good choice? We just had some confusion on ring sharing across forks. Not sure using an fd value is a sane "key" to use across processes.

As I see it, the problem with @mm is that uring is dead-bound to it. For example, a process can create and send uring (e.g. via socket), and then be killed. And that basically means
1. @mm of the process is locked just because of the sent uring instance.
2. a process may have an io_uring, which bound to @mm of another process, even though the layouts may be completely different.

File descriptors are different here, because io_uring doesn't know about them, They are controlled by the userspace (send, dup, fork, etc), and don't sabotage all isolation work done in the kernel. A dire example here is stealing io-wq from within a container, which is trivial with global self-made id. I would love to hear, if I am mistaken somewhere.

Is there some better option?

Quoted 7 lines

Quoted 7 lines

OK, so how about this:

- We use the 'fd' as the lookup key. This makes it easy since we can just check if it's a io_uring instance or not, we don't need to do any tracking on the side. It also means that the application asking for sharing must already have some relationship to the process that created the ring.

Yeah, that's exactly the point.

Quoted 1 line

Quoted 1 line

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

- mm/creds must be transferred through the work item. Any SQE done on behalf of io_uring_enter() directly already has that, if punted we must pass the creds and mm. This means we break the static setup of io_wq->mm/creds. It also means that we probably have to add that to io_wq_work, which kind of sucks, but...

ehh, juggling mm's... But don't have anything nicer myself.

We already do juggle mm's, this is no different. A worker potentially retain the mm across works if they are the same.

Quoted 11 lines

Quoted 8 lines

It'd fix Stefan's worry too.

Quoted 2 lines

I think with that we have a decent setup, that's also safe. I've dropped the sharing patches for now, from the 5.6 tree.

So one concern might be SQPOLL, it'll have to use the ctx creds and mm as usual. I guess that is ok.

OK. I'll send the patches for the first part now, and take a look at the second one a bit latter if isn't done until then.

Hang on a second, I'm doing the mm and creds bits right now. I'll push that to a branch, if you want to do the actual fd stuff on top of that, that would be great.

Quoted 1 line

Sure, should be trivially mergeable.

https://git.kernel.dk/cgit/linux-block/log/?h=for-5.6/io_uring-vfs-wq

Top patch there is the mm/creds passing. I kind of like it even if it means we're growing io_wq_worker (and subsequently io_kiocb) by 16 bytes, as it means we can be more flexible. This solves it for this use case, but also the case that Stefan was worried about.

Quoted 23 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

OK, so how about this:

- We use the 'fd' as the lookup key. This makes it easy since we can just check if it's a io_uring instance or not, we don't need to do any tracking on the side. It also means that the application asking for sharing must already have some relationship to the process that created the ring.

- mm/creds must be transferred through the work item. Any SQE done on behalf of io_uring_enter() directly already has that, if punted we must pass the creds and mm. This means we break the static setup of io_wq->mm/creds. It also means that we probably have to add that to io_wq_work, which kind of sucks, but...

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

It'd fix Stefan's worry too.

Quoted 2 lines

I think with that we have a decent setup, that's also safe. I've dropped the sharing patches for now, from the 5.6 tree.

So one concern might be SQPOLL, it'll have to use the ctx creds and mm as usual. I guess that is ok.

https://git.kernel.dk/cgit/linux-block/log/?h=for-5.6/io_uring-vfs-wq

Quoted 4 lines

Top patch there is the mm/creds passing. I kind of like it even if it means we're growing io_wq_worker (and subsequently io_kiocb) by 16 bytes, as it means we can be more flexible. This solves it for this use case, but also the case that Stefan was worried about.

Ok, that means that ctx->creds is only used in the IORING_SETUP_SQPOLL case and there it's used for all requests as get_current_cred() is the same as ctx->creds from within io_sq_thread(), correct?

And in all other cases get_current_cred() is used at io_uring_enter() time.

That's good in order to make the behavior consistent again and prevents potential random security problems.

BTW: you need to revert/drop 44d282796f81eb1debc1d7cb53245b4cb3214cb5 in that branch. Or just rebase on v5.5 final?

Thanks!
metze

Quoted 2 lines

That's good in order to make the behavior consistent again and prevents potential random security problems.

BTW, there also can be problems with registered resources. E.g. for buffers io_uring can get_user_pages() of one process, and then use the pages from another process by passing a buffer index. This is not as bad, however.

Hi Pavel,

Quoted 7 lines

Quoted 2 lines

That's good in order to make the behavior consistent again and prevents potential random security problems.

BTW, there also can be problems with registered resources. E.g. for buffers io_uring can get_user_pages() of one process, and then use the pages from another process by passing a buffer index. This is not as bad, however.

Yes, but that can only happen by intention, right?
And not randomly depending on the cache state. If the application has confidential data in the registered buffers, files then it should not share the ring fd with untrusted processes.