Introduce support for read-only, write-only, and read-write binary files
in Rust debugfs. This adds:

- BinaryWriter and BinaryReader traits for writing to and reading from
  user slices in binary form.
- New Dir methods: read_binary_file(), write_binary_file(),
  `read_write_binary_file`.
- Corresponding FileOps implementations: BinaryReadFile,
  BinaryWriteFile, BinaryReadWriteFile.

This allows kernel modules to expose arbitrary binary data through
debugfs, with proper support for offsets and partial reads/writes.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/debugfs.rs          |  67 ++++++++++++++-
 rust/kernel/debugfs/file_ops.rs | 144 +++++++++++++++++++++++++++++++-
 rust/kernel/debugfs/traits.rs   |  45 +++++++++-
 3 files changed, 249 insertions(+), 7 deletions(-)

diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index 381c23b3dd83..b1a3adca7fd4 100644
--- a/rust/kernel/debugfs.rs
+++ b/rust/kernel/debugfs.rs
@@ -21,12 +21,15 @@
 use core::ops::Deref;
 
 mod traits;
-pub use traits::{Reader, Writer};
+pub use traits::{BinaryReader, BinaryWriter, Reader, Writer};
 
 mod callback_adapters;
 use callback_adapters::{FormatAdapter, NoWriter, WritableAdapter};
 mod file_ops;
-use file_ops::{FileOps, ReadFile, ReadWriteFile, WriteFile};
+use file_ops::{
+    BinaryReadFile, BinaryReadWriteFile, BinaryWriteFile, FileOps, ReadFile, ReadWriteFile,
+    WriteFile,
+};
 #[cfg(CONFIG_DEBUG_FS)]
 mod entry;
 #[cfg(CONFIG_DEBUG_FS)]
@@ -150,6 +153,33 @@ pub fn read_only_file<'a, T, E: 'a>(
         self.create_file(name, data, file_ops)
     }
 
+    /// Creates a read-only binary file in this directory.
+    ///
+    /// The file's contents are produced by invoking [`BinaryWriter::write_to_slice`] on the value
+    /// initialized by `data`.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// # use kernel::prelude::*;
+    /// # let dir = Dir::new(c_str!("my_debugfs_dir"));
+    /// let file = KBox::pin_init(dir.read_binary_file(c_str!("foo"), [0x1, 0x2]), GFP_KERNEL)?;
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn read_binary_file<'a, T, E: 'a>(
+        &'a self,
+        name: &'a CStr,
+        data: impl PinInit<T, E> + 'a,
+    ) -> impl PinInit<File<T>, E> + 'a
+    where
+        T: BinaryWriter + Send + Sync + 'static,
+    {
+        let file_ops = &<T as BinaryReadFile<_>>::FILE_OPS;
+        self.create_file(name, data, file_ops)
+    }
+
     /// Creates a read-only file in this directory, with contents from a callback.
     ///
     /// `f` must be a function item or a non-capturing closure.
@@ -206,6 +236,22 @@ pub fn read_write_file<'a, T, E: 'a>(
         self.create_file(name, data, file_ops)
     }
 
+    /// Creates a read-write binary file in this directory.
+    ///
+    /// Reading the file uses the [`BinaryWriter`] implementation.
+    /// Writing to the file uses the [`BinaryReader`] implementation.
+    pub fn read_write_binary_file<'a, T, E: 'a>(
+        &'a self,
+        name: &'a CStr,
+        data: impl PinInit<T, E> + 'a,
+    ) -> impl PinInit<File<T>, E> + 'a
+    where
+        T: BinaryWriter + BinaryReader + Send + Sync + 'static,
+    {
+        let file_ops = &<T as BinaryReadWriteFile<_>>::FILE_OPS;
+        self.create_file(name, data, file_ops)
+    }
+
     /// Creates a read-write file in this directory, with logic from callbacks.
     ///
     /// Reading from the file is handled by `f`. Writing to the file is handled by `w`.
@@ -248,6 +294,23 @@ pub fn write_only_file<'a, T, E: 'a>(
         self.create_file(name, data, &T::FILE_OPS)
     }
 
+    /// Creates a write-only binary file in this directory.
+    ///
+    /// The file owns its backing data. Writing to the file uses the [`BinaryReader`]
+    /// implementation.
+    ///
+    /// The file is removed when the returned [`File`] is dropped.
+    pub fn write_binary_file<'a, T, E: 'a>(
+        &'a self,
+        name: &'a CStr,
+        data: impl PinInit<T, E> + 'a,
+    ) -> impl PinInit<File<T>, E> + 'a
+    where
+        T: BinaryReader + Send + Sync + 'static,
+    {
+        self.create_file(name, data, &T::FILE_OPS)
+    }
+
     /// Creates a write-only file in this directory, with write logic from a callback.
     ///
     /// `w` must be a function item or a non-capturing closure.
diff --git a/rust/kernel/debugfs/file_ops.rs b/rust/kernel/debugfs/file_ops.rs
index 50fead17b6f3..ef31cac4cd44 100644
--- a/rust/kernel/debugfs/file_ops.rs
+++ b/rust/kernel/debugfs/file_ops.rs
@@ -1,13 +1,13 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (C) 2025 Google LLC.
 
-use super::{Reader, Writer};
+use super::{BinaryReader, BinaryWriter, Reader, Writer};
 use crate::debugfs::callback_adapters::Adapter;
 use crate::prelude::*;
 use crate::seq_file::SeqFile;
 use crate::seq_print;
 use crate::uaccess::UserSlice;
