[PATCHSET] io_uring: add support for accept(4)

[PATCHSET] io_uring: add support for accept(4)

[Jens Axboe]

This series adds support for applications doing async accept()
through io_uring.

Patch 1 is just a prep patch, adding support for inheriting a process
file table for commands.

Patch 2 abstracts out __sys_accept4_file(), which is the same as
__sys_accept4(), except it takes a struct file and extra file flags.
Should not have any functional changes.

And finally patch 3 adds support for IORING_OP_ACCEPT. sqe->fd is
the file descriptor, sqe->addr holds a pointer to struct sockaddr,
sqe->addr2 holds a pointer to socklen_t, and finally sqe->accept_flags
holds the flags for accept(4).

The series is against my for-5.5/io_uring tree, and also exists
as a for-5.5/io_uring-test branch. I've got a test case for this
that I haven't pushed to liburing yet, will do so shortly.

 fs/io_uring.c                 | 56 ++++++++++++++++++++++++++++--
 include/linux/socket.h        |  3 ++
 include/uapi/linux/io_uring.h |  7 +++-
 net/socket.c                  | 65 ++++++++++++++++++++++-------------
 4 files changed, 103 insertions(+), 28 deletions(-)

-- 
Jens Axboe

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

[Jens Axboe]

This is in preparation for adding opcodes that need to modify files
in a process file table, either adding new ones or closing old ones.

If an opcode needs this, it must set REQ_F_NEED_FILES in the request
structure. If work that needs to get punted to async context have this
set, they will grab a reference to the process file table. When the
work is completed, the reference is dropped again.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/io_uring.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 635856023fdf..ad462237275e 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -267,10 +267,11 @@ struct io_ring_ctx {
 struct sqe_submit {
 	const struct io_uring_sqe	*sqe;
 	unsigned short			index;
+	bool				has_user : 1;
+	bool				in_async : 1;
+	bool				needs_fixed_file : 1;
 	u32				sequence;
-	bool				has_user;
-	bool				in_async;
-	bool				needs_fixed_file;
+	struct files_struct		*files;
 };
 
 /*
@@ -323,6 +324,7 @@ struct io_kiocb {
 #define REQ_F_FAIL_LINK		256	/* fail rest of links */
 #define REQ_F_SHADOW_DRAIN	512	/* link-drain shadow req */
 #define REQ_F_TIMEOUT		1024	/* timeout request */
+#define REQ_F_NEED_FILES	2048	/* needs to assume file table */
 	u64			user_data;
 	u32			result;
 	u32			sequence;
@@ -2191,6 +2193,7 @@ static inline bool io_sqe_needs_user(const struct io_uring_sqe *sqe)
 static void io_sq_wq_submit_work(struct work_struct *work)
 {
 	struct io_kiocb *req = container_of(work, struct io_kiocb, work);
+	struct files_struct *old_files = NULL;
 	struct io_ring_ctx *ctx = req->ctx;
 	struct mm_struct *cur_mm = NULL;
 	struct async_list *async_list;
@@ -2220,6 +2223,10 @@ static void io_sq_wq_submit_work(struct work_struct *work)
 				set_fs(USER_DS);
 			}
 		}
+		if (s->files && !old_files) {
+			old_files = current->files;
+			current->files = s->files;
+		}
 
 		if (!ret) {
 			s->has_user = cur_mm != NULL;
@@ -2312,6 +2319,11 @@ static void io_sq_wq_submit_work(struct work_struct *work)
 		unuse_mm(cur_mm);
 		mmput(cur_mm);
 	}
