Re: [RFC PATCH v1 1/4] rust: core abstractions for network PHY drivers

[Trevor Gross <tmgross@umich.edu>]

On Sun, Sep 17, 2023 at 3:08 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sun, Sep 17, 2023 at 02:42:02PM -0400, Trevor Gross wrote:
> > On Sun, Sep 17, 2023 at 11:07 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > > [...]
> > > > If abstraction code just declare a function, a driver always need to
> > > > implement that function.
> > >
> > > Ah, that is not going to scale well. There is something like 250
> > > struct phy_driver at the moment. We keep adding new ops to that
> > > structure. I really don't want to have to modify all 250 of those
> > > structures when i add a new op which only one or two drivers need.
> >
> > This concern is about adding a function pointer field to the C struct
> > without needing to update all future Rust drivers, correct?
>
> Partially. Assuming Rust is successful, and in 10 years time we have
> 250 devices supported by Rust, i also don't want to have to edit all
> 250 entries when a new member is added to the Rust version of struct
> phy_driver.
> [...]
> O.K, that is the behaviour we want, in terms of NULL, or a driver
> specific function pointer.

Currently if you add a field to the struct in C, everything in Rust
will continue to work without changes. If you want to be able to use
it in the Rust side, you need to update both the `Driver` trait and
`fn driver(&self) -> bindings::phy_driver` [1] but none of the
implementations.

> A follow up question to this. This currently does not apply to PHY
> drivers, but the concept of a structure of function pointers and maybe
> a few other data fields exists all over the kernel. There has been an
> effort to make them all const, so the C compiler puts them in the
> .rodata section. The kernel then uses the MMU to protect it, so it
> really is read only. That protects from a class to attacks where a
> function pointer gets overwritten and then jumped through.
>
> Can Rust create these structures read only in the .rodata section? Can
> this driver actually do this, which would be an improvement over the
> current C PHY drivers?
>
>         Andrew

Generally, this should be possible. Any function marked `const` can be
assigned to a static:

    const fn make_device() -> Device {
        todo!()
    }

    #[no_mangle]
    static _some_driver: Device = make_device();

That function can only call other const functions, but I don't think
that is a problem here. A caveat is that in a simple test I'm trying,
anything that contains a function pointer seems to go into `.data`
instead of `.rodata`. I'm trying to figure out why this but will have
to get back about it.

Is that something you would be interested in for the `phy_driver`? I
may be able to try it out if so, unless Fujita would like to.
`phy_driver_register` doesn't seem to take a pointer to const data,
but I assume it doesn't do any modification?

[1]: Updating `Driver` but not the mapping function would mean that
the default function in `Driver` may get called, but this would
generally be incorrect. That is why I suggested making them default to
something that `BUG()`s, but I'm also not sure what policy is on this.