netdev - Re: [PATCH net-next v7 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 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: Tue, 21 Nov 2023 11:13:06 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: tmgross@...ch.edu
Cc: boqun.feng@...il.com, fujita.tomonori@...il.com, andrew@...n.ch,
 gregkh@...uxfoundation.org, aliceryhl@...gle.com, benno.lossin@...ton.me,
 miguel.ojeda.sandonis@...il.com, netdev@...r.kernel.org,
 rust-for-linux@...r.kernel.org, wedsonaf@...il.com
Subject: Re: [PATCH net-next v7 1/5] rust: core abstractions for network
 PHY drivers

On Sun, 19 Nov 2023 05:06:23 -0600
Trevor Gross <tmgross@...ch.edu> wrote:

>> +pub struct Registration {
>> +    drivers: Pin<&'static mut [DriverVTable]>,
>> +}
>>
>> [...]
>>
>> +// SAFETY: `Registration` does not expose any of its state across threads.
>> +unsafe impl Send for Registration {}
>>
>> +// SAFETY: `Registration` does not expose any of its state across threads.
>> +unsafe impl Sync for Registration {}
> 
> I don't think the impl here actually makes sense. `Registration` is a
> buffer of references to `DriverVTable`. That type isn't marked Sync so
> by the above rules, its references should not be either.
> 
> Tomo, does this need to be Sync at all? Probably easiest to drop the
> impls if not, otherwise I think it is more correct to move them to
> `DriverVTable`.  You may have had this before, I'm not sure if
> discussion made you change it at some point...

This needs to be Sync:

601 | pub struct Registration {
    |            ^^^^^^^^^^^^
note: required because it appears within the type `Module`
   --> drivers/net/phy/foo_rust.rs:5:1
    |
5   | / kernel::module_phy_driver! {
6   | |     drivers: [Foo],
7   | |     device_table: [
8   | |         DeviceId::new_with_driver::<Foo>()
...   |
13  | |     license: "GPL",
14  | | }
    | |_^
note: required by a bound in `kernel::Module`
   --> /home/ubuntu/git/linux/rust/kernel/lib.rs:69:27
    |
69  | pub trait Module: Sized + Sync {
    |                           ^^^^ required by this bound in `Module`
    = note: this error originates in the macro `kernel::module_phy_driver` (in Nightly builds, run with -Z macro-backtrace for more info)


I'm not sure we discussed but making DriverVTable Sync works.

#[repr(transparent)]
pub struct DriverVTable(Opaque<bindings::phy_driver>);

// SAFETY: DriverVTable has no &self methods, so immutable references to it are useless.
unsafe impl Sync for DriverVTable {}


looks correct?