Re: [PATCH v2 2/9] rust: debugfs: add SeqShow trait and seq_file file operations

On Fri, Jul 31, 2026 at 6:52=E2=80=AFPM Alvin Sun <[email protected]> wro=
te:
>
>
> On 7/31/26 21:08, Alice Ryhl wrote:
> > On Fri, Jul 31, 2026 at 01:05:40AM +0800, Alvin Sun wrote:
> >> +/// Show callback for `SeqShow` types.
> >> +///
> >> +/// # Safety
> >> +///
> >> +/// The seq_file core guarantees that `seq` points to a live `seq_fil=
e` and that
> >> +/// `seq->private` is a valid pointer to a `T` with no outstanding mu=
table
> >> +/// references.
> >> +unsafe extern "C" fn seq_file_show<S: SeqShow<T>, T: Sync>(
> >> +    seq: *mut bindings::seq_file,
> >> +    _: *mut crate::ffi::c_void,
> >> +) -> crate::ffi::c_int {
> >> +    // SAFETY: `seq->private` is a valid `T` pointer with no outstand=
ing
> >> +    // mutable references.
> >> +    let data =3D unsafe { &*((*seq).private.cast::<T>()) };
> >> +    // SAFETY: `seq` points to a live `seq_file`.
> >> +    let m =3D unsafe { SeqFile::from_raw(seq) };
> >> +    from_result(|| S::show(data, m).map(|()| 0))
> >> +}
> >> +
> >> +/// Renders `data` into a seq_file.
> >> +///
> >> +/// `data` is the value stashed as the debugfs file's `i_private` at =
creation
> >> +/// time. `show` is invoked on each read to produce the file's conten=
ts.
> >> +///
> >> +/// `open` and `release` are optional lifecycle hooks called during f=
ile open
> >> +/// and release. They can be used to manage the lifetime of `data` (e=
.g.,
> >> +/// reference counting). Default implementations are no-ops.
> >> +pub trait SeqShow<T> {
> >> +    /// Writes debugfs output for the file.
> >> +    fn show(data: &T, m: &SeqFile) -> Result;
> >> +
> >> +    /// Called during file open, before `single_open`.
> >> +    fn open(_data: &T) -> Result {
> >> +        Ok(())
> >> +    }
> >> +
> >> +    /// Called during file release, before `single_release`.
> >> +    ///
> >> +    /// # Safety
> >> +    ///
> >> +    /// `data` must point to valid memory, kept alive by actions take=
n in
> >> +    /// [`Self::open`] (e.g., incrementing a reference count).
> >> +    unsafe fn release(_data: NonNull<T>) {}
> >> +}
> > Hmm. So in a later patch you implement SeqShow in the following manner:
> >
> >      fn open(dev: &drm::Device<T, drm::Normal>) -> Result {
> >          // Hold a device reference so the device is not freed while th=
e file is open.
> >          dev.inc_ref();
> >          Ok(())
> >      }
> >
> >      unsafe fn release(dev: NonNull<drm::Device<T, drm::Normal>>) {
> >          // Drop the reference taken in `open`.
> >          // SAFETY: `dev` is valid per this function's safety contract.
> >          unsafe { drm::Device::<T>::dec_ref(dev) };
> >      }
> >
> > I don't understand why this increment/decrement pair is necessary.
> > Just based on the code, it *looks* like you have them because otherwise
> > `show()` might get called with a dangling pointer to the drm device, bu=
t
> > the inc_ref() in open() ensures it stays alive.
> >
> > However, earlier in this patch you said:
> >
> >> The seq_file core guarantees that `seq` points to a live `seq_file` an=
d
> >> that `seq->private` is a valid pointer to a `T` with no outstanding
> >> mutable references.
> > So based on this, you are saying that open()/release() do not need to d=
o
> > anything special for the value to stay alive.
> >
> > These seem contradictory. Could you clarify the situation?
>
> You're right. The `SeqShow` trait documentation was inaccurate -- the
> framework does not manage the lifetime of `data`, so `inc_ref`/`dec_ref`
> in `DrmSeqShow` is necessary to prevent `show()` from accessing a freed
> device.
>
> Will fix the doc in the next revision.

I'm not convinced that works soundness-wise. If I just leave
open/release empty, then I will be able to access a &T inside of
show(), but there's nothing keeping it alive, so I could dereference a
freed &T in safe code that way.

I think we need the seq file to instead own some sort of value. Then,
show() can be passed the a reference to value it has ownership of, and
release() can be said type's destructor.

Alice