+	if (old_files) {
+		struct files_struct *files = current->files;
+		current->files = old_files;
+		put_files_struct(files);
+	}
 }
 
 /*
@@ -2413,6 +2425,8 @@ static int __io_queue_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
 
 			s->sqe = sqe_copy;
 			memcpy(&req->submit, s, sizeof(*s));
+			if (req->flags & REQ_F_NEED_FILES)
+				req->submit.files = get_files_struct(current);
 			list = io_async_list_from_sqe(ctx, s->sqe);
 			if (!io_add_to_prev_work(list, req)) {
 				if (list)
@@ -2633,6 +2647,7 @@ static bool io_get_sqring(struct io_ring_ctx *ctx, struct sqe_submit *s)
 		s->index = head;
 		s->sqe = &ctx->sq_sqes[head];
 		s->sequence = ctx->cached_sq_head;
+		s->files = NULL;
 		ctx->cached_sq_head++;
 		return true;
 	}
-- 
2.17.1

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

[Jann Horn]

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

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

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

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

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

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

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

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

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

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

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

[Jens Axboe]

On 10/17/19 8:41 PM, Jann Horn wrote:
> On Fri, Oct 18, 2019 at 4:01 AM Jens Axboe <axboe@kernel.dk> wrote:
>> This is in preparation for adding opcodes that need to modify files
>> in a process file table, either adding new ones or closing old ones.
> 
> Closing old ones would be tricky. Basically if you call
> get_files_struct() while you're between an fdget()/fdput() pair (e.g.
> from sys_io_uring_enter()), you're not allowed to use that
> files_struct reference to replace or close existing FDs through that
> reference. (Or more accurately, if you go through fdget() with
> files_struct refcount 1, you must not replace/close FDs in there in
> any way until you've passed the corresponding fdput().)
> 
> You can avoid that if you ensure that you never use fdget()/fdput() in
> the relevant places, only fget()/fput().

That's a good point, I didn't think the closing aspect through when
writing that changelog. File addition is the most interesting aspect,
obviously, and the only part that I care about in this patch set. I'll
change the wording.

>> If an opcode needs this, it must set REQ_F_NEED_FILES in the request
>> structure. If work that needs to get punted to async context have this
>> set, they will grab a reference to the process file table. When the
>> work is completed, the reference is dropped again.
> [...]
>> @@ -2220,6 +2223,10 @@ static void io_sq_wq_submit_work(struct work_struct *work)
>>                                  set_fs(USER_DS);
>>                          }
>>                  }
>> +               if (s->files && !old_files) {
>> +                       old_files = current->files;
>> +                       current->files = s->files;
>> +               }
> 
> AFAIK e.g. stuff like proc_fd_link() in procfs can concurrently call
> get_files_struct() even on kernel tasks, so you should take the
> task_lock(current) while fiddling with the ->files pointer.

Fixed up, thanks!

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

Like the mm, we should only have the one file table. But there's no
reason to not handle this properly, I've amended the commit to properly
swap so it works for any number of file tables.

>>                  if (!ret) {
>>                          s->has_user = cur_mm != NULL;
>> @@ -2312,6 +2319,11 @@ static void io_sq_wq_submit_work(struct work_struct *work)
>>                  unuse_mm(cur_mm);
>>                  mmput(cur_mm);
>>          }
>> +       if (old_files) {
>> +               struct files_struct *files = current->files;
>> +               current->files = old_files;
>> +               put_files_struct(files);
>> +       }
> 
> And then here the first files_struct reference is dropped, and the
> rest of them leak?

Fixed with the above change.

>> @@ -2413,6 +2425,8 @@ static int __io_queue_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
>>
>>                          s->sqe = sqe_copy;
>>                          memcpy(&req->submit, s, sizeof(*s));
>> +                       if (req->flags & REQ_F_NEED_FILES)
>> +                               req->submit.files = get_files_struct(current);
> 
> Stupid question: How does this interact with sqpoll mode? In that
> case, this function is running on a kernel thread that isn't sharing
> the application's files_struct, right?

Not a stupid question! It doesn't work with sqpoll. We need to be
entered on behalf of the task, and we never see that with sqpoll (except
if NEED_WAKE is set in flags).

For now I'll just forbid it explicitly in io_accept(), just like we do
for IORING_SETUP_IOPOLL.

Updated patch1:

http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=df6caac708dae8ee9a74c9016e479b02ad78d436

and patch 3:

http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=442bb35fc4f8f28c29ea220475c45babb44ee49c

-- 
Jens Axboe

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

[Jann Horn]

On Fri, Oct 18, 2019 at 4:01 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 10/17/19 8:41 PM, Jann Horn wrote:
> > On Fri, Oct 18, 2019 at 4:01 AM Jens Axboe <axboe@kernel.dk> wrote:
> >> This is in preparation for adding opcodes that need to modify files
> >> in a process file table, either adding new ones or closing old ones.
[...]
> Updated patch1:
>
> http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=df6caac708dae8ee9a74c9016e479b02ad78d436

I don't understand what you're doing with old_files in there. In the
"s->files && !old_files" branch, "current->files = s->files" happens
without holding task_lock(), but current->files and s->files are also
the same already at that point anyway. And what's the intent behind
assigning stuff to old_files inside the loop? Isn't that going to
cause the workqueue to keep a modified current->files beyond the
runtime of the work?

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

[Jens Axboe]

On 10/18/19 8:34 AM, Jann Horn wrote:
> On Fri, Oct 18, 2019 at 4:01 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 10/17/19 8:41 PM, Jann Horn wrote:
>>> On Fri, Oct 18, 2019 at 4:01 AM Jens Axboe <axboe@kernel.dk> wrote:
>>>> This is in preparation for adding opcodes that need to modify files
>>>> in a process file table, either adding new ones or closing old ones.
> [...]
>> Updated patch1:
>>
>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=df6caac708dae8ee9a74c9016e479b02ad78d436
> 
> I don't understand what you're doing with old_files in there. In the
> "s->files && !old_files" branch, "current->files = s->files" happens
> without holding task_lock(), but current->files and s->files are also
> the same already at that point anyway. And what's the intent behind
> assigning stuff to old_files inside the loop? Isn't that going to
> cause the workqueue to keep a modified current->files beyond the
> runtime of the work?

I simply forgot to remove the old block, it should only have this one:

if (s->files && s->files != cur_files) {                        
        task_lock(current);                                     
        current->files = s->files;                              
        task_unlock(current);                                   
        if (cur_files)                                          
                put_files_struct(cur_files);                    
        cur_files = s->files;                                   
}

-- 
Jens Axboe

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

[Jann Horn]

On Fri, Oct 18, 2019 at 4:37 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 10/18/19 8:34 AM, Jann Horn wrote:
> > On Fri, Oct 18, 2019 at 4:01 PM Jens Axboe <axboe@kernel.dk> wrote:
> >> On 10/17/19 8:41 PM, Jann Horn wrote:
> >>> On Fri, Oct 18, 2019 at 4:01 AM Jens Axboe <axboe@kernel.dk> wrote:
> >>>> This is in preparation for adding opcodes that need to modify files
> >>>> in a process file table, either adding new ones or closing old ones.
> > [...]
> >> Updated patch1:
> >>
> >> http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=df6caac708dae8ee9a74c9016e479b02ad78d436
> >
> > I don't understand what you're doing with old_files in there. In the
> > "s->files && !old_files" branch, "current->files = s->files" happens
> > without holding task_lock(), but current->files and s->files are also
> > the same already at that point anyway. And what's the intent behind
> > assigning stuff to old_files inside the loop? Isn't that going to
> > cause the workqueue to keep a modified current->files beyond the
> > runtime of the work?
>
> I simply forgot to remove the old block, it should only have this one:
>
> if (s->files && s->files != cur_files) {
>         task_lock(current);
>         current->files = s->files;
>         task_unlock(current);
>         if (cur_files)
>                 put_files_struct(cur_files);
>         cur_files = s->files;
> }

Don't you still need a put_files_struct() in the case where "s->files
== cur_files"?

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

[Jens Axboe]

On 10/18/19 8:40 AM, Jann Horn wrote:
> On Fri, Oct 18, 2019 at 4:37 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 10/18/19 8:34 AM, Jann Horn wrote:
>>> On Fri, Oct 18, 2019 at 4:01 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>> On 10/17/19 8:41 PM, Jann Horn wrote:
>>>>> On Fri, Oct 18, 2019 at 4:01 AM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>> This is in preparation for adding opcodes that need to modify files
>>>>>> in a process file table, either adding new ones or closing old ones.
>>> [...]
>>>> Updated patch1:
>>>>
>>>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=df6caac708dae8ee9a74c9016e479b02ad78d436
>>>
>>> I don't understand what you're doing with old_files in there. In the
>>> "s->files && !old_files" branch, "current->files = s->files" happens
>>> without holding task_lock(), but current->files and s->files are also
>>> the same already at that point anyway. And what's the intent behind
>>> assigning stuff to old_files inside the loop? Isn't that going to
>>> cause the workqueue to keep a modified current->files beyond the
>>> runtime of the work?
>>
>> I simply forgot to remove the old block, it should only have this one:
>>
>> if (s->files && s->files != cur_files) {
>>          task_lock(current);
>>          current->files = s->files;
>>          task_unlock(current);
>>          if (cur_files)
>>                  put_files_struct(cur_files);
>>          cur_files = s->files;
>> }
> 
> Don't you still need a put_files_struct() in the case where "s->files
> == cur_files"?

I want to hold on to the files for as long as I can, to avoid unnecessary
shuffling of it. But I take it your worry here is that we'll be calling
something that manipulates ->files? Nothing should do that, unless
s->files is set. We didn't hide the workqueue ->files[] before this
change either.

-- 
Jens Axboe

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

[Jann Horn]

On Fri, Oct 18, 2019 at 4:43 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 10/18/19 8:40 AM, Jann Horn wrote:
> > On Fri, Oct 18, 2019 at 4:37 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>
> >> On 10/18/19 8:34 AM, Jann Horn wrote:
> >>> On Fri, Oct 18, 2019 at 4:01 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>> On 10/17/19 8:41 PM, Jann Horn wrote:
> >>>>> On Fri, Oct 18, 2019 at 4:01 AM Jens Axboe <axboe@kernel.dk> wrote:
> >>>>>> This is in preparation for adding opcodes that need to modify files
> >>>>>> in a process file table, either adding new ones or closing old ones.
> >>> [...]
> >>>> Updated patch1:
> >>>>
> >>>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=df6caac708dae8ee9a74c9016e479b02ad78d436
> >>>
> >>> I don't understand what you're doing with old_files in there. In the
> >>> "s->files && !old_files" branch, "current->files = s->files" happens
> >>> without holding task_lock(), but current->files and s->files are also
> >>> the same already at that point anyway. And what's the intent behind
> >>> assigning stuff to old_files inside the loop? Isn't that going to
> >>> cause the workqueue to keep a modified current->files beyond the
> >>> runtime of the work?
> >>
> >> I simply forgot to remove the old block, it should only have this one:
> >>
> >> if (s->files && s->files != cur_files) {
> >>          task_lock(current);
> >>          current->files = s->files;
> >>          task_unlock(current);
> >>          if (cur_files)
> >>                  put_files_struct(cur_files);
> >>          cur_files = s->files;
> >> }
> >
> > Don't you still need a put_files_struct() in the case where "s->files
> > == cur_files"?
>
> I want to hold on to the files for as long as I can, to avoid unnecessary
> shuffling of it. But I take it your worry here is that we'll be calling
> something that manipulates ->files? Nothing should do that, unless
> s->files is set. We didn't hide the workqueue ->files[] before this
> change either.

No, my worry is that the refcount of the files_struct is left too
high. From what I can tell, the "do" loop in io_sq_wq_submit_work()
iterates over multiple instances of struct sqe_submit. If there are
two sqe_submit instances with the same ->files (each holding a
reference from the get_files_struct() in __io_queue_sqe()), then:

When processing the first sqe_submit instance, current->files and
cur_files are set to $user_files.
When processing the second sqe_submit instance, nothing happens
(s->files == cur_files).
After the loop, at the end of the function, put_files_struct() is
called once on $user_files.

So get_files_struct() has been called twice, but put_files_struct()
has only been called once. That leaves the refcount too high, and by
repeating this, an attacker can make the refcount wrap around and then
cause a use-after-free.

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

[Jens Axboe]

On 10/18/19 8:52 AM, Jann Horn wrote:
> On Fri, Oct 18, 2019 at 4:43 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 10/18/19 8:40 AM, Jann Horn wrote:
>>> On Fri, Oct 18, 2019 at 4:37 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>
>>>> On 10/18/19 8:34 AM, Jann Horn wrote:
>>>>> On Fri, Oct 18, 2019 at 4:01 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>> On 10/17/19 8:41 PM, Jann Horn wrote:
>>>>>>> On Fri, Oct 18, 2019 at 4:01 AM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>>>> This is in preparation for adding opcodes that need to modify files
>>>>>>>> in a process file table, either adding new ones or closing old ones.
>>>>> [...]
>>>>>> Updated patch1:
>>>>>>
>>>>>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=df6caac708dae8ee9a74c9016e479b02ad78d436
>>>>>
>>>>> I don't understand what you're doing with old_files in there. In the
>>>>> "s->files && !old_files" branch, "current->files = s->files" happens
>>>>> without holding task_lock(), but current->files and s->files are also
>>>>> the same already at that point anyway. And what's the intent behind
>>>>> assigning stuff to old_files inside the loop? Isn't that going to
>>>>> cause the workqueue to keep a modified current->files beyond the
>>>>> runtime of the work?
>>>>
>>>> I simply forgot to remove the old block, it should only have this one:
>>>>
>>>> if (s->files && s->files != cur_files) {
>>>>           task_lock(current);
>>>>           current->files = s->files;
>>>>           task_unlock(current);
>>>>           if (cur_files)
>>>>                   put_files_struct(cur_files);
>>>>           cur_files = s->files;
>>>> }
>>>
>>> Don't you still need a put_files_struct() in the case where "s->files
>>> == cur_files"?
>>
>> I want to hold on to the files for as long as I can, to avoid unnecessary
>> shuffling of it. But I take it your worry here is that we'll be calling
>> something that manipulates ->files? Nothing should do that, unless
>> s->files is set. We didn't hide the workqueue ->files[] before this
>> change either.
> 
> No, my worry is that the refcount of the files_struct is left too
> high. From what I can tell, the "do" loop in io_sq_wq_submit_work()
> iterates over multiple instances of struct sqe_submit. If there are
> two sqe_submit instances with the same ->files (each holding a
> reference from the get_files_struct() in __io_queue_sqe()), then:
> 
> When processing the first sqe_submit instance, current->files and
> cur_files are set to $user_files.
> When processing the second sqe_submit instance, nothing happens
> (s->files == cur_files).
> After the loop, at the end of the function, put_files_struct() is
> called once on $user_files.
> 
> So get_files_struct() has been called twice, but put_files_struct()
> has only been called once. That leaves the refcount too high, and by
> repeating this, an attacker can make the refcount wrap around and then
> cause a use-after-free.

Ah now I see what you are getting at, yes that's clearly a bug! I wonder
how we best safely can batch the drops. We can track the number of times
we've used the same files, and do atomic_sub_and_test() in a
put_files_struct_many() type addition. But that would leave us open to
the issue you describe, where someone could maliciously overflow the
files ref count.

Probably not worth over-optimizing, as long as we can avoid the
current->files task lock/unlock and shuffle.

I'll update the patch.

-- 
Jens Axboe

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

[Jens Axboe]

On 10/18/19 9:00 AM, Jens Axboe wrote:
> On 10/18/19 8:52 AM, Jann Horn wrote:
>> On Fri, Oct 18, 2019 at 4:43 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>> On 10/18/19 8:40 AM, Jann Horn wrote:
>>>> On Fri, Oct 18, 2019 at 4:37 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>
>>>>> On 10/18/19 8:34 AM, Jann Horn wrote:
>>>>>> On Fri, Oct 18, 2019 at 4:01 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>>> On 10/17/19 8:41 PM, Jann Horn wrote:
>>>>>>>> On Fri, Oct 18, 2019 at 4:01 AM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>>>>> This is in preparation for adding opcodes that need to modify files
>>>>>>>>> in a process file table, either adding new ones or closing old ones.
>>>>>> [...]
>>>>>>> Updated patch1:
>>>>>>>
>>>>>>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=df6caac708dae8ee9a74c9016e479b02ad78d436
>>>>>>
>>>>>> I don't understand what you're doing with old_files in there. In the
>>>>>> "s->files && !old_files" branch, "current->files = s->files" happens
>>>>>> without holding task_lock(), but current->files and s->files are also
>>>>>> the same already at that point anyway. And what's the intent behind
>>>>>> assigning stuff to old_files inside the loop? Isn't that going to
>>>>>> cause the workqueue to keep a modified current->files beyond the
>>>>>> runtime of the work?
>>>>>
>>>>> I simply forgot to remove the old block, it should only have this one:
>>>>>
>>>>> if (s->files && s->files != cur_files) {
>>>>>            task_lock(current);
>>>>>            current->files = s->files;
>>>>>            task_unlock(current);
>>>>>            if (cur_files)
>>>>>                    put_files_struct(cur_files);
>>>>>            cur_files = s->files;
>>>>> }
>>>>
>>>> Don't you still need a put_files_struct() in the case where "s->files
>>>> == cur_files"?
>>>
>>> I want to hold on to the files for as long as I can, to avoid unnecessary
>>> shuffling of it. But I take it your worry here is that we'll be calling
>>> something that manipulates ->files? Nothing should do that, unless
>>> s->files is set. We didn't hide the workqueue ->files[] before this
>>> change either.
>>
>> No, my worry is that the refcount of the files_struct is left too
>> high. From what I can tell, the "do" loop in io_sq_wq_submit_work()
>> iterates over multiple instances of struct sqe_submit. If there are
>> two sqe_submit instances with the same ->files (each holding a
>> reference from the get_files_struct() in __io_queue_sqe()), then:
>>
>> When processing the first sqe_submit instance, current->files and
>> cur_files are set to $user_files.
>> When processing the second sqe_submit instance, nothing happens
>> (s->files == cur_files).
>> After the loop, at the end of the function, put_files_struct() is
>> called once on $user_files.
>>
>> So get_files_struct() has been called twice, but put_files_struct()
>> has only been called once. That leaves the refcount too high, and by
>> repeating this, an attacker can make the refcount wrap around and then
>> cause a use-after-free.
> 
> Ah now I see what you are getting at, yes that's clearly a bug! I wonder
> how we best safely can batch the drops. We can track the number of times
> we've used the same files, and do atomic_sub_and_test() in a
> put_files_struct_many() type addition. But that would leave us open to
> the issue you describe, where someone could maliciously overflow the
> files ref count.
> 
> Probably not worth over-optimizing, as long as we can avoid the
> current->files task lock/unlock and shuffle.
> 
> I'll update the patch.

Alright, this incremental on top should do it. And full updated patch
here:

http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=40449c5a3d3b16796fa13e9469c69d62986e961c

Let me know what you think.

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 68eda36bcc9c..2fed0badad38 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2289,13 +2289,13 @@ static void io_sq_wq_submit_work(struct work_struct *work)
 				set_fs(USER_DS);
 			}
 		}
-		if (s->files && s->files != cur_files) {
+		if (cur_files)
+			put_files_struct(cur_files);
+		cur_files = s->files;
+		if (cur_files && cur_files != current->files) {
 			task_lock(current);
-			current->files = s->files;
+			current->files = cur_files;
 			task_unlock(current);
-			if (cur_files)
-				put_files_struct(cur_files);
-			cur_files = s->files;
 		}
 
 		if (!ret) {
@@ -2393,8 +2393,9 @@ static void io_sq_wq_submit_work(struct work_struct *work)
 		task_lock(current);
 		current->files = old_files;
 		task_unlock(current);
-		put_files_struct(cur_files);
 	}
+	if (cur_files)
+		put_files_struct(cur_files);
 }
 
 /*

-- 
Jens Axboe

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

[Jann Horn]

On Fri, Oct 18, 2019 at 5:55 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 10/18/19 9:00 AM, Jens Axboe wrote:
> > On 10/18/19 8:52 AM, Jann Horn wrote:
> >> On Fri, Oct 18, 2019 at 4:43 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>
> >>> On 10/18/19 8:40 AM, Jann Horn wrote:
> >>>> On Fri, Oct 18, 2019 at 4:37 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>>>
> >>>>> On 10/18/19 8:34 AM, Jann Horn wrote:
> >>>>>> On Fri, Oct 18, 2019 at 4:01 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>>>>> On 10/17/19 8:41 PM, Jann Horn wrote:
> >>>>>>>> On Fri, Oct 18, 2019 at 4:01 AM Jens Axboe <axboe@kernel.dk> wrote:
> >>>>>>>>> This is in preparation for adding opcodes that need to modify files
> >>>>>>>>> in a process file table, either adding new ones or closing old ones.
> >>>>>> [...]
> >>>>>>> Updated patch1:
> >>>>>>>
> >>>>>>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=df6caac708dae8ee9a74c9016e479b02ad78d436
> >>>>>>
> >>>>>> I don't understand what you're doing with old_files in there. In the
> >>>>>> "s->files && !old_files" branch, "current->files = s->files" happens
> >>>>>> without holding task_lock(), but current->files and s->files are also
> >>>>>> the same already at that point anyway. And what's the intent behind
> >>>>>> assigning stuff to old_files inside the loop? Isn't that going to
> >>>>>> cause the workqueue to keep a modified current->files beyond the
> >>>>>> runtime of the work?
> >>>>>
> >>>>> I simply forgot to remove the old block, it should only have this one:
> >>>>>
> >>>>> if (s->files && s->files != cur_files) {
> >>>>>            task_lock(current);
> >>>>>            current->files = s->files;
> >>>>>            task_unlock(current);
> >>>>>            if (cur_files)
> >>>>>                    put_files_struct(cur_files);
> >>>>>            cur_files = s->files;
> >>>>> }
> >>>>
> >>>> Don't you still need a put_files_struct() in the case where "s->files
> >>>> == cur_files"?
> >>>
> >>> I want to hold on to the files for as long as I can, to avoid unnecessary
> >>> shuffling of it. But I take it your worry here is that we'll be calling
> >>> something that manipulates ->files? Nothing should do that, unless
> >>> s->files is set. We didn't hide the workqueue ->files[] before this
> >>> change either.
> >>
> >> No, my worry is that the refcount of the files_struct is left too
> >> high. From what I can tell, the "do" loop in io_sq_wq_submit_work()
> >> iterates over multiple instances of struct sqe_submit. If there are
> >> two sqe_submit instances with the same ->files (each holding a
> >> reference from the get_files_struct() in __io_queue_sqe()), then:
> >>
> >> When processing the first sqe_submit instance, current->files and
> >> cur_files are set to $user_files.
> >> When processing the second sqe_submit instance, nothing happens
> >> (s->files == cur_files).
> >> After the loop, at the end of the function, put_files_struct() is
> >> called once on $user_files.
> >>
> >> So get_files_struct() has been called twice, but put_files_struct()
> >> has only been called once. That leaves the refcount too high, and by
> >> repeating this, an attacker can make the refcount wrap around and then
> >> cause a use-after-free.
> >
> > Ah now I see what you are getting at, yes that's clearly a bug! I wonder
> > how we best safely can batch the drops. We can track the number of times
> > we've used the same files, and do atomic_sub_and_test() in a
> > put_files_struct_many() type addition. But that would leave us open to
> > the issue you describe, where someone could maliciously overflow the
> > files ref count.
> >
> > Probably not worth over-optimizing, as long as we can avoid the
> > current->files task lock/unlock and shuffle.
> >
> > I'll update the patch.
>
> Alright, this incremental on top should do it. And full updated patch
> here:
>
> http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=40449c5a3d3b16796fa13e9469c69d62986e961c
>
> Let me know what you think.

Ignoring the locking elision, basically the logic is now this:

static void io_sq_wq_submit_work(struct work_struct *work)
{
        struct io_kiocb *req = container_of(work, struct io_kiocb, work);
        struct files_struct *cur_files = NULL, *old_files;
        [...]
        old_files = current->files;
        [...]
        do {
                struct sqe_submit *s = &req->submit;
                [...]
                if (cur_files)
                        /* drop cur_files reference; borrow lifetime must
                         * end before here */
                        put_files_struct(cur_files);
                /* move reference ownership to cur_files */
                cur_files = s->files;
                if (cur_files) {
                        task_lock(current);
                        /* current->files borrows reference from cur_files;
                         * existing borrow from previous loop ends here */
                        current->files = cur_files;
                        task_unlock(current);
                }

                [call __io_submit_sqe()]
                [...]
        } while (req);
        [...]
        /* existing borrow ends here */
        task_lock(current);
        current->files = old_files;
        task_unlock(current);
        if (cur_files)
                /* drop cur_files reference; borrow lifetime must
                 * end before here */
                put_files_struct(cur_files);
}

