netdev - Re: [net-next] net: phy: fix issue with loading PHY driver w/o initramfs

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:   Tue, 22 Jan 2019 16:52:45 +0100
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     Krzysztof Kozlowski <krzk@...nel.org>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
        Geert Uytterhoeven <geert@...ux-m68k.org>
Subject: Re: [net-next] net: phy: fix issue with loading PHY driver w/o initramfs

	Hi Heiner,

> It was reported that on a system with nfsboot and w/o initramfs network
> fails because trying to load the PHY driver returns -ENOENT. Reason was
> that due to missing initramfs the modprobe binary isn't available.
> So we have to ignore error code -ENOENT.
> 
> Fixes: 13d0ab6750b2 ("net: phy: check return code when requesting PHY driver module")
> Reported-by: Krzysztof Kozlowski <krzk@...nel.org>
> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>

Thanks for your patch!

>
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -552,10 +552,12 @@ static int phy_request_driver_module(struct phy_device *dev, int phy_id)
>  
>  	ret = request_module(MDIO_MODULE_PREFIX MDIO_ID_FMT,
>  			     MDIO_ID_ARGS(phy_id));
> -	/* we only check for failures in executing the usermode binary,
> -	 * not whether a PHY driver module exists for the PHY ID
> +	/* We only check for failures in executing the usermode binary,
> +	 * not whether a PHY driver module exists for the PHY ID.
> +	 * Accept -ENOENT because this may occur in case no initramfs exists,
> +	 * then modprobe isn't available.
>  	 */
> -	if (IS_ENABLED(CONFIG_MODULES) && ret < 0) {
> +	if (IS_ENABLED(CONFIG_MODULES) && ret < 0 && ret != -ENOENT) {
>  		phydev_err(dev, "error %d loading PHY driver module for ID 0x%08x\n",
>  			   ret, phy_id);
>  		return ret;

While I believe this patch fixes the particular issue I was seeing, I'm
not convinced it fixes other possible issues.  There are several
different error cases in both the kernel (e.g. -ETIME) and userland that
can cause request_module() to fail, without actually having any impact,
if the particular PHY driver is builtin or already loaded.

IMHO, if the wanted PHY is available, all errors returned by
request_module() should be ignored.

In other subsystems, this is handled like:

    if (driver_is_missing)
        request_module(...);
    if (driver_is_missing)
        return -E...;

Note that most callers of request_module() don't even check the error
code it returns: the only thing that matters is if the (possibly loaded)
functionality is available afterwards or not.

Thanks!

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds