Re: [PATCH net-next v7 1/5] rust: core abstractions for network PHY drivers

[Boqun Feng <boqun.feng@gmail.com>]

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@gmail.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