Skip to content

Commit 5d04f81

Browse files
committed
fusedev: free uring entries only after the kernel gives them back
A uring worker freed its ring entries as soon as it stopped. When it stopped on an error while the connection was still up, such as a failed command or an interrupted io_uring_enter(), its other entries were still registered, and the kernel could copy the next request into freed memory. Closing the ring doesn't help in time, because the kernel cancels the commands later, from a workqueue. The worker now counts the entries whose command the kernel hasn't completed, and waits for those completions before it frees them. Once the connection is torn down they arrive at once. If they don't arrive within a second, the worker leaks the entries instead of freeing memory the kernel may still write to. A signal that interrupts the worker's wait no longer stops the worker. The new mount test serves a PassthroughFs over io_uring and checks that dropping the serving layer still returns well within that second. It can't reproduce the race itself. Signed-off-by: Ilan Joselevich <personal@ilanjoselevich.com>
1 parent d4c3cd1 commit 5d04f81

2 files changed

Lines changed: 137 additions & 10 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,10 @@
7070
and no code in the workspace relied on it.
7171

7272
### Fixed
73+
- A `UringFuseServing` worker that stops frees its ring entries only after the kernel has given
74+
them all back, and leaks them if that takes more than a second. A worker that stopped on an
75+
error while the connection was up freed entries the kernel could still copy requests into. A
76+
signal that interrupts the worker's wait no longer stops it.
7377
- [278](https://github.com/cloud-hypervisor/fuse-backend-rs/pull/278): Treat `EOPNOTSUPP`/`ENOTSUP` and `ENOSYS` from `getxattr` as "not opaque"
7478
in `Layer::is_opaque()`, so `OverlayFs::import()` no longer aborts when a layer's
7579
backing filesystem has no xattr support at all (tmpfs answers `EOPNOTSUPP` to

‎crates/fuse-backend-fusedev/src/uring_session.rs‎

Lines changed: 133 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ use std::sync::atomic::{AtomicBool, Ordering};
2424
use std::sync::mpsc::{channel, Sender};
2525
use std::sync::Arc;
2626
use std::thread;
27+
use std::time::{Duration, Instant};
2728

2829
use io_uring::{cqueue, opcode, squeue, types, IoUring};
2930
use vm_memory::ByteValued;
@@ -580,7 +581,8 @@ impl<F: FileSystem + Send + Sync + 'static> UringWorker<F> {
580581
let mut sq = ring.submission();
581582
for (idx, entry) in entries.iter().enumerate() {
582583
let sqe = entry.cmd_sqe(fd, FUSE_IO_URING_CMD_REGISTER, 0, idx as u64);
583-
// Safe because entries live for the whole lifetime of the loop.
584+
// Safe because release_entries() keeps the entries alive
585+
// while the kernel holds them.
584586
unsafe { sq.push(&sqe) }
585587
.map_err(|e| io::Error::other(format!("submission queue full: {e}")))?;
586588
}
@@ -593,9 +595,16 @@ impl<F: FileSystem + Send + Sync + 'static> UringWorker<F> {
593595
Err(e) => Err(io::Error::other(e.to_string())),
594596
});
595597
registered?;
596-
597-
loop {
598-
ring.submit_and_wait(1)?;
598+
// The number of entries whose command the kernel hasn't completed.
599+
let mut armed = entries.len();
600+
601+
let result = 'serve: loop {
602+
match ring.submit_and_wait(1) {
603+
Ok(_) => {}
604+
// A signal interrupted the wait, which is no reason to stop.
605+
Err(e) if e.raw_os_error() == Some(libc::EINTR) => continue,
606+
Err(e) => break 'serve Err(e),
607+
}
599608

600609
// Drain the completion queue first so that its borrow is
601610
// released before entries are processed and commits submitted.
@@ -606,6 +615,11 @@ impl<F: FileSystem + Send + Sync + 'static> UringWorker<F> {
606615
if completed.is_empty() {
607616
continue;
608617
}
618+
let returned = completed
619+
.iter()
620+
.filter(|(idx, _)| *idx < entries.len())
621+
.count();
622+
armed = armed.saturating_sub(returned);
609623

610624
for (idx, res) in completed {
611625
if idx >= entries.len() {
@@ -615,11 +629,11 @@ impl<F: FileSystem + Send + Sync + 'static> UringWorker<F> {
615629
let entry = &mut entries[idx];
616630
if res < 0 {
617631
if exit.load(Ordering::Relaxed) {
618-
return Ok(());
632+
break 'serve Ok(());
619633
}
620634
// Registration/commit failures leave the entry in an
621635
// undefined state; stop serving rather than guessing.
622-
return Err(io::Error::from_raw_os_error(-res));
636+
break 'serve Err(io::Error::from_raw_os_error(-res));
623637
}
624638

625639
if let Err(e) = self.process(entry) {
@@ -637,7 +651,8 @@ impl<F: FileSystem + Send + Sync + 'static> UringWorker<F> {
637651
loop {
638652
let pushed = {
639653
let mut sq = ring.submission();
640-
// Safe because the entry outlives the ring usage.
654+
// Safe because release_entries() keeps the entries
655+
// alive while the kernel holds them.
641656
unsafe { sq.push(&sqe) }.is_ok()
642657
};
643658
if pushed {
@@ -646,11 +661,20 @@ impl<F: FileSystem + Send + Sync + 'static> UringWorker<F> {
646661
// The submission queue can never hold more outstanding
647662
// commits than completed entries, so submitting must
648663
// eventually make room.
649-
ring.submit()?;
664+
if let Err(e) = ring.submit() {
665+
break 'serve Err(e);
666+
}
650667
}
668+
// Count the commit once it is queued, because
669+
// release_entries() submits anything still queued.
670+
armed += 1;
651671
}
652-
ring.submit()?;
653-
}
672+
if let Err(e) = ring.submit() {
673+
break 'serve Err(e);
674+
}
675+
};
676+
release_entries(ring, entries, armed);
677+
result
654678
}
655679

656680
/// Handle one delivered request: reassemble the header/args in the
@@ -779,6 +803,56 @@ impl<F: FileSystem + Send + Sync + 'static> UringWorker<F> {
779803
}
780804
}
781805

