netdev - Re: [PATCH net-next v10 1/4] 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: Wed, 13 Dec 2023 08:01:32 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: benno.lossin@...ton.me
Cc: fujita.tomonori@...il.com, boqun.feng@...il.com, alice@...l.io,
 netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch,
 tmgross@...ch.edu, miguel.ojeda.sandonis@...il.com, wedsonaf@...il.com,
 aliceryhl@...gle.com
Subject: Re: [PATCH net-next v10 1/4] rust: core abstractions for network
 PHY drivers

On Tue, 12 Dec 2023 17:35:34 +0000
Benno Lossin <benno.lossin@...ton.me> wrote:

> On 12/12/23 14:02, FUJITA Tomonori wrote:
>> On Mon, 11 Dec 2023 22:11:15 -0800
>> Boqun Feng <boqun.feng@...il.com> wrote:
>> 
>>>>> // SAFETY: `phydev` points to valid object per the type invariant of
>>>>> // `Self`, also the following just minics what `phy_read()` does in C
>>>>> // side, which should be safe as long as `phydev` is valid.
>>>>>
>>>>> ?
>>>>
>>>> Looks ok to me but after a quick look at in-tree Rust code, I can't
>>>> find a comment like X is valid for the first argument in this C
>>>> function. What I found are comments like X points to valid memory.
>>>
>>> Hmm.. maybe "is valid" could be a confusing term, so the point is: if
>>> `phydev` is pointing to a properly maintained struct phy_device, then an
>>> open code of phy_read() should be safe. Maybe "..., which should be safe
>>> as long as `phydev` points to a valid struct phy_device" ?
>> 
>> As Alice suggested, I updated the comment. The current comment is:
>> 
>> // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
>> // So it's just an FFI call.
>> let ret = unsafe {
>>     bindings::mdiobus_read((*phydev).mdio.bus, (*phydev).mdio.addr, regnum.into())
>> };
> 
> I still think you need to justify why `mdio.bus` is a pointer that you
> can give to `midobus_read`. After looking at the C code, it seems like
> that the pointer needs to point to a valid `struct mii_bus`.
> This *could* just be an invariant of `struct phy_device` [1], but where
> do we document that?

If phy_device points to a valid object, phy_device.mdio is valid.

A mii_bus must exist before a phy device. A bus is scanned and then a
phy device is found (so phy_device object is crated).

https://elixir.bootlin.com/linux/v6.6.5/source/drivers/net/phy/phy_device.c#L634