Re: [PATCH] rust: dma: return zero for Coherent reads past EOF

["Alexandre Courbot" <acourbot@nvidia.com>]

On Fri Jul 24, 2026 at 10:22 AM JST, Danilo Krummrich wrote:
> On Fri Jul 24, 2026 at 2:46 AM CEST, Gary Guo wrote:
>> On Fri Jul 24, 2026 at 1:18 AM BST, Alexandre Courbot wrote:
>>> On Fri Jul 24, 2026 at 8:32 AM JST, Gary Guo wrote:
>>>> On Mon Jul 20, 2026 at 8:21 PM BST, Younes Akhouayri via B4 Relay wrote:
>>>>> From: Younes Akhouayri <git@younes.io>
>>>>>
>>>>> Coherent<T>::write_to_slice() calculates a zero-byte copy when the file
>>>>> offset is beyond the allocation, but still calls
>>>>> UserSliceWriter::write_dma(). The latter rejects offsets beyond the
>>>>> allocation even when the copy length is zero, so a debugfs read past EOF
>>>>> returns -ERANGE.
>>>>>
>>>>> Return before calling write_dma() when the offset is at or beyond the
>>>>> allocation, matching simple_read_from_buffer() EOF semantics.
>>>>>
>>>>> Fixes: 016818513936 ("rust: dma: implement BinaryWriter for Coherent<[u8]>")
>>>>> Signed-off-by: Younes Akhouayri <git@younes.io>
>>>>> ---
>>>>>  rust/kernel/dma.rs | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
>>>>> index 200def84fb69..36b72fb9ab46 100644
>>>>> --- a/rust/kernel/dma.rs
>>>>> +++ b/rust/kernel/dma.rs
>>>>> @@ -1005,6 +1005,10 @@ fn write_to_slice(
>>>>>              return Ok(0);
>>>>>          };
>>>>>  
>>>>> +        if offset_val >= self.size() {
>>>>> +            return Ok(0);
>>>>> +        }
>>>>> +
>>>>>          let count = self.size().saturating_sub(offset_val).min(writer.len());
>>>>
>>>> This saturating_sub is redundant now and should just use -.
>>>
>>> Or even better, you could use `checked_sub` to perform the test and the
>>> subtraction in one go, while avoiding potentially panicking operands.
>>
>> I think the code is clearer with separate EOF check length calc. This also needs
>> to return `Ok` so it's not like that you can use `?`.
>
> At a first glance I also thought of checked_sub(), and I think
>
> 	let Some(count) = self.size().checked_sub(offset_val) else {
> 	    return Ok(0);
> 	};
>
> 	writer.write_dma(self, offset_val, count.min(writer.len()))?;
>
> would work perfectly fine, but compared to Gary's proposal it still calls
> write_dma() for offset_val == self.size(), which is harmless but unnecessary.
>
> Another option to avoid the panicking operand would be to keep saturating_sub()
> and add the check below the existing saturating_sub() call.
>
> 	if count == 0 {
> 	    return Ok(0);
> 	}
>
> But this is also less obvious, as it hides a bit the fact that the problem is
> about offset_val when calling write_dma().

So far this is the version I like best; a comment to clarify the
intent of the code should also remove all ambiguity.

What I had in mind was something like

    let count = match self.size().checked_sub(offset_val) {
          Some(0) | None => return Ok(0),
          Some(count) => count.min(writer.len()),
    };

... but I understand the concern that this is less legible.

The most important imho is that this check gives us an opportunity to
remove a panicking operator; granted, the check is enough to guarantee
the panic case will never be met, but it's still cleaner to not use
these when it is possible and easy to do so, which is the case here.