netdev - Re: [PATCH] include: mdio: Guard inline function with CONFIG

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]

Message-ID: <CAKmqyKNkPGPg8xsYDY9FtNvqJQsFmQ1o8KYHQXutrN1kHxsPww@mail.gmail.com>
Date: Tue, 5 Nov 2024 11:21:40 +1000
From: Alistair Francis <alistair23@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: linux-kernel@...r.kernel.org, netdev@...r.kernel.org, 
	linux@...linux.org.uk, hkallweit1@...il.com, 
	Alistair Francis <alistair.francis@....com>
Subject: Re: [PATCH] include: mdio: Guard inline function with CONFIG_MDIO

On Tue, Nov 5, 2024 at 10:37 AM Andrew Lunn <andrew@...n.ch> wrote:
>
> On Tue, Nov 05, 2024 at 10:21:15AM +1000, Alistair Francis wrote:
> > On Mon, Nov 4, 2024 at 11:49 PM Andrew Lunn <andrew@...n.ch> wrote:
> > >
> > > On Mon, Nov 04, 2024 at 05:09:50PM +1000, Alistair Francis wrote:
> > > > The static inline functions mdio45_ethtool_gset() and
> > > > mdio45_ethtool_ksettings_get() call mdio45_ethtool_gset_npage() and
> > > > mdio45_ethtool_ksettings_get_npage() which are both guarded by
> > > > CONFIG_MDIO. So let's only expose mdio45_ethtool_gset() and
> > > > mdio45_ethtool_ksettings_get() if CONFIG_MDIO is defined.
> > >
> > > Why? Are you fixing a linker error? A compiler error?
> >
> > I'm investigating generating Rust bindings for static inline functions
> > (like mdio45_ethtool_gset() for example). But it fails to build when
> > there are functions defined in header files that call C functions that
> > aren't built due to Kconfig options.
>
> Since this does not appear to be an issue for C, i assume these
> functions are not actually used in that configuration. And this is
> probably not an issue specific to MDIO. It will probably appear all
> over the kernel. Adding lots of #ifdef in header files will probably
> not be liked.

It's not actually a Rust issue, it's a problem with linking.

This is the type of errors I get

```
ld: vmlinux.o: in function `mdio45_ethtool_gset':
/scratch/alistair/software/linux/./include/linux/mdio.h:189:(.text+0x59e819):
undefined reference to `mdio45_ethtool_gset_npage'
ld: vmlinux.o: in function `mdio45_ethtool_ksettings_get':
/scratch/alistair/software/linux/./include/linux/mdio.h:206:(.text+0x59e839):
undefined reference to `mdio45_ethtool_ksettings_get_npage'
make[2]: *** [scripts/Makefile.vmlinux:34: vmlinux] Error 1
```

Which comes from autogenerated C code like this

```
void mdio45_ethtool_gset__extern(const struct mdio_if_info *mdio,
struct ethtool_cmd *ecmd) { mdio45_ethtool_gset(mdio, ecmd); }
```

mdio45_ethtool_gset__extern() is never called, so I'm not clear why
it's not optimised out.

It's not only MDIO that hits this, but so far there aren't too many
cases. That will obviously depend on the config used though.

There will be issues like this over the kernel. I'm not sure fixing
them all is the right approach as it might be too much work and too
hard to narrow down all occurance. But to me it seems like the corect
fix as the current code is calling a function that might not exist,
hence the patch :)

If it's not something that we think should be fixed then that's fine,
I can work around it.

Alistair

>
> Does Rust have the concept of inline functions? If it is never used,
> it never gets compiled? Or at least, it gets optimised out before it
> gets linked, which i think is your issue here.
>
>         Andrew