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
| ||
|
Message-ID: <ZT1bt8FknDEeUotm@Boquns-Mac-mini.home> Date: Sat, 28 Oct 2023 12:06:31 -0700 From: Boqun Feng <boqun.feng@...il.com> To: Benno Lossin <benno.lossin@...ton.me> Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, andrew@...n.ch, netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, tmgross@...ch.edu, miguel.ojeda.sandonis@...il.com, wedsonaf@...il.com Subject: Re: [PATCH net-next v7 1/5] rust: core abstractions for network PHY drivers On Sat, Oct 28, 2023 at 04:39:08PM +0000, Benno Lossin wrote: > On 28.10.23 18:09, FUJITA Tomonori wrote: > > On Sat, 28 Oct 2023 16:53:30 +0200 > > Andrew Lunn <andrew@...n.ch> wrote: > > > >>>> We need to be careful here, since doing this creates a reference > >>>> `&bindings::phy_device` which asserts that it is immutable. That is not > >>>> the case, since the C side might change it at any point (this is the > >>>> reason we wrap things in `Opaque`, since that allows mutatation even > >>>> through sharde references). > >>> > >>> You meant that the C code might modify it independently anytime, not > >>> the C code called the Rust abstractions might modify it, right? > >> > >> The whole locking model is base around that not happening. Things > >> should only change with the lock held. I you make a call into the C Here is my understanding, I treat references in Rust as a special pointer, so having a `&bindings::phy_device` means the *entire* object is immutable unless the fields are interior mutable, for example, behind a `UnsafeCell` or `Opaque`, examples of interior mutable types are atomic and locks. Now since C doesn't have the "interior mutable" concept or these types, so when bindgen generates the `bindings::phy_device`, none of the fields of a lock or atomic is marked as `UnsafeCell` or `Opaque`. That's why `Opaque` is needed for defining `Device`: pub struct Device(Opaque<bindings::phy_device); `Opaque` basically does two things: 1) tell compilers that the underlying content may be modified (of course in some sort of serialization) when there exists a `&Device` or `&mut Device`. 2) only provide `*mut bindings::phy_device`, so accessing the underlying object has to use unsafe. Now let's look back into struct phy_device, it does have a few locks in it, and at least even with phydev->lock held, the content of phydev->lock itself can be changed (e.g tick locks), hence it breaks the requirement of the existence of a `&bindings::phy_device`. Of course, if we can define our own `bindings::phy_device` or ask bindgen to automatically add `Opaque` to certain types, then `&bindings::phy_device` is still possible to use. If we are OK to not use `&bindings::phy_device` then Benno's proposal in bindgen is one way to work with this. Regards, Boqun > >> side, then yes, it can and will change it. So you should not cache a > >> value over a C call. > > > > Yeah, I understand that. But if I understand Benno correctly, from > > Rust perspective, such might happen. > > Yes, that is what I meant. Sure the C side might never modify the > value, but this is not good enough for Rust. It must somehow be ensured > that it never is modified, in order for us to rely on it. > > -- > Cheers, > Benno > >
Powered by blists - more mailing lists