Il lun 17 giu 2024, 23:45 Manos Pitsidianakis
<manos.pitsidiana...@linaro.org> ha scritto:
> Secondly, are you implying that these callbacks are not operated under
> the BQL?
No, I'm implying that if you had the following nested calls:
unsafe read callback receives the opaque point
-> cast to &mut to call safe read callback
-> chardev function accessing the opaque value
-> unsafe chardev callback receives the opaque pointer
-> cast to & or &mut to call safe chardev callback
Then you would violate the Rust invariant that there can only be one
active reference at a single time if it is &mut. The only exception is
that you can create an &mut UnsafeCell<T> for an active
&UnsafeCell<T>.
It doesn't matter if this cannot happen because of invariants in the
QEMU code. If you are writing safe Rust, every cast to &mut requires
very strong justification. The way you do it instead is to use
RefCell, and borrow_mut() instead of casting to &mut. This way, at
least, the invariant is checked at runtime.
And in fact this _can_ happen. I hadn't checked until now because I
was mostly worried about the conceptual problem, not any specific way
undefined behavior can happen. But here it is:
PL011State::read takes &mut
-> qemu_chr_fe_accept_input
-> mux_chr_accept_input
-> pl011_can_receive
-> creates & which is undefined behavior
You probably can write a 20 lines test case in pure Rust that miri
complains about.
Should we make it a requirement for v3, that all callback methods in
PL011State (read, write, can_receive, receive, event) take a &self and
achieve interior mutability through RefCell? Probably not, a comment
is enough even though technically this is _not_ valid Rust. But it is
a high priority task after the initial commit.
> Just say this directly instead of writing all these amounts of text
I said in the first review that the state should be behind a RefCell.
https://lore.kernel.org/qemu-devel/CABgObfY8BS0yCw2CxgDQTBA4np9BZgGJF3N=t6eoBcdACAE=n...@mail.gmail.com/
> Finally, this is Rust code. You cannot turn off the type system, you
> cannot turn off the borrow checker, you can only go around creating new
> types and references out of raw memory addresses and tell the compiler
> 'trust me on this'
I am sorry if this sounds condescending. But this is _exactly_ what
I'm complaining about: that there is too much trust placed in the
programmer.
Converting from * to & does not turn off the borrow checker, but still
you cannot trust anymore what the borrow checker says; and that's why
you do it inside an abstraction, not in each and every callback of
each and every device. This is what Marc-André did for QAPI; and he
probably erred in the other direction for a PoC, but we _must_ agree
that something as complete as his work is the direction that we _have_
to take.
Again: I am sorry that you feel this way about my remark, because I
thought I had only praised your work. I didn't think I was being
condescending or dismissing. But I am worried that without the proper
abstractions this is going to be a technical debt magnet with only
marginal benefits over C.
And frankly I am saying this from experience. Check out CVE-2019-18960
which was an out of bounds access in Firecracker caused by not using
proper abstractions for DMA. And that's in a 100% Rust code base.
Since we're starting from and interfacing with C it's only going to be
worse; skimping on abstractions is simply something that we cannot
afford.
It's also the way Linux is introducing Rust in the code base. Whenever
something needs access to C functionality they introduce bindings to
it. It's slower, but it's sustainable.
> Ignoring that misses the entire point of Rust. The
> statement 'this is not Rust code but as good as C' is dishonest and
> misguided. Check for example the source code of the nix crate, which
> exposes libc and various POSIX/*nix APIs. Is it the same as C and not
> Rust code?
That's exactly my point. Libc provides mostly unsafe functions, which
is on the level of what bindgen generates. Other crates on top provide
*safe abstractions* such as nix's Dir
(https://docs.rs/nix/0.29.0/nix/dir/struct.Dir.html). If possible,
leaf crates use safe Rust (nix), and avoid unsafe (libc) as much as
possible.
Instead you're using the unsafe functions in the device itself. It's
missing an equivalent of nix.
> If you have specific scenarios in mind where such things exist in the
> code and end up doing invalid behavior please be kind and write them
> down explicitly and demonstrate them on code review.
It doesn't matter whether they exist or not. The point of Rust is that
the type system ensures that these invalid behaviors are caught either
at compile time or (with RefCell) at run time. As things stand, your
code cannot catch them and the language provides a false sense of
security.
On the other hand, I want to be clear agin that this is not a problem
at all—but only as long as we agree that it's not the desired final
state
> This approach of
> 'yes but no' is not constructive because it is not addressing any
> specific problems directly. Instead it comes out as vague dismissive FUD
> and I'm sure that is not what you or anyone else wants.
>
> Please take some time to understand my POV here, it'd help both of us
> immensely.
I can ask the same though. Please take the time to understand that I
am not being dismissive! My position is exactly "yes but no". Yes,
this is a great way to introduce Rust in the code base. No, this is
not a sustainable way to mix Rust and C—but I am willing to help.
Paolo
>
> Sincerely thank you in advance,
> Manos
>
