On Fri, 13 Oct 2023 10:03:43 +0000
Benno Lossin [off-list ref] wrote:

On 13.10.23 11:53, FUJITA Tomonori wrote:

quoted

On Fri, 13 Oct 2023 07:56:07 +0000
Benno Lossin [off-list ref] wrote:

quoted

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.

quoted

quoted

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?

quoted

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?

quoted

What's the practical benefit from the check?

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

hmm.

quoted

quoted

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.