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 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]

Message-ID: <ZT72no2gdASP0STS@boqun-archlinux>
Date: Sun, 29 Oct 2023 17:19:42 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: benno.lossin@...ton.me, 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 Mon, Oct 30, 2023 at 07:58:52AM +0900, FUJITA Tomonori wrote:
> On Sun, 29 Oct 2023 09:48:41 -0700
> Boqun Feng <boqun.feng@...il.com> wrote:
> 
> > On Sun, Oct 29, 2023 at 01:21:12PM +0900, FUJITA Tomonori wrote:
> > [...]
> >> 
> >> The current code is fine from Rust perspective because the current
> >> code copies phy_driver on stack and makes a reference to the copy, if
> >> I undertand correctly.
> >> 
> > 
> > I had the same thought Benno brought the issue on `&`, but unfortunately
> > it's not true ;-) In the following code:
> > 
> > 	let phydev = unsafe { *self.0.get() };
> > 
> > , semantically the *whole* `bindings::phy_device` is being read, so if
> > there is any modification (i.e. write) that may happen in the meanwhile,
> > it's data race, and data races are UB (even in C).
> 
> Benno said so? I'm not sure about the logic (whole v.s. partial). Even

We can wait for Benno's response, but there is an example where Miri
says it's data race:

	https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=c7097644aa5f02a0a436e5b8b8624824

> if you read partially, the part might be modified by the C side during
> reading.

If you read the part protected by phy_device->lock, C side shouldn't
modify it, but the case here is not all fields in phy_device stay
unchanged when phy_device->lock (and Rust side doesn't mark them
interior mutable), see the discussion drom Andrew and me.

> 
> For me, the issue is that creating &T for an object that might be
> modified.

The reason a `&phy_device` cannot be created here is because concurrent
writes may cause a invalid phy_device (i.e. data race), the same applies
to a copy.

Regards,
Boqun