Message-ID: <ef6f7294-0afe-46af-8714-ed4a4aaee558@proton.me>
Date: Sat, 17 Aug 2024 13:19:55 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Sami Tolvanen <samitolvanen@...gle.com>
Cc: Masahiro Yamada <masahiroy@...nel.org>, Luis Chamberlain <mcgrof@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, Matthew Maurer <mmaurer@...gle.com>, Alex Gaynor <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, Gary Guo <gary@...yguo.net>, Petr Pavlu <petr.pavlu@...e.com>, Neal Gompa <neal@...pa.dev>, Hector Martin <marcan@...can.st>, Janne Grunau <j@...nau.net>, Asahi Linux <asahi@...ts.linux.dev>, linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org, linux-modules@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2 16/19] gendwarfksyms: Add support for reserved structure fields

On 17.08.24 09:41, Greg Kroah-Hartman wrote:
> On Fri, Aug 16, 2024 at 08:50:53AM -0700, Sami Tolvanen wrote:
>> On Fri, Aug 16, 2024 at 12:20 AM Greg Kroah-Hartman
>> <gregkh@...uxfoundation.org> wrote:
>>> On Thu, Aug 15, 2024 at 05:39:20PM +0000, Sami Tolvanen wrote:
>>> Especially as I have no idea how you are going to do
>>> this with the rust side of things, this all will work for any structures
>>> defined in .rs code, right?
>>
>> Yes, Rust structures can use the same scheme. Accessing union members
>> might be less convenient than in C, but can presumably be wrapped in
>> helper macros if needed.
> 
> That feels ripe for problems for any rust code as forcing a helper macro
> for a "normal" access to a structure field is going to be a lot of churn
> over time.  Is the need for a macro due to the fact that accessing a
> union is always considered "unsafe" in rust?  If that's the case, ick,
> this is going to get even messier even faster as the need for sprinkling
> unsafe accesses everywhere for what used to be a normal/safe one will
> cause people to get nervous...

The reason for union field access being unsafe in Rust is that you can
easily shoot yourself in the foot. For example:

    union Foo {
        a: bool,
        b: i32,
    }

    let foo = Foo { b: 3 };
    println!("{}", unsafe { foo.a });

This is UB, since `3` is of course not a valid value for `bool`. With
unions the compiler doesn't know which variant is active.

Since unions are unsafe in Rust, we don't really use them directly (in
the `kernel` crate, we have 0 union definitions). Instead we use certain
unions from the stdlib such as `MaybeUninit`. But the fields of that
union are private and never accessed.

In general, unions in Rust are very important primitive types, but they
are seldomly used directly. Instead enums are used a lot more, since you
don't need to roll your own tagged unions.

For this use-case (the one in the patch), I don't really know if we want
to copy the approach from C. Do we even support exporting kABI from
Rust? If yes, then we I would recommend we tag it in the source code
instead of using a union. Here the example from the patch adapted:

    #[repr(C)] // needed for layout stability
    pub struct Struct1 {
        a: u64,
        #[kabi_reserved(u64)] // this marker is new
        _reserved: u64,
    }

And then to use the reserved field, you would do this:
    
    #[repr(C)]
    pub struct Struct1 {
        a: u64,
        #[kabi_reserved(u64)]
        b: Struct2,
    }

    #[repr(C)]
    pub struct Struct2 {
        b: i32,
        v: i32,
    }

The attribute would check that the size of the two types match and
gendwarfksyms would use the type given in "()" instead of the actual
type.

---
Cheers,
Benno

Powered by blists - more mailing lists