From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 9770B1FF0E1 for ; Mon, 10 Aug 2026 11:51:35 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 5E064215F5; Mon, 10 Aug 2026 11:51:35 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Mon, 10 Aug 2026 11:51:28 +0200 Message-Id: Subject: Re: [PATCH proxmox-backup 1/2] pbs-tape: fix undefined behavior in block header deallocation To: "Robert Obkircher" From: "Max R. Carrara" X-Mailer: aerc 0.18.2-0-ge037c095a049 References: <20260805141620.4190773-1-m.carrara@proxmox.com> <20260805141620.4190773-2-m.carrara@proxmox.com> <178601383145.99099.751352518821996428.b4-review@b4> <590f88b8-e034-446d-a307-8505e2ca787d@proxmox.com> In-Reply-To: <590f88b8-e034-446d-a307-8505e2ca787d@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1786355477246 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.727 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: HTGCWGR5HNAIHWOA4542O3LZIGCPJK7Q X-Message-ID-Hash: HTGCWGR5HNAIHWOA4542O3LZIGCPJK7Q X-MailFrom: m.carrara@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pbs-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Mon Aug 10, 2026 at 10:34 AM CEST, Robert Obkircher wrote: > > On 07.08.26 14:47, Max R. Carrara wrote: > > On Thu Aug 6, 2026 at 12:57 PM CEST, Robert Obkircher wrote: > >>> 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()`. > >>> [...] > >>> + > >>> +impl BlockHeader { > >>> + pub const SIZE: usize =3D 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::() =3D=3D 1, but it's better to be e= xplicit about the arithmetic. > >>> + const PAYLOAD_LEN: usize =3D > >>> + (Self::SIZE - _impl::BLOCK_HEADER_META_SIZE) / _impl::PAYLOA= D_ELEMENT_SIZE; > >> Imo having constants like this all over the place makes it much harder > >> to reason about the correctness of new. > > How so exactly? > Just that previously you had to keep 1 type and 1 constant in mind and > now it's four of each. I mean, fair, but before you also had to be aware that the fields before the payload had a total size of 16 due to their layout + alignment, which isn't really something that obvious if you're not used to working with such types. I mean, I don't mind changing this per se; I'm just not sure how this makes it harder to reason about the correctness of `::new()` in particular. Could you perhaps show what would make it easier to reason about for you instead? Maybe I'm just missing something here. > >>> + > >>> + pub fn new() -> Result { > >>> + // SAFETY: layout is non-zero by construction > >>> + let raw_thin_ptr =3D unsafe { alloc::alloc_zeroed(*BLOCK_HEA= DER_ALLOC_LAYOUT) }; > >>> + > >>> + let Some(nonnull_thin) =3D NonNull::new(raw_thin_ptr) else { > >>> + // NOTE: Getting a null pointer here means that we eithe= r ran out of memory, or that > >>> + // our allocator doesn't support the given size or align= ment. Since the latter is very, > >>> + // very unlikely, we assume that we only ever hit this b= ranch 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. > > ACK, good point. > > > >>> + }; > >>> + > >>> + // A bit of weirdness regarding dynamically sized types: > >>> + // > >>> + // Since BlockHeaderData is dynamically sized, we need to ex= plicitly construct a fat > >>> + // pointer for it. A fat pointer consists of a memory addres= s plus the number of elements > >>> + // that the pointee stores, called the length. Therefore, si= ze_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 c= ast 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 p= ayload slice in BlockHeaderData > >>> + // a concrete size of PAYLOAD_LEN. > >>> + // > >>> + // This only works because the unsized payload slice is at t= he 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 f= rom doing either.) > >>> + // > >>> + // SAFETY: We ensured that PAYLOAD_LEN is the total size min= us the size of the metadata by > >>> + // construction. We also ensured that our thin pointer is no= t null. > >>> + let raw_fat_ptr =3D > >>> + 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 =3D unsafe { NonNull::new_unchecked(raw_fat_ptr)= }; > >>> + > >>> + // SAFETY: We are allowed to convert the pointer to a refere= nce. See impl Deref. > >> Why not just construct Self and write the field via DerefMut then? > > Oh, true... missed that one here. Thanks for spotting this! > > > >>> + unsafe { ptr.as_mut().set_magic(PROXMOX_TAPE_BLOCK_HEADER_MA= GIC_1_0) }; > >>> + > >>> + debug_assert_eq!(unsafe { ptr.as_ref().payload().len() }, Se= lf::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 =3D _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. > > Agree with this as well, will add this in v2 too. > You could even get rid of unsafe by storing ManuallyDrop Box like the > aligned_box crate. I'll check it out, thanks! > > > >>> + unsafe { self.ptr.as_mut() } > >>> + } > >>> +} > >>> + > >>> +impl AsRef for BlockHeader { > >>> + fn as_ref(&self) -> &BlockHeaderData { > >>> + &*self > >>> + } > >>> +} > >>> + > >>> +impl AsMut 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 pr= ivate fields > >> This seems like a confusing, unrelated change with no benefit. > > The main benefit here is that private fields cannot be accessed anymore= . > I'm not fully convinced that the definition of an extern format needs > to be hidden from someone willing to work at that level. What do you mean with hidden? All fields are still accessible through their respective methods, it's just that *directly* modifying those fields is not as easy as before anymore. There wasn't any point in exposing the fields as `pub` in the first place IMO, and I also think that doing that encourages bad patterns ... For example, in both the [reader] and [writer] implementations we are casting the entire struct to a byte slice directly, even though that could've been a method each instead, keeping the implementation of such an operation local to the struct. That's why I introduced `as_bytes()` and `as_bytes_mut()`, which do exactly that. Don't get me wrong, if a struct really only stores plain old data, then I don't mind `pub` fields, but as soon as there's any kind of state involved, it's vastly easier to reason about a type if you know what can be done with it, IMO. [reader] https://git.proxmox.com/?p=3Dproxmox-backup.git;a=3Dblob;f=3Dpbs-t= ape/src/blocked_reader.rs;h=3D22803371c3e36753c7fe987312713bb98ca0d1a1;hb= =3Drefs/heads/master#l94 [writer] https://git.proxmox.com/?p=3Dproxmox-backup.git;a=3Dblob;f=3Dpbs-t= ape/src/blocked_writer.rs;h=3D7380af2435d62ee11fbab47188f782360ebab474;hb= =3Drefs/heads/master#l48 > > > > I was also thinking of moving this out of `lib.rs` and instead making a > > separate module like e.g. `header.rs` (or something), since private > > fields should then remain private throughout the crate. > That seems nicer than manual multiple inheritance of accessors. Just > do that in a separate commit please. ACK, will do that. > > > >> Leaving the original code as-is and simply replacing Box with a new > >> PageAlignedBox would have been so much easier to review. > > I didn't want to introduce a new generic type for a one-off smart > > pointer -- and moreover, what in particular made it harder to review, i= f > > I may ask? I'm simply moving the implementation into separate structs > > here. > There is a [2]nd one that I had posted on Zulip, and the third could > be a unit test that checks an Rc count. > > I just prefer mechanical changes like moving code to be in separate > commits. > > [2] > https://git.proxmox.com/?p=3Dproxmox-backup.git;a=3Dblob;f=3Dpbs-tape/src= /sgutils2.rs;h=3D340616c73675b994c8761a6981fd41e2e249f953;hb=3DHEAD#l447 Hmm, fair point! I'll see if I can fix the second instance in v2 too, then. Which third one are you referring to exactly? > > > > >>> +mod _impl { > >>> + use super::BlockHeaderFlags; > >>> + > >>> + type PayloadType =3D u8; > >>> + > >>> + pub(super) const BLOCK_HEADER_META_SIZE: usize =3D size_of::(); > >>> + pub(super) const PAYLOAD_ELEMENT_SIZE: usize =3D size_of::(); > >>> + > >>> + /// 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 =3D 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 si= ze 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 bot= h 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 bot= h metadata > >>> + /// and payload, as a mutable byte slice. > >>> + /// > >>> + /// While this method in itself is safe, be aware that it ne= vertheless > >>> + /// 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 =3D 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 =3D size.to_le_bytes(); > >>> + // Since we only need three bytes, we can copy them manu= ally > >>> + // instead --> allows this function to be const, since r= ange 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. > > Good point, but there's also no downside in declaring them as const at > > the same time. IMO it's just good hygiene to make things const if they > > can be made const, but I don't mind dropping it if you insist. > I was just confused about the rewrite. > > (In general I'd also be a bit careful about promising that a public > function is const before there is a use for it, because that turns > adding a side-effect like logging into a breaking change.) That's a fair point, actually. I'll make these functions non-const for now, since there isn't really any benefit in having them be const anyway. (Though I do think that for small getters / setters it's perfectly fine to make them const from the beginning, since it's very rare to add logging inside those kinds of methods anyway, IMO.) > > >