Message ID | 20230720221858.135240-7-axboe@kernel.dk (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | Add io_uring futex/futexv support | expand |
On Thu, Jul 20, 2023 at 04:18:54PM -0600, Jens Axboe wrote: > +struct io_futex { > + struct file *file; > + u32 __user *uaddr; > + unsigned int futex_val; > + unsigned int futex_flags; > + unsigned int futex_mask; > +}; So in the futex patches I just posted I went with 'unsigned long' (syscall) or 'u64' (data structures) for the futex, such that, on 64bit platforms, we might support 64bit futexes in the future (I still need to audit the whole futex internals and convert u32 to unsigned long in order to enable that). So would something like: struct io_futex { struct file *file; void __user *uaddr; u64 futex_val; u64 futex_mask; u32 futex_flags; }; work to match the futex2 syscalls? > +int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) > +{ > + struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex); > + > + if (unlikely(sqe->fd || sqe->addr2 || sqe->buf_index || sqe->addr3)) > + return -EINVAL; > + > + iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr)); > + iof->futex_val = READ_ONCE(sqe->len); > + iof->futex_mask = READ_ONCE(sqe->file_index); > + iof->futex_flags = READ_ONCE(sqe->futex_flags); sqe->addr, u64 sqe->len, u32 sqe->file_index, u32 sqe->futex_flags, u32 > + if (iof->futex_flags & FUTEX_CMD_MASK) FUTEX2_MASK (which would need lifting from syscall.c to kernel/futex/futex.h I suppose) > + return -EINVAL; > + > + return 0; > +} > diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h > index 36f9c73082de..3bd2d765f593 100644 > --- a/include/uapi/linux/io_uring.h > +++ b/include/uapi/linux/io_uring.h > @@ -65,6 +65,7 @@ struct io_uring_sqe { > __u32 xattr_flags; > __u32 msg_ring_flags; > __u32 uring_cmd_flags; > + __u32 futex_flags; > }; > __u64 user_data; /* data to be passed back at completion time */ > /* pack this to avoid bogus arm OABI complaints */ Perhaps extend it like so? diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h index 08720c7bd92f..c1d28bf64d11 100644 --- a/include/uapi/linux/io_uring.h +++ b/include/uapi/linux/io_uring.h @@ -35,6 +35,7 @@ struct io_uring_sqe { union { __u64 off; /* offset into file */ __u64 addr2; + __u64 futex_val; struct { __u32 cmd_op; __u32 __pad1; @@ -65,6 +66,7 @@ struct io_uring_sqe { __u32 xattr_flags; __u32 msg_ring_flags; __u32 uring_cmd_flags; + __u32 futex_flags; }; __u64 user_data; /* data to be passed back at completion time */ /* pack this to avoid bogus arm OABI complaints */ @@ -87,6 +89,7 @@ struct io_uring_sqe { union { struct { __u64 addr3; + __u64 futex_mask; __u64 __pad2[1]; }; /* So that we can write something roughtly like: iof->uaddr = sqe->addr; iof->val = sqe->futex_val; iof->mask = sqe->futex_mask; iof->flags = sqe->futex_flags; if (iof->flags & ~FUTEX2_MASK) return -EINVAL;
On Fri, Jul 21, 2023 at 01:30:31PM +0200, Peter Zijlstra wrote: Sorry, I was too quick.. iof->uaddr = sqe->addr; iof->val = sqe->futex_val; iof->mask = sqe->futex_mask; flags = sqe->futex_flags; if (flags & ~FUTEX2_MASK) return -EINVAL; iof->flags = futex2_to_flags(flags); if (!futex_flags_valid(iof->flags)) return -EINVAL; if (!futex_validate_input(iof->flags, iof->val) || !futex_validate_input(iof->flags, iof->mask)) return -EINVAL
On 7/21/23 5:37?AM, Peter Zijlstra wrote: > On Fri, Jul 21, 2023 at 01:30:31PM +0200, Peter Zijlstra wrote: > > Sorry, I was too quick.. > > iof->uaddr = sqe->addr; > iof->val = sqe->futex_val; > iof->mask = sqe->futex_mask; > flags = sqe->futex_flags; > > if (flags & ~FUTEX2_MASK) > return -EINVAL; > > iof->flags = futex2_to_flags(flags); > if (!futex_flags_valid(iof->flags)) > return -EINVAL; > > if (!futex_validate_input(iof->flags, iof->val) || > !futex_validate_input(iof->flags, iof->mask)) > return -EINVAL Something like that should work, with some variable names fixed up. I just went with 'addr' for the futex address, addr2 for the value, and addr3 for the mask. Rebased on top of your first 4 updated patches, and added a single patch that moves FUTEX2_MASK, will run some testing to validate it's all still sane.
On 7/21/23 8:43?AM, Jens Axboe wrote: > On 7/21/23 5:37?AM, Peter Zijlstra wrote: >> On Fri, Jul 21, 2023 at 01:30:31PM +0200, Peter Zijlstra wrote: >> >> Sorry, I was too quick.. >> >> iof->uaddr = sqe->addr; >> iof->val = sqe->futex_val; >> iof->mask = sqe->futex_mask; >> flags = sqe->futex_flags; >> >> if (flags & ~FUTEX2_MASK) >> return -EINVAL; >> >> iof->flags = futex2_to_flags(flags); >> if (!futex_flags_valid(iof->flags)) >> return -EINVAL; >> >> if (!futex_validate_input(iof->flags, iof->val) || >> !futex_validate_input(iof->flags, iof->mask)) >> return -EINVAL > > Something like that should work, with some variable names fixed up. I > just went with 'addr' for the futex address, addr2 for the value, and > addr3 for the mask. > > Rebased on top of your first 4 updated patches, and added a single patch > that moves FUTEX2_MASK, will run some testing to validate it's all still > sane. FWIW, here's the io_uring incremental after that rebase. Update the liburing futex branch as well, updating the prep helpers to take 64 bit values for mask/val and also add the flags argument that was missing as well. Only other addition was adding those 4 new patches instead of the old 3 ones, and adding single patch that just moves FUTEX2_MASK to futex.h. All checks out fine, tests pass and it works. diff --git a/io_uring/futex.c b/io_uring/futex.c index 93df54dffaa0..4c9f2c841b98 100644 --- a/io_uring/futex.c +++ b/io_uring/futex.c @@ -18,11 +18,11 @@ struct io_futex { u32 __user *uaddr; struct futex_waitv __user *uwaitv; }; - unsigned int futex_val; - unsigned int futex_flags; - unsigned int futex_mask; - unsigned int futex_nr; + unsigned long futex_val; + unsigned long futex_mask; unsigned long futexv_owned; + u32 futex_flags; + unsigned int futex_nr; }; struct io_futex_data { @@ -171,15 +171,28 @@ bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task, int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) { struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex); + u32 flags; - if (unlikely(sqe->fd || sqe->buf_index || sqe->addr3)) + if (unlikely(sqe->fd || sqe->buf_index || sqe->file_index)) return -EINVAL; iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr)); - iof->futex_val = READ_ONCE(sqe->len); - iof->futex_mask = READ_ONCE(sqe->file_index); - iof->futex_flags = READ_ONCE(sqe->futex_flags); - if (iof->futex_flags & FUTEX_CMD_MASK) + iof->futex_val = READ_ONCE(sqe->addr2); + iof->futex_mask = READ_ONCE(sqe->addr3); + iof->futex_nr = READ_ONCE(sqe->len); + if (iof->futex_nr && req->opcode != IORING_OP_FUTEX_WAITV) + return -EINVAL; + + flags = READ_ONCE(sqe->futex_flags); + if (flags & ~FUTEX2_MASK) + return -EINVAL; + + iof->futex_flags = futex2_to_flags(flags); + if (!futex_flags_valid(iof->futex_flags)) + return -EINVAL; + + if (!futex_validate_input(iof->futex_flags, iof->futex_val) || + !futex_validate_input(iof->futex_flags, iof->futex_mask)) return -EINVAL; iof->futexv_owned = 0; @@ -211,7 +224,6 @@ int io_futexv_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) if (ret) return ret; - iof->futex_nr = READ_ONCE(sqe->off); if (!iof->futex_nr || iof->futex_nr > FUTEX_WAITV_MAX) return -EINVAL;
On Fri, Jul 21, 2023 at 09:29:14AM -0600, Jens Axboe wrote: > FWIW, here's the io_uring incremental after that rebase. Update the > liburing futex branch as well, updating the prep helpers to take 64 bit > values for mask/val and also add the flags argument that was missing as > well. Only other addition was adding those 4 new patches instead of the > old 3 ones, and adding single patch that just moves FUTEX2_MASK to > futex.h. > > All checks out fine, tests pass and it works. > > > diff --git a/io_uring/futex.c b/io_uring/futex.c > index 93df54dffaa0..4c9f2c841b98 100644 > --- a/io_uring/futex.c > +++ b/io_uring/futex.c > @@ -18,11 +18,11 @@ struct io_futex { > u32 __user *uaddr; > struct futex_waitv __user *uwaitv; > }; > + unsigned long futex_val; > + unsigned long futex_mask; > unsigned long futexv_owned; > + u32 futex_flags; > + unsigned int futex_nr; > }; > > struct io_futex_data { > @@ -171,15 +171,28 @@ bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task, > int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) > { > struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex); > + u32 flags; > > + if (unlikely(sqe->fd || sqe->buf_index || sqe->file_index)) > return -EINVAL; > > iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr)); > + iof->futex_val = READ_ONCE(sqe->addr2); > + iof->futex_mask = READ_ONCE(sqe->addr3); > + iof->futex_nr = READ_ONCE(sqe->len); > + if (iof->futex_nr && req->opcode != IORING_OP_FUTEX_WAITV) > + return -EINVAL; > + Hmm, would something like: if (req->opcode == IORING_OP_FUTEX_WAITV) { if (iof->futex_val && iof->futex_mask) return -EINVAL; /* sys_futex_waitv() doesn't take @flags as of yet */ if (iof->futex_flags) return -EINVAL; if (!iof->futex_nr) return -EINVAL; } else { /* sys_futex_{wake,wait}() don't take @nr */ if (iof->futex_nr) return -EINVAL; /* both take @flags and @mask */ flags = READ_ONCE(sqe->futex_flags); if (flags & ~FUTEX2_MASK) return -EINVAL; iof->futex_flags = futex2_to_flags(flags); if (!futex_flags_valid(iof->futex_flags)) return -EINVAL; if (!futex_validate_input(iof->futex_flags, iof->futex_mask)) return -EINVAL; /* sys_futex_wait() takes @val */ if (req->iocode == IORING_OP_FUTEX_WAIT) { if (!futex_validate_input(iof->futex_flags, iof->futex_val)) return -EINVAL; } else { if (iof->futex_val) return -EINVAL; } } work? The waitv thing is significantly different from the other two.
On 7/25/23 7:00?AM, Peter Zijlstra wrote: > On Fri, Jul 21, 2023 at 09:29:14AM -0600, Jens Axboe wrote: > >> FWIW, here's the io_uring incremental after that rebase. Update the >> liburing futex branch as well, updating the prep helpers to take 64 bit >> values for mask/val and also add the flags argument that was missing as >> well. Only other addition was adding those 4 new patches instead of the >> old 3 ones, and adding single patch that just moves FUTEX2_MASK to >> futex.h. >> >> All checks out fine, tests pass and it works. >> >> >> diff --git a/io_uring/futex.c b/io_uring/futex.c >> index 93df54dffaa0..4c9f2c841b98 100644 >> --- a/io_uring/futex.c >> +++ b/io_uring/futex.c >> @@ -18,11 +18,11 @@ struct io_futex { >> u32 __user *uaddr; >> struct futex_waitv __user *uwaitv; >> }; >> + unsigned long futex_val; >> + unsigned long futex_mask; >> unsigned long futexv_owned; >> + u32 futex_flags; >> + unsigned int futex_nr; >> }; >> >> struct io_futex_data { >> @@ -171,15 +171,28 @@ bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task, >> int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) >> { >> struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex); >> + u32 flags; >> >> + if (unlikely(sqe->fd || sqe->buf_index || sqe->file_index)) >> return -EINVAL; >> >> iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr)); >> + iof->futex_val = READ_ONCE(sqe->addr2); >> + iof->futex_mask = READ_ONCE(sqe->addr3); >> + iof->futex_nr = READ_ONCE(sqe->len); >> + if (iof->futex_nr && req->opcode != IORING_OP_FUTEX_WAITV) >> + return -EINVAL; >> + > > Hmm, would something like: > > if (req->opcode == IORING_OP_FUTEX_WAITV) { > if (iof->futex_val && iof->futex_mask) > return -EINVAL; > > /* sys_futex_waitv() doesn't take @flags as of yet */ > if (iof->futex_flags) > return -EINVAL; > > if (!iof->futex_nr) > return -EINVAL; > > } else { > /* sys_futex_{wake,wait}() don't take @nr */ > if (iof->futex_nr) > return -EINVAL; > > /* both take @flags and @mask */ > flags = READ_ONCE(sqe->futex_flags); > if (flags & ~FUTEX2_MASK) > return -EINVAL; > > iof->futex_flags = futex2_to_flags(flags); > if (!futex_flags_valid(iof->futex_flags)) > return -EINVAL; > > if (!futex_validate_input(iof->futex_flags, iof->futex_mask)) > return -EINVAL; > > /* sys_futex_wait() takes @val */ > if (req->iocode == IORING_OP_FUTEX_WAIT) { > if (!futex_validate_input(iof->futex_flags, iof->futex_val)) > return -EINVAL; > } else { > if (iof->futex_val) > return -EINVAL; > } > } > > work? The waitv thing is significantly different from the other two. I think I'll just have prep and prepv totally separate. It only makes sense to share parts of them if one is a subset of the other. That'll get rid of the odd conditionals and sectioning of it.
On 7/25/23 7:48?AM, Jens Axboe wrote: > On 7/25/23 7:00?AM, Peter Zijlstra wrote: >> On Fri, Jul 21, 2023 at 09:29:14AM -0600, Jens Axboe wrote: >> >>> FWIW, here's the io_uring incremental after that rebase. Update the >>> liburing futex branch as well, updating the prep helpers to take 64 bit >>> values for mask/val and also add the flags argument that was missing as >>> well. Only other addition was adding those 4 new patches instead of the >>> old 3 ones, and adding single patch that just moves FUTEX2_MASK to >>> futex.h. >>> >>> All checks out fine, tests pass and it works. >>> >>> >>> diff --git a/io_uring/futex.c b/io_uring/futex.c >>> index 93df54dffaa0..4c9f2c841b98 100644 >>> --- a/io_uring/futex.c >>> +++ b/io_uring/futex.c >>> @@ -18,11 +18,11 @@ struct io_futex { >>> u32 __user *uaddr; >>> struct futex_waitv __user *uwaitv; >>> }; >>> + unsigned long futex_val; >>> + unsigned long futex_mask; >>> unsigned long futexv_owned; >>> + u32 futex_flags; >>> + unsigned int futex_nr; >>> }; >>> >>> struct io_futex_data { >>> @@ -171,15 +171,28 @@ bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task, >>> int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) >>> { >>> struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex); >>> + u32 flags; >>> >>> + if (unlikely(sqe->fd || sqe->buf_index || sqe->file_index)) >>> return -EINVAL; >>> >>> iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr)); >>> + iof->futex_val = READ_ONCE(sqe->addr2); >>> + iof->futex_mask = READ_ONCE(sqe->addr3); >>> + iof->futex_nr = READ_ONCE(sqe->len); >>> + if (iof->futex_nr && req->opcode != IORING_OP_FUTEX_WAITV) >>> + return -EINVAL; >>> + >> >> Hmm, would something like: >> >> if (req->opcode == IORING_OP_FUTEX_WAITV) { >> if (iof->futex_val && iof->futex_mask) >> return -EINVAL; >> >> /* sys_futex_waitv() doesn't take @flags as of yet */ >> if (iof->futex_flags) >> return -EINVAL; >> >> if (!iof->futex_nr) >> return -EINVAL; >> >> } else { >> /* sys_futex_{wake,wait}() don't take @nr */ >> if (iof->futex_nr) >> return -EINVAL; >> >> /* both take @flags and @mask */ >> flags = READ_ONCE(sqe->futex_flags); >> if (flags & ~FUTEX2_MASK) >> return -EINVAL; >> >> iof->futex_flags = futex2_to_flags(flags); >> if (!futex_flags_valid(iof->futex_flags)) >> return -EINVAL; >> >> if (!futex_validate_input(iof->futex_flags, iof->futex_mask)) >> return -EINVAL; >> >> /* sys_futex_wait() takes @val */ >> if (req->iocode == IORING_OP_FUTEX_WAIT) { >> if (!futex_validate_input(iof->futex_flags, iof->futex_val)) >> return -EINVAL; >> } else { >> if (iof->futex_val) >> return -EINVAL; >> } >> } >> >> work? The waitv thing is significantly different from the other two. > > I think I'll just have prep and prepv totally separate. It only makes > sense to share parts of them if one is a subset of the other. That'll > get rid of the odd conditionals and sectioning of it. Something like the below - totally untested, but just to show what I mean. Will need to get split and folded into the two separate patches. Will test and fold them later today. diff --git a/io_uring/futex.c b/io_uring/futex.c index 4c9f2c841b98..b0f90154d974 100644 --- a/io_uring/futex.c +++ b/io_uring/futex.c @@ -168,7 +168,7 @@ bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task, return found; } -int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) +static int __io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) { struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex); u32 flags; @@ -179,9 +179,6 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr)); iof->futex_val = READ_ONCE(sqe->addr2); iof->futex_mask = READ_ONCE(sqe->addr3); - iof->futex_nr = READ_ONCE(sqe->len); - if (iof->futex_nr && req->opcode != IORING_OP_FUTEX_WAITV) - return -EINVAL; flags = READ_ONCE(sqe->futex_flags); if (flags & ~FUTEX2_MASK) @@ -191,14 +188,36 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) if (!futex_flags_valid(iof->futex_flags)) return -EINVAL; - if (!futex_validate_input(iof->futex_flags, iof->futex_val) || - !futex_validate_input(iof->futex_flags, iof->futex_mask)) + if (!futex_validate_input(iof->futex_flags, iof->futex_mask)) return -EINVAL; - iof->futexv_owned = 0; return 0; } +int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) +{ + struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex); + int ret; + + if (unlikely(sqe->len)) + return -EINVAL; + + ret = __io_futex_prep(req, sqe); + if (ret) + return ret; + + /* sys_futex_wait() takes @val */ + if (req->opcode == IORING_OP_FUTEX_WAIT) { + if (!futex_validate_input(iof->futex_flags, iof->futex_val)) + return -EINVAL; + } else { + if (iof->futex_val) + return -EINVAL; + } + + return 0; +} + static void io_futex_wakev_fn(struct wake_q_head *wake_q, struct futex_q *q) { struct io_kiocb *req = q->wake_data; @@ -220,10 +239,15 @@ int io_futexv_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) struct futex_vector *futexv; int ret; - ret = io_futex_prep(req, sqe); + ret = __io_futex_prep(req, sqe); if (ret) return ret; + /* No flags supported for waitv */ + if (iof->futex_flags) + return -EINVAL; + + iof->futex_nr = READ_ONCE(sqe->len); if (!iof->futex_nr || iof->futex_nr > FUTEX_WAITV_MAX) return -EINVAL; @@ -238,6 +262,7 @@ int io_futexv_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) return ret; } + iof->futexv_owned = 0; req->flags |= REQ_F_ASYNC_DATA; req->async_data = futexv; return 0;
On Tue, Jul 25, 2023 at 07:48:30AM -0600, Jens Axboe wrote: > I think I'll just have prep and prepv totally separate. It only makes > sense to share parts of them if one is a subset of the other. That'll > get rid of the odd conditionals and sectioning of it. Ah, yes. Fair enough.
On Tue, Jul 25, 2023 at 07:57:28AM -0600, Jens Axboe wrote: > Something like the below - totally untested, but just to show what I > mean. Will need to get split and folded into the two separate patches. > Will test and fold them later today. > > > diff --git a/io_uring/futex.c b/io_uring/futex.c > index 4c9f2c841b98..b0f90154d974 100644 > --- a/io_uring/futex.c > +++ b/io_uring/futex.c > @@ -168,7 +168,7 @@ bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task, > return found; > } > > -int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) > +static int __io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) > { > struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex); > u32 flags; > @@ -179,9 +179,6 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) > iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr)); > iof->futex_val = READ_ONCE(sqe->addr2); > iof->futex_mask = READ_ONCE(sqe->addr3); > - iof->futex_nr = READ_ONCE(sqe->len); > - if (iof->futex_nr && req->opcode != IORING_OP_FUTEX_WAITV) > - return -EINVAL; > > flags = READ_ONCE(sqe->futex_flags); > if (flags & ~FUTEX2_MASK) > @@ -191,14 +188,36 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) > if (!futex_flags_valid(iof->futex_flags)) > return -EINVAL; > > - if (!futex_validate_input(iof->futex_flags, iof->futex_val) || > - !futex_validate_input(iof->futex_flags, iof->futex_mask)) > + if (!futex_validate_input(iof->futex_flags, iof->futex_mask)) > return -EINVAL; > > - iof->futexv_owned = 0; > return 0; > } I think you can/should split more into io_futex_prep(), specifically waitv should also have zero @val and @mask. But yes, something like this makes sense.
On 7/25/23 9:19?AM, Peter Zijlstra wrote: > On Tue, Jul 25, 2023 at 07:57:28AM -0600, Jens Axboe wrote: > >> Something like the below - totally untested, but just to show what I >> mean. Will need to get split and folded into the two separate patches. >> Will test and fold them later today. >> >> >> diff --git a/io_uring/futex.c b/io_uring/futex.c >> index 4c9f2c841b98..b0f90154d974 100644 >> --- a/io_uring/futex.c >> +++ b/io_uring/futex.c >> @@ -168,7 +168,7 @@ bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task, >> return found; >> } >> >> -int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) >> +static int __io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) >> { >> struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex); >> u32 flags; >> @@ -179,9 +179,6 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) >> iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr)); >> iof->futex_val = READ_ONCE(sqe->addr2); >> iof->futex_mask = READ_ONCE(sqe->addr3); >> - iof->futex_nr = READ_ONCE(sqe->len); >> - if (iof->futex_nr && req->opcode != IORING_OP_FUTEX_WAITV) >> - return -EINVAL; >> >> flags = READ_ONCE(sqe->futex_flags); >> if (flags & ~FUTEX2_MASK) >> @@ -191,14 +188,36 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) >> if (!futex_flags_valid(iof->futex_flags)) >> return -EINVAL; >> >> - if (!futex_validate_input(iof->futex_flags, iof->futex_val) || >> - !futex_validate_input(iof->futex_flags, iof->futex_mask)) >> + if (!futex_validate_input(iof->futex_flags, iof->futex_mask)) >> return -EINVAL; >> >> - iof->futexv_owned = 0; >> return 0; >> } > > I think you can/should split more into io_futex_prep(), specifically > waitv should also have zero @val and @mask. Yep, I'll include that. Updating them now...
On 7/25/23 2:42?PM, Jens Axboe wrote: > On 7/25/23 9:19?AM, Peter Zijlstra wrote: >> On Tue, Jul 25, 2023 at 07:57:28AM -0600, Jens Axboe wrote: >> >>> Something like the below - totally untested, but just to show what I >>> mean. Will need to get split and folded into the two separate patches. >>> Will test and fold them later today. >>> >>> >>> diff --git a/io_uring/futex.c b/io_uring/futex.c >>> index 4c9f2c841b98..b0f90154d974 100644 >>> --- a/io_uring/futex.c >>> +++ b/io_uring/futex.c >>> @@ -168,7 +168,7 @@ bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task, >>> return found; >>> } >>> >>> -int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) >>> +static int __io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) >>> { >>> struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex); >>> u32 flags; >>> @@ -179,9 +179,6 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) >>> iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr)); >>> iof->futex_val = READ_ONCE(sqe->addr2); >>> iof->futex_mask = READ_ONCE(sqe->addr3); >>> - iof->futex_nr = READ_ONCE(sqe->len); >>> - if (iof->futex_nr && req->opcode != IORING_OP_FUTEX_WAITV) >>> - return -EINVAL; >>> >>> flags = READ_ONCE(sqe->futex_flags); >>> if (flags & ~FUTEX2_MASK) >>> @@ -191,14 +188,36 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) >>> if (!futex_flags_valid(iof->futex_flags)) >>> return -EINVAL; >>> >>> - if (!futex_validate_input(iof->futex_flags, iof->futex_val) || >>> - !futex_validate_input(iof->futex_flags, iof->futex_mask)) >>> + if (!futex_validate_input(iof->futex_flags, iof->futex_mask)) >>> return -EINVAL; >>> >>> - iof->futexv_owned = 0; >>> return 0; >>> } >> >> I think you can/should split more into io_futex_prep(), specifically >> waitv should also have zero @val and @mask. > > Yep, I'll include that. Updating them now... It ends up just being this incremental for the very last patch, moving all the waitv related prep to the wait prep and not relying on the non-vectored one at all. diff --git a/io_uring/futex.c b/io_uring/futex.c index 4c9f2c841b98..e885aac12df8 100644 --- a/io_uring/futex.c +++ b/io_uring/futex.c @@ -179,9 +179,6 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr)); iof->futex_val = READ_ONCE(sqe->addr2); iof->futex_mask = READ_ONCE(sqe->addr3); - iof->futex_nr = READ_ONCE(sqe->len); - if (iof->futex_nr && req->opcode != IORING_OP_FUTEX_WAITV) - return -EINVAL; flags = READ_ONCE(sqe->futex_flags); if (flags & ~FUTEX2_MASK) @@ -195,7 +192,6 @@ int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) !futex_validate_input(iof->futex_flags, iof->futex_mask)) return -EINVAL; - iof->futexv_owned = 0; return 0; } @@ -220,10 +216,13 @@ int io_futexv_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) struct futex_vector *futexv; int ret; - ret = io_futex_prep(req, sqe); - if (ret) - return ret; + /* No flags or mask supported for waitv */ + if (unlikely(sqe->fd || sqe->buf_index || sqe->file_index || + sqe->addr2 || sqe->addr3)) + return -EINVAL; + iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr)); + iof->futex_nr = READ_ONCE(sqe->len); if (!iof->futex_nr || iof->futex_nr > FUTEX_WAITV_MAX) return -EINVAL; @@ -238,6 +237,7 @@ int io_futexv_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) return ret; } + iof->futexv_owned = 0; req->flags |= REQ_F_ASYNC_DATA; req->async_data = futexv; return 0;
diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h index f04ce513fadb..a7f03d8d879f 100644 --- a/include/linux/io_uring_types.h +++ b/include/linux/io_uring_types.h @@ -273,6 +273,9 @@ struct io_ring_ctx { struct io_wq_work_list locked_free_list; unsigned int locked_free_nr; + struct hlist_head futex_list; + struct io_alloc_cache futex_cache; + const struct cred *sq_creds; /* cred used for __io_sq_thread() */ struct io_sq_data *sq_data; /* if using sq thread polling */ diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h index 36f9c73082de..3bd2d765f593 100644 --- a/include/uapi/linux/io_uring.h +++ b/include/uapi/linux/io_uring.h @@ -65,6 +65,7 @@ struct io_uring_sqe { __u32 xattr_flags; __u32 msg_ring_flags; __u32 uring_cmd_flags; + __u32 futex_flags; }; __u64 user_data; /* data to be passed back at completion time */ /* pack this to avoid bogus arm OABI complaints */ @@ -235,6 +236,8 @@ enum io_uring_op { IORING_OP_URING_CMD, IORING_OP_SEND_ZC, IORING_OP_SENDMSG_ZC, + IORING_OP_FUTEX_WAIT, + IORING_OP_FUTEX_WAKE, /* this goes last, obviously */ IORING_OP_LAST, diff --git a/io_uring/Makefile b/io_uring/Makefile index 8cc8e5387a75..2e4779bc550c 100644 --- a/io_uring/Makefile +++ b/io_uring/Makefile @@ -7,5 +7,7 @@ obj-$(CONFIG_IO_URING) += io_uring.o xattr.o nop.o fs.o splice.o \ openclose.o uring_cmd.o epoll.o \ statx.o net.o msg_ring.o timeout.o \ sqpoll.o fdinfo.o tctx.o poll.o \ - cancel.o kbuf.o rsrc.o rw.o opdef.o notif.o + cancel.o kbuf.o rsrc.o rw.o opdef.o \ + notif.o obj-$(CONFIG_IO_WQ) += io-wq.o +obj-$(CONFIG_FUTEX) += futex.o diff --git a/io_uring/cancel.c b/io_uring/cancel.c index 7b23607cf4af..3dba8ccb1cd8 100644 --- a/io_uring/cancel.c +++ b/io_uring/cancel.c @@ -15,6 +15,7 @@ #include "tctx.h" #include "poll.h" #include "timeout.h" +#include "futex.h" #include "cancel.h" struct io_cancel { @@ -119,6 +120,10 @@ int io_try_cancel(struct io_uring_task *tctx, struct io_cancel_data *cd, if (ret != -ENOENT) return ret; + ret = io_futex_cancel(ctx, cd, issue_flags); + if (ret != -ENOENT) + return ret; + spin_lock(&ctx->completion_lock); if (!(cd->flags & IORING_ASYNC_CANCEL_FD)) ret = io_timeout_cancel(ctx, cd); diff --git a/io_uring/cancel.h b/io_uring/cancel.h index fc98622e6166..c0a8e7c520b6 100644 --- a/io_uring/cancel.h +++ b/io_uring/cancel.h @@ -1,4 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 +#ifndef IORING_CANCEL_H +#define IORING_CANCEL_H #include <linux/io_uring_types.h> @@ -22,3 +24,5 @@ void init_hash_table(struct io_hash_table *table, unsigned size); int io_sync_cancel(struct io_ring_ctx *ctx, void __user *arg); bool io_cancel_req_match(struct io_kiocb *req, struct io_cancel_data *cd); + +#endif diff --git a/io_uring/futex.c b/io_uring/futex.c new file mode 100644 index 000000000000..0114fda797e1 --- /dev/null +++ b/io_uring/futex.c @@ -0,0 +1,220 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <linux/kernel.h> +#include <linux/errno.h> +#include <linux/fs.h> +#include <linux/file.h> +#include <linux/io_uring.h> + +#include <uapi/linux/io_uring.h> + +#include "../kernel/futex/futex.h" +#include "io_uring.h" +#include "rsrc.h" +#include "futex.h" + +struct io_futex { + struct file *file; + u32 __user *uaddr; + unsigned int futex_val; + unsigned int futex_flags; + unsigned int futex_mask; +}; + +struct io_futex_data { + union { + struct futex_q q; + struct io_cache_entry cache; + }; + struct io_kiocb *req; +}; + +void io_futex_cache_init(struct io_ring_ctx *ctx) +{ + io_alloc_cache_init(&ctx->futex_cache, IO_NODE_ALLOC_CACHE_MAX, + sizeof(struct io_futex_data)); +} + +static void io_futex_cache_entry_free(struct io_cache_entry *entry) +{ + kfree(container_of(entry, struct io_futex_data, cache)); +} + +void io_futex_cache_free(struct io_ring_ctx *ctx) +{ + io_alloc_cache_free(&ctx->futex_cache, io_futex_cache_entry_free); +} + +static void io_futex_complete(struct io_kiocb *req, struct io_tw_state *ts) +{ + struct io_futex_data *ifd = req->async_data; + struct io_ring_ctx *ctx = req->ctx; + + io_tw_lock(ctx, ts); + if (!io_alloc_cache_put(&ctx->futex_cache, &ifd->cache)) + kfree(ifd); + req->async_data = NULL; + hlist_del_init(&req->hash_node); + io_req_task_complete(req, ts); +} + +static bool __io_futex_cancel(struct io_ring_ctx *ctx, struct io_kiocb *req) +{ + struct io_futex_data *ifd = req->async_data; + + /* futex wake already done or in progress */ + if (!futex_unqueue(&ifd->q)) + return false; + + hlist_del_init(&req->hash_node); + io_req_set_res(req, -ECANCELED, 0); + req->io_task_work.func = io_futex_complete; + io_req_task_work_add(req); + return true; +} + +int io_futex_cancel(struct io_ring_ctx *ctx, struct io_cancel_data *cd, + unsigned int issue_flags) +{ + struct hlist_node *tmp; + struct io_kiocb *req; + int nr = 0; + + if (cd->flags & (IORING_ASYNC_CANCEL_FD|IORING_ASYNC_CANCEL_FD_FIXED)) + return -ENOENT; + + io_ring_submit_lock(ctx, issue_flags); + hlist_for_each_entry_safe(req, tmp, &ctx->futex_list, hash_node) { + if (req->cqe.user_data != cd->data && + !(cd->flags & IORING_ASYNC_CANCEL_ANY)) + continue; + if (__io_futex_cancel(ctx, req)) + nr++; + if (!(cd->flags & IORING_ASYNC_CANCEL_ALL)) + break; + } + io_ring_submit_unlock(ctx, issue_flags); + + if (nr) + return nr; + + return -ENOENT; +} + +bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task, + bool cancel_all) +{ + struct hlist_node *tmp; + struct io_kiocb *req; + bool found = false; + + lockdep_assert_held(&ctx->uring_lock); + + hlist_for_each_entry_safe(req, tmp, &ctx->futex_list, hash_node) { + if (!io_match_task_safe(req, task, cancel_all)) + continue; + __io_futex_cancel(ctx, req); + found = true; + } + + return found; +} + +int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) +{ + struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex); + + if (unlikely(sqe->fd || sqe->addr2 || sqe->buf_index || sqe->addr3)) + return -EINVAL; + + iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr)); + iof->futex_val = READ_ONCE(sqe->len); + iof->futex_mask = READ_ONCE(sqe->file_index); + iof->futex_flags = READ_ONCE(sqe->futex_flags); + if (iof->futex_flags & FUTEX_CMD_MASK) + return -EINVAL; + + return 0; +} + +static void io_futex_wake_fn(struct wake_q_head *wake_q, struct futex_q *q) +{ + struct io_futex_data *ifd = container_of(q, struct io_futex_data, q); + struct io_kiocb *req = ifd->req; + + if (unlikely(!__futex_wake_mark(q))) + return; + + io_req_set_res(req, 0, 0); + req->io_task_work.func = io_futex_complete; + io_req_task_work_add(req); +} + +static struct io_futex_data *io_alloc_ifd(struct io_ring_ctx *ctx) +{ + struct io_cache_entry *entry; + + entry = io_alloc_cache_get(&ctx->futex_cache); + if (entry) + return container_of(entry, struct io_futex_data, cache); + + return kmalloc(sizeof(struct io_futex_data), GFP_NOWAIT); +} + +int io_futex_wait(struct io_kiocb *req, unsigned int issue_flags) +{ + struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex); + struct io_ring_ctx *ctx = req->ctx; + struct io_futex_data *ifd = NULL; + struct futex_hash_bucket *hb; + int ret; + + if (!iof->futex_mask) { + ret = -EINVAL; + goto done; + } + + io_ring_submit_lock(ctx, issue_flags); + ifd = io_alloc_ifd(ctx); + if (!ifd) { + ret = -ENOMEM; + goto done_unlock; + } + + req->async_data = ifd; + ifd->q = futex_q_init; + ifd->q.bitset = iof->futex_mask; + ifd->q.wake = io_futex_wake_fn; + ifd->req = req; + + ret = futex_wait_setup(iof->uaddr, iof->futex_val, + futex2_to_flags(iof->futex_flags), &ifd->q, &hb); + if (!ret) { + hlist_add_head(&req->hash_node, &ctx->futex_list); + io_ring_submit_unlock(ctx, issue_flags); + + futex_queue(&ifd->q, hb); + return IOU_ISSUE_SKIP_COMPLETE; + } + +done_unlock: + io_ring_submit_unlock(ctx, issue_flags); +done: + if (ret < 0) + req_set_fail(req); + io_req_set_res(req, ret, 0); + kfree(ifd); + return IOU_OK; +} + +int io_futex_wake(struct io_kiocb *req, unsigned int issue_flags) +{ + struct io_futex *iof = io_kiocb_to_cmd(req, struct io_futex); + int ret; + + ret = futex_wake(iof->uaddr, futex2_to_flags(iof->futex_flags), + iof->futex_val, iof->futex_mask); + if (ret < 0) + req_set_fail(req); + io_req_set_res(req, ret, 0); + return IOU_OK; +} diff --git a/io_uring/futex.h b/io_uring/futex.h new file mode 100644 index 000000000000..ddc9e0d73c52 --- /dev/null +++ b/io_uring/futex.h @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include "cancel.h" + +int io_futex_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe); +int io_futex_wait(struct io_kiocb *req, unsigned int issue_flags); +int io_futex_wake(struct io_kiocb *req, unsigned int issue_flags); + +#if defined(CONFIG_FUTEX) +int io_futex_cancel(struct io_ring_ctx *ctx, struct io_cancel_data *cd, + unsigned int issue_flags); +bool io_futex_remove_all(struct io_ring_ctx *ctx, struct task_struct *task, + bool cancel_all); +void io_futex_cache_init(struct io_ring_ctx *ctx); +void io_futex_cache_free(struct io_ring_ctx *ctx); +#else +static inline int io_futex_cancel(struct io_ring_ctx *ctx, + struct io_cancel_data *cd, + unsigned int issue_flags) +{ + return 0; +} +static inline bool io_futex_remove_all(struct io_ring_ctx *ctx, + struct task_struct *task, bool cancel_all) +{ + return false; +} +static inline void io_futex_cache_init(struct io_ring_ctx *ctx) +{ +} +static inline void io_futex_cache_free(struct io_ring_ctx *ctx) +{ +} +#endif diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c index 135da2fd0eda..e52cbdcb29b8 100644 --- a/io_uring/io_uring.c +++ b/io_uring/io_uring.c @@ -92,6 +92,7 @@ #include "cancel.h" #include "net.h" #include "notif.h" +#include "futex.h" #include "timeout.h" #include "poll.h" @@ -330,6 +331,7 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p) sizeof(struct async_poll)); io_alloc_cache_init(&ctx->netmsg_cache, IO_ALLOC_CACHE_MAX, sizeof(struct io_async_msghdr)); + io_futex_cache_init(ctx); init_completion(&ctx->ref_comp); xa_init_flags(&ctx->personalities, XA_FLAGS_ALLOC1); mutex_init(&ctx->uring_lock); @@ -349,6 +351,7 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p) INIT_LIST_HEAD(&ctx->tctx_list); ctx->submit_state.free_list.next = NULL; INIT_WQ_LIST(&ctx->locked_free_list); + INIT_HLIST_HEAD(&ctx->futex_list); INIT_DELAYED_WORK(&ctx->fallback_work, io_fallback_req_func); INIT_WQ_LIST(&ctx->submit_state.compl_reqs); return ctx; @@ -2869,6 +2872,7 @@ static __cold void io_ring_ctx_free(struct io_ring_ctx *ctx) io_eventfd_unregister(ctx); io_alloc_cache_free(&ctx->apoll_cache, io_apoll_cache_free); io_alloc_cache_free(&ctx->netmsg_cache, io_netmsg_cache_free); + io_futex_cache_free(ctx); io_destroy_buffers(ctx); mutex_unlock(&ctx->uring_lock); if (ctx->sq_creds) @@ -3281,6 +3285,7 @@ static __cold bool io_uring_try_cancel_requests(struct io_ring_ctx *ctx, ret |= io_cancel_defer_files(ctx, task, cancel_all); mutex_lock(&ctx->uring_lock); ret |= io_poll_remove_all(ctx, task, cancel_all); + ret |= io_futex_remove_all(ctx, task, cancel_all); mutex_unlock(&ctx->uring_lock); ret |= io_kill_timeouts(ctx, task, cancel_all); if (task) diff --git a/io_uring/opdef.c b/io_uring/opdef.c index 3b9c6489b8b6..c9f23c21a031 100644 --- a/io_uring/opdef.c +++ b/io_uring/opdef.c @@ -33,6 +33,7 @@ #include "poll.h" #include "cancel.h" #include "rw.h" +#include "futex.h" static int io_no_issue(struct io_kiocb *req, unsigned int issue_flags) { @@ -426,11 +427,26 @@ const struct io_issue_def io_issue_defs[] = { .issue = io_sendmsg_zc, #else .prep = io_eopnotsupp_prep, +#endif + }, + [IORING_OP_FUTEX_WAIT] = { +#if defined(CONFIG_FUTEX) + .prep = io_futex_prep, + .issue = io_futex_wait, +#else + .prep = io_eopnotsupp_prep, +#endif + }, + [IORING_OP_FUTEX_WAKE] = { +#if defined(CONFIG_FUTEX) + .prep = io_futex_prep, + .issue = io_futex_wake, +#else + .prep = io_eopnotsupp_prep, #endif }, }; - const struct io_cold_def io_cold_defs[] = { [IORING_OP_NOP] = { .name = "NOP", @@ -648,6 +664,12 @@ const struct io_cold_def io_cold_defs[] = { .fail = io_sendrecv_fail, #endif }, + [IORING_OP_FUTEX_WAIT] = { + .name = "FUTEX_WAIT", + }, + [IORING_OP_FUTEX_WAKE] = { + .name = "FUTEX_WAKE", + }, }; const char *io_uring_get_opcode(u8 opcode)
Add support for FUTEX_WAKE/WAIT primitives. IORING_OP_FUTEX_WAKE is mix of FUTEX_WAKE and FUTEX_WAKE_BITSET, as it does support passing in a bitset. Similary, IORING_OP_FUTEX_WAIT is a mix of FUTEX_WAIT and FUTEX_WAIT_BITSET. FUTEX_WAKE is straight forward, as those can always be done directly from the io_uring submission without needing async handling. For FUTEX_WAIT, things are a bit more complicated. If the futex isn't ready, then we rely on a callback via futex_queue->wake() when someone wakes up the futex. From that calback, we queue up task_work with the original task, which will post a CQE and wake it, if necessary. Cancelations are supported, both from the application point-of-view, but also to be able to cancel pending waits if the ring exits before all events have occurred. This is just the barebones wait/wake support. PI or REQUEUE support is not added at this point, unclear if we might look into that later. Likewise, explicit timeouts are not supported either. It is expected that users that need timeouts would do so via the usual io_uring mechanism to do that using linked timeouts. Signed-off-by: Jens Axboe <axboe@kernel.dk> --- include/linux/io_uring_types.h | 3 + include/uapi/linux/io_uring.h | 3 + io_uring/Makefile | 4 +- io_uring/cancel.c | 5 + io_uring/cancel.h | 4 + io_uring/futex.c | 220 +++++++++++++++++++++++++++++++++ io_uring/futex.h | 34 +++++ io_uring/io_uring.c | 5 + io_uring/opdef.c | 24 +++- 9 files changed, 300 insertions(+), 2 deletions(-) create mode 100644 io_uring/futex.c create mode 100644 io_uring/futex.h