netdev - Re: [PATCH net-next v5 1/5] 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: Mon, 23 Oct 2023 06:37:46 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch, miguel.ojeda.sandonis@...il.com, tmgross@...ch.edu, boqun.feng@...il.com, wedsonaf@...il.com, greg@...ah.com
Subject: Re: [PATCH net-next v5 1/5] rust: core abstractions for network PHY drivers

On 23.10.23 08:35, Benno Lossin wrote:
> On 21.10.23 23:45, FUJITA Tomonori wrote:
>> On Sat, 21 Oct 2023 13:35:57 +0000
>> Benno Lossin <benno.lossin@...ton.me> wrote:
>>
>>>> Currently, it needs &'static DriverVTable
>>>> array so it works.
>>>
>>> That is actually also incorrect. As the C side is going to modify
>>> the `DriverVTable`, you should actually use `&'static mut DriverVTable`.
>>> But since it is not allowed to be moved you have to use
>>> `Pin<&'static mut DriverVTable>`.
>>
>> I updated Registration::register(). Needs to add comments on requirement?
>>
>> impl Registration {
>>       /// Registers a PHY driver.
>>       pub fn register(
>>           module: &'static crate::ThisModule,
>>           drivers: Pin<&'static mut [DriverVTable]>,
>>       ) -> Result<Self> {
>>           // SAFETY: The type invariants of [`DriverVTable`] ensure that all elements of the `drivers` slice
>>           // are initialized properly. So an FFI call with a valid pointer.
> 
> This SAFETY comment needs to mention that `drivers[0].0.get()` are

Sorry, I meant `drivers` instead of `drivers[0].0.get()`

-- 
Cheers,
Benno

> pinned and will not change address.
> 
>>           to_result(unsafe {
>>               bindings::phy_drivers_register(drivers[0].0.get(), drivers.len().try_into()?, module.0)
>>           })?;
>>           // INVARIANT: The `drivers` slice is successfully registered to the kernel via `phy_drivers_register`.
>>           Ok(Registration { drivers })
>>       }
>> }
> 
> Otherwise this looks good.
> 
>>
>>
>>>> The C side uses static allocation too. If someone asks for, we could
>>>> loosen the restriction with a complicated implentation. But I doubt
>>>> that someone would ask for such.
>>>
>>> With Wedson's patch you also would be using the static allocation
>>> from `module!`. What my problem is, is that you are using a `static mut`
>>> which is `unsafe` and you do not actually have to use it (with
>>> Wedson's patch of course).
>>
>> Like your vtable patch, I improve the code when something useful is
>> available.
> 
> Sure. If you have the time though, it would be helpful to know
> if the patch actually fixes the issue. I am pretty sure it will,
> but you never know unless you try.