Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [Low] Missing lifetime specifiers in trait implementations for reference types. Pre-existing issues: - [High] The safe trait `Io` provides safe methods that perform unchecked `unsafe` operations relying on safe, overridable methods, violating Rust's soundness guarantees. -- commit d5084ac7c26ce79e46d5cb18b40703205bee53dd Author: Gary Guo <[email protected]> rust: io: implement Io on reference types instead Currently, Io is implemented on owned I/O objects (e.g. Bar). This is going to change with I/O projections, as then Io need to work both for owned objects and views of them. Views are themselves reference-like (however they obviously cannot be references, because they belong to a different address space). To faciliate the change, change Io to be implemented on reference types for the owned I/O objects, and make methods take self instead of &self. When I/O views are implemented, we can then naturally implement Io for these objects. > diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs > index 38281636b4da3..3b478dcf10f95 100644 > --- a/rust/kernel/io.rs > +++ b/rust/kernel/io.rs [ ... ] > @@ -338,7 +338,7 @@ fn io_addr<U>(&self, offset: usize) -> Result<usize> { > > /// Fallible 8-bit read with runtime bounds check. > #[inline(always)] > - fn try_read8(&self, offset: usize) -> Result<u8> > + fn try_read8(self, offset: usize) -> Result<u8> [Severity: High] This is a pre-existing issue, but does the Io trait need to be marked as unsafe? The safe trait Io provides safe methods like try_read8() which contain an unsafe block calling self.io_read(). The safety of this block assumes that self.io_addr() always returns a valid memory address. However, since Io is a safe trait, any safe Rust code can implement it and arbitrarily override methods like io_addr(), addr(), or maxsize() to return invalid addresses. If this occurs, calling the safe try_read8() function will result in undefined behavior. Should Io or IoCapable be an unsafe trait so that the implementor is held formally responsible for upholding the invariants required by the unsafe blocks in the default methods? [ ... ] > @@ -800,13 +800,13 @@ fn update<T, L, F>(&self, location: L, f: F) > macro_rules! impl_mmio_io_capable { > ($mmio:ident, $(#[$attr:meta])* $ty:ty, $read_fn:ident, $write_fn:ident) > => { > $(#[$attr])* > - impl<const SIZE: usize> IoCapable<$ty> for $mmio<SIZE> { > - unsafe fn io_read(&self, address: usize) -> $ty { > + impl<const SIZE: usize> IoCapable<$ty> for &$mmio<SIZE> { [Severity: Low] Does this implementation cause a compile error due to a missing lifetime specifier? Implementing a trait for a reference type requires an explicit lifetime parameter for that reference. Should this be updated to include the lifetime? For example: impl<'a, const SIZE: usize> IoCapable<$ty> for &'a $mmio<SIZE>? [ ... ] > diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs > index b4996aa059d81..505305cd9b86c 100644 > --- a/rust/kernel/pci/io.rs > +++ b/rust/kernel/pci/io.rs > @@ -79,8 +79,8 @@ pub struct ConfigSpace<'a, S: ?Sized + ConfigSpaceKind = > Extended> { > /// Implements [`IoCapable`] on [`ConfigSpace`] for `$ty` using `$read_fn` > and `$write_fn`. > macro_rules! impl_config_space_io_capable { > ($ty:ty, $read_fn:ident, $write_fn:ident) => { > - impl<'a, S: ?Sized + ConfigSpaceKind> IoCapable<$ty> for > ConfigSpace<'a, S> { > - unsafe fn io_read(&self, address: usize) -> $ty { > + impl<'a, S: ?Sized + ConfigSpaceKind> IoCapable<$ty> for > &ConfigSpace<'a, S> { [Severity: Low] Similar to the regression in io.rs, is an outer lifetime specifier required for this reference type to compile? For example: impl<'a, 'b, S: ?Sized + ConfigSpaceKind> IoCapable<$ty> for &'b ConfigSpace<'a, S>? [ ... ] > @@ -112,18 +112,18 @@ unsafe fn io_write(&self, value: $ty, address: usize) { > impl_config_space_io_capable!(u16, pci_read_config_word, > pci_write_config_word); > impl_config_space_io_capable!(u32, pci_read_config_dword, > pci_write_config_dword); > > -impl<'a, S: ?Sized + ConfigSpaceKind> Io for ConfigSpace<'a, S> { > +impl<'a, S: ?Sized + ConfigSpaceKind> Io for &ConfigSpace<'a, S> { [Severity: Low] As with the previous implementation, does this also need an outer lifetime specifier for the reference itself? > type Target = S; > > /// Returns the base address of the I/O region. It is always 0 for > configuration space. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