-use core::fmt::{Display, Formatter, Result};
+use core::fmt;
 use core::marker::PhantomData;
 
 #[cfg(CONFIG_DEBUG_FS)]
@@ -65,8 +65,8 @@ fn deref(&self) -> &Self::Target {
 
 struct WriterAdapter<T>(T);
 
-impl<'a, T: Writer> Display for WriterAdapter<&'a T> {
-    fn fmt(&self, f: &mut Formatter<'_>) -> Result {
+impl<'a, T: Writer> fmt::Display for WriterAdapter<&'a T> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         self.0.write(f)
     }
 }
@@ -245,3 +245,139 @@ impl<T: Reader + Sync> WriteFile<T> for T {
         unsafe { FileOps::new(operations, 0o200) }
     };
 }
+
+extern "C" fn blob_read<T: BinaryWriter>(
+    file: *mut bindings::file,
+    buf: *mut c_char,
+    count: usize,
+    ppos: *mut bindings::loff_t,
+) -> isize {
+    // SAFETY:
+    // - `file` is a valid pointer to a `struct file`.
+    // - The type invariant of `FileOps` guarantees that `private_data` points to a valid `T`.
+    let this = unsafe { &*((*file).private_data.cast::<T>()) };
+
+    // SAFETY: `ppos` is a valid `loff_t` pointer.
+    let pos = unsafe { &mut *ppos };
+
+    let mut writer = UserSlice::new(UserPtr::from_ptr(buf.cast()), count).writer();
+
+    let ret = || -> Result<isize> {
+        let offset = (*pos).try_into()?;
+
+        let written = this.write_to_slice(&mut writer, offset)?;
+        *pos += bindings::loff_t::try_from(written)?;
+
+        Ok(written.try_into()?)
+    }();
+
+    match ret {
+        Ok(n) => n,
+        Err(e) => e.to_errno() as isize,
+    }
+}
+
+pub(crate) trait BinaryReadFile<T> {
+    const FILE_OPS: FileOps<T>;
+}
+
+impl<T: BinaryWriter + Sync> BinaryReadFile<T> for T {
+    const FILE_OPS: FileOps<T> = {
+        let operations = bindings::file_operations {
+            read: Some(blob_read::<T>),
+            llseek: Some(bindings::default_llseek),
+            open: Some(bindings::simple_open),
+            // SAFETY: `file_operations` supports zeroes in all fields.
+            ..unsafe { core::mem::zeroed() }
+        };
+
+        // SAFETY:
+        // - The private data of `struct inode` does always contain a pointer to a valid `T`.
+        // - `simple_open()` stores the `struct inode`'s private data in the private data of the
+        //   corresponding `struct file`.
+        // - `blob_read()` re-creates a reference to `T` from the `struct file`'s private data.
+        // - `default_llseek()` does not access the `struct file`'s private data.
+        unsafe { FileOps::new(operations, 0o400) }
+    };
+}
+
+extern "C" fn blob_write<T: BinaryReader>(
+    file: *mut bindings::file,
+    buf: *const c_char,
+    count: usize,
+    ppos: *mut bindings::loff_t,
+) -> isize {
+    // SAFETY:
+    // - `file` is a valid pointer to a `struct file`.
+    // - The type invariant of `FileOps` guarantees that `private_data` points to a valid `T`.
+    let this = unsafe { &*((*file).private_data.cast::<T>()) };
+
+    // SAFETY: `ppos` is a valid `loff_t` pointer.
+    let pos = unsafe { &mut *ppos };
+
+    let mut reader = UserSlice::new(UserPtr::from_ptr(buf.cast_mut().cast()), count).reader();
+
+    let ret = || -> Result<isize> {
+        let offset = (*pos).try_into()?;
+
+        let read = this.read_from_slice(&mut reader, offset)?;
+        *pos += bindings::loff_t::try_from(read)?;
+
+        Ok(read.try_into()?)
+    }();
+
+    match ret {
+        Ok(n) => n,
+        Err(e) => e.to_errno() as isize,
+    }
+}
+
+pub(crate) trait BinaryWriteFile<T> {
+    const FILE_OPS: FileOps<T>;
+}
+
+impl<T: BinaryReader + Sync> BinaryWriteFile<T> for T {
+    const FILE_OPS: FileOps<T> = {
+        let operations = bindings::file_operations {
+            write: Some(blob_write::<T>),
+            llseek: Some(bindings::default_llseek),
+            open: Some(bindings::simple_open),
+            // SAFETY: `file_operations` supports zeroes in all fields.
+            ..unsafe { core::mem::zeroed() }
+        };
+
+        // SAFETY:
+        // - The private data of `struct inode` does always contain a pointer to a valid `T`.
+        // - `simple_open()` stores the `struct inode`'s private data in the private data of the
+        //   corresponding `struct file`.
+        // - `blob_write()` re-creates a reference to `T` from the `struct file`'s private data.
+        // - `default_llseek()` does not access the `struct file`'s private data.
+        unsafe { FileOps::new(operations, 0o200) }
+    };
+}
+
+pub(crate) trait BinaryReadWriteFile<T> {
+    const FILE_OPS: FileOps<T>;
+}
+
+impl<T: BinaryWriter + BinaryReader + Sync> BinaryReadWriteFile<T> for T {
+    const FILE_OPS: FileOps<T> = {
+        let operations = bindings::file_operations {
+            read: Some(blob_read::<T>),
+            write: Some(blob_write::<T>),
+            llseek: Some(bindings::default_llseek),
+            open: Some(bindings::simple_open),
+            // SAFETY: `file_operations` supports zeroes in all fields.
+            ..unsafe { core::mem::zeroed() }
+        };
+
+        // SAFETY:
+        // - The private data of `struct inode` does always contain a pointer to a valid `T`.
+        // - `simple_open()` stores the `struct inode`'s private data in the private data of the
+        //   corresponding `struct file`.
+        // - `blob_read()` re-creates a reference to `T` from the `struct file`'s private data.
+        // - `blob_write()` re-creates a reference to `T` from the `struct file`'s private data.
+        // - `default_llseek()` does not access the `struct file`'s private data.
+        unsafe { FileOps::new(operations, 0o600) }
+    };
+}
diff --git a/rust/kernel/debugfs/traits.rs b/rust/kernel/debugfs/traits.rs
index ab009eb254b3..60a6ee6c6b58 100644
--- a/rust/kernel/debugfs/traits.rs
+++ b/rust/kernel/debugfs/traits.rs
@@ -5,7 +5,8 @@
 
 use crate::prelude::*;
 use crate::sync::Mutex;