If you run two iterations of this loop, with a first element that has
a ->files pointer and a second element that doesn't, then in the
second run through the loop, the reference to the files_struct will be
dropped while current->files still points to it; current->files is
only reset after the loop has ended. If someone accesses
current->files through procfs directly after that, AFAICS you'd get a
use-after-free.

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

[Jens Axboe]

On 10/18/19 10:20 AM, Jann Horn wrote:
> On Fri, Oct 18, 2019 at 5:55 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 10/18/19 9:00 AM, Jens Axboe wrote:
>>> On 10/18/19 8:52 AM, Jann Horn wrote:
>>>> On Fri, Oct 18, 2019 at 4:43 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>
>>>>> On 10/18/19 8:40 AM, Jann Horn wrote:
>>>>>> On Fri, Oct 18, 2019 at 4:37 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>>>
>>>>>>> On 10/18/19 8:34 AM, Jann Horn wrote:
>>>>>>>> On Fri, Oct 18, 2019 at 4:01 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>>>>> On 10/17/19 8:41 PM, Jann Horn wrote:
>>>>>>>>>> On Fri, Oct 18, 2019 at 4:01 AM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>>>>>>> This is in preparation for adding opcodes that need to modify files
>>>>>>>>>>> in a process file table, either adding new ones or closing old ones.
>>>>>>>> [...]
>>>>>>>>> Updated patch1:
>>>>>>>>>
>>>>>>>>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=df6caac708dae8ee9a74c9016e479b02ad78d436
>>>>>>>>
>>>>>>>> I don't understand what you're doing with old_files in there. In the
>>>>>>>> "s->files && !old_files" branch, "current->files = s->files" happens
>>>>>>>> without holding task_lock(), but current->files and s->files are also
>>>>>>>> the same already at that point anyway. And what's the intent behind
>>>>>>>> assigning stuff to old_files inside the loop? Isn't that going to
>>>>>>>> cause the workqueue to keep a modified current->files beyond the
>>>>>>>> runtime of the work?
>>>>>>>
>>>>>>> I simply forgot to remove the old block, it should only have this one:
>>>>>>>
>>>>>>> if (s->files && s->files != cur_files) {
>>>>>>>             task_lock(current);
>>>>>>>             current->files = s->files;
>>>>>>>             task_unlock(current);
>>>>>>>             if (cur_files)
>>>>>>>                     put_files_struct(cur_files);
>>>>>>>             cur_files = s->files;
>>>>>>> }
>>>>>>
>>>>>> Don't you still need a put_files_struct() in the case where "s->files
>>>>>> == cur_files"?
>>>>>
>>>>> I want to hold on to the files for as long as I can, to avoid unnecessary
>>>>> shuffling of it. But I take it your worry here is that we'll be calling
>>>>> something that manipulates ->files? Nothing should do that, unless
>>>>> s->files is set. We didn't hide the workqueue ->files[] before this
>>>>> change either.
>>>>
>>>> No, my worry is that the refcount of the files_struct is left too
>>>> high. From what I can tell, the "do" loop in io_sq_wq_submit_work()
>>>> iterates over multiple instances of struct sqe_submit. If there are
>>>> two sqe_submit instances with the same ->files (each holding a
>>>> reference from the get_files_struct() in __io_queue_sqe()), then:
>>>>
>>>> When processing the first sqe_submit instance, current->files and
>>>> cur_files are set to $user_files.
>>>> When processing the second sqe_submit instance, nothing happens
>>>> (s->files == cur_files).
>>>> After the loop, at the end of the function, put_files_struct() is
>>>> called once on $user_files.
>>>>
>>>> So get_files_struct() has been called twice, but put_files_struct()
>>>> has only been called once. That leaves the refcount too high, and by
>>>> repeating this, an attacker can make the refcount wrap around and then
>>>> cause a use-after-free.
>>>
>>> Ah now I see what you are getting at, yes that's clearly a bug! I wonder
>>> how we best safely can batch the drops. We can track the number of times
>>> we've used the same files, and do atomic_sub_and_test() in a
>>> put_files_struct_many() type addition. But that would leave us open to
>>> the issue you describe, where someone could maliciously overflow the
>>> files ref count.
>>>
>>> Probably not worth over-optimizing, as long as we can avoid the
>>> current->files task lock/unlock and shuffle.
>>>
>>> I'll update the patch.
>>
>> Alright, this incremental on top should do it. And full updated patch
>> here:
>>
>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=40449c5a3d3b16796fa13e9469c69d62986e961c
>>
>> Let me know what you think.
> 
> Ignoring the locking elision, basically the logic is now this:
> 
> static void io_sq_wq_submit_work(struct work_struct *work)
> {
>          struct io_kiocb *req = container_of(work, struct io_kiocb, work);
>          struct files_struct *cur_files = NULL, *old_files;
>          [...]
>          old_files = current->files;
>          [...]
>          do {
>                  struct sqe_submit *s = &req->submit;
>                  [...]
>                  if (cur_files)
>                          /* drop cur_files reference; borrow lifetime must
>                           * end before here */
>                          put_files_struct(cur_files);
>                  /* move reference ownership to cur_files */
>                  cur_files = s->files;
>                  if (cur_files) {
>                          task_lock(current);
>                          /* current->files borrows reference from cur_files;
>                           * existing borrow from previous loop ends here */
>                          current->files = cur_files;
>                          task_unlock(current);
>                  }
> 
>                  [call __io_submit_sqe()]
>                  [...]
>          } while (req);
>          [...]
>          /* existing borrow ends here */
>          task_lock(current);
>          current->files = old_files;
>          task_unlock(current);
>          if (cur_files)
>                  /* drop cur_files reference; borrow lifetime must
>                   * end before here */
>                  put_files_struct(cur_files);
> }
> 
> If you run two iterations of this loop, with a first element that has
> a ->files pointer and a second element that doesn't, then in the
> second run through the loop, the reference to the files_struct will be
> dropped while current->files still points to it; current->files is
> only reset after the loop has ended. If someone accesses
> current->files through procfs directly after that, AFAICS you'd get a
> use-after-free.

Amazing how this is still broken. You are right, and it's especially
annoying since that's exactly the case I originally talked about (not
flipping current->files if we don't have to). I just did it wrong, so
we'll leave a dangling pointer in ->files.

The by far most common case is if one sqe has a files it needs to
attach, then others that also have files will be the same set. So I want
to optimize for the case where we only flip current->files once when we
see the files, and once when we're done with the loop.

Let me see if I can get this right...

-- 
Jens Axboe

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

[Jens Axboe]

On 10/18/19 10:36 AM, Jens Axboe wrote:
>> Ignoring the locking elision, basically the logic is now this:
>>
>> static void io_sq_wq_submit_work(struct work_struct *work)
>> {
>>           struct io_kiocb *req = container_of(work, struct io_kiocb, work);
>>           struct files_struct *cur_files = NULL, *old_files;
>>           [...]
>>           old_files = current->files;
>>           [...]
>>           do {
>>                   struct sqe_submit *s = &req->submit;
>>                   [...]
>>                   if (cur_files)
>>                           /* drop cur_files reference; borrow lifetime must
>>                            * end before here */
>>                           put_files_struct(cur_files);
>>                   /* move reference ownership to cur_files */
>>                   cur_files = s->files;
>>                   if (cur_files) {
>>                           task_lock(current);
>>                           /* current->files borrows reference from cur_files;
>>                            * existing borrow from previous loop ends here */
>>                           current->files = cur_files;
>>                           task_unlock(current);
>>                   }
>>
>>                   [call __io_submit_sqe()]
>>                   [...]
>>           } while (req);
>>           [...]
>>           /* existing borrow ends here */
>>           task_lock(current);
>>           current->files = old_files;
>>           task_unlock(current);
>>           if (cur_files)
>>                   /* drop cur_files reference; borrow lifetime must
>>                    * end before here */
>>                   put_files_struct(cur_files);
>> }
>>
>> If you run two iterations of this loop, with a first element that has
>> a ->files pointer and a second element that doesn't, then in the
>> second run through the loop, the reference to the files_struct will be
>> dropped while current->files still points to it; current->files is
>> only reset after the loop has ended. If someone accesses
>> current->files through procfs directly after that, AFAICS you'd get a
>> use-after-free.
> 
> Amazing how this is still broken. You are right, and it's especially
> annoying since that's exactly the case I originally talked about (not
> flipping current->files if we don't have to). I just did it wrong, so
> we'll leave a dangling pointer in ->files.
> 
> The by far most common case is if one sqe has a files it needs to
> attach, then others that also have files will be the same set. So I want
> to optimize for the case where we only flip current->files once when we
> see the files, and once when we're done with the loop.
> 
> Let me see if I can get this right...

I _think_ the simplest way to do it is simply to have both cur_files and
current->files hold a reference to the file table. That won't really add
any extra cost as the double increments / decrements are following each
other. Something like this incremental, totally untested.


diff --git a/fs/io_uring.c b/fs/io_uring.c
index 2fed0badad38..b3cf3f3d7911 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2293,9 +2293,14 @@ static void io_sq_wq_submit_work(struct work_struct *work)
 			put_files_struct(cur_files);
 		cur_files = s->files;
 		if (cur_files && cur_files != current->files) {
+			struct files_struct *old;
+
+			atomic_inc(&cur_files->count);
 			task_lock(current);
+			old = current->files;
 			current->files = cur_files;
 			task_unlock(current);
+			put_files_struct(old);
 		}
 
 		if (!ret) {
@@ -2390,9 +2395,13 @@ static void io_sq_wq_submit_work(struct work_struct *work)
 		mmput(cur_mm);
 	}
 	if (old_files != current->files) {
+		struct files_struct *old;
+
 		task_lock(current);
+		old = current->files;
 		current->files = old_files;
 		task_unlock(current);
+		put_files_struct(old);
 	}
 	if (cur_files)
 		put_files_struct(cur_files);

-- 
Jens Axboe

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

[Jann Horn]

On Fri, Oct 18, 2019 at 7:05 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 10/18/19 10:36 AM, Jens Axboe wrote:
> >> Ignoring the locking elision, basically the logic is now this:
> >>
> >> static void io_sq_wq_submit_work(struct work_struct *work)
> >> {
> >>           struct io_kiocb *req = container_of(work, struct io_kiocb, work);
> >>           struct files_struct *cur_files = NULL, *old_files;
> >>           [...]
> >>           old_files = current->files;
> >>           [...]
> >>           do {
> >>                   struct sqe_submit *s = &req->submit;
> >>                   [...]
> >>                   if (cur_files)
> >>                           /* drop cur_files reference; borrow lifetime must
> >>                            * end before here */
> >>                           put_files_struct(cur_files);
> >>                   /* move reference ownership to cur_files */
> >>                   cur_files = s->files;
> >>                   if (cur_files) {
> >>                           task_lock(current);
> >>                           /* current->files borrows reference from cur_files;
> >>                            * existing borrow from previous loop ends here */
> >>                           current->files = cur_files;
> >>                           task_unlock(current);
> >>                   }
> >>
> >>                   [call __io_submit_sqe()]
> >>                   [...]
> >>           } while (req);
> >>           [...]
> >>           /* existing borrow ends here */
> >>           task_lock(current);
> >>           current->files = old_files;
> >>           task_unlock(current);
> >>           if (cur_files)
> >>                   /* drop cur_files reference; borrow lifetime must
> >>                    * end before here */
> >>                   put_files_struct(cur_files);
> >> }
> >>
> >> If you run two iterations of this loop, with a first element that has
> >> a ->files pointer and a second element that doesn't, then in the
> >> second run through the loop, the reference to the files_struct will be
> >> dropped while current->files still points to it; current->files is
> >> only reset after the loop has ended. If someone accesses
> >> current->files through procfs directly after that, AFAICS you'd get a
> >> use-after-free.
> >
> > Amazing how this is still broken. You are right, and it's especially
> > annoying since that's exactly the case I originally talked about (not
> > flipping current->files if we don't have to). I just did it wrong, so
> > we'll leave a dangling pointer in ->files.
> >
> > The by far most common case is if one sqe has a files it needs to
> > attach, then others that also have files will be the same set. So I want
> > to optimize for the case where we only flip current->files once when we
> > see the files, and once when we're done with the loop.
> >
> > Let me see if I can get this right...
>
> I _think_ the simplest way to do it is simply to have both cur_files and
> current->files hold a reference to the file table. That won't really add
> any extra cost as the double increments / decrements are following each
> other. Something like this incremental, totally untested.
>
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 2fed0badad38..b3cf3f3d7911 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -2293,9 +2293,14 @@ static void io_sq_wq_submit_work(struct work_struct *work)
>                         put_files_struct(cur_files);
>                 cur_files = s->files;
>                 if (cur_files && cur_files != current->files) {
> +                       struct files_struct *old;
> +
> +                       atomic_inc(&cur_files->count);
>                         task_lock(current);
> +                       old = current->files;
>                         current->files = cur_files;
>                         task_unlock(current);
> +                       put_files_struct(old);
>                 }
>
>                 if (!ret) {
> @@ -2390,9 +2395,13 @@ static void io_sq_wq_submit_work(struct work_struct *work)
>                 mmput(cur_mm);
>         }
>         if (old_files != current->files) {
> +               struct files_struct *old;
> +
>                 task_lock(current);
> +               old = current->files;
>                 current->files = old_files;
>                 task_unlock(current);
> +               put_files_struct(old);
>         }
>         if (cur_files)
>                 put_files_struct(cur_files);

