Re: [RFC PATCH 2/2] io_uring/futex: add persistent waits and registered wakes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] io_uring/futex: NULL deref in io_futex_node_get() on !CONFIG_MMU
- [High] io_uring/futex: busy-spin on teardown from uncleared futex_pending
- [Medium] io_uring/futex: 32-bit truncation of SQE fields in io_futex_prep()
--

commit d39ae03333a1ccb677abc6626226b97d34d99fdf
Author: Austin Schuh <austin.linux@gmail.com>

io_uring/futex: add persistent waits and registered wakes

This commit adds persistent futex waits and registered futex wakes to
io_uring to avoid futex hash bucket lock contention on PREEMPT_RT. It
introduces registration opcodes for futex words and uses an RCU-walked node
table and per-ring pending bitmaps to notify waiting rings without taking
sleeping locks in the wake path.

> diff --git a/io_uring/futex.c b/io_uring/futex.c
> index 9203998654ffb..9f5050bfd3d44 100644
> --- a/io_uring/futex.c
> +++ b/io_uring/futex.c

[ ... ]

> +/*
> + * A key without FUT_OFF_INODE names its mm by pointer.  The node keeps the
> + * mm_struct allocated so the pointer can't be reused for another mm while
> + * the node can still be found by it.
> + */
> +static bool io_futex_key_has_mm(union futex_key *key)
> +{
> +	return !(key->both.offset & FUT_OFF_INODE);
> +}
> +
> +static struct io_futex_node *io_futex_node_get(u32 __user *uaddr,
> +					       unsigned int flags)
> +{
> +	union futex_key key = FUTEX_KEY_INIT;
> +	struct io_futex_node *node, *new = NULL;
> +	struct io_futex_bucket *b;
> +	int ret;
> +
> +	/* Write access, as a waker needs: reading the word is not enough. */
> +	ret = get_futex_key(uaddr, flags, &key, FUTEX_WRITE);
> +	if (ret)
> +		return ERR_PTR(ret);
> +	b = io_futex_bucket(&key);
> +again:
> +	spin_lock(&b->lock);
> +	hlist_for_each_entry(node, &b->nodes, hnode) {
> +		if (futex_match(&node->key, &key)) {
> +			/* A node at zero has already left the bucket. */
> +			refcount_inc(&node->ref);
> +			spin_unlock(&b->lock);
> +			kfree(new);
> +			return node;
> +		}
> +	}
> +	if (!new) {
> +		/* Allocate only on a miss, and not under the lock. */
> +		spin_unlock(&b->lock);
> +		new = kzalloc_obj(*new, GFP_KERNEL_ACCOUNT);
> +		if (!new)
> +			return ERR_PTR(-ENOMEM);
> +		goto again;
> +	}
> +	new->key = key;
> +	new->bucket = b;
> +	INIT_HLIST_HEAD(&new->members);
> +	refcount_set(&new->ref, 1);
> +	if (io_futex_key_has_mm(&key))
> +		mmgrab(key.private.mm);
> +	hlist_add_head(&new->hnode, &b->nodes);
> +	spin_unlock(&b->lock);
> +	return new;
> +}
> +
> +static void io_futex_node_put(struct io_futex_node *node)
> +{
> +	if (!refcount_dec_and_lock(&node->ref, &node->bucket->lock))
> +		return;
> +	hlist_del(&node->hnode);
> +	spin_unlock(&node->bucket->lock);
> +	if (io_futex_key_has_mm(&node->key))
> +		mmdrop(node->key.private.mm);

[Severity: High]
Can this trigger a NULL pointer dereference in io_futex_node_get() and
io_futex_node_put() on !CONFIG_MMU kernels?

On !CONFIG_MMU builds, FLAGS_SHARED is defined as 0x0000 in
kernel/futex/futex.h, so get_futex_key() in kernel/futex/core.c always takes
the !fshared path, leaves FUT_OFF_INODE unset in key->both.offset, and sets
key->private.mm to NULL:

kernel/futex/core.c:get_futex_key() {
	if (!fshared) {
		if (IS_ENABLED(CONFIG_MMU))
			key->private.mm = mm;
		else
			key->private.mm = NULL;

		key->private.address = address;
		return 0;
	}
}

Because io_futex_key_has_mm() only checks !(key->both.offset & FUT_OFF_INODE),
it returns true when CONFIG_MMU is disabled, causing io_futex_node_get() to
call mmgrab(NULL) and io_futex_node_put() to call mmdrop(NULL). Should
io_futex_key_has_mm() also check key->private.mm != NULL (or
IS_ENABLED(CONFIG_MMU))?

> +	/* A registered wake may still be walking it. */
> +	kfree_rcu(node, rcu);
> +}

[ ... ]

