netdev - Re: [PATCH v2 1/3] rust: core abstractions for network PHY drivers

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: Sun, 8 Oct 2023 02:07:44 -0400
From: Trevor Gross <tmgross@...ch.edu>
To: Andrew Lunn <andrew@...n.ch>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org, 
	rust-for-linux@...r.kernel.org, miguel.ojeda.sandonis@...il.com, 
	greg@...ah.com
Subject: Re: [PATCH v2 1/3] rust: core abstractions for network PHY drivers

On Sat, Oct 7, 2023 at 11:13 AM Andrew Lunn <andrew@...n.ch> wrote:
>
> > The safety comment here still needs something like
> >
> >     with the exception of fields that are synchronized via the `lock` mutex
>
> I'm not sure that really adds much useful information. Which values
> are protected by the lock? More importantly, which are not protected
> by the lock?
>
> As a general rule of thumb, driver writers don't understand
> locking. Yes, there are some which do, but many don't. So the
> workaround to that is make it so they don't need to understand
> locking. All the locking happens in the core.
>
> The exception is suspend and resume, which are called without the
> lock. So if i was to add a comment about locking, i would only put a
> comment on those two.

This doesn't get used by driver implementations, it's only used within
the abstractions here. I think anyone who needs the details can refer
to the C side, I just suggested to note the locking caveat based on
your second comment at
https://lore.kernel.org/rust-for-linux/ec6d8479-f893-4a3f-bf3e-aa0c81c4adad@lunn.ch/

Fujita - since this doesn't get exposed, could this be pub(crate)?)

> > Andrew, are there any restrictions about calling phy_init_hw more than
> > once? Or are there certain things that you are not allowed to do until
> > you call that function?
>
> phy_init_hw can be called multiple times. It used by drivers as a work
> around to broken hardware/firmware to get the device back into a good
> state. It is also used during resume, since often the PHY looses its
> settings when suspended.

Great, thank you for the clarification

> > > +    unsafe extern "C" fn read_mmd_callback(
> > > +        phydev: *mut bindings::phy_device,
> > > +        devnum: i32,
> > > +        regnum: u16,
> > > +    ) -> i32 {
> > > +        from_result(|| {
> > > +            // SAFETY: The C API guarantees that `phydev` is valid while this function is running.
> > > +            let dev = unsafe { Device::from_raw(phydev) };
> > > +            let ret = T::read_mmd(dev, devnum as u8, regnum)?;
> > > +            Ok(ret.into())
> > > +        })
> > > +    }
> >
> > Since your're reading a bus, it probably doesn't hurt to do a quick
> > check when converting
> >
> >     let devnum_u8 = u8::try_from(devnum).(|_| {
> >         warn_once!("devnum {devnum} exceeds u8 limits");
> >         code::EINVAL
> >     })?
>
> I would actually say this is the wrong place to do that. Such checks
> should happen in the core, so it checks all drivers, not just the
> current one Rust driver. Feel free to submit a C patch adding this.
>
>         Andrew

I guess it does that already:
https://elixir.bootlin.com/linux/v6.6-rc4/source/drivers/net/phy/phy-core.c#L556

Fujita, I think we started doing comments when we know that
lossy/bitwise `as` casts are correct. Maybe just leave the code as-is
but add

    // CAST: the C side verifies devnum < 32

?