linux-kernel - Re: [RFC PATCH 11/11] rust: PCI: add BAR request and ioremap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 21 May 2024 01:27:23 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: pstanner@...hat.com, Danilo Krummrich <dakr@...hat.com>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, bhelgaas@...gle.com, 
	ojeda@...nel.org, alex.gaynor@...il.com, wedsonaf@...il.com, 
	boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com, 
	benno.lossin@...ton.me, a.hindborg@...sung.com, aliceryhl@...gle.com, 
	airlied@...il.com, fujita.tomonori@...il.com, lina@...hilina.net, 
	ajanulgu@...hat.com, lyude@...hat.com, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [RFC PATCH 11/11] rust: PCI: add BAR request and ioremap

Hi Philipp,

A few quick nits I noticed for this one...

On Mon, May 20, 2024 at 7:27 PM Danilo Krummrich <dakr@...hat.com> wrote:
>
> +/// A PCI BAR to perform IO-Operations on.

Some more documentation, examples, and/or references would be nice.

> +pub struct Bar {
> +    pdev: Device,
> +    iomem: IoMem,
> +    num: u8,
> +}
> +
> +impl Bar {
> +    fn new(pdev: Device, num: u8, name: &CStr) -> Result<Self> {
> +        let barnr = num as i32;

Would it make sense to use newtypes for `num`/`barnr`, perhaps?

> +        let barlen = pdev.resource_len(num)?;
> +        if barlen == 0 {
> +            return Err(ENOMEM);
> +        }
> +
> +        // SAFETY:
> +        // `pdev` is always valid.

Please explain why it is always valid -- the point of a `SAFETY`
comment is not to say something is OK, but why it is so.

> +        // `barnr` is checked for validity at the top of the function.

Where was it checked? I may be missing something, but I only see a
widening cast.

> +        // SAFETY:
> +        // `pdev` is always valid.
> +        // `barnr` is checked for validity at the top of the function.
> +        // `name` is always valid.

Please use the convention we have elsewhere for this kind of lists,
i.e. use a list with the `-` bullet list marker.

> +        let ioptr: usize = unsafe { bindings::pci_iomap(pdev.as_raw(), barnr, 0) } as usize;

Is the type needed, since there is an explicit cast?

> +        if ioptr == 0 {
> +            // SAFETY:
> +            // `pdev` is always valid.
> +            // `barnr` is checked for validity at the top of the function.
> +            unsafe { bindings::pci_release_region(pdev.as_raw(), barnr) };
> +            return Err(ENOMEM);
> +        }

Should the region be managed in a RAII type instead?

> +    fn index_is_valid(i: u8) -> bool {
> +        // A pci_dev on the C side owns an array of resources with at most
> +        // PCI_NUM_RESOURCES entries.

Missing Markdown. There are other instances as well.

> +        if i as i32 >= bindings::PCI_NUM_RESOURCES as i32 {
> +            return false;
> +        }
> +
> +        true

The body of the function could just be `... < ...`, i.e. no `if` needed.

> +    // SAFETY: The caller should ensure that `ioptr` is valid.
> +    unsafe fn do_release(pdev: &Device, ioptr: usize, num: u8) {

This should not be a comment but documentation, and it should be a `#
Safety` section, not a `// SAFETY:` comment.

> +    /// Returns the size of the given PCI bar resource.
> +    pub fn resource_len(&self, bar: u8) -> Result<bindings::resource_size_t> {

Sometimes `bindings::` in signatures of public methods may be
justified -- is it the case here? Or should a newtype or similar be
provided instead? I only see this function being called inside the
module, anyway.

> +    /// Mapps an entire PCI-BAR after performing a region-request on it.

Typo.

Cheers,
Miguel