[PATCH proxmox-backup 9/9] fix #7244: datastore: always mmap index files from offset 0

[Robert Obkircher <r.obkircher@proxmox.com>]

Always memory-map index files from the start, to allow running on
systems where the page size is larger than 4 kB. Previously, such
systems received an error because the offset needs to be a multiple of
the page size.

Since the header is now also mapped into memory, it is read that way
as well. This avoids the seek and read, but it may slightly increase
the risk of encountering SIGBUS on the first access.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/dynamic_index.rs |  63 +++--------
 pbs-datastore/src/fixed_index.rs   | 102 ++++-------------
 pbs-datastore/src/index_mmap.rs    | 171 +++++++++++++++++++++++++++++
 pbs-datastore/src/lib.rs           |   1 +
 4 files changed, 207 insertions(+), 130 deletions(-)
 create mode 100644 pbs-datastore/src/index_mmap.rs

diff --git a/pbs-datastore/src/dynamic_index.rs b/pbs-datastore/src/dynamic_index.rs
index c03ab6c29..0f609a635 100644
--- a/pbs-datastore/src/dynamic_index.rs
+++ b/pbs-datastore/src/dynamic_index.rs
@@ -1,17 +1,13 @@
 use std::fs::File;
 use std::io::{BufWriter, Seek, SeekFrom, Write};
 use std::ops::Range;
-use std::os::unix::io::AsRawFd;
 use std::path::{Path, PathBuf};
 use std::pin::Pin;
 use std::sync::{Arc, Mutex};
 use std::task::Context;
 
 use anyhow::{Error, bail, format_err};
-use nix::sys::stat::{SFlag, fstat};
 
-use proxmox_io::ReadExt;
-use proxmox_sys::mmap::Mmap;
 use proxmox_uuid::Uuid;
 use pxar::accessor::{MaybeReady, ReadAt, ReadAtOperation};
 
@@ -19,6 +15,7 @@ use pbs_tools::lru_cache::LruCache;
 
 use crate::file_formats;
 use crate::index::{ChunkReadInfo, IndexFile};
+use crate::index_mmap::IndexMmap;
 use crate::read_chunk::ReadChunk;
 
 /// Header format definition for dynamic index files (`.dixd`)
