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

["Danilo Krummrich" <dakr@kernel.org>]

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. :)