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

[FUJITA Tomonori <fujita.tomonori@gmail.com>]

On Fri, 13 Oct 2023 10:03:43 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> 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

Yeah, I know. I wrote multiple times, let's go with a temporary
solution and will use the better solution when it will be available.


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

Can you give a pointer on how it can introduce such?


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

Your case is that C side has a good value but somehow gives a bad
value to the abstractions?

The abstractions can't handle this. The abstractions works as the part
of a PHY driver; The abstractions do only what The driver asks.

The PHY driver asks the state from the abstractions then the
abstractions ask the state from PHYLIB. So when the abstractions get a
bad value from PHYLIB, the abstractions must return something to the
PHY driver. As I wrote, the abstractions return a random value or an
error. In either way, probably the system cannot continue.


> maybe a warn_once and then choose some sane default state?

What sane default? PHY_ERROR?


>> What's the practical benefit from the check?
> 
> The practical use of the check is that we do not introduce UB.

hmm.


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

Great, if you have other concerns on v4 patchset, please let me
know. I tried to address all your comments.