The only part I still feel a bit twitchy about is this part at the end:

        if (old_files != current->files) {
                struct files_struct *old;

                task_lock(current);
                old = current->files;
                current->files = old_files;
                task_unlock(current);
                put_files_struct(old);
        }

If it was possible for the initial ->files to be the same as the
->files of a submission, and we got two submissions with first a
different files_struct and then our old one, then this branch would
not be executed even though it should, which would leave the refcount
of the files_struct one too high. But that probably can't happen?
Since kernel workers should be running with &init_files (I think?) and
that thing is never used for userspace tasks. But still, I'd feel
better if you could change it like this:

diff --git a/fs/io_uring.c b/fs/io_uring.c
index f9f5c70564f0..7673035d6bfe 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2265,6 +2265,7 @@ static void io_sq_wq_submit_work(struct work_struct *work)
 {
        struct io_kiocb *req = container_of(work, struct io_kiocb, work);
        struct files_struct *cur_files = NULL, *old_files;
+       bool restore_current_files = false;
        struct io_ring_ctx *ctx = req->ctx;
        struct mm_struct *cur_mm = NULL;
        struct async_list *async_list;
@@ -2313,6 +2314,7 @@ static void io_sq_wq_submit_work(struct work_struct *work)
                        current->files = cur_files;
                        task_unlock(current);
                        put_files_struct(old);
+                       restore_current_files = true;
                }

                if (!ret) {
@@ -2406,7 +2408,7 @@ static void io_sq_wq_submit_work(struct work_struct *work)
                unuse_mm(cur_mm);
                mmput(cur_mm);
        }
-       if (old_files != current->files) {
+       if (restore_current_files) {
                struct files_struct *old;

                task_lock(current);


But actually, by the way: Is this whole files_struct thing creating a
reference loop? The files_struct has a reference to the uring file,
and the uring file has ACCEPT work that has a reference to the
files_struct. If the task gets killed and the accept work blocks, the
entire files_struct will stay alive, right?

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

[Jens Axboe]

On 10/18/19 12:06 PM, Jann Horn wrote:
> On Fri, Oct 18, 2019 at 7:05 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 10/18/19 10:36 AM, Jens Axboe wrote:
>>>> Ignoring the locking elision, basically the logic is now this:
>>>>
>>>> static void io_sq_wq_submit_work(struct work_struct *work)
>>>> {
>>>>            struct io_kiocb *req = container_of(work, struct io_kiocb, work);
>>>>            struct files_struct *cur_files = NULL, *old_files;
>>>>            [...]
>>>>            old_files = current->files;
>>>>            [...]
>>>>            do {
>>>>                    struct sqe_submit *s = &req->submit;
>>>>                    [...]
>>>>                    if (cur_files)
>>>>                            /* drop cur_files reference; borrow lifetime must
>>>>                             * end before here */
>>>>                            put_files_struct(cur_files);
>>>>                    /* move reference ownership to cur_files */
>>>>                    cur_files = s->files;
>>>>                    if (cur_files) {
>>>>                            task_lock(current);
>>>>                            /* current->files borrows reference from cur_files;
>>>>                             * existing borrow from previous loop ends here */
>>>>                            current->files = cur_files;
>>>>                            task_unlock(current);
>>>>                    }
>>>>
>>>>                    [call __io_submit_sqe()]
>>>>                    [...]
>>>>            } while (req);
>>>>            [...]
>>>>            /* existing borrow ends here */
>>>>            task_lock(current);
>>>>            current->files = old_files;
>>>>            task_unlock(current);
>>>>            if (cur_files)
>>>>                    /* drop cur_files reference; borrow lifetime must
>>>>                     * end before here */
>>>>                    put_files_struct(cur_files);
>>>> }
>>>>
>>>> If you run two iterations of this loop, with a first element that has
>>>> a ->files pointer and a second element that doesn't, then in the
>>>> second run through the loop, the reference to the files_struct will be
>>>> dropped while current->files still points to it; current->files is
>>>> only reset after the loop has ended. If someone accesses
>>>> current->files through procfs directly after that, AFAICS you'd get a
>>>> use-after-free.
>>>
>>> Amazing how this is still broken. You are right, and it's especially
>>> annoying since that's exactly the case I originally talked about (not
>>> flipping current->files if we don't have to). I just did it wrong, so
>>> we'll leave a dangling pointer in ->files.
>>>
>>> The by far most common case is if one sqe has a files it needs to
>>> attach, then others that also have files will be the same set. So I want
>>> to optimize for the case where we only flip current->files once when we
>>> see the files, and once when we're done with the loop.
>>>
>>> Let me see if I can get this right...
>>
>> I _think_ the simplest way to do it is simply to have both cur_files and
>> current->files hold a reference to the file table. That won't really add
>> any extra cost as the double increments / decrements are following each
>> other. Something like this incremental, totally untested.
>>
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 2fed0badad38..b3cf3f3d7911 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -2293,9 +2293,14 @@ static void io_sq_wq_submit_work(struct work_struct *work)
>>                          put_files_struct(cur_files);
>>                  cur_files = s->files;
>>                  if (cur_files && cur_files != current->files) {
>> +                       struct files_struct *old;
>> +
>> +                       atomic_inc(&cur_files->count);
>>                          task_lock(current);
>> +                       old = current->files;
>>                          current->files = cur_files;
>>                          task_unlock(current);
>> +                       put_files_struct(old);
>>                  }
>>
>>                  if (!ret) {
>> @@ -2390,9 +2395,13 @@ static void io_sq_wq_submit_work(struct work_struct *work)
>>                  mmput(cur_mm);
>>          }
>>          if (old_files != current->files) {
>> +               struct files_struct *old;
>> +
>>                  task_lock(current);
>> +               old = current->files;
>>                  current->files = old_files;
>>                  task_unlock(current);
>> +               put_files_struct(old);
>>          }
>>          if (cur_files)
>>                  put_files_struct(cur_files);
> 
> The only part I still feel a bit twitchy about is this part at the end:
> 
>          if (old_files != current->files) {
>                  struct files_struct *old;
> 
>                  task_lock(current);
>                  old = current->files;
>                  current->files = old_files;
>                  task_unlock(current);
>                  put_files_struct(old);
>          }
> 
> If it was possible for the initial ->files to be the same as the
> ->files of a submission, and we got two submissions with first a
> different files_struct and then our old one, then this branch would
> not be executed even though it should, which would leave the refcount
> of the files_struct one too high. But that probably can't happen?
> Since kernel workers should be running with &init_files (I think?) and
> that thing is never used for userspace tasks. But still, I'd feel
> better if you could change it like this:

Right, that is never going to happen. But your solution is simpler,
so... I'll throw some testing at it.

> But actually, by the way: Is this whole files_struct thing creating a
> reference loop? The files_struct has a reference to the uring file,
> and the uring file has ACCEPT work that has a reference to the
> files_struct. If the task gets killed and the accept work blocks, the
> entire files_struct will stay alive, right?

Yes, for the lifetime of the request, it does create a loop. So if the
application goes away, I think you're right, the files_struct will stay.
And so will the io_uring, for that matter, as we depend on the closing
of the files to do the final reap.

Hmm, not sure how best to handle that, to be honest. We need some way to
break the loop, if the request never finishes.

-- 
Jens Axboe

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

[Jann Horn]

On Fri, Oct 18, 2019 at 8:16 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 10/18/19 12:06 PM, Jann Horn wrote:
> > But actually, by the way: Is this whole files_struct thing creating a
> > reference loop? The files_struct has a reference to the uring file,
> > and the uring file has ACCEPT work that has a reference to the
> > files_struct. If the task gets killed and the accept work blocks, the
> > entire files_struct will stay alive, right?
>
> Yes, for the lifetime of the request, it does create a loop. So if the
> application goes away, I think you're right, the files_struct will stay.
> And so will the io_uring, for that matter, as we depend on the closing
> of the files to do the final reap.
>
> Hmm, not sure how best to handle that, to be honest. We need some way to
> break the loop, if the request never finishes.

A wacky and dubious approach would be to, instead of taking a
reference to the files_struct, abuse f_op->flush() to synchronously
flush out pending requests with references to the files_struct... But
it's probably a bad idea, given that in f_op->flush(), you can't
easily tell which files_struct the close is coming from. I suppose you
could keep a list of (fdtable, fd) pairs through which ACCEPT requests
have come in and then let f_op->flush() probe whether the file
pointers are gone from them...

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

[Jens Axboe]

On 10/18/19 12:50 PM, Jann Horn wrote:
> On Fri, Oct 18, 2019 at 8:16 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 10/18/19 12:06 PM, Jann Horn wrote:
>>> But actually, by the way: Is this whole files_struct thing creating a
>>> reference loop? The files_struct has a reference to the uring file,
>>> and the uring file has ACCEPT work that has a reference to the
>>> files_struct. If the task gets killed and the accept work blocks, the
>>> entire files_struct will stay alive, right?
>>
>> Yes, for the lifetime of the request, it does create a loop. So if the
>> application goes away, I think you're right, the files_struct will stay.
>> And so will the io_uring, for that matter, as we depend on the closing
>> of the files to do the final reap.
>>
>> Hmm, not sure how best to handle that, to be honest. We need some way to
>> break the loop, if the request never finishes.
> 
> A wacky and dubious approach would be to, instead of taking a
> reference to the files_struct, abuse f_op->flush() to synchronously
> flush out pending requests with references to the files_struct... But
> it's probably a bad idea, given that in f_op->flush(), you can't
> easily tell which files_struct the close is coming from. I suppose you
> could keep a list of (fdtable, fd) pairs through which ACCEPT requests
> have come in and then let f_op->flush() probe whether the file
> pointers are gone from them...

Got back to this after finishing the io-wq stuff, which we need for the
cancel.

Here's an updated patch:

http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=1ea847edc58d6a54ca53001ad0c656da57257570

that seems to work for me (lightly tested), we correctly find and cancel
work that is holding on to the file table.

The full series sits on top of my for-5.5/io_uring-wq branch, and can be
viewed here:

http://git.kernel.dk/cgit/linux-block/log/?h=for-5.5/io_uring-test

Let me know what you think!

-- 
Jens Axboe

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

[Jann Horn]

On Thu, Oct 24, 2019 at 9:41 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 10/18/19 12:50 PM, Jann Horn wrote:
> > On Fri, Oct 18, 2019 at 8:16 PM Jens Axboe <axboe@kernel.dk> wrote:
> >> On 10/18/19 12:06 PM, Jann Horn wrote:
> >>> But actually, by the way: Is this whole files_struct thing creating a
> >>> reference loop? The files_struct has a reference to the uring file,
> >>> and the uring file has ACCEPT work that has a reference to the
> >>> files_struct. If the task gets killed and the accept work blocks, the
> >>> entire files_struct will stay alive, right?
> >>
> >> Yes, for the lifetime of the request, it does create a loop. So if the
> >> application goes away, I think you're right, the files_struct will stay.
> >> And so will the io_uring, for that matter, as we depend on the closing
> >> of the files to do the final reap.
> >>
> >> Hmm, not sure how best to handle that, to be honest. We need some way to
> >> break the loop, if the request never finishes.
> >
> > A wacky and dubious approach would be to, instead of taking a
> > reference to the files_struct, abuse f_op->flush() to synchronously
> > flush out pending requests with references to the files_struct... But
> > it's probably a bad idea, given that in f_op->flush(), you can't
> > easily tell which files_struct the close is coming from. I suppose you
> > could keep a list of (fdtable, fd) pairs through which ACCEPT requests
> > have come in and then let f_op->flush() probe whether the file
> > pointers are gone from them...
>
> Got back to this after finishing the io-wq stuff, which we need for the
> cancel.
>
> Here's an updated patch:
>
> http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=1ea847edc58d6a54ca53001ad0c656da57257570
>
> that seems to work for me (lightly tested), we correctly find and cancel
> work that is holding on to the file table.
>
> The full series sits on top of my for-5.5/io_uring-wq branch, and can be
> viewed here:
>
> http://git.kernel.dk/cgit/linux-block/log/?h=for-5.5/io_uring-test
>
> Let me know what you think!

Ah, I didn't realize that the second argument to f_op->flush is a
pointer to the files_struct. That's neat.


Security: There is no guarantee that ->flush() will run after the last
io_uring_enter() finishes. You can race like this, with threads A and
B in one process and C in another one:

A: sends uring fd to C via unix domain socket
A: starts syscall io_uring_enter(fd, ...)
A: calls fdget(fd), takes reference to file
B: starts syscall close(fd)
B: fd table entry is removed
B: f_op->flush is invoked and finds no pending transactions
B: syscall close() returns
A: continues io_uring_enter(), grabbing current->files
A: io_uring_enter() returns
A and B: exit
worker: use-after-free access to files_struct

I think the solution to this would be (unless you're fine with adding
some broad global read-write mutex) something like this in
__io_queue_sqe(), where "fd" and "f" are the variables from
io_uring_enter(), plumbed through the stack somehow:

if (req->flags & REQ_F_NEED_FILES) {
  rcu_read_lock();
  spin_lock_irq(&ctx->inflight_lock);
  if (fcheck(fd) == f) {
    list_add(&req->inflight_list,
      &ctx->inflight_list);
    req->work.files = current->files;
    ret = 0;
  } else {
    ret = -EBADF;
  }
  spin_unlock_irq(&ctx->inflight_lock);
  rcu_read_unlock();
  if (ret)
    goto put_req;
}


Minor note: If a process uses dup() to duplicate the uring fd, then
closes the duplicated fd, that will cause work cancellations - but I
guess that's fine?


Style nit: I find it a bit confusing to name both the list head and
the list member heads "inflight_list". Maybe name them "inflight_list"
and "inflight_entry", or something like that?


Correctness: Why is the wait in io_uring_flush() TASK_INTERRUPTIBLE?
Shouldn't it be TASK_UNINTERRUPTIBLE? If someone sends a signal to the
task while it's at that schedule(), it's just going to loop back
around and retry what it was doing already, right?


Security + Correctness: If there is more than one io_wqe, it seems to
me that io_uring_flush() calls io_wq_cancel_work(), which calls
io_wqe_cancel_work(), which may return IO_WQ_CANCEL_OK if the first
request it looks at is pending. In that case, io_wq_cancel_work() will
immediately return, and io_uring_flush() will also immediately return.
It looks like any other requests will continue running?

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

[Jens Axboe]

On 10/24/19 2:31 PM, Jann Horn wrote:
> On Thu, Oct 24, 2019 at 9:41 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 10/18/19 12:50 PM, Jann Horn wrote:
>>> On Fri, Oct 18, 2019 at 8:16 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>> On 10/18/19 12:06 PM, Jann Horn wrote:
>>>>> But actually, by the way: Is this whole files_struct thing creating a
>>>>> reference loop? The files_struct has a reference to the uring file,
>>>>> and the uring file has ACCEPT work that has a reference to the
>>>>> files_struct. If the task gets killed and the accept work blocks, the
>>>>> entire files_struct will stay alive, right?
>>>>
>>>> Yes, for the lifetime of the request, it does create a loop. So if the
>>>> application goes away, I think you're right, the files_struct will stay.
>>>> And so will the io_uring, for that matter, as we depend on the closing
>>>> of the files to do the final reap.
>>>>
>>>> Hmm, not sure how best to handle that, to be honest. We need some way to
>>>> break the loop, if the request never finishes.
>>>
>>> A wacky and dubious approach would be to, instead of taking a
>>> reference to the files_struct, abuse f_op->flush() to synchronously
>>> flush out pending requests with references to the files_struct... But
>>> it's probably a bad idea, given that in f_op->flush(), you can't
>>> easily tell which files_struct the close is coming from. I suppose you
>>> could keep a list of (fdtable, fd) pairs through which ACCEPT requests
>>> have come in and then let f_op->flush() probe whether the file
>>> pointers are gone from them...
>>
>> Got back to this after finishing the io-wq stuff, which we need for the
>> cancel.
>>
>> Here's an updated patch:
>>
>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=1ea847edc58d6a54ca53001ad0c656da57257570
>>
>> that seems to work for me (lightly tested), we correctly find and cancel
>> work that is holding on to the file table.
>>
>> The full series sits on top of my for-5.5/io_uring-wq branch, and can be
>> viewed here:
>>
>> http://git.kernel.dk/cgit/linux-block/log/?h=for-5.5/io_uring-test
>>
>> Let me know what you think!
> 
> Ah, I didn't realize that the second argument to f_op->flush is a
> pointer to the files_struct. That's neat.
> 
> 
> Security: There is no guarantee that ->flush() will run after the last
> io_uring_enter() finishes. You can race like this, with threads A and
> B in one process and C in another one:
> 
> A: sends uring fd to C via unix domain socket
> A: starts syscall io_uring_enter(fd, ...)
> A: calls fdget(fd), takes reference to file
> B: starts syscall close(fd)
> B: fd table entry is removed
> B: f_op->flush is invoked and finds no pending transactions
> B: syscall close() returns
> A: continues io_uring_enter(), grabbing current->files
> A: io_uring_enter() returns
> A and B: exit
> worker: use-after-free access to files_struct
> 
> I think the solution to this would be (unless you're fine with adding
> some broad global read-write mutex) something like this in
> __io_queue_sqe(), where "fd" and "f" are the variables from
> io_uring_enter(), plumbed through the stack somehow:
> 
> if (req->flags & REQ_F_NEED_FILES) {
>    rcu_read_lock();
>    spin_lock_irq(&ctx->inflight_lock);
>    if (fcheck(fd) == f) {
>      list_add(&req->inflight_list,
>        &ctx->inflight_list);
>      req->work.files = current->files;
>      ret = 0;
>    } else {
>      ret = -EBADF;
>    }
>    spin_unlock_irq(&ctx->inflight_lock);
>    rcu_read_unlock();
>    if (ret)
>      goto put_req;
> }

First of all, thanks for the thorough look at this! We already have f
available here, it's req->file. And we just made a copy of the sqe, so
we have sqe->fd available as well. I fixed this up.

> Minor note: If a process uses dup() to duplicate the uring fd, then
> closes the duplicated fd, that will cause work cancellations - but I
> guess that's fine?

I don't think that's a concern.

> Style nit: I find it a bit confusing to name both the list head and
> the list member heads "inflight_list". Maybe name them "inflight_list"
> and "inflight_entry", or something like that?

Fixed

> Correctness: Why is the wait in io_uring_flush() TASK_INTERRUPTIBLE?
> Shouldn't it be TASK_UNINTERRUPTIBLE? If someone sends a signal to the
> task while it's at that schedule(), it's just going to loop back
> around and retry what it was doing already, right?

Fixed

> Security + Correctness: If there is more than one io_wqe, it seems to
> me that io_uring_flush() calls io_wq_cancel_work(), which calls
> io_wqe_cancel_work(), which may return IO_WQ_CANCEL_OK if the first
> request it looks at is pending. In that case, io_wq_cancel_work() will
> immediately return, and io_uring_flush() will also immediately return.
> It looks like any other requests will continue running?

Ah good point, I missed that. We need to keep looping until we get
NOTFOUND returned. Fixed as well.

Also added cancellation if the task is going away. Here's the
incremental patch, I'll resend with the full version.


diff --git a/fs/io_uring.c b/fs/io_uring.c
index 43ae0e04fd09..ec9dadfa90d2 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -326,7 +326,7 @@ struct io_kiocb {
 	u32			result;
 	u32			sequence;
 
-	struct list_head	inflight_list;
+	struct list_head	inflight_entry;
 
 	struct io_wq_work	work;
 };
@@ -688,7 +688,7 @@ static void __io_free_req(struct io_kiocb *req)
 		unsigned long flags;
 
 		spin_lock_irqsave(&ctx->inflight_lock, flags);
-		list_del(&req->inflight_list);
+		list_del(&req->inflight_entry);
 		if (wq_has_sleeper(&ctx->inflight_wait))
 			wake_up(&ctx->inflight_wait);
 		spin_unlock_irqrestore(&ctx->inflight_lock, flags);
@@ -2329,6 +2329,24 @@ static int io_req_set_file(struct io_ring_ctx *ctx, const struct sqe_submit *s,
 	return 0;
 }
 
+static int io_grab_files(struct io_ring_ctx *ctx, struct io_kiocb *req,
+			 struct io_uring_sqe *sqe)
+{
+	int ret = -EBADF;
+
+	rcu_read_lock();
+	spin_lock_irq(&ctx->inflight_lock);
+	if (fcheck(sqe->fd) == req->file) {
+		list_add(&req->inflight_entry, &ctx->inflight_list);
+		req->work.files = current->files;
+		ret = 0;
+	}
+	spin_unlock_irq(&ctx->inflight_lock);
+	rcu_read_unlock();
+
+	return ret;
+}
+
 static int __io_queue_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
 			struct sqe_submit *s)
 {
@@ -2349,23 +2367,24 @@ static int __io_queue_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
 			s->sqe = sqe_copy;
 			memcpy(&req->submit, s, sizeof(*s));
 			if (req->flags & REQ_F_NEED_FILES) {
-				spin_lock_irq(&ctx->inflight_lock);
-				list_add(&req->inflight_list,
-						&ctx->inflight_list);
-				req->work.files = current->files;
-				spin_unlock_irq(&ctx->inflight_lock);
+				ret = io_grab_files(ctx, req, sqe_copy);
+				if (ret) {
+					kfree(sqe_copy);
+					goto err;
+				}
 			}
-			io_queue_async_work(ctx, req);
 
 			/*
 			 * Queued up for async execution, worker will release
 			 * submit reference when the iocb is actually submitted.
 			 */
+			io_queue_async_work(ctx, req);
 			return 0;
 		}
 	}
 
 	/* drop submission reference */
