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

[Benno Lossin <benno.lossin@proton.me>]

On 13.10.23 12:53, FUJITA Tomonori wrote:
>>>> 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?

First, I can point you to [1] that is a list of UB that can occur in
Rust. Second, I can give you an example [2] of UB leading to
miscompilations, compare the executions of both release and debug mode.

[1]: https://doc.rust-lang.org/nomicon/what-unsafe-does.html#what-unsafe-rust-can-do
[2]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=856cdd7434350e38d3891162e04424db

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

Just think of the C side having some weird bug.

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

Sure then let the system BUG if it cannot continue. I think that
allowing UB is worse than BUGing.

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

Sure.

-- 
Cheers,
Benno