> @@ -127,23 +847,69 @@ bool io_futex_remove_all(struct io_ring_ctx *ctx, struct io_uring_task *tctx,
>  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;
> +	u32 flags, uflags;
>  
> -	if (unlikely(sqe->len || sqe->futex_flags || sqe->buf_index ||
> -		     sqe->file_index))
> +	if (unlikely(sqe->len))
>  		return -EINVAL;
>  
> +	uflags = READ_ONCE(sqe->futex_flags);
> +	iof->iou_flags = uflags;
> +	iof->member = NULL;
>  	iof->uaddr = u64_to_user_ptr(READ_ONCE(sqe->addr));
>  	iof->futex_val = READ_ONCE(sqe->addr2);
>  	iof->futex_mask = READ_ONCE(sqe->addr3);
>  	flags = READ_ONCE(sqe->fd);
>  
> +	if (req->opcode == IORING_OP_FUTEX_WAIT) {
> +		if (uflags & ~(IORING_FUTEX_PERSISTENT | IORING_FUTEX_REARM) ||
> +		    uflags == (IORING_FUTEX_PERSISTENT | IORING_FUTEX_REARM) ||
> +		    sqe->buf_index)
> +			return -EINVAL;
> +		/*
> +		 * A persistent wait's slot, as a direct descriptor's: index + 1,
> +		 * or IORING_FILE_INDEX_ALLOC.  A re-arm names the slot the same
> +		 * way, as a close does.
> +		 */
> +		iof->futex_nr = READ_ONCE(sqe->file_index);
> +		if (!(uflags & (IORING_FUTEX_PERSISTENT | IORING_FUTEX_REARM)) !=
> +		    !iof->futex_nr)
> +			return -EINVAL;
> +		if (uflags & IORING_FUTEX_REARM) {
> +			if (iof->uaddr || iof->futex_val || iof->futex_mask ||
> +			    flags || iof->futex_nr == IORING_FILE_INDEX_ALLOC)
> +				return -EINVAL;
> +			iof->futex_nr--;
> +			return 0;
> +		}
> +	} else if (sqe->file_index) {
> +		return -EINVAL;
> +	} else if (uflags) {
> +		/* Woken by slot: no word, no count, only the bitset. */
> +		if (uflags != IORING_FUTEX_REGISTERED || iof->uaddr ||
> +		    iof->futex_val || flags || !iof->futex_mask ||
> +		    iof->futex_mask > U32_MAX)
> +			return -EINVAL;

[Severity: Medium]
On 32-bit architectures, struct io_futex defines uaddr as void __user * and
futex_val and futex_mask as unsigned long:

struct io_futex {
	struct file	*file;
	void __user	*uaddr;
	unsigned long	futex_val;
	unsigned long	futex_mask;
	...
};

When io_futex_prep() reads the 64-bit sqe->addr, sqe->addr2, and sqe->addr3
fields into iof->uaddr, iof->futex_val, and iof->futex_mask, doesn't that
truncate the upper 32 bits on 32-bit kernels before the validation checks
run?

In addition, on 32-bit architectures where unsigned long is 32 bits wide,
won't the check iof->futex_mask > U32_MAX in io_futex_prep() always be false
and trigger a compiler tautological-compare warning while failing to reject
non-zero upper 32 bits in sqe->addr3?

[ ... ]

> diff --git a/io_uring/tw.h b/io_uring/tw.h
> index 3ade5ad577fda..dc9125ebdd829 100644
> --- a/io_uring/tw.h
> +++ b/io_uring/tw.h

[ ... ]

> @@ -90,7 +91,8 @@ static inline int io_run_task_work(void)
>  
>  static inline bool io_local_work_pending(struct io_ring_ctx *ctx)
>  {
> -	return !mpscq_empty(&ctx->work_list);
> +	return !mpscq_empty(&ctx->work_list) ||
> +	       atomic_read(&ctx->futex_pending);
>  }

[Severity: High]
Could including ctx->futex_pending in io_local_work_pending() cause a busy-spin
in io_ring_ctx_wait_inflight() during ring teardown?

ctx->futex_pending is only cleared in __io_run_local_work(), which requires
ctx->submitter_task == current. During ring teardown, io_ring_ctx_cancel()
calls io_cancel_local_task_work(), which drains ctx->work_list under
uring_lock but does not clear ctx->futex_pending:

io_uring/tw.c:io_cancel_local_task_work() {
	guard(mutex)(&ctx->uring_lock);

	while (!mpscq_empty(&ctx->work_list)) {
		...
	}
	io_submit_flush_completions(ctx);
}

If the ring fd is closed from a task other than ctx->submitter_task (or a
concurrent waker calls io_ring_futex_kick() during teardown) while an
uncancelable request such as storage I/O is still in flight,
io_ring_ctx_wait_inflight() sees io_local_work_pending(ctx) remain true on
every iteration:

io_uring/io_uring.c:io_ring_ctx_wait_inflight() {
		/* don't sleep on work that's already there and that we can run */
		if (ran_own && ((tctx && !mpscq_empty(&tctx->task_list)) ||
		    io_local_work_pending(ctx)))
			__set_current_state(TASK_RUNNING);
		else
			schedule_timeout(1);
}

Wouldn't that skip schedule_timeout(1) and spin in a tight loop until all
in-flight requests complete unless io_cancel_local_task_work() also clears
ctx->futex_pending?

>  
>  static inline bool io_task_work_pending(struct io_ring_ctx *ctx)

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022024701.1574811-1-austin.linux@gmail.com?part=2