806+
/// How long a stopping worker waits for the kernel to give back the ring
807+
/// entries it still holds.
808+
const RELEASE_TIMEOUT: Duration = Duration::from_secs(1);
809+
810+
/// Drop `ring` and `entries` once the kernel holds none of the entries.
811+
///
812+
/// The kernel may copy a request into an entry until it completes the
813+
/// entry's command, and closing the ring only cancels the commands later,
814+
/// from a workqueue. So wait for the `armed` outstanding completions first,
815+
/// which arrive quickly once the connection is torn down. If they don't
816+
/// arrive within `RELEASE_TIMEOUT`, leak the entries instead of freeing
817+
/// memory the kernel may still write to.
818+
fn release_entries(
819+
mut ring: IoUring<squeue::Entry128, cqueue::Entry>,
820+
entries: Vec<UringEntry>,
821+
mut armed: usize,
822+
) {
823+
let deadline = Instant::now() + RELEASE_TIMEOUT;
824+
while armed > 0 {
825+
let left = deadline.saturating_duration_since(Instant::now());
826+
if left.is_zero() {
827+
break;
828+
}
829+
let ts = types::Timespec::new()
830+
.sec(left.as_secs())
831+
.nsec(left.subsec_nanos());
832+
let args = types::SubmitArgs::new().timespec(&ts);
833+
match ring.submitter().submit_with_args(1, &args) {
834+
Ok(_) => {}
835+
Err(e)
836+
if matches!(
837+
e.raw_os_error(),
838+
Some(libc::ETIME | libc::EINTR | libc::EBUSY | libc::EAGAIN)
839+
) => {}
840+
Err(_) => break,
841+
}
842+
let len = entries.len();
843+
armed = armed.saturating_sub(
844+
ring.completion()
845+
.filter(|cqe| (cqe.user_data() as usize) < len)
846+
.count(),
847+
);
848+
}
849+
if armed > 0 {
850+
warn!("uring worker: leaking {armed} ring entries that the kernel still holds");
851+
std::mem::forget(entries);
852+
}
853+
drop(ring);
854+
}
855+
782856
/// Serve FUSE requests over io_uring (experimental).
783857
///
784858
/// The serving layer takes ownership of the mounted session: dropping it
@@ -1463,4 +1537,53 @@ mod tests {
14631537
assert_eq!(w.available_bytes(), offset);
14641538
assert_eq!(other.available_bytes(), 256 - 100);
14651539
}
1540+
1541+
/// Serve a PassthroughFs over io_uring, read and write through the
1542+
/// mount, then check that dropping the serving layer gets the ring
1543+
/// entries back from the kernel and unmounts.
1544+
#[test]
1545+
fn test_uring_serving_mount() {
1546+
let src = TempDir::new().unwrap();
1547+
std::fs::write(src.as_path().join("hello"), b"over io_uring").unwrap();
1548+
let mnt = TempDir::new().unwrap();
1549+
let fs = PassthroughFs::<()>::new(Config {
1550+
root_dir: src.as_path().to_string_lossy().to_string(),
1551+
do_import: false,
1552+
..Default::default()
1553+
})
1554+
.unwrap();
1555+
fs.import().unwrap();
1556+
let server = Arc::new(Server::new(fs));
1557+
server.set_uring(true);
1558+
let mut se = FuseSession::new(mnt.as_path(), "uring_serving_test", "", false).unwrap();
1559+
se.mount().unwrap();
1560+
let cfg = UringConfig {
1561+
workers: 2,
1562+
entries_per_queue: 2,
1563+
};
1564+
let serving = match UringFuseServing::new(se, server, cfg) {
1565+
Ok(serving) => serving,
1566+
Err(UringNotSupported) => {
1567+
eprintln!("FUSE-over-io_uring is not available, skipping");
1568+
return;
1569+
}
1570+
Err(e) => panic!("uring serving failed: {}", e),
1571+
};
1572+
1573+
assert_eq!(
1574+
std::fs::read(mnt.as_path().join("hello")).unwrap(),
1575+
b"over io_uring"
1576+
);
1577+
let created = mnt.as_path().join("created");
1578+
std::fs::write(&created, b"written over io_uring").unwrap();
1579+
assert_eq!(std::fs::read(&created).unwrap(), b"written over io_uring");
1580+
1581+
// Tearing the connection down completes the commands of all armed
1582+
// entries, so the workers get their entries back well within
1583+
// RELEASE_TIMEOUT instead of waiting it out and leaking them.
1584+
let start = Instant::now();
1585+
drop(serving);
1586+
assert!(start.elapsed() < RELEASE_TIMEOUT);
1587+
assert!(std::fs::read(mnt.as_path().join("hello")).is_err());
1588+
}
14661589
}

0 commit comments

Comments
 (0)