-use crate::uaccess::UserSliceReader;
+use crate::transmute::{AsBytes, FromBytes};
+use crate::uaccess::{UserSliceReader, UserSliceWriter};
 use core::fmt::{self, Debug, Formatter};
 use core::str::FromStr;
 use core::sync::atomic::{
@@ -39,6 +40,30 @@ fn write(&self, f: &mut Formatter<'_>) -> fmt::Result {
     }
 }
 
+/// Trait for types that can be written out as binary.
+pub trait BinaryWriter {
+    /// Writes the binary form of `self` into `writer`.
+    ///
+    /// `offset` is the requested offset into the binary representation of `self`.
+    ///
+    /// On success, returns the number of bytes written in to `writer`.
+    fn write_to_slice(&self, writer: &mut UserSliceWriter, offset: usize) -> Result<usize>;
+}
+
+impl<T: AsBytes> BinaryWriter for T {
+    fn write_to_slice(&self, writer: &mut UserSliceWriter, offset: usize) -> Result<usize> {
+        writer.write_slice_partial(self.as_bytes(), offset)
+    }
+}
+
+impl<T: BinaryWriter> BinaryWriter for Mutex<T> {
+    fn write_to_slice(&self, writer: &mut UserSliceWriter, offset: usize) -> Result<usize> {
+        let guard = self.lock();
+
+        guard.write_to_slice(writer, offset)
+    }
+}
+
 /// A trait for types that can be updated from a user slice.
 ///
 /// This works similarly to `FromStr`, but operates on a `UserSliceReader` rather than a &str.
@@ -66,6 +91,24 @@ fn read_from_slice(&self, reader: &mut UserSliceReader) -> Result {
     }
 }
 
+/// Trait for types that can be constructed from a binary representation.
+pub trait BinaryReader {
+    /// Reads the binary form of `self` from `reader`.
+    ///
+    /// `offset` is the requested offset into the binary representation of `self`.
+    ///
+    /// On success, returns the number of bytes read from `reader`.
+    fn read_from_slice(&self, reader: &mut UserSliceReader, offset: usize) -> Result<usize>;
+}
+
+impl<T: AsBytes + FromBytes> BinaryReader for Mutex<T> {
+    fn read_from_slice(&self, reader: &mut UserSliceReader, offset: usize) -> Result<usize> {
+        let mut this = self.lock();
+
+        reader.read_slice_partial(this.as_bytes_mut(), offset)
+    }
+}
+
 macro_rules! impl_reader_for_atomic {
     ($(($atomic_type:ty, $int_type:ty)),*) => {
         $(
-- 
2.51.0

On Sat, Oct 04, 2025 at 12:26:40AM +0200, Danilo Krummrich wrote:
> Introduce support for read-only, write-only, and read-write binary files
> in Rust debugfs. This adds:
> 
> - BinaryWriter and BinaryReader traits for writing to and reading from
>   user slices in binary form.
> - New Dir methods: read_binary_file(), write_binary_file(),
>   `read_write_binary_file`.
> - Corresponding FileOps implementations: BinaryReadFile,
>   BinaryWriteFile, BinaryReadWriteFile.
> 
> This allows kernel modules to expose arbitrary binary data through
> debugfs, with proper support for offsets and partial reads/writes.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

> +extern "C" fn blob_write<T: BinaryReader>(
> +    file: *mut bindings::file,
> +    buf: *const c_char,
> +    count: usize,
> +    ppos: *mut bindings::loff_t,
> +) -> isize {
> +    // SAFETY:
> +    // - `file` is a valid pointer to a `struct file`.
> +    // - The type invariant of `FileOps` guarantees that `private_data` points to a valid `T`.
> +    let this = unsafe { &*((*file).private_data.cast::<T>()) };
> +
> +    // SAFETY: `ppos` is a valid `loff_t` pointer.
> +    let pos = unsafe { &mut *ppos };
> +
> +    let mut reader = UserSlice::new(UserPtr::from_ptr(buf.cast_mut().cast()), count).reader();
> +
> +    let ret = || -> Result<isize> {
> +        let offset = (*pos).try_into()?;

So offsets larger than the buffer result in Ok(0) unless the offset
doesn't fit in an usize, in which case it's an error instead? I think we
should treat offsets that are too large in the same manner no matter
how large they are.

> +        let read = this.read_from_slice(&mut reader, offset)?;
> +        *pos += bindings::loff_t::try_from(read)?;

This addition could overflow and panic the kernel.

> +        Ok(read.try_into()?)
> +    }();
> +
> +    match ret {
> +        Ok(n) => n,
> +        Err(e) => e.to_errno() as isize,
> +    }
> +}
> +
> +pub(crate) trait BinaryWriteFile<T> {
> +    const FILE_OPS: FileOps<T>;
> +}

Hmm ... this is inconsistent with how we do vtables in other parts of
`kernel`. Normally a struct is used instead of a trait (see e.g.
miscdevice or block). But the inconsistency is already present.

Alice

On Fri Oct 17, 2025 at 2:59 PM CEST, Alice Ryhl wrote:
> On Sat, Oct 04, 2025 at 12:26:40AM +0200, Danilo Krummrich wrote:
>> Introduce support for read-only, write-only, and read-write binary files
>> in Rust debugfs. This adds:
>> 
>> - BinaryWriter and BinaryReader traits for writing to and reading from
>>   user slices in binary form.
>> - New Dir methods: read_binary_file(), write_binary_file(),
>>   `read_write_binary_file`.
>> - Corresponding FileOps implementations: BinaryReadFile,
>>   BinaryWriteFile, BinaryReadWriteFile.
>> 
>> This allows kernel modules to expose arbitrary binary data through
>> debugfs, with proper support for offsets and partial reads/writes.
>> 
>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>
>> +extern "C" fn blob_write<T: BinaryReader>(
>> +    file: *mut bindings::file,
>> +    buf: *const c_char,
>> +    count: usize,
>> +    ppos: *mut bindings::loff_t,
>> +) -> isize {
>> +    // SAFETY:
>> +    // - `file` is a valid pointer to a `struct file`.
>> +    // - The type invariant of `FileOps` guarantees that `private_data` points to a valid `T`.
>> +    let this = unsafe { &*((*file).private_data.cast::<T>()) };
>> +
>> +    // SAFETY: `ppos` is a valid `loff_t` pointer.
>> +    let pos = unsafe { &mut *ppos };
>> +
>> +    let mut reader = UserSlice::new(UserPtr::from_ptr(buf.cast_mut().cast()), count).reader();
>> +
>> +    let ret = || -> Result<isize> {
>> +        let offset = (*pos).try_into()?;
>
> So offsets larger than the buffer result in Ok(0) unless the offset
> doesn't fit in an usize, in which case it's an error instead? I think we
> should treat offsets that are too large in the same manner no matter
> how large they are.

The offset being larger than thhe buffer is fine, userspace has to try to read
until the kernel indicates that there are no more bytes left to read by
returning zero.

But if the offset is larger than a usize there isn't a chance this can ever be
successful in the first place, hence I'd consider this an error.

>> +        let read = this.read_from_slice(&mut reader, offset)?;
>> +        *pos += bindings::loff_t::try_from(read)?;
>
> This addition could overflow and panic the kernel.

I don't see a real scenario where this could overflow when read_from_slice() was
successful, but I (also) think this should be checked_add() instead.

>> +        Ok(read.try_into()?)
>> +    }();
>> +
>> +    match ret {
>> +        Ok(n) => n,
>> +        Err(e) => e.to_errno() as isize,
>> +    }
>> +}
>> +
>> +pub(crate) trait BinaryWriteFile<T> {
>> +    const FILE_OPS: FileOps<T>;
>> +}
>
> Hmm ... this is inconsistent with how we do vtables in other parts of
> `kernel`. Normally a struct is used instead of a trait (see e.g.
> miscdevice or block). But the inconsistency is already present.

The reason I went with a trait is because that's consistent within the file.

Otherwise, I don't mind one or the other. If we always want to use a struct, I'm
fine with that. :)

On Fri, Oct 17, 2025 at 04:37:48PM +0200, Danilo Krummrich wrote:
> On Fri Oct 17, 2025 at 2:59 PM CEST, Alice Ryhl wrote:
> > On Sat, Oct 04, 2025 at 12:26:40AM +0200, Danilo Krummrich wrote:
> >> Introduce support for read-only, write-only, and read-write binary files
> >> in Rust debugfs. This adds:
> >> 
> >> - BinaryWriter and BinaryReader traits for writing to and reading from
> >>   user slices in binary form.
> >> - New Dir methods: read_binary_file(), write_binary_file(),
> >>   `read_write_binary_file`.
> >> - Corresponding FileOps implementations: BinaryReadFile,
> >>   BinaryWriteFile, BinaryReadWriteFile.
> >> 
> >> This allows kernel modules to expose arbitrary binary data through
> >> debugfs, with proper support for offsets and partial reads/writes.
> >> 
> >> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> >
> >> +extern "C" fn blob_write<T: BinaryReader>(
> >> +    file: *mut bindings::file,
> >> +    buf: *const c_char,
> >> +    count: usize,
> >> +    ppos: *mut bindings::loff_t,
> >> +) -> isize {
> >> +    // SAFETY:
> >> +    // - `file` is a valid pointer to a `struct file`.
> >> +    // - The type invariant of `FileOps` guarantees that `private_data` points to a valid `T`.
> >> +    let this = unsafe { &*((*file).private_data.cast::<T>()) };
> >> +
> >> +    // SAFETY: `ppos` is a valid `loff_t` pointer.
> >> +    let pos = unsafe { &mut *ppos };
> >> +
> >> +    let mut reader = UserSlice::new(UserPtr::from_ptr(buf.cast_mut().cast()), count).reader();
> >> +
> >> +    let ret = || -> Result<isize> {
> >> +        let offset = (*pos).try_into()?;
> >
> > So offsets larger than the buffer result in Ok(0) unless the offset
> > doesn't fit in an usize, in which case it's an error instead? I think we
> > should treat offsets that are too large in the same manner no matter
> > how large they are.
> 
> The offset being larger than thhe buffer is fine, userspace has to try to read
> until the kernel indicates that there are no more bytes left to read by
> returning zero.
> 
> But if the offset is larger than a usize there isn't a chance this can ever be
> successful in the first place, hence I'd consider this an error.

I don't really agree with this. Obviously we have to return Ok(0) if the
position is equal to the buffer size. But for positions strictly larger
than the buffer size I think it's reasonable to choose between Ok(0) or
an error. But please, let's be consistent about whether we return Ok(0)
or errors for positions larger than the buffer size.

Besides, it could succeed. Imagine a debugfs file whose contents aren't
stored in memory as a big byte array, but can be fetched / computed on
the fly based on any offset. Such a file could be larger than 4GB on a
32-bit machine no problem.

Alice

On Sun Oct 19, 2025 at 11:50 AM CEST, Alice Ryhl wrote:
> On Fri, Oct 17, 2025 at 04:37:48PM +0200, Danilo Krummrich wrote:
>> On Fri Oct 17, 2025 at 2:59 PM CEST, Alice Ryhl wrote:
>> > On Sat, Oct 04, 2025 at 12:26:40AM +0200, Danilo Krummrich wrote:
>> >> Introduce support for read-only, write-only, and read-write binary files
>> >> in Rust debugfs. This adds:
>> >> 
>> >> - BinaryWriter and BinaryReader traits for writing to and reading from
>> >>   user slices in binary form.
>> >> - New Dir methods: read_binary_file(), write_binary_file(),
>> >>   `read_write_binary_file`.
>> >> - Corresponding FileOps implementations: BinaryReadFile,
>> >>   BinaryWriteFile, BinaryReadWriteFile.
>> >> 
>> >> This allows kernel modules to expose arbitrary binary data through
>> >> debugfs, with proper support for offsets and partial reads/writes.
>> >> 
>> >> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> >
>> >> +extern "C" fn blob_write<T: BinaryReader>(
>> >> +    file: *mut bindings::file,
>> >> +    buf: *const c_char,
>> >> +    count: usize,
>> >> +    ppos: *mut bindings::loff_t,
>> >> +) -> isize {
>> >> +    // SAFETY:
>> >> +    // - `file` is a valid pointer to a `struct file`.
>> >> +    // - The type invariant of `FileOps` guarantees that `private_data` points to a valid `T`.
>> >> +    let this = unsafe { &*((*file).private_data.cast::<T>()) };
>> >> +
>> >> +    // SAFETY: `ppos` is a valid `loff_t` pointer.
>> >> +    let pos = unsafe { &mut *ppos };
>> >> +
>> >> +    let mut reader = UserSlice::new(UserPtr::from_ptr(buf.cast_mut().cast()), count).reader();
>> >> +
>> >> +    let ret = || -> Result<isize> {
>> >> +        let offset = (*pos).try_into()?;
>> >
>> > So offsets larger than the buffer result in Ok(0) unless the offset
>> > doesn't fit in an usize, in which case it's an error instead? I think we
>> > should treat offsets that are too large in the same manner no matter
>> > how large they are.
>> 
>> The offset being larger than thhe buffer is fine, userspace has to try to read
>> until the kernel indicates that there are no more bytes left to read by
>> returning zero.
>> 
>> But if the offset is larger than a usize there isn't a chance this can ever be
>> successful in the first place, hence I'd consider this an error.
>
> I don't really agree with this. Obviously we have to return Ok(0) if the
> position is equal to the buffer size. But for positions strictly larger
> than the buffer size I think it's reasonable to choose between Ok(0) or
> an error. But please, let's be consistent about whether we return Ok(0)
> or errors for positions larger than the buffer size.

There's not really a choice, it has to be Ok(0), otherwise we break userspace.

However, you do have a point with how the offset conversion to usize should be
handled. We shouldn't try to convert it to usize in the first place, but rather
pass it through as it is and make it a common offset-larger-buffer case.

We probably also want a type alias for bindings::loff_t.

On Sun, Oct 19, 2025 at 01:24:05PM +0200, Danilo Krummrich wrote:
> On Sun Oct 19, 2025 at 11:50 AM CEST, Alice Ryhl wrote:
> > On Fri, Oct 17, 2025 at 04:37:48PM +0200, Danilo Krummrich wrote:
> >> On Fri Oct 17, 2025 at 2:59 PM CEST, Alice Ryhl wrote:
> >> > On Sat, Oct 04, 2025 at 12:26:40AM +0200, Danilo Krummrich wrote:
> >> >> Introduce support for read-only, write-only, and read-write binary files
> >> >> in Rust debugfs. This adds:
> >> >> 
> >> >> - BinaryWriter and BinaryReader traits for writing to and reading from
> >> >>   user slices in binary form.
> >> >> - New Dir methods: read_binary_file(), write_binary_file(),
> >> >>   `read_write_binary_file`.
> >> >> - Corresponding FileOps implementations: BinaryReadFile,
> >> >>   BinaryWriteFile, BinaryReadWriteFile.
> >> >> 
> >> >> This allows kernel modules to expose arbitrary binary data through
> >> >> debugfs, with proper support for offsets and partial reads/writes.
> >> >> 
> >> >> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> >> >
> >> >> +extern "C" fn blob_write<T: BinaryReader>(
> >> >> +    file: *mut bindings::file,
> >> >> +    buf: *const c_char,
> >> >> +    count: usize,
> >> >> +    ppos: *mut bindings::loff_t,
> >> >> +) -> isize {
> >> >> +    // SAFETY:
> >> >> +    // - `file` is a valid pointer to a `struct file`.
> >> >> +    // - The type invariant of `FileOps` guarantees that `private_data` points to a valid `T`.
> >> >> +    let this = unsafe { &*((*file).private_data.cast::<T>()) };
> >> >> +
> >> >> +    // SAFETY: `ppos` is a valid `loff_t` pointer.
> >> >> +    let pos = unsafe { &mut *ppos };
> >> >> +
> >> >> +    let mut reader = UserSlice::new(UserPtr::from_ptr(buf.cast_mut().cast()), count).reader();
> >> >> +
> >> >> +    let ret = || -> Result<isize> {
> >> >> +        let offset = (*pos).try_into()?;
> >> >
> >> > So offsets larger than the buffer result in Ok(0) unless the offset
> >> > doesn't fit in an usize, in which case it's an error instead? I think we
> >> > should treat offsets that are too large in the same manner no matter
> >> > how large they are.
> >> 
> >> The offset being larger than thhe buffer is fine, userspace has to try to read
> >> until the kernel indicates that there are no more bytes left to read by
> >> returning zero.
> >> 
> >> But if the offset is larger than a usize there isn't a chance this can ever be
> >> successful in the first place, hence I'd consider this an error.
> >
> > I don't really agree with this. Obviously we have to return Ok(0) if the
> > position is equal to the buffer size. But for positions strictly larger
> > than the buffer size I think it's reasonable to choose between Ok(0) or
> > an error. But please, let's be consistent about whether we return Ok(0)
> > or errors for positions larger than the buffer size.
> 
> There's not really a choice, it has to be Ok(0), otherwise we break userspace.
> 
> However, you do have a point with how the offset conversion to usize should be
> handled. We shouldn't try to convert it to usize in the first place, but rather
> pass it through as it is and make it a common offset-larger-buffer case.

SGTM.

> We probably also want a type alias for bindings::loff_t.

I ended up using i64 for simple_read_from_buffer in iov.rs instead of
loff_t. But if they can differ, then yeah let's introduce a loff_t type
alias.

Alice

On Mon Oct 20, 2025 at 10:13 AM CEST, Alice Ryhl wrote:
> I ended up using i64 for simple_read_from_buffer in iov.rs instead of
> loff_t. But if they can differ, then yeah let's introduce a loff_t type
> alias.

No, I don't think they can differ (I used i64 in earlier version that didn't
make it to the list as well), but I think it could still make sense to indicate
the relationship with loff_t. When I see an i64, an offset into a buffer is not
the first thing that comes to my mind.

What about uaccess::Offset?

On Mon, Oct 20, 2025 at 11:35:15AM +0200, Danilo Krummrich wrote:
> On Mon Oct 20, 2025 at 10:13 AM CEST, Alice Ryhl wrote:
> > I ended up using i64 for simple_read_from_buffer in iov.rs instead of
> > loff_t. But if they can differ, then yeah let's introduce a loff_t type
> > alias.
> 
> No, I don't think they can differ (I used i64 in earlier version that didn't
> make it to the list as well), but I think it could still make sense to indicate
> the relationship with loff_t. When I see an i64, an offset into a buffer is not
> the first thing that comes to my mind.
> 
> What about uaccess::Offset?

Hmm. That seems wrong. loff_t is a *file position*, so it should go in
kernel::fs, right? We're only using it in uaccess/iov because they
happen to have utility methods to help with implementing fops entries.
None of the "base" uaccess/iov functions use loff_t for anything since
they deal with sizes in the address space, for which usize is the
correct type.

Alice

On Mon Oct 20, 2025 at 11:39 AM CEST, Alice Ryhl wrote:
> On Mon, Oct 20, 2025 at 11:35:15AM +0200, Danilo Krummrich wrote:
>> On Mon Oct 20, 2025 at 10:13 AM CEST, Alice Ryhl wrote:
>> > I ended up using i64 for simple_read_from_buffer in iov.rs instead of
>> > loff_t. But if they can differ, then yeah let's introduce a loff_t type
>> > alias.
>> 
>> No, I don't think they can differ (I used i64 in earlier version that didn't
>> make it to the list as well), but I think it could still make sense to indicate
>> the relationship with loff_t. When I see an i64, an offset into a buffer is not
>> the first thing that comes to my mind.
>> 
>> What about uaccess::Offset?
>
> Hmm. That seems wrong. loff_t is a *file position*, so it should go in
> kernel::fs, right? We're only using it in uaccess/iov because they
> happen to have utility methods to help with implementing fops entries.
> None of the "base" uaccess/iov functions use loff_t for anything since
> they deal with sizes in the address space, for which usize is the
> correct type.

Indeed, fair enough.

On Fri Oct 17, 2025 at 4:37 PM CEST, Danilo Krummrich wrote:
> The reason I went with a trait is because that's consistent within the file.
>
> Otherwise, I don't mind one or the other. If we always want to use a struct, I'm
> fine with that. :)

Actually, there's another reason I forgot about since I sent the series. :)

We need it because we derive it from blanket implementations:

	impl<T: BinaryWriter + Sync> BinaryReadFile<T> for T
	impl<T: BinaryReader + Sync> BinaryWriteFile<T> for T
	impl<T: BinaryWriter + BinaryReader + Sync> BinaryReadWriteFile<T> for T

On Fri, Oct 17, 2025 at 04:53:09PM +0200, Danilo Krummrich wrote:
> On Fri Oct 17, 2025 at 4:37 PM CEST, Danilo Krummrich wrote:
> > The reason I went with a trait is because that's consistent within the file.
> >
> > Otherwise, I don't mind one or the other. If we always want to use a struct, I'm
> > fine with that. :)
> 
> Actually, there's another reason I forgot about since I sent the series. :)
> 
> We need it because we derive it from blanket implementations:
> 
> 	impl<T: BinaryWriter + Sync> BinaryReadFile<T> for T
> 	impl<T: BinaryReader + Sync> BinaryWriteFile<T> for T
> 	impl<T: BinaryWriter + BinaryReader + Sync> BinaryReadWriteFile<T> for T

You can still use a struct:

struct BinaryWriterVtable<T: BinaryWriter + Sync>;

impl<T: BinaryWriter + Sync> BinaryWriterVtable<T> {
    const VTABLE: bindings::foo = ...;
}

On Sun Oct 19, 2025 at 11:44 AM CEST, Alice Ryhl wrote:
> On Fri, Oct 17, 2025 at 04:53:09PM +0200, Danilo Krummrich wrote:
>> On Fri Oct 17, 2025 at 4:37 PM CEST, Danilo Krummrich wrote:
>> > The reason I went with a trait is because that's consistent within the file.
>> >
>> > Otherwise, I don't mind one or the other. If we always want to use a struct, I'm
>> > fine with that. :)
>> 
>> Actually, there's another reason I forgot about since I sent the series. :)
>> 
>> We need it because we derive it from blanket implementations:
>> 
>> 	impl<T: BinaryWriter + Sync> BinaryReadFile<T> for T
>> 	impl<T: BinaryReader + Sync> BinaryWriteFile<T> for T
>> 	impl<T: BinaryWriter + BinaryReader + Sync> BinaryReadWriteFile<T> for T
>
> You can still use a struct:
>
> struct BinaryWriterVtable<T: BinaryWriter + Sync>;
>
> impl<T: BinaryWriter + Sync> BinaryWriterVtable<T> {
>     const VTABLE: bindings::foo = ...;
> }

Yeah, but do we get something for adding yet another type in this case?

Another point to consider is if we want a more generic fops abstraction type.

In any case, I'd like to add this as good first issue for the whole file to be
changed accordingly.

On Sun, Oct 19, 2025 at 02:01:03PM +0200, Danilo Krummrich wrote:
> On Sun Oct 19, 2025 at 11:44 AM CEST, Alice Ryhl wrote:
> > On Fri, Oct 17, 2025 at 04:53:09PM +0200, Danilo Krummrich wrote:
> >> On Fri Oct 17, 2025 at 4:37 PM CEST, Danilo Krummrich wrote:
> >> > The reason I went with a trait is because that's consistent within the file.
> >> >
> >> > Otherwise, I don't mind one or the other. If we always want to use a struct, I'm
> >> > fine with that. :)
> >> 
> >> Actually, there's another reason I forgot about since I sent the series. :)
> >> 
> >> We need it because we derive it from blanket implementations:
> >> 
> >> 	impl<T: BinaryWriter + Sync> BinaryReadFile<T> for T
> >> 	impl<T: BinaryReader + Sync> BinaryWriteFile<T> for T
> >> 	impl<T: BinaryWriter + BinaryReader + Sync> BinaryReadWriteFile<T> for T
> >
> > You can still use a struct:
> >
> > struct BinaryWriterVtable<T: BinaryWriter + Sync>;
> >
> > impl<T: BinaryWriter + Sync> BinaryWriterVtable<T> {
> >     const VTABLE: bindings::foo = ...;
> > }
> 
> Yeah, but do we get something for adding yet another type in this case?
> 
> Another point to consider is if we want a more generic fops abstraction type.
> 
> In any case, I'd like to add this as good first issue for the whole file to be
> changed accordingly.

Yes, keep it as-is for consistency with the rest of the file, even if
the file is inconsistent with the rest of `kernel`. Please go ahead and
file a good-first-issue for this.

Alice

On Mon Oct 20, 2025 at 10:12 AM CEST, Alice Ryhl wrote:
> On Sun, Oct 19, 2025 at 02:01:03PM +0200, Danilo Krummrich wrote:
>> On Sun Oct 19, 2025 at 11:44 AM CEST, Alice Ryhl wrote:
>> > On Fri, Oct 17, 2025 at 04:53:09PM +0200, Danilo Krummrich wrote:
>> >> On Fri Oct 17, 2025 at 4:37 PM CEST, Danilo Krummrich wrote:
>> >> > The reason I went with a trait is because that's consistent within the file.
>> >> >
>> >> > Otherwise, I don't mind one or the other. If we always want to use a struct, I'm
>> >> > fine with that. :)
>> >> 
>> >> Actually, there's another reason I forgot about since I sent the series. :)
>> >> 
>> >> We need it because we derive it from blanket implementations:
>> >> 
>> >> 	impl<T: BinaryWriter + Sync> BinaryReadFile<T> for T
>> >> 	impl<T: BinaryReader + Sync> BinaryWriteFile<T> for T
>> >> 	impl<T: BinaryWriter + BinaryReader + Sync> BinaryReadWriteFile<T> for T
>> >
>> > You can still use a struct:
>> >
>> > struct BinaryWriterVtable<T: BinaryWriter + Sync>;
>> >
>> > impl<T: BinaryWriter + Sync> BinaryWriterVtable<T> {
>> >     const VTABLE: bindings::foo = ...;
>> > }
>> 
>> Yeah, but do we get something for adding yet another type in this case?
>> 
>> Another point to consider is if we want a more generic fops abstraction type.
>> 
>> In any case, I'd like to add this as good first issue for the whole file to be
>> changed accordingly.
>
> Yes, keep it as-is for consistency with the rest of the file, even if
> the file is inconsistent with the rest of `kernel`. Please go ahead and
> file a good-first-issue for this.

Before doing so, can you please answer the question above? While I'm all for
consistency, in this specific case it seems we'd need another indirection for
that. And I'm not convinced that's an improvement.

On Mon, Oct 20, 2025 at 11:40 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon Oct 20, 2025 at 10:12 AM CEST, Alice Ryhl wrote:
> > On Sun, Oct 19, 2025 at 02:01:03PM +0200, Danilo Krummrich wrote:
> >> On Sun Oct 19, 2025 at 11:44 AM CEST, Alice Ryhl wrote:
> >> > On Fri, Oct 17, 2025 at 04:53:09PM +0200, Danilo Krummrich wrote:
> >> >> On Fri Oct 17, 2025 at 4:37 PM CEST, Danilo Krummrich wrote:
> >> >> > The reason I went with a trait is because that's consistent within the file.
> >> >> >
> >> >> > Otherwise, I don't mind one or the other. If we always want to use a struct, I'm
> >> >> > fine with that. :)
> >> >>
> >> >> Actually, there's another reason I forgot about since I sent the series. :)
> >> >>
> >> >> We need it because we derive it from blanket implementations:
> >> >>
> >> >>   impl<T: BinaryWriter + Sync> BinaryReadFile<T> for T
> >> >>   impl<T: BinaryReader + Sync> BinaryWriteFile<T> for T
> >> >>   impl<T: BinaryWriter + BinaryReader + Sync> BinaryReadWriteFile<T> for T
> >> >
> >> > You can still use a struct:
> >> >
> >> > struct BinaryWriterVtable<T: BinaryWriter + Sync>;
> >> >
> >> > impl<T: BinaryWriter + Sync> BinaryWriterVtable<T> {
> >> >     const VTABLE: bindings::foo = ...;
> >> > }
> >>
> >> Yeah, but do we get something for adding yet another type in this case?
> >>
> >> Another point to consider is if we want a more generic fops abstraction type.
> >>
> >> In any case, I'd like to add this as good first issue for the whole file to be
> >> changed accordingly.
> >
> > Yes, keep it as-is for consistency with the rest of the file, even if
> > the file is inconsistent with the rest of `kernel`. Please go ahead and
> > file a good-first-issue for this.
>
> Before doing so, can you please answer the question above? While I'm all for
> consistency, in this specific case it seems we'd need another indirection for
> that. And I'm not convinced that's an improvement.

