Re: [PATCH proxmox-backup 1/2] pbs-tape: fix undefined behavior in block header deallocation

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

> The `BlockHeader` struct in `pbs-tape` is a dynamically sized type
> whose data we allocate with an alignment equal to the page size.
> However, since `BlockHeader` uses `#[repr(C, packed)]`, the Rust
> compiler will always treat the type as having an alignment of 1
> instead. This is undefined behavior as reported by miri [0], since
> Rust requires the alignment used during deallocation to match that
> used during allocation, even if the underlying allocator doesn't
> actually care about alignment on `free()`.
> 
> While this should be harmless with the current standard library on
> *nix and WASM, there is no guarantee that this won't change in the
> future. If we were to target Windows, it would already be a problem [1].
> 
> To solve this, promote `BlockHeader` to a smart pointer instead, which
> manages the allocation of a new `BlockHeaderData` struct, whose
> purpose is to hold the actual data. `BlockHeaderData` has the exact
> same field ordering, layout and alignment as the previous
> implementation of `BlockHeader`, but is confined to a private module
> and then re-exported to prevent accessing its fields directly.
> 
> For convenience, move any "metadata" fields into a newly introduced
> `BlockHeaderMeta`, which is not exposed publicly. This makes computing
> the payload's size more explicit, as we can now subtract
> `size_of::<BlockHeaderMeta>()` from the total block size instead of
> using a hard-coded numeric constant.
> 
> While the data format is unlikely to change -- and if it does, we
> would need a new struct with a new magic constant for the anyways --
> this nevertheless should make understanding the code easier for future
> readers, in particular because dynamically sized types are not that
> trivial if one is not used to them.
> 
> Furthermore, store the layout used for block header allocations using
> a `LazyLock`, which allows us to avoid querying the currently used
> page size on each new block allocation. We can do this because the
> page size is a constant [2].
> 
> Additionally, implement various helper methods on `BlockHeaderData`
> that make handling the block header a little less error-prone.
> Notably, `unsafe` calls to `slice::from_raw_parts` and
> `slice::from_raw_parts_mut` are implemented as methods instead of
> being called directly in the reader / writer code.
> 
> Finally, sites using `BlockHeader` are adapted corresponding to the
> changes above, which mainly means that direct field accesses are
> replaced with method calls in most cases.
> 
> [0]: https://github.com/rust-lang/miri
> [1]: https://github.com/rust-lang/rust/blob/c9ff496891c278ad660bc0ab85c1f0b72059464a/library/std/src/sys/alloc/windows.rs#L182
> [2]: `man 3 sysconf`
> 
> Reported-by: Robert Obkircher <r.obkircher@proxmox.com>
> Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
>
> diff --git a/pbs-tape/src/blocked_reader.rs b/pbs-tape/src/blocked_reader.rs
> index 22803371c..33c5db780 100644
> --- a/pbs-tape/src/blocked_reader.rs
> +++ b/pbs-tape/src/blocked_reader.rs
> @@ -18,7 +18,7 @@ use crate::{
>  /// the end of the stream).
>  pub struct BlockedReader<R> {
>      reader: R,
> -    buffer: Box<BlockHeader>,
> +    buffer: BlockHeader,
>      seq_nr: u32,
>      found_end_marker: bool,
>      incomplete: bool,
> @@ -33,7 +33,7 @@ impl<R: BlockRead> BlockedReader<R> {
>      /// This tries to read the first block. Please inspect the error
>      /// to detect EOF and EOT.
>      pub fn open(mut reader: R) -> Result<Self, BlockReadError> {
> -        let mut buffer = BlockHeader::new();
> +        let mut buffer = BlockHeader::new()?;
>  
>          Self::read_block_frame(&mut buffer, &mut reader)?;
>  
> @@ -43,7 +43,7 @@ impl<R: BlockRead> BlockedReader<R> {
>          let mut got_eod = false;
>  
>          if found_end_marker {
> -            incomplete = buffer.flags.contains(BlockHeaderFlags::INCOMPLETE);
> +            incomplete = buffer.flags().contains(BlockHeaderFlags::INCOMPLETE);
>              Self::consume_eof_marker(&mut reader)?;
>              got_eod = true;
>          }
> @@ -61,7 +61,7 @@ impl<R: BlockRead> BlockedReader<R> {
>      }
>  
>      fn check_buffer(buffer: &BlockHeader, seq_nr: u32) -> Result<(usize, bool), std::io::Error> {
> -        if buffer.magic != PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0 {
> +        if buffer.magic() != PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0 {
>              proxmox_lang::io_bail!(
>                  "got tape block with unknown magic number - not written by PBS or incompatible LTO version"
>              );
> @@ -76,13 +76,13 @@ impl<R: BlockRead> BlockedReader<R> {
>          }
>  
>          let size = buffer.size();
> -        let found_end_marker = buffer.flags.contains(BlockHeaderFlags::END_OF_STREAM);
> +        let found_end_marker = buffer.flags().contains(BlockHeaderFlags::END_OF_STREAM);
>  
> -        if size > buffer.payload.len() {
> +        if size > buffer.payload().len() {
>              proxmox_lang::io_bail!(
>                  "detected tape block with wrong payload size ({} > {}",
>                  size,
> -                buffer.payload.len()
> +                buffer.payload().len()
>              );
>          } else if size == 0 && !found_end_marker {
>              proxmox_lang::io_bail!("detected tape block with zero payload size");
> @@ -92,14 +92,7 @@ impl<R: BlockRead> BlockedReader<R> {
>      }
>  
>      fn read_block_frame(buffer: &mut BlockHeader, reader: &mut R) -> Result<(), BlockReadError> {
> -        let data = unsafe {
> -            std::slice::from_raw_parts_mut(
> -                (buffer as *mut BlockHeader) as *mut u8,
> -                BlockHeader::SIZE,
> -            )
> -        };
> -
> -        let bytes = reader.read_block(data)?;
> +        let bytes = reader.read_block(buffer.as_bytes_mut())?;
>  
>          if bytes != BlockHeader::SIZE {
>              return Err(proxmox_lang::io_format_err!("got wrong block size").into());
> @@ -127,7 +120,7 @@ impl<R: BlockRead> BlockedReader<R> {
>              Ok(()) => { /* ok */ }
>              Err(BlockReadError::EndOfFile) => {
>                  self.got_eod = true;
> -                self.read_pos = self.buffer.payload.len();
> +                self.read_pos = self.buffer.payload().len();
>                  if !self.found_end_marker && check_end_marker {
>                      proxmox_lang::io_bail!("detected tape stream without end marker");
>                  }
> @@ -147,7 +140,7 @@ impl<R: BlockRead> BlockedReader<R> {
>          if found_end_marker {
>              // consume EOF mark
>              self.found_end_marker = true;
> -            self.incomplete = self.buffer.flags.contains(BlockHeaderFlags::INCOMPLETE);
> +            self.incomplete = self.buffer.flags().contains(BlockHeaderFlags::INCOMPLETE);
>              Self::consume_eof_marker(&mut self.reader)?;
>              self.got_eod = true;
>          }
> @@ -225,8 +218,11 @@ impl<R: BlockRead> Read for BlockedReader<R> {
>              } else {
>                  rest as usize
>              };
> -            buffer[..copy_len]
> -                .copy_from_slice(&self.buffer.payload[self.read_pos..(self.read_pos + copy_len)]);
> +
> +            let (read_start, read_end) = (self.read_pos, self.read_pos + copy_len);
> +            let payload = self.buffer.payload();
> +            buffer[..copy_len].copy_from_slice(&payload[read_start..read_end]);
> +
>              self.read_pos += copy_len;
>              Ok(copy_len)
>          }
> diff --git a/pbs-tape/src/blocked_writer.rs b/pbs-tape/src/blocked_writer.rs
> index 7380af243..57721ed4a 100644
> --- a/pbs-tape/src/blocked_writer.rs
> +++ b/pbs-tape/src/blocked_writer.rs
> @@ -9,7 +9,7 @@ use crate::{BlockHeader, BlockHeaderFlags, BlockWrite, TapeWrite};
>  /// to the underlying writer.
>  pub struct BlockedWriter<W: BlockWrite> {
>      writer: W,
> -    buffer: Box<BlockHeader>,
> +    buffer: BlockHeader,
>      buffer_pos: usize,
>      seq_nr: u32,
>      logical_end_of_media: bool,
> @@ -36,7 +36,7 @@ impl<W: BlockWrite> BlockedWriter<W> {
>      pub fn new(writer: W) -> Self {
>          Self {
>              writer,
> -            buffer: BlockHeader::new(),
> +            buffer: BlockHeader::new().expect("failed to create block header"),
>              buffer_pos: 0,
>              seq_nr: 0,
>              logical_end_of_media: false,
> @@ -46,13 +46,7 @@ impl<W: BlockWrite> BlockedWriter<W> {
>      }
>  
>      fn write_block(buffer: &BlockHeader, writer: &mut W) -> Result<bool, std::io::Error> {
> -        let data = unsafe {
> -            std::slice::from_raw_parts(
> -                (buffer as *const BlockHeader) as *const u8,
> -                BlockHeader::SIZE,
> -            )
> -        };
> -        writer.write_block(data)
> +        writer.write_block(buffer.as_bytes())
>      }
>  
>      fn write_eof(&mut self) -> Result<(), std::io::Error> {
> @@ -69,16 +63,20 @@ impl<W: BlockWrite> BlockedWriter<W> {
>              return Ok(0);
>          }
>  
> -        let rest = self.buffer.payload.len() - self.buffer_pos;
> +        let rest = self.buffer.payload().len() - self.buffer_pos;
>          let bytes = if data.len() < rest { data.len() } else { rest };
> -        self.buffer.payload[self.buffer_pos..(self.buffer_pos + bytes)]
> -            .copy_from_slice(&data[..bytes]);
> +
> +        let (write_start, write_end) = (self.buffer_pos, self.buffer_pos + bytes);
> +
> +        let payload = self.buffer.payload_mut();
> +        payload[write_start..write_end].copy_from_slice(&data[..bytes]);
>  
>          let rest = rest - bytes;
>  
>          if rest == 0 {
> -            self.buffer.flags = BlockHeaderFlags::empty();
> -            self.buffer.set_size(self.buffer.payload.len());
> +            self.buffer.set_flags(BlockHeaderFlags::empty());
> +            let payload_len = self.buffer.payload().len();
> +            self.buffer.set_size(payload_len);
>              self.buffer.set_seq_nr(self.seq_nr);
>              self.seq_nr += 1;
>              let leom = Self::write_block(&self.buffer, &mut self.writer)?;
> @@ -115,10 +113,13 @@ impl<W: BlockWrite> TapeWrite for BlockedWriter<W> {
>      /// Note: This may write an empty block just including the
>      /// END_OF_STREAM flag.
>      fn finish(&mut self, incomplete: bool) -> Result<bool, std::io::Error> {
> -        vec::clear(&mut self.buffer.payload[self.buffer_pos..]);
> -        self.buffer.flags = BlockHeaderFlags::END_OF_STREAM;
> +        let payload = self.buffer.payload_mut();
> +        vec::clear(&mut payload[self.buffer_pos..]);
> +
> +        self.buffer.set_flags(BlockHeaderFlags::END_OF_STREAM);
>          if incomplete {
> -            self.buffer.flags |= BlockHeaderFlags::INCOMPLETE;
> +            let flags = self.buffer.flags() | BlockHeaderFlags::INCOMPLETE;
> +            self.buffer.set_flags(flags);
>          }
>          self.buffer.set_size(self.buffer_pos);
>          self.buffer.set_seq_nr(self.seq_nr);
> diff --git a/pbs-tape/src/lib.rs b/pbs-tape/src/lib.rs
> index 0fe55a749..f77d83d37 100644
> --- a/pbs-tape/src/lib.rs
> +++ b/pbs-tape/src/lib.rs
> @@ -1,4 +1,8 @@
> +use std::alloc;
>  use std::collections::HashSet;
> +use std::io;
> +use std::ptr::NonNull;
> +use std::sync::LazyLock;
>  
>  use anyhow::{Error, bail};
>  use bitflags::bitflags;
> @@ -49,28 +53,148 @@ pub const PROXMOX_BACKUP_MEDIA_LABEL_MAGIC_1_0: [u8; 8] = [42, 5, 191, 60, 176,
>  // openssl::sha::sha256(b"Proxmox Backup MediaSet Label v1.0")
>  pub const PROXMOX_BACKUP_MEDIA_SET_LABEL_MAGIC_1_0: [u8; 8] = [8, 96, 99, 249, 47, 151, 83, 216];
>  
> -/// Tape Block Header with data payload
> +static BLOCK_HEADER_ALLOC_LAYOUT: LazyLock<alloc::Layout> = LazyLock::new(|| {
> +    // See `man 3 sysconf` -- this is a constant and does therefore not change
> +    // during the lifetime of a process.
> +    // SAFETY: Should always be safe to call, and we check for errors afterwards.
> +    let page_size = unsafe { libc::sysconf(libc::_SC_PAGESIZE) };
> +
> +    if page_size == -1 {
> +        panic!("failed to query PAGESIZE");
The man page also guarantees page_size > 0.

It should even be assert!(page_size >= align_of::<T>()), which is the
same thing in this case.

> +    }
> +
> +    // Aligned to PAGESIZE, so that we can use it with SG_IO
> +    let layout = alloc::Layout::from_size_align(BlockHeader::SIZE, page_size as usize)
> +        .expect("creating layout with page size alignment failed");
> +
> +    debug_assert!(layout.size() != 0, "layout has a size of zero!");
> +
> +    layout
> +});
> +
> +/// Tape Block Header with data payload.
>  ///
>  /// All tape files are written as sequence of blocks.
>  ///
> -/// Note: this struct is large, never put this on the stack!
> -/// so we use an unsized type to avoid that.
> -///
> -/// Tape data block are always read/written with a fixed size
> -/// (`PROXMOX_TAPE_BLOCK_SIZE`). But they may contain less data, so the
> -/// header has an additional size field. For streams of blocks, there
> -/// is a sequence number (`seq_nr`) which may be use for additional
> +/// Tape data blocks are always read/written with a fixed size
> +/// (`PROXMOX_TAPE_BLOCK_SIZE`), but since they may contain less data, the
> +/// header also has an additional size field. For streams of blocks, there
> +/// is a sequence number (`seq_nr`) which may be used for additional
>  /// error checking.
> -#[repr(C, packed)]
> +///
> +/// This struct acts as a smart pointer to the underlying block header
> +/// implementation, [`BlockHeaderData`], which is a dynamically sized type.
> +/// [`BlockHeaderData`]'s memory is aligned to `PAGESIZE`, but because it uses a
> +/// `packed` layout, the compiler will think its alignment is `1` instead.
> +///
> +/// Since Rust accounts for the alignment even on deallocation, this
> +/// smart pointer therefore ensures that [`BlockHeaderData`] is deallocated
> +/// with the same alignment that was used when it was allocated. Using a
> +/// different alignment would otherwise be undefined behavior, which [`miri`]
> +/// would report as such.
> +///
> +/// [`miri`]: https://github.com/rust-lang/miri
> +#[repr(transparent)]
>  pub struct BlockHeader {
> -    /// fixed value `PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0`
> -    pub magic: [u8; 8],
> -    pub flags: BlockHeaderFlags,
> -    /// size as 3 bytes unsigned, little endian
> -    pub size: [u8; 3],
> -    /// block sequence number
> -    pub seq_nr: u32,
> -    pub payload: [u8],
> +    ptr: NonNull<BlockHeaderData>,
> +}
> +
> +impl BlockHeader {
> +    pub const SIZE: usize = PROXMOX_TAPE_BLOCK_SIZE;
> +
> +    // We are subtracting the metadata size first before dividing by the size of the
> +    // payload's element type, since we need to ensure that we don't mix units.
> +    //
> +    // Note that the division at the end here is actually redundant, since
> +    // size_of::<PayloadElement>() == 1, but it's better to be explicit about the arithmetic.
> +    const PAYLOAD_LEN: usize =
> +        (Self::SIZE - _impl::BLOCK_HEADER_META_SIZE) / _impl::PAYLOAD_ELEMENT_SIZE;
Imo having constants like this all over the place makes it much harder
to reason about the correctness of new.
> +
> +    pub fn new() -> Result<Self, io::Error> {
> +        // SAFETY: layout is non-zero by construction
> +        let raw_thin_ptr = unsafe { alloc::alloc_zeroed(*BLOCK_HEADER_ALLOC_LAYOUT) };
> +
> +        let Some(nonnull_thin) = NonNull::new(raw_thin_ptr) else {
> +            // NOTE: Getting a null pointer here means that we either ran out of memory, or that
> +            // our allocator doesn't support the given size or alignment. Since the latter is very,
> +            // very unlikely, we assume that we only ever hit this branch if we're OOM.
> +            return Err(io::Error::new(
> +                io::ErrorKind::OutOfMemory,
> +                "failed to allocate memory for block header",
> +            ));
io::Error::new allocates, so it might be better to just call
handle_alloc_error.
> +        };
> +
> +        // A bit of weirdness regarding dynamically sized types:
> +        //
> +        // Since BlockHeaderData is dynamically sized, we need to explicitly construct a fat
> +        // pointer for it. A fat pointer consists of a memory address plus the number of elements
> +        // that the pointee stores, called the length. Therefore, size_of::<*mut u8>() (thin) is
> +        // not the same as size_of::<*mut [u8]>() (fat), usually the latter is twice as large.
> +        //
> +        // The length that we specify here is the number of elements our trailing payload can
> +        // store, which we defined earlier as PAYLOAD_LEN. We then cast our fat *mut [u8] to
> +        // *mut BlockHeaderData -- since the length stored in a fat pointer is *not* converted or
> +        // altered when casting, we successfully gave the trailing payload slice in BlockHeaderData
> +        // a concrete size of PAYLOAD_LEN.
> +        //
> +        // This only works because the unsized payload slice is at the end of the BlockHeaderData
> +        // struct. The compiler will know what the length in the fat pointer corresponds to that
> +        // slice. (Note that a different position for the payload in the struct wouldn't work, and
> +        // neither would multiple slices; the compiler prevents us from doing either.)
> +        //
> +        // SAFETY: We ensured that PAYLOAD_LEN is the total size minus the size of the metadata by
> +        // construction. We also ensured that our thin pointer is not null.
> +        let raw_fat_ptr =
> +            core::ptr::slice_from_raw_parts_mut(nonnull_thin.as_ptr(), Self::PAYLOAD_LEN)
> +                as *mut _impl::BlockHeaderData;

> +
> +        // SAFETY: The pointer returned by slice_from_raw_parts_mut() is never null.
> +        let mut ptr = unsafe { NonNull::new_unchecked(raw_fat_ptr) };
> +
> +        // SAFETY: We are allowed to convert the pointer to a reference. See impl Deref.
Why not just construct Self and write the field via DerefMut then?
> +        unsafe { ptr.as_mut().set_magic(PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0) };
> +
> +        debug_assert_eq!(unsafe { ptr.as_ref().payload().len() }, Self::PAYLOAD_LEN);
> +
> +        Ok(Self { ptr })
> +    }
> +}
> +
> +impl Drop for BlockHeader {
> +    fn drop(&mut self) {
> +        // SAFETY: We allocated ptr with the given layout earlier.
> +        unsafe { alloc::dealloc(self.ptr.as_ptr() as *mut u8, *BLOCK_HEADER_ALLOC_LAYOUT) };
> +    }
> +}
> +
> +impl std::ops::Deref for BlockHeader {
> +    type Target = _impl::BlockHeaderData;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: Safe to convert ptr to a shared reference, since it's aligned, the data
> +        // initialized, dereferenceable, and explicitly non-null.
> +        unsafe { self.ptr.as_ref() }
> +    }
> +}
> +
> +impl std::ops::DerefMut for BlockHeader {
> +    fn deref_mut(&mut self) -> &mut Self::Target {
> +        // SAFETY: Safe to convert ptr to a unique reference, since it's aligned, the data
> +        // initialized, dereferenceable, and explicitly non-null.
The important thing is that &mut self provides mutual exclusion with
other places that dereference.
> +        unsafe { self.ptr.as_mut() }
> +    }
> +}
> +
> +impl AsRef<BlockHeaderData> for BlockHeader {
> +    fn as_ref(&self) -> &BlockHeaderData {
> +        &*self
> +    }
> +}
> +
> +impl AsMut<BlockHeaderData> for BlockHeader {
> +    fn as_mut(&mut self) -> &mut BlockHeaderData {
> +        &mut *self
> +    }
>  }
>  
>  bitflags! {
> @@ -84,6 +208,160 @@ bitflags! {
>      }
>  }
>  
> +// Putting underlying implementation in here to prevent access to private fields
This seems like a confusing, unrelated change with no benefit.

Leaving the original code as-is and simply replacing Box<T> with a new
PageAlignedBox<T> would have been so much easier to review.

> +mod _impl {
> +    use super::BlockHeaderFlags;
> +
> +    type PayloadType = u8;
> +
> +    pub(super) const BLOCK_HEADER_META_SIZE: usize = size_of::<BlockHeaderMeta>();
> +    pub(super) const PAYLOAD_ELEMENT_SIZE: usize = size_of::<PayloadType>();
> +
> +    /// Dynamically sized type that represents a tape block header.
> +    ///
> +    /// This struct should never be created manually and is instead accessed through
> +    /// its associated [`BlockHeader`] smart pointer.
> +    #[repr(C, packed)]
> +    pub struct BlockHeaderData {
> +        meta: BlockHeaderMeta,
> +        payload: [PayloadType],
> +    }
> +
> +    impl BlockHeaderData {
> +        /// Returns the magic value for the block header.
> +        pub const fn magic(&self) -> [u8; 8] {
> +            self.meta.magic()
> +        }
> +
> +        pub(super) const fn set_magic(&mut self, magic: [u8; 8]) {
> +            self.meta.magic = magic;
> +        }
> +
> +        /// Returns the currently used [`BlockHeaderFlags`].
> +        pub const fn flags(&self) -> BlockHeaderFlags {
> +            self.meta.flags()
> +        }
> +
> +        /// Sets the [`BlockHeaderFlags`] for the header.
> +        pub const fn set_flags(&mut self, flags: BlockHeaderFlags) {
> +            self.meta.set_flags(flags)
> +        }
> +
> +        /// Returns the size of the header.
> +        ///
> +        /// Note that this value is at most `2^24 - 1`, since the size is represented using 24 bits
> +        /// under the hood.
> +        pub const fn size(&self) -> usize {
> +            self.meta.size()
> +        }
> +
> +        /// Sets the size of the header.
> +        ///
> +        /// Note that the passed value will be truncated to 24 bits, since the size is represented
> +        /// using 24 bits under the hood.
> +        pub const fn set_size(&mut self, size: usize) {
> +            self.meta.set_size(size)
> +        }
> +
> +        /// Sets the sequence number of the header.
> +        pub const fn set_seq_nr(&mut self, seq_nr: u32) {
> +            self.meta.set_seq_nr(seq_nr);
> +        }
> +
> +        /// Returns the sequence number of the header.
> +        pub const fn seq_nr(&self) -> u32 {
> +            self.meta.seq_nr()
> +        }
> +
> +        /// Returns the underlying payload as a slice.
> +        pub const fn payload(&self) -> &[PayloadType] {
> +            &self.payload
> +        }
> +
> +        /// Returns the underlying payload as a mutable slice.
> +        pub const fn payload_mut(&mut self) -> &mut [PayloadType] {
> +            &mut self.payload
> +        }
> +
> +        /// Returns the entirety of [`BlockHeaderData`], meaning both metadata
> +        /// and payload, as a byte slice.
> +        pub const fn as_bytes(&self) -> &[u8] {
> +            unsafe {
> +                std::slice::from_raw_parts(
> +                    (self as *const BlockHeaderData) as *const u8,
> +                    super::BlockHeader::SIZE,
> +                )
> +            }
> +        }
> +
> +        /// Returns the entirety of [`BlockHeaderData`], meaning both metadata
> +        /// and payload, as a mutable byte slice.
> +        ///
> +        /// While this method in itself is safe, be aware that it nevertheless
> +        /// allows you to overwrite metadata in this struct.
> +        pub const fn as_bytes_mut(&mut self) -> &mut [u8] {
> +            unsafe {
> +                std::slice::from_raw_parts_mut(
> +                    (self as *mut BlockHeaderData) as *mut u8,
> +                    super::BlockHeader::SIZE,
> +                )
> +            }
> +        }
> +    }
> +
> +    /// Metadata for tape block headers, which precedes the header's payload.
> +    #[repr(C, packed)]
> +    struct BlockHeaderMeta {
> +        /// Fixed value, set to `PROXMOX_TAPE_BLOCK_HEADER_MAGIC_1_0`.
> +        magic: [u8; 8],
> +        flags: BlockHeaderFlags,
> +        /// Size as 3 bytes unsigned, little endian.
> +        size: [u8; 3],
> +        /// Block sequence number.
> +        seq_nr: u32,
> +    }
> +
> +    impl BlockHeaderMeta {
> +        const fn magic(&self) -> [u8; 8] {
> +            self.magic
> +        }
> +
> +        const fn flags(&self) -> BlockHeaderFlags {
> +            self.flags
> +        }
> +
> +        const fn set_flags(&mut self, flags: BlockHeaderFlags) {
> +            self.flags = flags;
> +        }
> +
> +        const fn size(&self) -> usize {
> +            (self.size[0] as usize)
> +                + ((self.size[1] as usize) << 8)
> +                + ((self.size[2] as usize) << 16)
> +        }
> +
> +        const fn set_size(&mut self, size: usize) {
> +            let size = size.to_le_bytes();
> +            // Since we only need three bytes, we can copy them manually
> +            // instead --> allows this function to be const, since range index
> +            // is not yet const stable
Seems kind of pointless to make these functions const if you cannot
allocatate a value at compile time in the first place.

-- 
Robert Obkircher <r.obkircher@proxmox.com>