netdev - Re: [PATCH net-next] rust: phy: use VTABLE_DEFAULT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 25 Jan 2024 19:03:58 -0600
From: Trevor Gross <tmgross@...ch.edu>
To: Andrew Lunn <andrew@...n.ch>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org, 
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH net-next] rust: phy: use VTABLE_DEFAULT_ERROR

On Thu, Jan 25, 2024 at 11:42 AM Andrew Lunn <andrew@...n.ch> wrote:
> > @@ -580,12 +580,12 @@ pub trait Driver {
> >
> >      /// Issues a PHY software reset.
> >      fn soft_reset(_dev: &mut Device) -> Result {
> > -        Err(code::ENOTSUPP)
> > +        kernel::build_error(VTABLE_DEFAULT_ERROR)
> >      }
>
> Dumb question, which i should probably duckduckgo myself.
>
> Why is it called VTABLE_DEFAULT_ERROR and not
> VTABLE_NOT_SUPPORTED_ERROR?
>
> Looking through the C code my guess would be -EINVAL would be the
> default, or maybe 0.
>
> The semantics of ENOTSUPP can also vary. Its often not an actual
> error, it just a means to indicate its not supported, and the caller
> might decide to do something different as a result. One typical use in
> the network stack is offloading functionality to hardware.
> e.g. blinking the LEDs on the RJ45 connected. The driver could be
> asked to blink to show activity of a received packet. Some hardware
> can only indicate activity for both receive and transmit, so the
> driver would return -EOPNOTSUPP. Software blinking would then be used
> instead of hardware blinking.
>
>          Andrew

`build_error` is a bit special and is implemented as follows:

1. If called in a `const fn`, it will fail the build
2. If the build_error symbol is in a final binary, tooling should
detect this and raise some kind of error (config
RUST_BUILD_ASSERT_ALLOW, not exactly positive how this is implemented)
3. If called at runtime, something is very unexpected so we panic

To make a trait function optional in rust, you need to provide a
default (since all functions in a trait must always be callable). But
if the C side can handle a null function pointer, it's better to just
set that instead of reimplementing the default from Rust.

So it's up to the abstraction to set these fields to NULL/None if not
implemented, e.g. from create_phy_driver in phy.rs:

        soft_reset: if T::HAS_SOFT_RESET {
            Some(Adapter::<T>::soft_reset_callback)
        } else {
            None
        },

If an abstraction does this wrong and tries to assign a default
function when it would be better to get a null pointer, then we get
the build error. So this is something that would pop up if the
abstraction is done wrong, not just if somebody writes a bad phy
driver.

VTABLE_DEFAULT_ERROR isn't a kernel errno, it's a string printed to be
printed by build_error or the tooling.

- Trevor