[blasrodri]

Motivation

In the io_uring backend, register_op pushes an SQE onto the submission queue
and then calls submit(). If submit() returns any error other than EBUSY,
the op is removed from the slab, which drops the operation and frees the buffer
the SQE points at.

This is unsound. Once the SQE is pushed into the submission queue, the kernel
may have already accepted it or will consume it on a subsequent submit(). In
that case the kernel can still write into the buffer we just freed
(use-after-free), and the eventual CQE refers to a slab slot that no longer
exists, which panics the driver or, if the slot has been reused, completes an
unrelated op.

Solution

Once the SQE has been pushed, cancel the op if submit() fails so the buffer
and fd stay alive in CancelData until the completion is observed, and the
error is still propagated to the caller. The pre-push submit (flushing a full
queue) is unchanged, since there the entry has not reached the submission ring
and removing it is correct.

Fixes: #8255

[martin-g]

Tokio does not use unit tests.
You should move this file (with a better name) to tokio/tests/...

[blasrodri]

The tricky part is that this test has to force a submit failure after the SQE is pushed, then check the slab — and register_op, the slab, and the ring fd are all pub(crate), so a test in tokio/tests/ can't reach them.

I could expose a test-util hook like time::pause does, but it's messier here: time is already a public module, whereas runtime::io isn't, so I'd have to open up a public path through runtime just for this.

Felt like more plumbing than the test is worth, especially since mod tests; is already used in a few places (runtime, time, sync, fs::file).

I added tokio/tests/fs_uring_cancel_read.rs for the cancel path that's reachable from the public API. Happy to do the test-util version if you'd prefer it though.

[mattiapitossi]

As mentioned by Martin, tokio tries to avoid unit tests, so I would remove this file.

Also, could you clarify what fs_uring_cancel_read is intended to cover here? From what I see, it doesn't seem to find any leak in the current implementation.

[blasrodri]

You're right about fs_uring_cancel_read: it cancels with handle.abort(), which goes through the normal State::Polled → Op::drop → cancel_op path. That path is unchanged by this PR, so the test doesn't cover the post-push failure this fixes.

The only way to trigger that branch is forcing a submit failure after the SQE is pushed, which needs crate-internal access (the BrokenRing fd trick) — so it can't be done from an integration test without either a unit test or fault-injection scaffolding in the driver.

Rather than add either, the fix routes the failure through the existing cancel_op/reclaim path, which is already exercised by the cancel tests.

[martin-g]

The slot is released once the completion is observed.

Could you please point me to the code where that would happen ?

I think we will always leak the op now.

1. register_op() fails and leads to state = Complete
2. Because the future is now complete, it is dropped, and cancel_op is never called.
3. Since the slot was not removed from ctx.ops during register_op, it remains in the slab forever (either as Waiting or Completed once a completion event is dispatched), resulting in a permanent memory leak of the slab slot.

[blasrodri]

You're right, that was leaking.

Keeping the slot as Waiting and returning the error meant the future went straight to State::Complete, so Drop never called cancel_op and nobody ever cleaned up the slot. Reworked it.

register_op now returns a RegisterResult so the caller knows where the submit failed. If it failed before the entry reached the kernel, we remove the slot and give the resources back, since the kernel never saw it. If it failed after the entry was pushed, it returns Pushed { index, error } and
keeps the slot.

In the Pushed case Op::poll calls cancel_op(index, data) before completing, which moves the slot to Lifecycle::Cancelled and parks the buffer/fd in CancelData. After that it's the same as a normally cancelled op: dispatch_completions sees the Cancelled state on the CQE, closes the fd if it was
an Open, and removes it from the slab. And if no completion ever shows up, Drop for UringContext drains what's left at shutdown and removes them.

So the slot goes away once the completion is observed (or at driver drop), and the buffer stays alive the whole time. No use-after-free and no leaked slot.

[mattiapitossi]

So, if I got this correctly this now means that for every error we are receiving from io_uring_enter (apart from EBUSY, because we checked it before) we don't remove entries from the slab? If so, I'm not totally sure this is the right approach. The Kernel could see the submission when interrupted but for most of errors this is not the case

[blasrodri]

You're right, thanks — keeping the slot on every error was too broad. I've narrowed it.

Looking at the man page, the only case where the kernel might have consumed our entry is EINTR (a signal interrupts io_uring_enter mid-submit). The other errnos io_uring_enter returns directly — EBADF, EINVAL, ENXIO, etc. — are whole-call failures about the ring/flags, not about a queued entry; per-SQE validation errors come back through a CQE, not the syscall. So for those the entry wasn't consumed and we can drop the slot and hand the resources back.

The reason we can't just check the consumed-count is that submit() discards it (returns io::Result<()>), so on EINTR we genuinely can't tell whether our entry made it. Keep-and-cancel is the safe choice there.

Updated so only EINTR returns Pushed; everything else is NotPushed now.

[Darksonn]

Even when io_uring_enter fails before the kernel consumes the SQE, submission().push(&entry) has already added the entry to the user-space submission ring (and io-uring has no way to un-push it). That means the entry stays in the ring and will still be submitted to the kernel on the next submit() call, so we have to keep the slab entry alive on all errors after push, not just EINTR.

[Darksonn]

Since this PR has been dormant for a while (sorry!), I took the liberty of rebasing it and making a minor tweak. I think the fix is correct now. Thanks a lot for the PR!