public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: "Max R. Carrara" <m.carrara@proxmox.com>
To: pbs-devel@lists.proxmox.com
Subject: [PATCH proxmox{,-backup} v3 0/9] Fix Undefined Behavior in Tape Block Header Deallocation
Date: Wed,  9 Sep 2026 17:40:14 +0200	[thread overview]
Message-ID: <20260909154027.595374-1-m.carrara@proxmox.com> (raw)

Fix Undefined Behavior in Tape Block Header Deallocation - v3
=============================================================

We allocate `BlockHeader` in `pbs-tape` with an alignment equal
to the page size, but because it uses packed / 1-byte alignment
(`#[repr(C, packed)]` to be precise), the compiler will treat it as if
it was allocated with 1-byte alignment.

Miri [0] will flag this as undefined behavior:

    error: Undefined Behavior: incorrect layout on deallocation:
    alloc46398 has size 262144 and alignment 4096, but gave size 262144
    and alignment 1

This is because Rust cares about a memory region's alignment on
deallocation, even if the underlying allocator does not -- hence why
this hasn't actually been a problem for us. Still, there is no guarantee
that this will not change at some point in the future.

The same kind of UB is also caused by the `alloc_page_aligned_buffer()`
function of the `sgutils2` module.

This series addresses both of these issues by introducing a new type
called `LayoutAwareBox`, which keeps track of the layout a given heap
allocation used.

Special thanks to @Robert, who brought this to my attention and totally
managed to nerd-snipe me with this. I have added a respective git
trailer in each of the patches that address the occurrences of UB.

Notable Changes
---------------

This refresh incorporates the feedback @Robert has given on v2. Many
thanks for the review, it's really appreciated! The most noteworthy
changes are found below.

proxmox:

- Drop the inner enum that was used to differ between "known" and
  "dynamic" layouts -- always store a `std::alloc::Layout` and handle
  zero-sized types explicitly instead

- Fix the `Clone` impls of `LayoutAwareBox<T>` by allocating the new
  memory with the same layout as that of `self`, then clone and write
  directly to that memory

- Introduce a new method to `LayoutAwareBox<T>` called
  `from_slice_align()` that allows the caller to easily create an aware
  box from a given `&[T]` with a particular alignment

  --> This also happens to be useful for the `Clone` impl for
      `LayoutAwareBox<[T]>`

- Prevent leaking / observing (uninitialized) memory on panic in
  `slice_fill()` and `slice_fill_with()` of `LayoutAwareBox<T>` by
  constructing the aware-boxed slice with a length of 0 first, and then
  updating the length of the allocated slice at the end of every
  iteration.

  This also means that drop-handlers of already initialized elements
  will get called.

- Add initial Debian packaging for the proxmox-alloc crate

- Drop the `examples/` directory

- Adapt SAFETY comments as per @Robert's feedback

- Adapt existing tests and add additional ones, such as for `From` and
  `Clone` impls

proxmox-backup:

- Adapt docstring of the `TapeBlock` type

- Adapt SAFETY comments as per @Robert's feedback

Testing
-------

Would be great if somebody with a working tape storage could give this a
spin -- miri does not report any UB anymore and the tests we have pass,
but some smoke-testing would nevertheless be appreciated. My virtual
tape library has unfortunately borked itself, and I haven't come around
to un-borking it yet.

[0]: https://github.com/rust-lang/miri

Previous Versions
-----------------

v1: https://lore.proxmox.com/pbs-devel/20260805141620.4190773-1-m.carrara@proxmox.com/
v2: https://lore.proxmox.com/pbs-devel/20260821140238.615302-1-m.carrara@proxmox.com/

Summary of Changes
------------------


proxmox:

Max R. Carrara (1):
  proxmox-alloc: introduce proxmox-alloc with `LayoutAwareBox<T>` type

 Cargo.toml                         |    1 +
 proxmox-alloc/Cargo.toml           |   20 +
 proxmox-alloc/debian/changelog     |    5 +
 proxmox-alloc/debian/control       |   30 +
 proxmox-alloc/debian/copyright     |   18 +
 proxmox-alloc/debian/debcargo.toml |    7 +
 proxmox-alloc/src/aware_boxed.rs   | 2548 ++++++++++++++++++++++++++++
 proxmox-alloc/src/lib.rs           |   21 +
 8 files changed, 2650 insertions(+)
 create mode 100644 proxmox-alloc/Cargo.toml
 create mode 100644 proxmox-alloc/debian/changelog
 create mode 100644 proxmox-alloc/debian/control
 create mode 100644 proxmox-alloc/debian/copyright
 create mode 100644 proxmox-alloc/debian/debcargo.toml
 create mode 100644 proxmox-alloc/src/aware_boxed.rs
 create mode 100644 proxmox-alloc/src/lib.rs


proxmox-backup:

Max R. Carrara (8):
  tape: move tape block structs into separate file module
  tape: rename `BlockHeader` and `BlockHeaderFlags`
  tape: blocked_{reader,writer}: rename `buffer` to `tape_block`
  tape: tape block: represent tape block header with its own struct
  tape: tape block: make `payload` field private
  tape: blocked_{reader,writer}: remove haphazard `unsafe` blocks
  tape: tape block: fix undefined behavior on tape block deallocation
  tape: sgutils2: fix undefined behavior in dealloc of buffer

 Cargo.toml                     |   3 +
 pbs-tape/Cargo.toml            |   1 +
 pbs-tape/src/blocked_reader.rs |  72 +++++++------
 pbs-tape/src/blocked_writer.rs |  51 +++++----
 pbs-tape/src/lib.rs            |  86 +++------------
 pbs-tape/src/sgutils2.rs       |  49 ++++++---
 pbs-tape/src/tape_block.rs     | 185 +++++++++++++++++++++++++++++++++
 7 files changed, 294 insertions(+), 153 deletions(-)
 create mode 100644 pbs-tape/src/tape_block.rs


Summary over all repositories:
  15 files changed, 2944 insertions(+), 153 deletions(-)

-- 
Generated by murpp 0.12.0




             reply	other threads:[~2026-09-09 15:40 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-09 15:40 Max R. Carrara [this message]
2026-09-09 15:40 ` [PATCH proxmox v3 1/9] proxmox-alloc: introduce proxmox-alloc with `LayoutAwareBox<T>` type Max R. Carrara
2026-09-09 15:40 ` [PATCH proxmox-backup v3 2/9] tape: move tape block structs into separate file module Max R. Carrara
2026-09-09 15:40 ` [PATCH proxmox-backup v3 3/9] tape: rename `BlockHeader` and `BlockHeaderFlags` Max R. Carrara
2026-09-09 15:40 ` [PATCH proxmox-backup v3 4/9] tape: blocked_{reader,writer}: rename `buffer` to `tape_block` Max R. Carrara
2026-09-09 15:40 ` [PATCH proxmox-backup v3 5/9] tape: tape block: represent tape block header with its own struct Max R. Carrara
2026-09-09 15:40 ` [PATCH proxmox-backup v3 6/9] tape: tape block: make `payload` field private Max R. Carrara
2026-09-09 15:40 ` [PATCH proxmox-backup v3 7/9] tape: blocked_{reader,writer}: remove haphazard `unsafe` blocks Max R. Carrara
2026-09-09 15:40 ` [PATCH proxmox-backup v3 8/9] tape: tape block: fix undefined behavior on tape block deallocation Max R. Carrara
2026-09-09 15:40 ` [PATCH proxmox-backup v3 9/9] tape: sgutils2: fix undefined behavior in dealloc of buffer Max R. Carrara

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260909154027.595374-1-m.carrara@proxmox.com \
    --to=m.carrara@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal