Re: [PATCH 3/7] rust: debugfs: support for binary large objects

[Alice Ryhl <aliceryhl@google.com>]

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