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

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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>>  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, I think using the potentially panicking operand is probably the best option,
but I'd also be fine taking the patch as it is.