Re: [RFC PATCH v5 0/8] Add Rust support, implement ARM PL011

[Paolo Bonzini]

On Thu, Jul 25, 2024 at 12:14 PM Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
> >Unfortunately that's a *mut, not a &mut. A &mut must be unique, so the cast
> >in pl011_read() is undefined behavior.
>
> Actually it's:
>
>   unsafe { qemu_chr_fe_accept_input(&mut self.char_backend) };
>
> And you can ensure there's no disjoint borrowing by making a wrapper
> function that mutably borrows self, e.g.
>
> fn accept_input(&mut self) {
>   unsafe { qemu_chr_fe_accept_input(&mut self.char_backend) };
> }
>
> This is not undefined behavior, since the cast in pl011_read creates a
> mutable reference that does not outlive the same call to pl011_read.

pl011_receive (called by qemu_chr_fe_accept_input) creates a mutable
reference that *overlaps* the lifetime of the outer reference created
by pl011_read. This is undefined behavior. You're effectively writing:

fn main() {
    let mut foo = String::from("foo");
    let x = &mut foo;
    let y = &mut foo;
    print!("{}", y);
    print!("{}", x);       // does not compile
}

And even if you're hiding it like this:

fn main() {
    let mut foo = String::from("foo");
    let x = &mut foo;
    let x_ptr: *mut String = x as *mut _;
    let y = &mut foo;
    print!("{}", y);
    print!("{}", unsafe { &mut *x_ptr });
}

It's still wrong. Quoting the docs:

https://doc.rust-lang.org/nightly/nomicon/references.html
-> "A mutable reference cannot be aliased"

https://doc.rust-lang.org/nightly/nomicon/aliasing.html
-> "variables and pointers alias if they refer to overlapping regions
of memory."

https://doc.rust-lang.org/reference/behavior-considered-undefined.html
-> "Rust programs must never cause undefined behavior. It is the
programmer's responsibility when writing unsafe code to ensure that
any safe code interacting with the unsafe code cannot trigger these
behaviors."

The QEMU code has even more layers hiding the problem but it's the
same thing: there are two &mut that are alive at the same time, and
refer to the same region of memory.

> Please demonstrate a scenario where said problem shows up, otherwise
> it's just hand-waving on a undefined (pun intended ;) ) hypothetical.

Again: there's no "scenario where the problem shows up", you just
can't do that; the code is wrong.

I'm not saying you should fix it now, because it's complicated to fix
it and in particular to fix it the right way. But I think it is a
requirement for merging Rust code that the community understands the
extra complexity introduced by writing Rust code that interacts with C
APIs, and does not hand wave it away as not a problem.

> >> Do you perhaps agree with adding a FIXME comment to replace the linker
> >> attributes with ctor when we get the cargo dependency story in meson
> >> sorted out?
> >
> >Yes, that's fine.
>
> Thanks! We also have to keep in mind we would like Debian to be able to
> package this, so we should be very conservative on what dependencies we
> use.

If we add Rust dependencies, Debian will have to figure out how to
package them in order to build QEMU. But I agree that it's a good idea
to add them slowly.

Paolo