netdev - Re: [PATCH net-next v3 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 for Android: free password hash cracker in your pocket

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

Date: Thu, 12 Oct 2023 09:10:41 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: FUJITA Tomonori <fujita.tomonori@...il.com>, tmgross@...ch.edu
Cc: boqun.feng@...il.com, netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch, miguel.ojeda.sandonis@...il.com, greg@...ah.com
Subject: Re: [PATCH net-next v3 1/3] rust: core abstractions for network PHY drivers

On 12.10.23 09:58, FUJITA Tomonori wrote:
> On Thu, 12 Oct 2023 03:32:44 -0400
> Trevor Gross <tmgross@...ch.edu> wrote:
> 
>> On Thu, Oct 12, 2023 at 3:13 AM Boqun Feng <boqun.feng@...il.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.

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