Re: [PATCH v2 2/2] rust: debugfs: avoid transmuting FileOps

[Matthew Maurer]

On Tue, May 26, 2026 at 11:08 AM Tamir Duberstein <tamird@xxxxxxxxxx> wrote:
> `FileOps` has the default Rust representation, which does not guarantee
> the same layout for different generic instantiations [1]. Thus the
> reference created by `adapt()` is not justified.

This is correct, though I'll note for the sake of anyone concerned
with an existing kernel that this can only break with
`-Zrandomize-fields` in the current compiler - the layout engine only
examines layout data + an optional random seed when that flag is set,
so swapping out a ZST, no-alignment-requirement field for another ZST,
no-alignment-requirement field will never result in a mismatch unless
you are actively randomizing fields.

> The callback operations do need to access private data through one of
> the transparent adapter types. Express that relationship directly: make
> `Adapter<D>` the unsafe proof that operations implemented for an adapter
> may access stored `D`, and instantiate the corresponding operations
> tables as `FileOps<D>`. The adapter implementations are justified by
> their transparent representations [2].

I think your approach here may need to be adjusted because:
1. `Adapter` requires additional knowledge at the trait usage site
(that the invariants of the target are upheld - specifically
type-inhabitedness).
2. You now access `FILE_OPS` directly off the type, rather than off a
value, so you no longer have any proof that types are inhabited that
you require.

You might be able to rework this so that you can track inhabitedness
properly, but maybe a simpler resolution might work - what if we just
add `#[repr(C)]` onto `FileOps`? The addition of an unaligned ZST
field to a `#[repr(C)]` struct should never change its representation.

> +/// Operations may reconstruct a shared reference to `Self` from a pointer to

I think this is the crux of the problem with this approach. Users of
the trait can't really know the invariants are upheld as-is. See later
comments.

> +/// `D`. Implementers must also arrange for any additional invariants of `Self`
> +/// to hold whenever such an operation is called.
> +pub(super) unsafe trait Adapter<D> {}
> +

>  ///
>  /// # Invariants
>  ///
> -/// If an instance for `WritableAdapter<_, W>` is constructed, `W` is inhabited.
> +/// When `WritableAdapter<_, W>` is used for file operations, `W` is inhabited.

I don't think this works. `WritableAdapter<Foo, Bar>` can be crated
for any D and W at compile time, there's nothing stopping it. That
means that the `FILE_OPS` constant produced must be valid or fail to
compile. Invariants on types are runtime statements about the object
level. You cannot check for `W` to be inhabited at compile time, so
this isn't a valid invariant.

>  #[repr(transparent)]
> -pub(crate) struct WritableAdapter<D, W> {
> +pub(super) struct WritableAdapter<D, W> {
>      inner: D,
>      _writer: PhantomData<W>,
>  }
>
> -// SAFETY: Stripping off the adapter only removes constraints
> -unsafe impl<D, W> Adapter for WritableAdapter<D, W> {
> -    type Inner = D;
> -}
> -
>  impl<D: Writer, W> Writer for WritableAdapter<D, W> {
>      fn write(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
>          self.inner.write(fmt)
> @@ -58,19 +58,20 @@ impl<D: Deref, W> Reader for WritableAdapter<D, W>
>      W: Fn(&D::Target, &mut UserSliceReader) -> Result + Send + Sync + 'static,
>  {
>      fn read_from_slice(&self, reader: &mut UserSliceReader) -> Result {
> -        // SAFETY: WritableAdapter<_, W> can only be constructed if W is inhabited

I don't think this is valid - you don't know that `read_from_slice` is
only called after creation through the callback API. If that's what we
want, then we need to make the trait method `read_from_slice` unsafe
and tell the caller they need to make sure that's how this was
constructed, and things will get out of hand. This is what I was
referring to earlier - we want to use the presence of a
`WritableAdapter<_, W>` as evidence that `W` is inhabited, but we no
longer can because of the invariant change.

> +        // SAFETY: The callback API obtains this implementation only after
> +        // receiving a `&'static W`, so `W` is inhabited.
>          let w: &W = unsafe { materialize_zst() };
>          w(self.inner.deref(), reader)
>      }
>  }
>
> -/// Adapter to implement `Writer` via a callback with the same representation as `T`.
> +/// Adapter to implement `Writer` via a callback with the same representation as `D`.
>  ///
>  /// # Invariants
>  ///
> -/// If an instance for `FormatAdapter<_, F>` is constructed, `F` is inhabited.
> +/// When `FormatAdapter<_, F>` is used for file operations, `F` is inhabited.

As above - access to FILE_OPS is not unsafe, so you cannot make the
caller required to prove something to access it. (And you certainly
can't do it here - such a declaration would need to be in the trait
defining the `FILE_OPS` constant at a minimum.)

>  #[repr(transparent)]
> -pub(crate) struct FormatAdapter<D, F> {
> +pub(super) struct FormatAdapter<D, F> {
>      inner: D,
>      _formatter: PhantomData<F>,
>  }
> @@ -87,27 +88,22 @@ impl<D, F> Writer for FormatAdapter<D, F>
>      F: Fn(&D, &mut fmt::Formatter<'_>) -> fmt::Result + 'static,
>  {
>      fn write(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
> -        // SAFETY: FormatAdapter<_, F> can only be constructed if F is inhabited
> +        // SAFETY: The callback API obtains this implementation only after
> +        // receiving a `&'static F`, so `F` is inhabited.
>          let f: &F = unsafe { materialize_zst() };
>          f(&self.inner, fmt)
>      }
>  }

> +// SAFETY: `FormatAdapter<D, F>` is transparent over `D`. The callback API
> +// receives a `&'static F`, which establishes its inhabitation invariant.

This is the problem - the impl itself can't know that we got a
`&'static F`. You'd have to demonstrate that at every cast site, and
the knowledge isn't there anymore because you removed the construction
invariant.

> +unsafe impl<D, F> Adapter<D> for FormatAdapter<D, F> {}