netdev - Re: [PATCH net-next v9 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: Mon, 4 Dec 2023 19:40:53 -0800
From: Boqun Feng <boqun.feng@...il.com>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch,
	tmgross@...ch.edu, miguel.ojeda.sandonis@...il.com,
	benno.lossin@...ton.me, wedsonaf@...il.com, aliceryhl@...gle.com
Subject: Re: [PATCH net-next v9 1/4] rust: core abstractions for network PHY
 drivers

On Tue, Dec 05, 2023 at 12:23:20PM +0900, FUJITA Tomonori wrote:
> On Mon, 4 Dec 2023 18:00:15 -0800
> Boqun Feng <boqun.feng@...il.com> wrote:
> 
> > On Tue, Dec 05, 2023 at 10:14:17AM +0900, FUJITA Tomonori wrote:
> > [...]
> >> +    /// Gets the current link state.
> >> +    ///
> >> +    /// It returns true if the link is up.
> >> +    pub fn is_link_up(&self) -> bool {
> >> +        const LINK_IS_UP: u64 = 1;
> >> +        // TODO: the code to access to the bit field will be replaced with automatically
> >> +        // generated code by bindgen when it becomes possible.
> >> +        // SAFETY: The struct invariant ensures that we may access
> >> +        // this field without additional synchronization.
> >> +        let bit_field = unsafe { &(*self.0.get())._bitfield_1 };
> >> +        bit_field.get(14, 1) == LINK_IS_UP
> > 
> > I made a mistake here [1], this should be:
> > 
> >     let bit_field = unsafe { &*(core::ptr::addr_of!((*self.0.get())._bitfield_1)) };
> >     bit_field.get(14, 1) == LINK_IS_UP
> > 
> > without `core::ptr::add_of!`, `*(self.0.get())` would still create a
> > temporary `&` to the underlying object I believe. `addr_of!` is the way
> > to avoid create the temporary reference. Same for the other functions.
> 
> If so, how about functions to access to non bit field like phy_id()?
> 
> pub fn phy_id(&self) -> u32 {
>     let phydev = self.0.get();
>     unsafe { (*phydev).phy_id }
> }

Good point, so cancel the above comment. I had a misunderstanding that a
place expression would create temporary references, but I was wrong.

Regards,
Boqun