+err:
 	io_put_req(req, NULL);
 
 	/* and drop final reference, if we failed */
@@ -3768,38 +3787,51 @@ static int io_uring_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
-static int io_uring_flush(struct file *file, void *data)
+static void io_uring_cancel_files(struct io_ring_ctx *ctx,
+				  struct files_struct *files)
 {
-	struct io_ring_ctx *ctx = file->private_data;
-	enum io_wq_cancel ret;
 	struct io_kiocb *req;
 	DEFINE_WAIT(wait);
 
-restart:
-	ret = IO_WQ_CANCEL_NOTFOUND;
+	while (!list_empty_careful(&ctx->inflight_list)) {
+		enum io_wq_cancel ret = IO_WQ_CANCEL_NOTFOUND;
 
-	spin_lock_irq(&ctx->inflight_lock);
-	list_for_each_entry(req, &ctx->inflight_list, inflight_list) {
-		if (req->work.files == data) {
-			ret = io_wq_cancel_work(ctx->io_wq, &req->work);
-			break;
+		spin_lock_irq(&ctx->inflight_lock);
+		list_for_each_entry(req, &ctx->inflight_list, inflight_entry) {
+			if (req->work.files == files) {
+				ret = io_wq_cancel_work(ctx->io_wq, &req->work);
+				break;
+			}
 		}
-	}
-	if (ret == IO_WQ_CANCEL_RUNNING)
-		prepare_to_wait(&ctx->inflight_wait, &wait, TASK_INTERRUPTIBLE);
+		if (ret == IO_WQ_CANCEL_RUNNING)
+			prepare_to_wait(&ctx->inflight_wait, &wait,
+					TASK_UNINTERRUPTIBLE);
 
-	spin_unlock_irq(&ctx->inflight_lock);
+		spin_unlock_irq(&ctx->inflight_lock);
 
-	/*
-	 * If it was found running, wait for one inflight request to finish.
-	 * Retry loop after that, as it may not have been the one we were
-	 * looking for.
-	 */
-	if (ret == IO_WQ_CANCEL_RUNNING) {
+		/*
+		 * We need to keep going until we get NOTFOUND. We only cancel
+		 * one work at the time.
+		 *
+		 * If we get CANCEL_RUNNING, then wait for a work to complete
+		 * before continuing.
+		 */
+		if (ret == IO_WQ_CANCEL_OK)
+			continue;
+		else if (ret != IO_WQ_CANCEL_RUNNING)
+			break;
 		schedule();
-		goto restart;
-	}
+	};
+}
 
+static int io_uring_flush(struct file *file, void *data)
+{
+	struct io_ring_ctx *ctx = file->private_data;
+
+	if (fatal_signal_pending(current) || (current->flags & PF_EXITING))
+		io_wq_cancel_all(ctx->io_wq);
+	else
+		io_uring_cancel_files(ctx, data);
 	return 0;
 }
 

-- 
Jens Axboe

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

[Jens Axboe]

On 10/24/19 4:04 PM, Jens Axboe wrote:
> Also added cancellation if the task is going away. Here's the
> incremental patch, I'll resend with the full version.

Full version:

http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=24f099c6dd49fdedec464c5a7c0dfe2ac5f500d0

-- 
Jens Axboe

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

[Jann Horn]

On Fri, Oct 25, 2019 at 12:04 AM Jens Axboe <axboe@kernel.dk> wrote:
> On 10/24/19 2:31 PM, Jann Horn wrote:
> > On Thu, Oct 24, 2019 at 9:41 PM Jens Axboe <axboe@kernel.dk> wrote:
> >> On 10/18/19 12:50 PM, Jann Horn wrote:
> >>> On Fri, Oct 18, 2019 at 8:16 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>> On 10/18/19 12:06 PM, Jann Horn wrote:
> >>>>> But actually, by the way: Is this whole files_struct thing creating a
> >>>>> reference loop? The files_struct has a reference to the uring file,
> >>>>> and the uring file has ACCEPT work that has a reference to the
> >>>>> files_struct. If the task gets killed and the accept work blocks, the
> >>>>> entire files_struct will stay alive, right?
> >>>>
> >>>> Yes, for the lifetime of the request, it does create a loop. So if the
> >>>> application goes away, I think you're right, the files_struct will stay.
> >>>> And so will the io_uring, for that matter, as we depend on the closing
> >>>> of the files to do the final reap.
> >>>>
> >>>> Hmm, not sure how best to handle that, to be honest. We need some way to
> >>>> break the loop, if the request never finishes.
> >>>
> >>> A wacky and dubious approach would be to, instead of taking a
> >>> reference to the files_struct, abuse f_op->flush() to synchronously
> >>> flush out pending requests with references to the files_struct... But
> >>> it's probably a bad idea, given that in f_op->flush(), you can't
> >>> easily tell which files_struct the close is coming from. I suppose you
> >>> could keep a list of (fdtable, fd) pairs through which ACCEPT requests
> >>> have come in and then let f_op->flush() probe whether the file
> >>> pointers are gone from them...
> >>
> >> Got back to this after finishing the io-wq stuff, which we need for the
> >> cancel.
> >>
> >> Here's an updated patch:
> >>
> >> http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=1ea847edc58d6a54ca53001ad0c656da57257570
> >>
> >> that seems to work for me (lightly tested), we correctly find and cancel
> >> work that is holding on to the file table.
> >>
> >> The full series sits on top of my for-5.5/io_uring-wq branch, and can be
> >> viewed here:
> >>
> >> http://git.kernel.dk/cgit/linux-block/log/?h=for-5.5/io_uring-test
> >>
> >> Let me know what you think!
> >
> > Ah, I didn't realize that the second argument to f_op->flush is a
> > pointer to the files_struct. That's neat.
> >
> >
> > Security: There is no guarantee that ->flush() will run after the last
> > io_uring_enter() finishes. You can race like this, with threads A and
> > B in one process and C in another one:
> >
> > A: sends uring fd to C via unix domain socket
> > A: starts syscall io_uring_enter(fd, ...)
> > A: calls fdget(fd), takes reference to file
> > B: starts syscall close(fd)
> > B: fd table entry is removed
> > B: f_op->flush is invoked and finds no pending transactions
> > B: syscall close() returns
> > A: continues io_uring_enter(), grabbing current->files
> > A: io_uring_enter() returns
> > A and B: exit
> > worker: use-after-free access to files_struct
> >
> > I think the solution to this would be (unless you're fine with adding
> > some broad global read-write mutex) something like this in
> > __io_queue_sqe(), where "fd" and "f" are the variables from
> > io_uring_enter(), plumbed through the stack somehow:
> >
> > if (req->flags & REQ_F_NEED_FILES) {
> >    rcu_read_lock();
> >    spin_lock_irq(&ctx->inflight_lock);
> >    if (fcheck(fd) == f) {
> >      list_add(&req->inflight_list,
> >        &ctx->inflight_list);
> >      req->work.files = current->files;
> >      ret = 0;
> >    } else {
> >      ret = -EBADF;
> >    }
> >    spin_unlock_irq(&ctx->inflight_lock);
> >    rcu_read_unlock();
> >    if (ret)
> >      goto put_req;
> > }
>
> First of all, thanks for the thorough look at this! We already have f
> available here, it's req->file. And we just made a copy of the sqe, so
> we have sqe->fd available as well. I fixed this up.

sqe->fd is the file descriptor we're doing I/O on, not the file
descriptor of the uring file, right? Same thing for req->file. This
check only detects whether the fd we're doing I/O on was closed, which
is irrelevant.

> > Security + Correctness: If there is more than one io_wqe, it seems to
> > me that io_uring_flush() calls io_wq_cancel_work(), which calls
> > io_wqe_cancel_work(), which may return IO_WQ_CANCEL_OK if the first
> > request it looks at is pending. In that case, io_wq_cancel_work() will
> > immediately return, and io_uring_flush() will also immediately return.
> > It looks like any other requests will continue running?
>
> Ah good point, I missed that. We need to keep looping until we get
> NOTFOUND returned. Fixed as well.
>
> Also added cancellation if the task is going away. Here's the
> incremental patch, I'll resend with the full version.
[...]
> +static int io_uring_flush(struct file *file, void *data)
> +{
> +       struct io_ring_ctx *ctx = file->private_data;
> +
> +       if (fatal_signal_pending(current) || (current->flags & PF_EXITING))
> +               io_wq_cancel_all(ctx->io_wq);

Looking at io_wq_cancel_all(), this will just send a signal to the
task without waiting for anything, right? Isn't that unsafe?


> +       else
> +               io_uring_cancel_files(ctx, data);
>         return 0;
>  }

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

[Jens Axboe]

On 10/24/19 5:13 PM, Jann Horn wrote:
> On Fri, Oct 25, 2019 at 12:04 AM Jens Axboe <axboe@kernel.dk> wrote:
>> On 10/24/19 2:31 PM, Jann Horn wrote:
>>> On Thu, Oct 24, 2019 at 9:41 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>> On 10/18/19 12:50 PM, Jann Horn wrote:
>>>>> On Fri, Oct 18, 2019 at 8:16 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>> On 10/18/19 12:06 PM, Jann Horn wrote:
>>>>>>> But actually, by the way: Is this whole files_struct thing creating a
>>>>>>> reference loop? The files_struct has a reference to the uring file,
>>>>>>> and the uring file has ACCEPT work that has a reference to the
>>>>>>> files_struct. If the task gets killed and the accept work blocks, the
>>>>>>> entire files_struct will stay alive, right?
>>>>>>
>>>>>> Yes, for the lifetime of the request, it does create a loop. So if the
>>>>>> application goes away, I think you're right, the files_struct will stay.
>>>>>> And so will the io_uring, for that matter, as we depend on the closing
>>>>>> of the files to do the final reap.
>>>>>>
>>>>>> Hmm, not sure how best to handle that, to be honest. We need some way to
>>>>>> break the loop, if the request never finishes.
>>>>>
>>>>> A wacky and dubious approach would be to, instead of taking a
>>>>> reference to the files_struct, abuse f_op->flush() to synchronously
>>>>> flush out pending requests with references to the files_struct... But
>>>>> it's probably a bad idea, given that in f_op->flush(), you can't
>>>>> easily tell which files_struct the close is coming from. I suppose you
>>>>> could keep a list of (fdtable, fd) pairs through which ACCEPT requests
>>>>> have come in and then let f_op->flush() probe whether the file
>>>>> pointers are gone from them...
>>>>
>>>> Got back to this after finishing the io-wq stuff, which we need for the
>>>> cancel.
>>>>
>>>> Here's an updated patch:
>>>>
>>>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=1ea847edc58d6a54ca53001ad0c656da57257570
>>>>
>>>> that seems to work for me (lightly tested), we correctly find and cancel
>>>> work that is holding on to the file table.
>>>>
>>>> The full series sits on top of my for-5.5/io_uring-wq branch, and can be
>>>> viewed here:
>>>>
>>>> http://git.kernel.dk/cgit/linux-block/log/?h=for-5.5/io_uring-test
>>>>
>>>> Let me know what you think!
>>>
>>> Ah, I didn't realize that the second argument to f_op->flush is a
>>> pointer to the files_struct. That's neat.
>>>
>>>
>>> Security: There is no guarantee that ->flush() will run after the last
>>> io_uring_enter() finishes. You can race like this, with threads A and
>>> B in one process and C in another one:
>>>
>>> A: sends uring fd to C via unix domain socket
>>> A: starts syscall io_uring_enter(fd, ...)
>>> A: calls fdget(fd), takes reference to file
>>> B: starts syscall close(fd)
>>> B: fd table entry is removed
>>> B: f_op->flush is invoked and finds no pending transactions
>>> B: syscall close() returns
>>> A: continues io_uring_enter(), grabbing current->files
>>> A: io_uring_enter() returns
>>> A and B: exit
>>> worker: use-after-free access to files_struct
>>>
>>> I think the solution to this would be (unless you're fine with adding
>>> some broad global read-write mutex) something like this in
>>> __io_queue_sqe(), where "fd" and "f" are the variables from
>>> io_uring_enter(), plumbed through the stack somehow:
>>>
>>> if (req->flags & REQ_F_NEED_FILES) {
>>>     rcu_read_lock();
>>>     spin_lock_irq(&ctx->inflight_lock);
>>>     if (fcheck(fd) == f) {
>>>       list_add(&req->inflight_list,
>>>         &ctx->inflight_list);
>>>       req->work.files = current->files;
>>>       ret = 0;
>>>     } else {
>>>       ret = -EBADF;
>>>     }
>>>     spin_unlock_irq(&ctx->inflight_lock);
>>>     rcu_read_unlock();
>>>     if (ret)
>>>       goto put_req;
>>> }
>>
>> First of all, thanks for the thorough look at this! We already have f
>> available here, it's req->file. And we just made a copy of the sqe, so
>> we have sqe->fd available as well. I fixed this up.
> 
> sqe->fd is the file descriptor we're doing I/O on, not the file
> descriptor of the uring file, right? Same thing for req->file. This
> check only detects whether the fd we're doing I/O on was closed, which
> is irrelevant.

Duh yes, I'm an idiot. Easily fixable, I'll update this for the ring fd.

>>> Security + Correctness: If there is more than one io_wqe, it seems to
>>> me that io_uring_flush() calls io_wq_cancel_work(), which calls
>>> io_wqe_cancel_work(), which may return IO_WQ_CANCEL_OK if the first
>>> request it looks at is pending. In that case, io_wq_cancel_work() will
>>> immediately return, and io_uring_flush() will also immediately return.
>>> It looks like any other requests will continue running?
>>
>> Ah good point, I missed that. We need to keep looping until we get
>> NOTFOUND returned. Fixed as well.
>>
>> Also added cancellation if the task is going away. Here's the
>> incremental patch, I'll resend with the full version.
> [...]
>> +static int io_uring_flush(struct file *file, void *data)
>> +{
>> +       struct io_ring_ctx *ctx = file->private_data;
>> +
>> +       if (fatal_signal_pending(current) || (current->flags & PF_EXITING))
>> +               io_wq_cancel_all(ctx->io_wq);
> 
> Looking at io_wq_cancel_all(), this will just send a signal to the
> task without waiting for anything, right? Isn't that unsafe?

Yes, that's a logic error, we should always do the
io_uring_cancel_files(). Ala:

	io_uring_cancel_files();
	if (fatal_signal_pending(current) || (current->flags & PF_EXITING))
		io_wq_cancel_all(ctx->io_wq);

Thanks!

-- 
Jens Axboe

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

[Jens Axboe]

On 10/24/19 6:35 PM, Jens Axboe wrote:
> On 10/24/19 5:13 PM, Jann Horn wrote:
>> On Fri, Oct 25, 2019 at 12:04 AM Jens Axboe <axboe@kernel.dk> wrote:
>>> On 10/24/19 2:31 PM, Jann Horn wrote:
>>>> On Thu, Oct 24, 2019 at 9:41 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>> On 10/18/19 12:50 PM, Jann Horn wrote:
>>>>>> On Fri, Oct 18, 2019 at 8:16 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>>>> On 10/18/19 12:06 PM, Jann Horn wrote:
>>>>>>>> But actually, by the way: Is this whole files_struct thing creating a
>>>>>>>> reference loop? The files_struct has a reference to the uring file,
>>>>>>>> and the uring file has ACCEPT work that has a reference to the
>>>>>>>> files_struct. If the task gets killed and the accept work blocks, the
>>>>>>>> entire files_struct will stay alive, right?
>>>>>>>
>>>>>>> Yes, for the lifetime of the request, it does create a loop. So if the
>>>>>>> application goes away, I think you're right, the files_struct will stay.
>>>>>>> And so will the io_uring, for that matter, as we depend on the closing
>>>>>>> of the files to do the final reap.
>>>>>>>
>>>>>>> Hmm, not sure how best to handle that, to be honest. We need some way to
>>>>>>> break the loop, if the request never finishes.
>>>>>>
>>>>>> A wacky and dubious approach would be to, instead of taking a
>>>>>> reference to the files_struct, abuse f_op->flush() to synchronously
>>>>>> flush out pending requests with references to the files_struct... But
>>>>>> it's probably a bad idea, given that in f_op->flush(), you can't
>>>>>> easily tell which files_struct the close is coming from. I suppose you
>>>>>> could keep a list of (fdtable, fd) pairs through which ACCEPT requests
>>>>>> have come in and then let f_op->flush() probe whether the file
>>>>>> pointers are gone from them...
>>>>>
>>>>> Got back to this after finishing the io-wq stuff, which we need for the
>>>>> cancel.
>>>>>
>>>>> Here's an updated patch:
>>>>>
>>>>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-5.5/io_uring-test&id=1ea847edc58d6a54ca53001ad0c656da57257570
>>>>>
>>>>> that seems to work for me (lightly tested), we correctly find and cancel
>>>>> work that is holding on to the file table.
>>>>>
>>>>> The full series sits on top of my for-5.5/io_uring-wq branch, and can be
>>>>> viewed here:
>>>>>
>>>>> http://git.kernel.dk/cgit/linux-block/log/?h=for-5.5/io_uring-test
>>>>>
>>>>> Let me know what you think!
>>>>
>>>> Ah, I didn't realize that the second argument to f_op->flush is a
>>>> pointer to the files_struct. That's neat.
>>>>
>>>>
>>>> Security: There is no guarantee that ->flush() will run after the last
>>>> io_uring_enter() finishes. You can race like this, with threads A and
>>>> B in one process and C in another one:
>>>>
>>>> A: sends uring fd to C via unix domain socket
>>>> A: starts syscall io_uring_enter(fd, ...)
>>>> A: calls fdget(fd), takes reference to file
>>>> B: starts syscall close(fd)
>>>> B: fd table entry is removed
>>>> B: f_op->flush is invoked and finds no pending transactions
>>>> B: syscall close() returns
>>>> A: continues io_uring_enter(), grabbing current->files
>>>> A: io_uring_enter() returns
>>>> A and B: exit
>>>> worker: use-after-free access to files_struct
>>>>
>>>> I think the solution to this would be (unless you're fine with adding
>>>> some broad global read-write mutex) something like this in
>>>> __io_queue_sqe(), where "fd" and "f" are the variables from
>>>> io_uring_enter(), plumbed through the stack somehow:
>>>>
>>>> if (req->flags & REQ_F_NEED_FILES) {
>>>>      rcu_read_lock();
>>>>      spin_lock_irq(&ctx->inflight_lock);
>>>>      if (fcheck(fd) == f) {
>>>>        list_add(&req->inflight_list,
>>>>          &ctx->inflight_list);
>>>>        req->work.files = current->files;
>>>>        ret = 0;
>>>>      } else {
>>>>        ret = -EBADF;
>>>>      }
>>>>      spin_unlock_irq(&ctx->inflight_lock);
>>>>      rcu_read_unlock();
>>>>      if (ret)
>>>>        goto put_req;
>>>> }
>>>
>>> First of all, thanks for the thorough look at this! We already have f
>>> available here, it's req->file. And we just made a copy of the sqe, so
>>> we have sqe->fd available as well. I fixed this up.
>>
>> sqe->fd is the file descriptor we're doing I/O on, not the file
>> descriptor of the uring file, right? Same thing for req->file. This
>> check only detects whether the fd we're doing I/O on was closed, which
>> is irrelevant.
> 
> Duh yes, I'm an idiot. Easily fixable, I'll update this for the ring fd.

Incremental:

diff --git a/fs/io_uring.c b/fs/io_uring.c
index ec9dadfa90d2..4d94886a3d13 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -262,11 +262,13 @@ struct io_ring_ctx {
 
 struct sqe_submit {
 	const struct io_uring_sqe	*sqe;
+	struct file			*ring_file;
 	unsigned short			index;
 	bool				has_user : 1;
 	bool				in_async : 1;
 	bool				needs_fixed_file : 1;
 	u32				sequence;
+	int				ring_fd;
 };
 
 /*
@@ -2329,14 +2331,13 @@ static int io_req_set_file(struct io_ring_ctx *ctx, const struct sqe_submit *s,
 	return 0;
 }
 
-static int io_grab_files(struct io_ring_ctx *ctx, struct io_kiocb *req,
-			 struct io_uring_sqe *sqe)
+static int io_grab_files(struct io_ring_ctx *ctx, struct io_kiocb *req)
 {
 	int ret = -EBADF;
 
 	rcu_read_lock();
 	spin_lock_irq(&ctx->inflight_lock);
-	if (fcheck(sqe->fd) == req->file) {
+	if (fcheck(req->submit.ring_fd) == req->submit.ring_file) {
 		list_add(&req->inflight_entry, &ctx->inflight_list);
 		req->work.files = current->files;
 		ret = 0;
@@ -2367,7 +2368,7 @@ static int __io_queue_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
 			s->sqe = sqe_copy;
 			memcpy(&req->submit, s, sizeof(*s));
 			if (req->flags & REQ_F_NEED_FILES) {
-				ret = io_grab_files(ctx, req, sqe_copy);
+				ret = io_grab_files(ctx, req);
 				if (ret) {
 					kfree(sqe_copy);
 					goto err;
@@ -2585,6 +2586,7 @@ static bool io_get_sqring(struct io_ring_ctx *ctx, struct sqe_submit *s)
 
 	head = READ_ONCE(sq_array[head & ctx->sq_mask]);
 	if (head < ctx->sq_entries) {
+		s->ring_file = NULL;
 		s->index = head;
 		s->sqe = &ctx->sq_sqes[head];
 		s->sequence = ctx->cached_sq_head;
@@ -2782,7 +2784,8 @@ static int io_sq_thread(void *data)
 	return 0;
 }
 
-static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit)
+static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit,
+			  struct file *ring_file, int ring_fd)
 {
 	struct io_submit_state state, *statep = NULL;
 	struct io_kiocb *link = NULL;
@@ -2824,9 +2827,11 @@ static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit)
 		}
 
 out:
+		s.ring_file = ring_file;
 		s.has_user = true;
 		s.in_async = false;
 		s.needs_fixed_file = false;
+		s.ring_fd = ring_fd;
 		submit++;
 		trace_io_uring_submit_sqe(ctx, true, false);
 		io_submit_sqe(ctx, &s, statep, &link);
@@ -3828,10 +3833,9 @@ static int io_uring_flush(struct file *file, void *data)
 {
 	struct io_ring_ctx *ctx = file->private_data;
 
+	io_uring_cancel_files(ctx, data);
 	if (fatal_signal_pending(current) || (current->flags & PF_EXITING))
 		io_wq_cancel_all(ctx->io_wq);
-	else
-		io_uring_cancel_files(ctx, data);
 	return 0;
 }
 
@@ -3903,7 +3907,7 @@ SYSCALL_DEFINE6(io_uring_enter, unsigned int, fd, u32, to_submit,
 		to_submit = min(to_submit, ctx->sq_entries);
 
 		mutex_lock(&ctx->uring_lock);
-		submitted = io_ring_submit(ctx, to_submit);
+		submitted = io_ring_submit(ctx, to_submit, f.file, fd);
 		mutex_unlock(&ctx->uring_lock);
 	}
 	if (flags & IORING_ENTER_GETEVENTS) {

-- 
Jens Axboe

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

[Wolfgang Bumiller]

On Thu, Oct 17, 2019 at 03:28:56PM -0600, Jens Axboe wrote:
> This is in preparation for adding opcodes that need to modify files
> in a process file table, either adding new ones or closing old ones.
> 
> If an opcode needs this, it must set REQ_F_NEED_FILES in the request
> structure. If work that needs to get punted to async context have this
> set, they will grab a reference to the process file table. When the
> work is completed, the reference is dropped again.

I think IORING_OP_SENDMSG and _RECVMSG need to set this flag due to
SCM_RIGHTS control messages.
Thought I'd reply here since I just now ran into the issue that I was
getting ever-increasing wrong file descriptor numbers on pretty much
ever "other" async recvmsg() call I did via io-uring while receiving
file descriptors from lxc for the seccomp-notify proxy. (I'm currently
running an ubuntu based 5.3.1 kernel)
I ended up finding them in /proc - they show up in all kernel threads,
eg.:

root:/root # grep Name /proc/9/status
Name:   mm_percpu_wq
root:/root # ls -l /proc/9/fd
total 0
lr-x------ 1 root root 64 Oct 23 12:00 0 -> '/proc/512 (deleted)'
lrwx------ 1 root root 64 Oct 23 12:00 1 -> /proc/512/mem
lr-x------ 1 root root 64 Oct 23 12:00 10 -> '/proc/11782 (deleted)'
lrwx------ 1 root root 64 Oct 23 12:00 11 -> /proc/11782/mem
lr-x------ 1 root root 64 Oct 23 12:00 12 -> '/proc/12210 (deleted)'
lrwx------ 1 root root 64 Oct 23 12:00 13 -> /proc/12210/mem
lr-x------ 1 root root 64 Oct 23 12:00 14 -> '/proc/12298 (deleted)'
lrwx------ 1 root root 64 Oct 23 12:00 15 -> /proc/12298/mem
lr-x------ 1 root root 64 Oct 23 12:00 16 -> '/proc/13955 (deleted)'
lrwx------ 1 root root 64 Oct 23 12:00 17 -> /proc/13955/mem
lr-x------ 1 root root 64 Oct 23 12:00 18 -> '/proc/13989 (deleted)'
lrwx------ 1 root root 64 Oct 23 12:00 19 -> /proc/13989/mem
lr-x------ 1 root root 64 Oct 23 12:00 2 -> '/proc/584 (deleted)'
lr-x------ 1 root root 64 Oct 23 12:00 20 -> '/proc/15502 (deleted)'
lrwx------ 1 root root 64 Oct 23 12:00 21 -> /proc/15502/mem
lr-x------ 1 root root 64 Oct 23 12:00 22 -> '/proc/15510 (deleted)'
lrwx------ 1 root root 64 Oct 23 12:00 23 -> /proc/15510/mem
lr-x------ 1 root root 64 Oct 23 12:00 24 -> '/proc/17833 (deleted)'
lrwx------ 1 root root 64 Oct 23 12:00 25 -> /proc/17833/mem
lr-x------ 1 root root 64 Oct 23 12:00 26 -> '/proc/17836 (deleted)'
lrwx------ 1 root root 64 Oct 23 12:00 27 -> /proc/17836/mem
lr-x------ 1 root root 64 Oct 23 12:00 28 -> '/proc/21929 (deleted)'
lrwx------ 1 root root 64 Oct 23 12:00 29 -> /proc/21929/mem
lrwx------ 1 root root 64 Oct 23 12:00 3 -> /proc/584/mem
lr-x------ 1 root root 64 Oct 23 12:00 30 -> '/proc/22214 (deleted)'
lrwx------ 1 root root 64 Oct 23 12:00 31 -> /proc/22214/mem
lr-x------ 1 root root 64 Oct 23 12:00 32 -> '/proc/22283 (deleted)'
lrwx------ 1 root root 64 Oct 23 12:00 33 -> /proc/22283/mem
lr-x------ 1 root root 64 Oct 23 12:00 34 -> '/proc/29795 (deleted)'
lrwx------ 1 root root 64 Oct 23 12:00 35 -> /proc/29795/mem
lr-x------ 1 root root 64 Oct 23 12:00 36 -> '/proc/30124 (deleted)'
lrwx------ 1 root root 64 Oct 23 12:00 37 -> /proc/30124/mem
lr-x------ 1 root root 64 Oct 23 12:00 38 -> '/proc/31016 (deleted)'
lrwx------ 1 root root 64 Oct 23 12:00 39 -> /proc/31016/mem
lr-x------ 1 root root 64 Oct 23 12:00 4 -> '/proc/1632 (deleted)'
lr-x------ 1 root root 64 Oct 23 12:00 40 -> '/proc/4137 (deleted)'
lrwx------ 1 root root 64 Oct 23 12:00 41 -> /proc/4137/mem
lrwx------ 1 root root 64 Oct 23 12:00 5 -> /proc/1632/mem
lr-x------ 1 root root 64 Oct 23 12:00 6 -> '/proc/3655 (deleted)'
lrwx------ 1 root root 64 Oct 23 12:00 7 -> /proc/3655/mem
lr-x------ 1 root root 64 Oct 23 12:00 8 -> '/proc/7075 (deleted)'
lrwx------ 1 root root 64 Oct 23 12:00 9 -> /proc/7075/mem
root:/root #

Those are the fds I expected to receive, and I get fd numbers
consistently increasing with them.
lxc sends the syscall-executing process' pidfd and its 'mem' fd via a
socket, but instead of making it to the receiver, they end up there...

I suspect that an async sendmsg() call could potentially end up
accessing those instead of the ones from the sender process, but I
haven't tested it...

> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  fs/io_uring.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 635856023fdf..ad462237275e 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -267,10 +267,11 @@ struct io_ring_ctx {
>  struct sqe_submit {
>  	const struct io_uring_sqe	*sqe;
>  	unsigned short			index;
> +	bool				has_user : 1;
> +	bool				in_async : 1;
> +	bool				needs_fixed_file : 1;
>  	u32				sequence;
> -	bool				has_user;
> -	bool				in_async;
> -	bool				needs_fixed_file;
> +	struct files_struct		*files;
>  };
>  
>  /*
> @@ -323,6 +324,7 @@ struct io_kiocb {
>  #define REQ_F_FAIL_LINK		256	/* fail rest of links */
>  #define REQ_F_SHADOW_DRAIN	512	/* link-drain shadow req */
>  #define REQ_F_TIMEOUT		1024	/* timeout request */
> +#define REQ_F_NEED_FILES	2048	/* needs to assume file table */
>  	u64			user_data;
>  	u32			result;
>  	u32			sequence;
> @@ -2191,6 +2193,7 @@ static inline bool io_sqe_needs_user(const struct io_uring_sqe *sqe)
>  static void io_sq_wq_submit_work(struct work_struct *work)
>  {
>  	struct io_kiocb *req = container_of(work, struct io_kiocb, work);
> +	struct files_struct *old_files = NULL;
>  	struct io_ring_ctx *ctx = req->ctx;
>  	struct mm_struct *cur_mm = NULL;
>  	struct async_list *async_list;
> @@ -2220,6 +2223,10 @@ static void io_sq_wq_submit_work(struct work_struct *work)
>  				set_fs(USER_DS);
>  			}
>  		}
> +		if (s->files && !old_files) {
> +			old_files = current->files;
> +			current->files = s->files;
> +		}
>  
>  		if (!ret) {
>  			s->has_user = cur_mm != NULL;
> @@ -2312,6 +2319,11 @@ static void io_sq_wq_submit_work(struct work_struct *work)
>  		unuse_mm(cur_mm);
>  		mmput(cur_mm);
>  	}
> +	if (old_files) {
> +		struct files_struct *files = current->files;
> +		current->files = old_files;
> +		put_files_struct(files);
> +	}
>  }
>  
>  /*
> @@ -2413,6 +2425,8 @@ static int __io_queue_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
>  
>  			s->sqe = sqe_copy;
>  			memcpy(&req->submit, s, sizeof(*s));
> +			if (req->flags & REQ_F_NEED_FILES)
> +				req->submit.files = get_files_struct(current);
>  			list = io_async_list_from_sqe(ctx, s->sqe);
>  			if (!io_add_to_prev_work(list, req)) {
>  				if (list)
> @@ -2633,6 +2647,7 @@ static bool io_get_sqring(struct io_ring_ctx *ctx, struct sqe_submit *s)
>  		s->index = head;
>  		s->sqe = &ctx->sq_sqes[head];
>  		s->sequence = ctx->cached_sq_head;
> +		s->files = NULL;
>  		ctx->cached_sq_head++;
>  		return true;
>  	}
> -- 
> 2.17.1

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

[Jens Axboe]

On 10/23/19 6:04 AM, Wolfgang Bumiller wrote:
> On Thu, Oct 17, 2019 at 03:28:56PM -0600, Jens Axboe wrote:
>> This is in preparation for adding opcodes that need to modify files
>> in a process file table, either adding new ones or closing old ones.
>>
>> If an opcode needs this, it must set REQ_F_NEED_FILES in the request
>> structure. If work that needs to get punted to async context have this
>> set, they will grab a reference to the process file table. When the
>> work is completed, the reference is dropped again.
> 
> I think IORING_OP_SENDMSG and _RECVMSG need to set this flag due to
> SCM_RIGHTS control messages.
> Thought I'd reply here since I just now ran into the issue that I was
> getting ever-increasing wrong file descriptor numbers on pretty much
> ever "other" async recvmsg() call I did via io-uring while receiving
> file descriptors from lxc for the seccomp-notify proxy. (I'm currently
> running an ubuntu based 5.3.1 kernel)
> I ended up finding them in /proc - they show up in all kernel threads,
> eg.:
> 
> root:/root # grep Name /proc/9/status
> Name:   mm_percpu_wq
> root:/root # ls -l /proc/9/fd
> total 0
> lr-x------ 1 root root 64 Oct 23 12:00 0 -> '/proc/512 (deleted)'
> lrwx------ 1 root root 64 Oct 23 12:00 1 -> /proc/512/mem
> lr-x------ 1 root root 64 Oct 23 12:00 10 -> '/proc/11782 (deleted)'
> lrwx------ 1 root root 64 Oct 23 12:00 11 -> /proc/11782/mem
> lr-x------ 1 root root 64 Oct 23 12:00 12 -> '/proc/12210 (deleted)'
> lrwx------ 1 root root 64 Oct 23 12:00 13 -> /proc/12210/mem
> lr-x------ 1 root root 64 Oct 23 12:00 14 -> '/proc/12298 (deleted)'
> lrwx------ 1 root root 64 Oct 23 12:00 15 -> /proc/12298/mem
> lr-x------ 1 root root 64 Oct 23 12:00 16 -> '/proc/13955 (deleted)'
> lrwx------ 1 root root 64 Oct 23 12:00 17 -> /proc/13955/mem
> lr-x------ 1 root root 64 Oct 23 12:00 18 -> '/proc/13989 (deleted)'
> lrwx------ 1 root root 64 Oct 23 12:00 19 -> /proc/13989/mem
> lr-x------ 1 root root 64 Oct 23 12:00 2 -> '/proc/584 (deleted)'
> lr-x------ 1 root root 64 Oct 23 12:00 20 -> '/proc/15502 (deleted)'
> lrwx------ 1 root root 64 Oct 23 12:00 21 -> /proc/15502/mem
> lr-x------ 1 root root 64 Oct 23 12:00 22 -> '/proc/15510 (deleted)'
> lrwx------ 1 root root 64 Oct 23 12:00 23 -> /proc/15510/mem
> lr-x------ 1 root root 64 Oct 23 12:00 24 -> '/proc/17833 (deleted)'
> lrwx------ 1 root root 64 Oct 23 12:00 25 -> /proc/17833/mem
> lr-x------ 1 root root 64 Oct 23 12:00 26 -> '/proc/17836 (deleted)'
> lrwx------ 1 root root 64 Oct 23 12:00 27 -> /proc/17836/mem
> lr-x------ 1 root root 64 Oct 23 12:00 28 -> '/proc/21929 (deleted)'
> lrwx------ 1 root root 64 Oct 23 12:00 29 -> /proc/21929/mem
> lrwx------ 1 root root 64 Oct 23 12:00 3 -> /proc/584/mem
> lr-x------ 1 root root 64 Oct 23 12:00 30 -> '/proc/22214 (deleted)'
> lrwx------ 1 root root 64 Oct 23 12:00 31 -> /proc/22214/mem
> lr-x------ 1 root root 64 Oct 23 12:00 32 -> '/proc/22283 (deleted)'
> lrwx------ 1 root root 64 Oct 23 12:00 33 -> /proc/22283/mem
> lr-x------ 1 root root 64 Oct 23 12:00 34 -> '/proc/29795 (deleted)'
> lrwx------ 1 root root 64 Oct 23 12:00 35 -> /proc/29795/mem
> lr-x------ 1 root root 64 Oct 23 12:00 36 -> '/proc/30124 (deleted)'
> lrwx------ 1 root root 64 Oct 23 12:00 37 -> /proc/30124/mem
> lr-x------ 1 root root 64 Oct 23 12:00 38 -> '/proc/31016 (deleted)'
> lrwx------ 1 root root 64 Oct 23 12:00 39 -> /proc/31016/mem
> lr-x------ 1 root root 64 Oct 23 12:00 4 -> '/proc/1632 (deleted)'
> lr-x------ 1 root root 64 Oct 23 12:00 40 -> '/proc/4137 (deleted)'
> lrwx------ 1 root root 64 Oct 23 12:00 41 -> /proc/4137/mem
> lrwx------ 1 root root 64 Oct 23 12:00 5 -> /proc/1632/mem
> lr-x------ 1 root root 64 Oct 23 12:00 6 -> '/proc/3655 (deleted)'
> lrwx------ 1 root root 64 Oct 23 12:00 7 -> /proc/3655/mem
> lr-x------ 1 root root 64 Oct 23 12:00 8 -> '/proc/7075 (deleted)'
> lrwx------ 1 root root 64 Oct 23 12:00 9 -> /proc/7075/mem
> root:/root #
> 
> Those are the fds I expected to receive, and I get fd numbers
> consistently increasing with them.
> lxc sends the syscall-executing process' pidfd and its 'mem' fd via a
> socket, but instead of making it to the receiver, they end up there...
> 
> I suspect that an async sendmsg() call could potentially end up
> accessing those instead of the ones from the sender process, but I
> haven't tested it...

Might "just" be a case of the sendmsg() being stuck, we can't currently
cancel work. So if they never complete, the ring won't go away.

Actually working on a small workqueue replacement for io_uring which
allow us to cancel things like that. It's a requirement for accept() as
well, but also for basic read/write send/recv on sockets. So used to
storage IO operations that complete in a finite amount of time...

But yes, I hope with that, and the flush trick that Jann suggested, that
we can make this 100% reliable for any type of operation.

-- 
Jens Axboe

[PATCH 2/3] net: add __sys_accept4_file() helper

[Jens Axboe]

This is identical to __sys_accept4(), except it takes a struct file
instead of an fd, and it also allows passing in extra file->f_flags
flags. The latter is done to support masking in O_NONBLOCK without
manipulating the original file flags.

No functional changes in this patch.

Cc: David Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/socket.h |  3 ++
 net/socket.c           | 65 ++++++++++++++++++++++++++----------------
 2 files changed, 44 insertions(+), 24 deletions(-)

diff --git a/include/linux/socket.h b/include/linux/socket.h
index fc0bed59fc84..dd061f741bc1 100644
--- a/include/linux/socket.h
+++ b/include/linux/socket.h
@@ -392,6 +392,9 @@ extern int __sys_recvfrom(int fd, void __user *ubuf, size_t size,
 extern int __sys_sendto(int fd, void __user *buff, size_t len,
 			unsigned int flags, struct sockaddr __user *addr,
 			int addr_len);
+extern int __sys_accept4_file(struct file *file, unsigned file_flags,
+			struct sockaddr __user *upeer_sockaddr,
+			 int __user *upeer_addrlen, int flags);
 extern int __sys_accept4(int fd, struct sockaddr __user *upeer_sockaddr,
 			 int __user *upeer_addrlen, int flags);
 extern int __sys_socket(int family, int type, int protocol);
diff --git a/net/socket.c b/net/socket.c
index 6a9ab7a8b1d2..40ab39f6c5d8 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1690,24 +1690,13 @@ SYSCALL_DEFINE2(listen, int, fd, int, backlog)
 	return __sys_listen(fd, backlog);
 }
 
-/*
- *	For accept, we attempt to create a new socket, set up the link
- *	with the client, wake up the client, then return the new
- *	connected fd. We collect the address of the connector in kernel
- *	space and move it to user at the very end. This is unclean because
- *	we open the socket then return an error.
- *
- *	1003.1g adds the ability to recvmsg() to query connection pending
- *	status to recvmsg. We need to add that support in a way thats
- *	clean when we restructure accept also.
- */
-
-int __sys_accept4(int fd, struct sockaddr __user *upeer_sockaddr,
-		  int __user *upeer_addrlen, int flags)
+int __sys_accept4_file(struct file *file, unsigned file_flags,
+		       struct sockaddr __user *upeer_sockaddr,
+		       int __user *upeer_addrlen, int flags)
 {
 	struct socket *sock, *newsock;
 	struct file *newfile;
-	int err, len, newfd, fput_needed;
+	int err, len, newfd;
 	struct sockaddr_storage address;
 
 	if (flags & ~(SOCK_CLOEXEC | SOCK_NONBLOCK))
@@ -1716,14 +1705,14 @@ int __sys_accept4(int fd, struct sockaddr __user *upeer_sockaddr,
 	if (SOCK_NONBLOCK != O_NONBLOCK && (flags & SOCK_NONBLOCK))
 		flags = (flags & ~SOCK_NONBLOCK) | O_NONBLOCK;
 
-	sock = sockfd_lookup_light(fd, &err, &fput_needed);
+	sock = sock_from_file(file, &err);
 	if (!sock)
 		goto out;
 
 	err = -ENFILE;
 	newsock = sock_alloc();
 	if (!newsock)
-		goto out_put;
+		goto out;
 
 	newsock->type = sock->type;
 	newsock->ops = sock->ops;
@@ -1738,20 +1727,21 @@ int __sys_accept4(int fd, struct sockaddr __user *upeer_sockaddr,
 	if (unlikely(newfd < 0)) {
 		err = newfd;
 		sock_release(newsock);
-		goto out_put;
+		goto out;
 	}
 	newfile = sock_alloc_file(newsock, flags, sock->sk->sk_prot_creator->name);
 	if (IS_ERR(newfile)) {
 		err = PTR_ERR(newfile);
 		put_unused_fd(newfd);
-		goto out_put;
+		goto out;
 	}
 
 	err = security_socket_accept(sock, newsock);
 	if (err)
 		goto out_fd;
 
-	err = sock->ops->accept(sock, newsock, sock->file->f_flags, false);
+	err = sock->ops->accept(sock, newsock, sock->file->f_flags | file_flags,
+					false);
 	if (err < 0)
 		goto out_fd;
 
@@ -1772,15 +1762,42 @@ int __sys_accept4(int fd, struct sockaddr __user *upeer_sockaddr,
 
 	fd_install(newfd, newfile);
 	err = newfd;
-
-out_put:
-	fput_light(sock->file, fput_needed);
 out:
 	return err;
 out_fd:
 	fput(newfile);
 	put_unused_fd(newfd);
-	goto out_put;
+	goto out;
+
+}
+
+/*
+ *	For accept, we attempt to create a new socket, set up the link
+ *	with the client, wake up the client, then return the new
+ *	connected fd. We collect the address of the connector in kernel
+ *	space and move it to user at the very end. This is unclean because
+ *	we open the socket then return an error.
+ *
+ *	1003.1g adds the ability to recvmsg() to query connection pending
+ *	status to recvmsg. We need to add that support in a way thats
+ *	clean when we restructure accept also.
+ */
+
+int __sys_accept4(int fd, struct sockaddr __user *upeer_sockaddr,
+		  int __user *upeer_addrlen, int flags)
+{
+	int ret = -EBADF;
+	struct fd f;
+
+	f = fdget(fd);
+	if (f.file) {
+		ret = __sys_accept4_file(f.file, 0, upeer_sockaddr,
+						upeer_addrlen, flags);
+		if (f.flags)
+			fput(f.file);
+	}
+
+	return ret;
 }
 
 SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr __user *, upeer_sockaddr,
-- 
2.17.1

[PATCH 3/3] io_uring: add support for IORING_OP_ACCEPT

[Jens Axboe]

This allows an application to call accept4() in an async fashion. Like
other opcodes, we first try a non-blocking accept, then punt to async
context if we have to.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 fs/io_uring.c                 | 35 +++++++++++++++++++++++++++++++++++
 include/uapi/linux/io_uring.h |  7 ++++++-
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index ad462237275e..8d183a6b08d4 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1694,6 +1694,38 @@ static int io_recvmsg(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 #endif
 }
 
+static int io_accept(struct io_kiocb *req, const struct io_uring_sqe *sqe,
+		     struct io_kiocb **nxt, bool force_nonblock)
+{
+#if defined(CONFIG_NET)
+	struct sockaddr __user *addr;
+	int __user *addr_len;
+	unsigned file_flags;
+	int flags, ret;
+
+	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
+		return -EINVAL;
+
+	addr = (struct sockaddr __user *) READ_ONCE(sqe->addr);
+	addr_len = (int __user *) READ_ONCE(sqe->addr2);
+	flags = READ_ONCE(sqe->accept_flags);
+	file_flags = force_nonblock ? O_NONBLOCK : 0;
+
+	ret = __sys_accept4_file(req->file, file_flags, addr, addr_len, flags);
+	if (ret == -EAGAIN && force_nonblock) {
+		req->flags |= REQ_F_NEED_FILES;
+		return -EAGAIN;
+	}
+	if (ret < 0 && (req->flags & REQ_F_LINK))
+		req->flags |= REQ_F_FAIL_LINK;
+	io_cqring_add_event(req->ctx, sqe->user_data, ret);
+	io_put_req(req, nxt);
+	return 0;
+#else
+	return -EOPNOTSUPP;
+#endif
+}
+
 static void io_poll_remove_one(struct io_kiocb *req)
 {
 	struct io_poll_iocb *poll = &req->poll;
@@ -2144,6 +2176,9 @@ static int __io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
 	case IORING_OP_TIMEOUT_REMOVE:
 		ret = io_timeout_remove(req, s->sqe);
 		break;
+	case IORING_OP_ACCEPT:
+		ret = io_accept(req, s->sqe, nxt, force_nonblock);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 6dc5ced1c37a..f82d90e617a6 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -19,7 +19,10 @@ struct io_uring_sqe {
 	__u8	flags;		/* IOSQE_ flags */
 	__u16	ioprio;		/* ioprio for the request */
 	__s32	fd;		/* file descriptor to do IO on */
-	__u64	off;		/* offset into file */
+	union {
+		__u64	off;	/* offset into file */
+		__u64	addr2;
+	};
 	__u64	addr;		/* pointer to buffer or iovecs */
 	__u32	len;		/* buffer size or number of iovecs */
 	union {
@@ -29,6 +32,7 @@ struct io_uring_sqe {
 		__u32		sync_range_flags;
 		__u32		msg_flags;
 		__u32		timeout_flags;
+		__u32		accept_flags;
 	};
 	__u64	user_data;	/* data to be passed back at completion time */
 	union {
@@ -65,6 +69,7 @@ struct io_uring_sqe {
 #define IORING_OP_RECVMSG	10
 #define IORING_OP_TIMEOUT	11
 #define IORING_OP_TIMEOUT_REMOVE	12
+#define IORING_OP_ACCEPT	13
 
 /*
  * sqe->fsync_flags
-- 
2.17.1