Re: [PATCH net-next v3 1/3] rust: core abstractions for network PHY drivers

[Boqun Feng <boqun.feng@gmail.com>]

On Thu, Oct 12, 2023 at 09:10:41AM +0000, Benno Lossin wrote:
> On 12.10.23 09:58, FUJITA Tomonori wrote:
> > On Thu, 12 Oct 2023 03:32:44 -0400
> > Trevor Gross <tmgross@umich.edu> wrote:
> > 
> >> On Thu, Oct 12, 2023 at 3:13 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> >>
> >>> If `Device::from_raw`'s safety requirement is "only called in callbacks
> >>> with phydevice->lock held, etc.", then the exclusive access is
> >>> guaranteed by the safety requirement, therefore `mut` can be drop. It's
> >>> a matter of the exact semantics of the APIs.
> >>>
> >>> Regards,
> >>> Boqun
> >>
> >> That is correct to my understanding, the core handles
> >> locking/unlocking and no driver functions are called if the core
> >> doesn't hold an exclusive lock first. Which also means the wrapper
> >> type can't be `Sync`.
> >>
> >> Andrew said a bit about it in the second comment here:
> >> https://lore.kernel.org/rust-for-linux/ec6d8479-f893-4a3f-bf3e-aa0c81c4adad@lunn.ch/
> > 
> > resume/suspend are called without the mutex hold but we don't need the
> > details. PHYLIB guarantees the exclusive access inside the
> > callbacks. I updated the comment and drop mut in Device's methods.
> 
> The details about this stuff are _extremely_ important, if there is a
> mistake with the way `unsafe` requirements are written/interpreted, then
> the Rust guarantee of "memory safety in safe code" flies out the window.
> The whole idea is to offload all the dangerous stuff into smaller regions
> that can be scrutinized more easily and for that we need all of the details.
> 

Thank Benno for calling this out.

After re-read my email exchange with Tomo, I realised I need to explain
this a little bit. The minimal requirement of a Rust binding is
soundness: it means if one only uses safe APIs, one cannot introduce
memory/type safety issue (i.e. cannot have an object in an invalid
state), this is a tall task, because you can have zero assumption of the
API users, you can only encode the usage requirement in the type system.

Of course the type system doesn't always work, hence we have unsafe API,
but still the soundness of Rust bindings means using safe APIs +
*correctly* using unsafe APIs cannot introduce memory/type safety
issues.

Tomo, this is why we gave you a hard time here ;-) Unsafe Rust APIs must
be very clear on the correct usage and safe Rust APIs must not assume
how users would call it. Hope this help explain a little bit, we are not
poking random things here, soundness is the team effort from everyone
;-)

Regards,
Boqun

> What would be really helpful for me, as I have extremely limited
> knowledge of the C side, would be an explaining comment in the phy
> abstractions that explains the way the C side phy abstractions work. So
> for example it would say that locking is handled by the phy core and (at
> the moment) neither the Rust abstractions nor the driver code needs to
> concern itself with locking. There you could also write that `resume` and
> `suspend` are called without the mutex being held. We don't really have a
> precedent for this (as there have been no drivers merged), but it would be
> really helpful for me. If this exists in some other documentation, feel
> free to just link that.
> 
> -- 
> Cheers,
> Benno
> 
>