netdev - Re: [PATCH net-next v7 2/5] rust: net::phy add module_phy

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: Fri, 17 Nov 2023 23:54:09 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Boqun Feng <boqun.feng@...il.com>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org,
	rust-for-linux@...r.kernel.org, tmgross@...ch.edu,
	miguel.ojeda.sandonis@...il.com, benno.lossin@...ton.me,
	wedsonaf@...il.com
Subject: Re: [PATCH net-next v7 2/5] rust: net::phy add module_phy_driver
 macro

On Fri, Nov 17, 2023 at 02:21:38PM -0800, Boqun Feng wrote:
> On Thu, Oct 26, 2023 at 09:10:47AM +0900, FUJITA Tomonori wrote:
> [...]
> > +
> > +/// Declares a kernel module for PHYs drivers.
> > +///
> > +/// This creates a static array of kernel's `struct phy_driver` and registers it.
> > +/// This also corresponds to the kernel's `MODULE_DEVICE_TABLE` macro, which embeds the information
> > +/// for module loading into the module binary file. Every driver needs an entry in `device_table`.
> > +///
> > +/// # Examples
> > +///
> > +/// ```
> > +/// # mod module_phy_driver_sample {
> > +/// use kernel::c_str;
> > +/// use kernel::net::phy::{self, DeviceId};
> > +/// use kernel::prelude::*;
> > +///
> > +/// kernel::module_phy_driver! {
> > +///     drivers: [PhyAX88772A],
> > +///     device_table: [
> > +///         DeviceId::new_with_driver::<PhyAX88772A>()
> > +///     ],
> > +///     name: "rust_asix_phy",
> > +///     author: "Rust for Linux Contributors",
> > +///     description: "Rust Asix PHYs driver",
> > +///     license: "GPL",
> > +/// }
> > +///
> > +/// struct PhyAX88772A;
> > +///
> > +/// #[vtable]
> > +/// impl phy::Driver for PhyAX88772A {
> > +///     const NAME: &'static CStr = c_str!("Asix Electronics AX88772A");
> > +///     const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x003b1861);
> > +/// }
> > +/// # }
> > +/// ```
> 
> When run the following kunit command:
> 
> ./tools/testing/kunit/kunit.py run --make_options LLVM=1 --arch x86_64 \
> 	--kconfig_add CONFIG_RUST=y \
> 	--kconfig_add CONFIG_RUST_PHYLIB_ABSTRACTIONS=y \
> 	--kconfig_add CONFIG_PHYLIB=y \
> 	--kconfig_add CONFIG_NETDEVICES=y \
> 	--kconfig_add CONFIG_NET=y \
> 	--kconfig_add CONFIG_AX88796B_RUST_PHY=y \
> 	--kconfig_add CONFIG_AX88796B_PHY=y
> 
> I got the following errors:
> 
> 	ERROR:root:ld.lld: error: duplicate symbol: __rust_asix_phy_init
> 	>>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0
> 	>>>            rust/doctests_kernel_generated.o:(__rust_asix_phy_init) in archive vmlinux.a
> 	>>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0
> 	>>>            drivers/net/phy/ax88796b_rust.o:(.text+0x160) in archive vmlinux.a
> 
> 	ld.lld: error: duplicate symbol: __rust_asix_phy_exit
> 	>>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0
> 	>>>            rust/doctests_kernel_generated.o:(__rust_asix_phy_exit) in archive vmlinux.a
> 	>>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0
> 	>>>            drivers/net/phy/ax88796b_rust.o:(.text+0x1E0) in archive vmlinux.a
> 
> 	ld.lld: error: duplicate symbol: __mod_mdio__phydev_device_table
> 	>>> defined at doctests_kernel_generated.5ed8fd29a53cf22f-cgu.0
> 	>>>            rust/doctests_kernel_generated.o:(__mod_mdio__phydev_device_table) in archive vmlinux.a
> 	>>> defined at ax88796b_rust.37fb93aefca595fa-cgu.0
> 	>>>            drivers/net/phy/ax88796b_rust.o:(.rodata+0x58) in archive vmlinux.a
> 
> Because kunit will use the above doc test to generate test, and since
> CONFIG_AX88796B_RUST_PHY is also selected, the `module_phy_driver!` has
> been called twice, and causes duplicate symbols.
> 
> For "rust_asix_phy_*" symbols, it's easy to fix: just rename the usage
> in the example. But for __mod_mdio__phydev_device_table, it's hard-coded
> in `module_phy_driver!`, I don't have a quick fix right now. Also, does
> it mean `module_phy_driver!` is only supposed to be "called" once for
> the entire kernel?

Each kernel module should be in its own symbol name space. The only
symbols which are visible outside of the module are those exported
using EXPORT_SYMBOL_GPL() or EXPORT_SYMBOL(). A PHY driver does not
export anything, in general.

Being built in also does not change this.

Neither drivers/net/phy/ax88796b_rust.o nor
rust/doctests_kernel_generated.o should have exported this symbol.

I've no idea how this actually works, i guess there are multiple
passes through the linker? Maybe once to resolve symbols across object
files within a module. Normal global symbols are then made local,
leaving only those exported with EXPORT_SYMBOL_GPL() or
EXPORT_SYMBOL()? A second pass through linker then links all the
exported symbols thorough the kernel?

	Andrew