The choice is between adding a new type or a new trait. There's no
intrinsic advantage to choosing either one, but the rest of `kernel`
chose "new type" over "new trait", so it makes sense to be consistent.

Alice

On Mon Oct 20, 2025 at 11:42 AM CEST, Alice Ryhl wrote:
> On Mon, Oct 20, 2025 at 11:40 AM Danilo Krummrich <dakr@kernel.org> wrote:
>>
>> On Mon Oct 20, 2025 at 10:12 AM CEST, Alice Ryhl wrote:
>> > On Sun, Oct 19, 2025 at 02:01:03PM +0200, Danilo Krummrich wrote:
>> >> On Sun Oct 19, 2025 at 11:44 AM CEST, Alice Ryhl wrote:
>> >> > On Fri, Oct 17, 2025 at 04:53:09PM +0200, Danilo Krummrich wrote:
>> >> >> On Fri Oct 17, 2025 at 4:37 PM CEST, Danilo Krummrich wrote:
>> >> >> > The reason I went with a trait is because that's consistent within the file.
>> >> >> >
>> >> >> > Otherwise, I don't mind one or the other. If we always want to use a struct, I'm
>> >> >> > fine with that. :)
>> >> >>
>> >> >> Actually, there's another reason I forgot about since I sent the series. :)
>> >> >>
>> >> >> We need it because we derive it from blanket implementations:
>> >> >>
>> >> >>   impl<T: BinaryWriter + Sync> BinaryReadFile<T> for T
>> >> >>   impl<T: BinaryReader + Sync> BinaryWriteFile<T> for T
>> >> >>   impl<T: BinaryWriter + BinaryReader + Sync> BinaryReadWriteFile<T> for T
>> >> >
>> >> > You can still use a struct:
>> >> >
>> >> > struct BinaryWriterVtable<T: BinaryWriter + Sync>;
>> >> >
>> >> > impl<T: BinaryWriter + Sync> BinaryWriterVtable<T> {
>> >> >     const VTABLE: bindings::foo = ...;
>> >> > }
>> >>
>> >> Yeah, but do we get something for adding yet another type in this case?
>> >>
>> >> Another point to consider is if we want a more generic fops abstraction type.
>> >>
>> >> In any case, I'd like to add this as good first issue for the whole file to be
>> >> changed accordingly.
>> >
>> > Yes, keep it as-is for consistency with the rest of the file, even if
>> > the file is inconsistent with the rest of `kernel`. Please go ahead and
>> > file a good-first-issue for this.
>>
>> Before doing so, can you please answer the question above? While I'm all for
>> consistency, in this specific case it seems we'd need another indirection for
>> that. And I'm not convinced that's an improvement.
>
> The choice is between adding a new type or a new trait. There's no
> intrinsic advantage to choosing either one, but the rest of `kernel`
> chose "new type" over "new trait", so it makes sense to be consistent.

My hesitation came from the assumption that we'd need another type (additional
to the existing trait). But we can indeed replace it, so that's fine.