repository: Atomically replace refs/ symlinks

Naming a new version of an image the same as the old version is pretty
standard, and this is currently giving EEXIST errors. We fix this by
doing an atomic symlink + rename operation to replace the symlink if it
doesn't already have the expected value.

Drop our existing Repository::ensure_symlink() function and use the new
function for that as well.

Co-authored-by: Alexander Larsson <alexl@redhat.com>
Signed-off-by: Alexander Larsson <alexl@redhat.com>
Signed-off-by: Allison Karlitskaya <allison.karlitskaya@redhat.com>

Author: allisonkarlitskaya

crates/composefs/Cargo.toml

@@ -29,6 +29,7 @@ tempfile = { version = "3.8.0", optional = true, default-features = false }
 xxhash-rust = { version = "0.8.2", default-features = false, features = ["xxh32"] }
 zerocopy = { version = "0.8.0", default-features = false, features = ["derive", "std"] }
 zstd = { version = "0.13.0", default-features = false }
+rand = { version = "0.9.1", default-features = true }
 
 [dev-dependencies]
 insta = "1.42.2"

crates/composefs/src/repository.rs

@@ -12,8 +12,8 @@ use anyhow::{bail, ensure, Context, Result};
 use once_cell::sync::OnceCell;
 use rustix::{
     fs::{
-        fdatasync, flock, linkat, mkdirat, open, openat, readlinkat, symlinkat, AtFlags, Dir,
-        FileType, FlockOperation, Mode, OFlags, CWD,
+        fdatasync, flock, linkat, mkdirat, open, openat, readlinkat, AtFlags, Dir, FileType,
+        FlockOperation, Mode, OFlags, CWD,
     },
     io::{Errno, Result as ErrnoResult},
 };
@@ -25,7 +25,7 @@ use crate::{
     },
     mount::mount_composefs_at,
     splitstream::{DigestMap, SplitStreamReader, SplitStreamWriter},
-    util::{filter_errno, proc_self_fd, Sha256Digest},
+    util::{filter_errno, proc_self_fd, replace_symlinkat, Sha256Digest},
 };
 
 /// Call openat() on the named subdirectory of "dirfd", possibly creating it first.
@@ -261,7 +261,7 @@ impl<ObjectID: FsVerityHashValue> Repository<ObjectID> {
         let stream_path = format!("streams/{}", hex::encode(sha256));
         let object_id = writer.done()?;
         let object_path = Self::format_object_path(&object_id);
-        self.ensure_symlink(&stream_path, &object_path)?;
+        self.symlink(&stream_path, &object_path)?;
 
         if let Some(name) = reference {
             let reference_path = format!("streams/refs/{name}");
@@ -310,7 +310,7 @@ impl<ObjectID: FsVerityHashValue> Repository<ObjectID> {
                 let object_id = writer.done()?;
 
                 let object_path = Self::format_object_path(&object_id);
-                self.ensure_symlink(&stream_path, &object_path)?;
+                self.symlink(&stream_path, &object_path)?;
                 object_id
             }
         };
@@ -366,7 +366,7 @@ impl<ObjectID: FsVerityHashValue> Repository<ObjectID> {
         let object_path = Self::format_object_path(&object_id);
         let image_path = format!("images/{}", object_id.to_hex());
 
-        self.ensure_symlink(&image_path, &object_path)?;
+        self.symlink(&image_path, &object_path)?;
 
         if let Some(reference) = name {
             let ref_path = format!("images/refs/{reference}");
@@ -432,14 +432,8 @@ impl<ObjectID: FsVerityHashValue> Repository<ObjectID> {
             relative.push(target_component);
         }
 
-        symlinkat(relative, &self.repository, name)
-    }
-
-    pub fn ensure_symlink<P: AsRef<Path>>(&self, name: P, target: &str) -> ErrnoResult<()> {
-        self.symlink(name, target).or_else(|e| match e {
-            Errno::EXIST => Ok(()),
-            _ => Err(e),
-        })
+        // Atomically replace existing symlink
+        replace_symlinkat(&relative, &self.repository, name)
     }
 
     fn read_symlink_hashvalue(dirfd: &OwnedFd, name: &CStr) -> Result<ObjectID> {

crates/composefs/src/util.rs

@@ -1,9 +1,17 @@
+use rand::{distr::Alphanumeric, Rng};
 use std::{
     io::{Error, ErrorKind, Read, Result},
-    os::fd::{AsFd, AsRawFd},
+    os::{
+        fd::{AsFd, AsRawFd, OwnedFd},
+        unix::ffi::OsStrExt,
+    },
+    path::Path,
 };
 
-use rustix::io::{Errno, Result as ErrnoResult};
+use rustix::{
+    fs::{readlinkat, renameat, symlinkat, unlinkat, AtFlags},
+    io::{Errno, Result as ErrnoResult},
+};
 use tokio::io::{AsyncRead, AsyncReadExt};
 
 /// Formats a string like "/proc/self/fd/3" for the given fd.  This can be used to work with kernel
@@ -109,6 +117,55 @@ pub(crate) fn filter_errno<T>(
     }
 }
 
+fn generate_tmpname(prefix: &str) -> String {
+    let rand_string: String = rand::rng()
+        .sample_iter(&Alphanumeric)
+        .take(12)
+        .map(char::from)
+        .collect();
+    format!("{}{}", prefix, rand_string)
+}
+
+pub(crate) fn replace_symlinkat(
+    target: impl AsRef<Path>,
+    dirfd: &OwnedFd,
+    name: impl AsRef<Path>,
+) -> ErrnoResult<()> {
+    let name = name.as_ref();
+    let target = target.as_ref();
+
+    // Step 1: try to create the symlink
+    if filter_errno(symlinkat(target, dirfd, name), Errno::EXIST)?.is_some() {
+        return Ok(());
+    };
+
+    // Step 2: the symlink already exists.  Maybe it already has the correct target?
+    if let Some(current_target) = filter_errno(readlinkat(dirfd, name, []), Errno::NOENT)? {
+        if current_target.into_bytes() == target.as_os_str().as_bytes() {
+            return Ok(());
+        }
+    }
+
+    // Step 3: full atomic replace path
+    for _ in 0..16 {
+        let tmp_name = generate_tmpname(".symlink-");
+        if filter_errno(symlinkat(target, dirfd, &tmp_name), Errno::EXIST)?.is_none() {
+            // This temporary filename already exists, try another
+            continue;
+        }
+
+        match renameat(dirfd, &tmp_name, dirfd, name) {
+            Ok(_) => return Ok(()),
+            Err(e) => {
+                let _ = unlinkat(dirfd, tmp_name, AtFlags::empty());
+                return Err(e);
+            }
+        }
+    }
+
+    Err(Errno::EXIST)
+}
+
 #[cfg(test)]
 mod test {
     use similar_asserts::assert_eq;