@@ -76,9 +73,8 @@ impl DynamicEntry {
 }
 
 pub struct DynamicIndexReader {
+    mmap: IndexMmap<DynamicIndexHeader, DynamicEntry>,
     _file: File,
-    pub size: usize,
-    index: Mmap<DynamicEntry>,
     pub uuid: [u8; 16],
     pub ctime: i64,
     pub index_csum: [u8; 32],
@@ -93,55 +89,28 @@ impl DynamicIndexReader {
     }
 
     pub fn index(&self) -> &[DynamicEntry] {
-        &self.index
+        self.mmap.index()
     }
 
-    pub fn new(mut file: std::fs::File) -> Result<Self, Error> {
-        let stat = fstat(file.as_raw_fd()).map_err(|e| format_err!("fstat failed - {e}"))?;
-        if (stat.st_mode & SFlag::S_IFMT.bits()) != SFlag::S_IFREG.bits() {
-            bail!("not a regular file");
-        }
-
-        // FIXME: This is NOT OUR job! Check the callers of this method and remove this!
-        file.seek(SeekFrom::Start(0))?;
-
-        let header_size = std::mem::size_of::<DynamicIndexHeader>();
-
-        let size = stat.st_size as usize;
+    pub fn new(file: File) -> Result<Self, Error> {
+        let mmap = unsafe { IndexMmap::map_read(&file)? };
 
-        if size < header_size {
-            bail!("index too small ({})", stat.st_size);
-        }
-
-        let header: Box<DynamicIndexHeader> = unsafe { file.read_host_value_boxed()? };
+        let header: &DynamicIndexHeader = mmap.header();
 
         if header.magic != file_formats::DYNAMIC_SIZED_CHUNK_INDEX_1_0 {
             bail!("got unknown magic number");
         }
 
-        let index_size = stat.st_size as usize - header_size;
-        let index_count = index_size / 40;
-        if index_count * 40 != index_size {
-            bail!("got unexpected file size");
+        if mmap.index().is_empty() {
+            bail!("index must not be empty");
         }
 
-        let index = unsafe {
-            Mmap::map_fd(
-                &file,
-                header_size as u64,
-                index_count,
-                nix::sys::mman::ProtFlags::PROT_READ,
-                nix::sys::mman::MapFlags::MAP_PRIVATE,
-            )?
-        };
-
         Ok(Self {
             _file: file,
-            size,
-            index,
             ctime: i64::from_le(header.ctime),
             uuid: header.uuid,
             index_csum: header.index_csum,
+            mmap,
         })
     }
 
@@ -244,7 +213,7 @@ impl IndexFile for DynamicIndexReader {
     }
 
     fn index_size(&self) -> usize {
-        self.size
+        self.mmap.size()
     }
 
     fn chunk_from_offset(&self, offset: u64) -> Option<(usize, u64)> {
@@ -622,7 +591,7 @@ mod tests {
         check_error_contains(DynamicIndexReader::open(&path), "No such file or directory");
 
         fs::write(&path, []).unwrap();
-        check_error_contains(DynamicIndexReader::open(&path), "index too small (0)");
+        check_error_contains(DynamicIndexReader::open(&path), "file too small (0)");
 
         let mut data = vec![0; size_of::<DynamicIndexHeader>()];
 
@@ -631,14 +600,14 @@ mod tests {
 
         data[..8].copy_from_slice(&file_formats::DYNAMIC_SIZED_CHUNK_INDEX_1_0);
         fs::write(&path, &data).unwrap();
-        check_error_contains(
-            DynamicIndexReader::open(&path),
-            "mapped length must not be zero",
-        );
+        check_error_contains(DynamicIndexReader::open(&path), "index must not be empty");
 
         data.extend_from_slice(&[0, 1, 2]);
         fs::write(&path, &data).unwrap();
-        check_error_contains(DynamicIndexReader::open(&path), "got unexpected file size");
+        check_error_contains(
+            DynamicIndexReader::open(&path),
+            "index size is not divisible by element size: 3",
+        );
 
         data.extend(3u8..80);
         fs::write(&path, &data).unwrap();
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 29927aeba..e8197fb01 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -1,17 +1,14 @@
 use std::fs::File;
-use std::io::{Seek, SeekFrom};
-use std::os::unix::io::AsRawFd;
 use std::path::{Path, PathBuf};
 use std::ptr::NonNull;
 
 use anyhow::{Context, Error, bail, format_err};
-use nix::sys::stat::{SFlag, fstat};
 
-use proxmox_io::ReadExt;
 use proxmox_uuid::Uuid;
 
 use crate::file_formats;
 use crate::index::{ChunkReadInfo, IndexFile};
+use crate::index_mmap::IndexMmap;
 
 /// Header format definition for fixed index files (`.fidx`)
 #[repr(C)]
@@ -30,28 +27,15 @@ proxmox_lang::static_assert_size!(FixedIndexHeader, 4096);
 // split image into fixed size chunks
 
 pub struct FixedIndexReader {
+    mmap: IndexMmap<FixedIndexHeader, [u8; 32]>,
     _file: File,
     pub chunk_size: usize,
     pub size: u64,
-    index_length: usize,
-    index: *mut u8,
     pub uuid: [u8; 16],
     pub ctime: i64,
     pub index_csum: [u8; 32],
 }
 
-// `index` is mmap()ed which cannot be thread-local so should be sendable
-unsafe impl Send for FixedIndexReader {}
-unsafe impl Sync for FixedIndexReader {}
-
-impl Drop for FixedIndexReader {
-    fn drop(&mut self) {
-        if let Err(err) = self.unmap() {
-            log::error!("Unable to unmap file - {}", err);
-        }
-    }
-}
-
 impl FixedIndexReader {
     pub fn open(path: &Path) -> Result<Self, Error> {
         File::open(path)
@@ -60,23 +44,10 @@ impl FixedIndexReader {
             .map_err(|err| format_err!("Unable to open fixed index {:?} - {}", path, err))
     }
 
-    pub fn new(mut file: std::fs::File) -> Result<Self, Error> {
-        let stat = fstat(file.as_raw_fd()).map_err(|e| format_err!("fstat failed - {e}"))?;
-        if (stat.st_mode & SFlag::S_IFMT.bits()) != SFlag::S_IFREG.bits() {
-            bail!("not a regular file");
-        }
-
-        file.seek(SeekFrom::Start(0))?;
-
-        let header_size = std::mem::size_of::<FixedIndexHeader>();
-
-        let size = stat.st_size as usize;
-
-        if size < header_size {
-            bail!("index too small ({})", stat.st_size);
-        }
+    pub fn new(file: File) -> Result<Self, Error> {
+        let mmap = unsafe { IndexMmap::map_read(&file)? };
 
-        let header: Box<FixedIndexHeader> = unsafe { file.read_host_value_boxed()? };
+        let header: &FixedIndexHeader = mmap.header();
 
         if header.magic != file_formats::FIXED_SIZED_CHUNK_INDEX_1_0 {
             bail!("got unknown magic number");
@@ -93,71 +64,36 @@ impl FixedIndexReader {
         let index_length = size.div_ceil(chunk_size) as usize;
         let index_size = index_length * 32;
 
-        let expected_index_size = (stat.st_size as usize) - header_size;
+        let expected_index_size = size_of_val(mmap.index());
         if index_size != expected_index_size {
-            bail!(
-                "got unexpected file size ({} != {})",
-                index_size,
-                expected_index_size
-            );
+            bail!("got unexpected index size ({index_size} != {expected_index_size})",);
         }
 
-        let chunk_size = usize::try_from(chunk_size)?;
+        if mmap.index().is_empty() {
+            bail!("index must not be empty");
+        }
 
-        let data = unsafe {
-            nix::sys::mman::mmap(
-                None,
-                std::num::NonZeroUsize::new(index_size)
-                    .ok_or_else(|| format_err!("invalid index size"))?,
-                nix::sys::mman::ProtFlags::PROT_READ,
-                nix::sys::mman::MapFlags::MAP_PRIVATE,
-                &file,
-                header_size as i64,
-            )
-        }?
-        .as_ptr()
-        .cast::<u8>();
+        let chunk_size = usize::try_from(chunk_size)?;
 
         Ok(Self {
             _file: file,
             chunk_size,
             size,
-            index_length,
-            index: data,
             ctime,
             uuid: header.uuid,
             index_csum: header.index_csum,
+            mmap,
         })
     }
-
-    fn unmap(&mut self) -> Result<(), Error> {
-        let Some(index) = NonNull::new(self.index as *mut std::ffi::c_void) else {
-            return Ok(());
-        };
-
-        let index_size = self.index_length * 32;
-
-        if let Err(err) = unsafe { nix::sys::mman::munmap(index, index_size) } {
-            bail!("unmap file failed - {}", err);
-        }
-
-        self.index = std::ptr::null_mut();
-
-        Ok(())
-    }
 }
 
 impl IndexFile for FixedIndexReader {
     fn index_count(&self) -> usize {
-        self.index_length
+        self.mmap.index().len()
     }
 
     fn index_digest(&self, pos: usize) -> Option<&[u8; 32]> {
-        if pos >= self.index_length {
-            None
-        } else {
-            Some(unsafe { &*(self.index.add(pos * 32) as *const [u8; 32]) })
-        }
+        self.mmap.index().get(pos)
     }
 
     fn index_bytes(&self) -> u64 {
@@ -181,7 +117,7 @@ impl IndexFile for FixedIndexReader {
     }
 
     fn index_size(&self) -> usize {
-        size_of::<FixedIndexHeader>() + self.index_length * 32
+        self.mmap.size()
     }
 
     fn compute_csum(&self) -> ([u8; 32], u64) {
@@ -613,7 +549,7 @@ mod tests {
         check_error_contains(FixedIndexReader::open(&path), "No such file or directory");
 
         fs::write(&path, []).unwrap();
-        check_error_contains(FixedIndexReader::open(&path), "index too small (0)");
+        check_error_contains(FixedIndexReader::open(&path), "file too small (0)");
 
         let mut data = vec![0; size_of::<FixedIndexHeader>()];
 
@@ -630,7 +566,7 @@ mod tests {
         let chunk_size = 4096 * 1024u64;
         data[72..][..8].copy_from_slice(&chunk_size.to_le_bytes());
         fs::write(&path, &data).unwrap();
-        check_error_contains(FixedIndexReader::open(&path), "invalid index size"); // 0
+        check_error_contains(FixedIndexReader::open(&path), "index must not be empty");
 
         let size = chunk_size + 1;
         data[64..][..8].copy_from_slice(&size.to_le_bytes());
@@ -638,14 +574,14 @@ mod tests {
         fs::write(&path, &data).unwrap();
         check_error_contains(
             FixedIndexReader::open(&path),
-            "got unexpected file size (64 != 3)",
+            "index size is not divisible by element size: 3",
         );
 
         data.extend(3u8..32);
         fs::write(&path, &data).unwrap();
         check_error_contains(
             FixedIndexReader::open(&path),
-            "got unexpected file size (64 != 32)",
+            "got unexpected index size (64 != 32)",
         );
 
         data.extend(32u8..64);
diff --git a/pbs-datastore/src/index_mmap.rs b/pbs-datastore/src/index_mmap.rs
new file mode 100644
index 000000000..af5010458
--- /dev/null
+++ b/pbs-datastore/src/index_mmap.rs
@@ -0,0 +1,171 @@
+use std::fs::File;
+use std::marker::PhantomData;
+use std::os::unix::io::AsRawFd;
+use std::ptr::NonNull;
+
+use anyhow::{Error, bail, format_err};
+use nix::sys::mman::{MapFlags, ProtFlags};
+use nix::sys::stat::{SFlag, fstat};
+
+use proxmox_sys::mmap::Mmap;
+
+pub struct IndexMmap<H, T> {
+    _header: PhantomData<H>,
+    _index: PhantomData<[T]>,
+    /// used for the Drop, Send, and Sync impls
+    memory: Mmap<u8>,
+    index_len: usize,
+}
+
+impl<H, T> IndexMmap<H, T> {
+    /// Map a read-only file consisting of a header followed by an array into memory.
+    ///
+    /// SAFETY:
+    /// * `H` and `T` must be:
+    ///     * plain data types
+    ///     * of non-zero size
+    ///     * without interior mutability
+    ///     * where any bit pattern is valid.
+    /// * `file` *must not* be modified by anyone else!
+    ///     * modifying the bytes while a reference exists is UB
+    ///     * truncation may cause SIGBUS on access
+    pub unsafe fn map_read(file: &File) -> Result<Self, Error> {
+        let stat = fstat(file.as_raw_fd()).map_err(|e| format_err!("fstat failed - {e}"))?;
+        if (stat.st_mode & SFlag::S_IFMT.bits()) != SFlag::S_IFREG.bits() {
+            bail!("not a regular file");
+        }
+
+        let size = usize::try_from(stat.st_size)
+            .map_err(|_| format_err!("file too small ({})", stat.st_size))?;
+
+        let index_size = size
+            .checked_sub(size_of::<H>())
+            .ok_or_else(|| format_err!("file too small ({size})"))?;
+
+        let index_len = index_size
+            .checked_div(size_of::<T>())
+            .filter(|l| l * size_of::<T>() == index_size)
+            .ok_or_else(|| {
+                format_err!("index size is not divisible by element size: {index_size}")
+            })?;
+
+        let (prot, flags) = (ProtFlags::PROT_READ, MapFlags::MAP_PRIVATE);
+
+        let memory = unsafe { Mmap::map_fd(file, 0, size, prot, flags) }
+            .map_err(|e| format_err!("mmap failed - {e}"))?;
+
+        let mmap = Self {
+            _header: PhantomData,
+            _index: PhantomData,
+            memory,
+            index_len,
+        };
+        if !mmap.header_ptr().is_aligned() || !mmap.index_ptr().is_aligned() {
+            bail!("alignment error");
+        }
+        Ok(mmap)
+    }
+
+    fn header_ptr(&self) -> NonNull<H> {
+        self.memory.as_non_null().cast::<H>()
+    }
+
+    fn index_ptr(&self) -> NonNull<T> {
+        let ptr = self.memory.as_non_null();
+        unsafe { ptr.byte_add(size_of::<H>()).cast::<T>() }
+    }
+
+    pub fn header(&self) -> &H {
+        unsafe { self.header_ptr().as_ref() }
+    }
+
+    pub fn index(&self) -> &[T] {
+        unsafe { NonNull::slice_from_raw_parts(self.index_ptr(), self.index_len).as_ref() }
+    }
+
+    pub fn size(&self) -> usize {
+        self.memory.len()
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use std::fs;
+    use std::fs::File;
+    use std::path::Path;
+
+    use tempfile::TempDir;
+
+    use super::*;
+
+    #[track_caller]
+    fn check_error_contains<T>(result: Result<T, Error>, substring: &str) {
+        let error = result.map(|_| ()).unwrap_err().to_string();
+        assert!(
+            error.contains(substring),
+            "'{error}' does not contain '{substring}'"
+        );
+    }
+
+    fn map<H, T>(path: &Path) -> Result<IndexMmap<H, T>, Error> {
+        let file = File::open(path).unwrap();
+        unsafe { IndexMmap::<H, T>::map_read(&file) }
+    }
+
+    #[repr(align(8))]
+    struct A8(u64);
+
+    type A1 = u8;
+
+    #[test]
+    fn test_empty() {
+        let dir = TempDir::new().unwrap();
+        let path = dir.path().join("file");
+
+        fs::write(&path, &[]).unwrap();
+        check_error_contains(map::<A8, A8>(&path), "file too small (0)");
+        check_error_contains(map::<A1, A1>(&path), "file too small (0)");
+    }
+
+    #[test]
+    fn test_small() {
+        let dir = TempDir::new().unwrap();
+        let path = dir.path().join("file");
+
+        fs::write(&path, [1, 2, 3, 4, 5, 6, 7]).unwrap();
+        check_error_contains(map::<A8, A1>(&path), "file too small (7)");
+
+        let mmap = map::<A1, A1>(&path).unwrap();
+        assert_eq!(mmap.size(), 7);
+        assert_eq!(mmap.header(), &1);
+        assert_eq!(mmap.index(), &[2, 3, 4, 5, 6, 7]);
+    }
+
+    #[test]
+    fn test_divisibility_and_alignment() {
+        let dir = TempDir::new().unwrap();
+        let path = dir.path().join("file");
+
+        fs::write(&path, [1; 33]).unwrap();
+        check_error_contains(
+            map::<A8, A8>(&path),
+            "index size is not divisible by element size: 25",
+        );
+
+        // could be prevented statically, but it makes a good test for the sanity check
+        check_error_contains(map::<A1, A8>(&path), "alignment error");
+    }
+
+    #[test]
+    fn test_medium() {
+        let dir = TempDir::new().unwrap();
+        let path = dir.path().join("file");
+
+        fs::write(&path, [1; 32]).unwrap();
+
+        let mmap = map::<A8, A8>(&path).unwrap();
+        assert_eq!(mmap.size(), 32);
+        assert_eq!(mmap.header().0, 0x0101010101010101);
+        assert_eq!(mmap.index().len(), 3);
+    }
+}
diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
index d9d7a4cb6..0da9c65fa 100644
--- a/pbs-datastore/src/lib.rs
+++ b/pbs-datastore/src/lib.rs
@@ -202,6 +202,7 @@ pub mod task_tracking;
 
 pub mod dynamic_index;
 pub mod fixed_index;
+mod index_mmap;
 
 pub use backup_info::{BackupDir, BackupGroup, BackupInfo};
 pub use checksum_reader::ChecksumReader;
-- 
2.47.3