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

From: Benno Lossin <benno.lossin@proton.me>
To: FUJITA Tomonori <fujita.tomonori@gmail.com>
Cc: boqun.feng@gmail.com, tmgross@umich.edu, netdev@vger.kernel.org,
	rust-for-linux@vger.kernel.org, andrew@lunn.ch,
	miguel.ojeda.sandonis@gmail.com, greg@kroah.com
Subject: Re: [PATCH net-next v3 1/3] rust: core abstractions for network PHY drivers
Date: Fri, 13 Oct 2023 10:03:43 +0000	[thread overview]
Message-ID: <7e0803b4-33da-45b0-8b6b-8baff98a9593@proton.me> (raw)
In-Reply-To: <20231013.185348.94552909652217598.fujita.tomonori@gmail.com>

On 13.10.23 11:53, FUJITA Tomonori wrote:
> On Fri, 13 Oct 2023 07:56:07 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
>> It's not that we do not trust the subsystems, for example when we register
>> a callback `foo` and the C side documents that it is ok to sleep within
>> `foo`, then we will assume so. If we would not trust the C side, then we
>> would have to disallow sleeping there, since sleeping while holding a
>> spinlock is UB (and the C side could accidentally be holding a spinlock).
>>
>> But there are certain things where we do not trust the subsystems, these
>> are mainly things where we can afford it from a performance and usability
>> perspective (in the example above we could not afford it from a usability
>> perspective).
> 
> You need maintenance cost too here. That's exactly the discussion
> point during reviewing the enum code, the kinda cut-and-paste from C
> code and match code that Andrew and Grek want to avoid.

Indeed, however Trevor already has opened an issue at bindgen [1]
that will fix this maintenance nightmare. It seems to me that the
bindgen developers are willing to implement this. It also seems that
this feature can be implemented rather quickly, so I would not worry
about the ergonomics and choose safety until we can use the new bindgen
feature.

[1]: https://github.com/rust-lang/rust-bindgen/issues/2646

>> In the enum case it would also be incredibly simple for the C side to just
>> make a slight mistake and set the integer to a value outside of the
>> specified range. This strengthens the case for checking validity here.
>> When an invalid value is given to Rust we have immediate UB. In Rust UB
>> always means that anything can happen so we must avoid it at all costs.
> 
> I'm not sure the general rules in Rust can be applied to linux kernel.

Rust UB is still forbidden, it can introduce arbitrary misscompilations.

> If the C side (PHYLIB) to set in an invalid value to the state,
> probably the network doesn't work; already anything can happen in the
> system at this point. Then the Rust abstractions get the invalid value
> from the C side and detect an error with a check. The abstractions
> return an error to a Rust PHY driver. Next what can the Rust PHY
> driver do? Stop working? Calling dev_err() to print something and then
> selects the state randomly and continue?

What if the C side has a bug and gives us a bad value by mistake? It is
not required for the network not working for us to receive an invalid
value. Ideally the PHY driver would not even notice this, the abstractions
should handle this fully. Not exactly sure what to do in the error case,
maybe a warn_once and then choose some sane default state?

> What's the practical benefit from the check?

The practical use of the check is that we do not introduce UB.

>> In this case having a check would not really hurt performance and in terms
>> of usability it also seems reasonable. If it would be bad for performance,
>> let us know.
> 
> Bad for maintenance cost. Please read the discussion in the review on rfc v1.

Since this will only be temporary, I believe it to be fine.

-- 
Cheers,
Benno