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;
--
Jens Axboe
