Re: [PATCH v11 2/3] rust: add dma coherent allocator abstraction.

[Abdiel Janulgue]

On 23/01/2025 14:30, Petr Tesařík wrote:
> On Thu, 23 Jan 2025 12:42:58 +0200
> Abdiel Janulgue <abdiel.janulgue@xxxxxxxxx> wrote:
> > +
> > +    /// Reads data from the region starting from `offset` as a slice.
> > +    /// `offset` and `count` are in units of `T`, not the number of bytes.
> > +    ///
> > +    /// Due to the safety requirements of slice, the data returned should be regarded by the
> > +    /// caller as a snapshot of the region when this function is called, as the region could
> > +    /// be modified by the device at anytime. For ringbuffer type of r/w access or use-cases
> > +    /// where the pointer to the live data is needed, `start_ptr()` or `start_ptr_mut()`
> > +    /// could be used instead.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that no hardware operations that involve the buffer are currently
> > +    /// taking place while the returned slice is live.
> > +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
> > +        if offset + count >= self.count {
>
> I'm probably missing something, but how do you know that this addition
> can't overflow? I mean, since this is a public function, users can do
> something dumb such as buf.as_slice(usize::MAX, n), can't they?
>
> What about something like:
>
>      let end = offset.checked_add(count).ok_or(EOVERFLOW)?;
>      if end >= self.count { ... }

Makes sense. This could also just return EINVAL instead of EOVERFLOW, 
but either way is fine with me.

Miguel, what do you think? Would it be possible just include this change 
and the one below locally if you think this series is ready for merging?

Regards,
Abdiel

> > +            return Err(EINVAL);
> > +        }
> > +        // SAFETY:
> > +        // - The pointer is valid due to type invariant on `CoherentAllocation`,
> > +        // we've just checked that the range and index is within bounds. The immutability of the
> > +        // of data is also guaranteed by the safety requirements of the function.
> > +        // - `offset` can't overflow since it is smaller than `self.count` and we've checked
> > +        // that `self.count` won't overflow early in the constructor.
> > +        Ok(unsafe { core::slice::from_raw_parts(self.cpu_addr.add(offset), count) })
> > +    }
> > +
> > +    /// Writes data to the region starting from `offset`. `offset` is in units of `T`, not the
> > +    /// number of bytes.
> > +    pub fn write(&self, src: &[T], offset: usize) -> Result {
> > +        if offset + src.len() >= self.count {
>
> Same here.
>
> Petr T