netdev - Re: [PATCH v3] ravb: Fixed to be able to unload modules

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] [day] [month] [year] [list]
Date:   Mon, 21 Sep 2020 15:16:19 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Yuusuke Ashizuka <ashiduka@...itsu.com>
Cc:     Sergei Shtylyov <sergei.shtylyov@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        netdev <netdev@...r.kernel.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        stable <stable@...r.kernel.org>
Subject: Re: [PATCH v3] ravb: Fixed to be able to unload modules

On Wed, Sep 16, 2020 at 11:31 AM Geert Uytterhoeven
<geert@...ux-m68k.org> wrote:
> On Thu, Aug 20, 2020 at 2:55 PM Yuusuke Ashizuka <ashiduka@...itsu.com> wrote:
> > When this driver is built as a module, I cannot rmmod it after insmoding
> > it.
> > This is because that this driver calls ravb_mdio_init() at the time of
> > probe, and module->refcnt is incremented by alloc_mdio_bitbang() called
> > after that.
> > Therefore, even if ifup is not performed, the driver is in use and rmmod
> > cannot be performed.
> >
> > $ lsmod
> > Module                  Size  Used by
> > ravb                   40960  1
> > $ rmmod ravb
> > rmmod: ERROR: Module ravb is in use
> >
> > Call ravb_mdio_init() at open and free_mdio_bitbang() at close, thereby
> > rmmod is possible in the ifdown state.
> >
> > Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> > Signed-off-by: Yuusuke Ashizuka <ashiduka@...itsu.com>
> > Reviewed-by: Sergei Shtylyov <sergei.shtylyov@...il.com>
>
> Thanks for your patch, which is now commit 1838d6c62f578366 ("ravb:
> Fixed to be able to unload modules") in v5.9-rc4 (backported to stable
> v4.4, v4.9, v4.14, v4.19, v5.4, and v5.8).
>
> This is causing a regression during resume from s2idle/s2ram on (at
> least) Salvator-X(S) and Ebisu.  Reverting that commit fixes this.
>
> During boot, the Micrel PHY is detected correctly:
>
>     Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: attached
> PHY driver [Micrel KSZ9031 Gigabit PHY]
> (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=228)
>     ravb e6800000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off

phy_device_register() calls device_add(), which is immediately bound to
the micrel driver.

> During resume, if CONFIG_MODULES=n, it falls back to the Generic PHY
> (case A):
>
>     Generic PHY e6800000.ethernet-ffffffff:00: attached PHY driver
> [Generic PHY] (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00,
> irq=POLL)
>     ravb e6800000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off
>
> and Ethernet still works (degraded, due to polling).
>
> During resume, if CONFIG_MODULES=y, MDIO initialization fails (case B):
>
>     mdio_bus e6800000.ethernet-ffffffff:00: error -16 loading PHY
> driver module for ID 0x00221622
>     ravb e6800000.ethernet eth0: failed to initialize MDIO
>     PM: dpm_run_callback(): ravb_resume+0x0/0x1b8 returns -16
>     PM: Device e6800000.ethernet failed to resume: error -16
>
> and Ethernet no longer works.
>
> Case B happens because usermodehelper_disabled is set to UMH_DISABLED
> during system suspend, causing request_module() to return -EBUSY.
> Ignoring -EBUSY in phy_request_driver_module(), like was done for
> -ENOENT in commit 21e194425abd65b5 ("net: phy: fix issue with loading
> PHY driver w/o initramfs"), makes it fall back to the Generic PHY, cfr.
> case A.
>
> For case A, I haven't found out yet why it falls back to the Generic PHY.

During system suspend, defer_all_probes is set to true, to avoid drivers
being probed while suspended.  Hence phy_device_register() calling
device_add() merely adds the device, but does not probe it yet
(really_probe() returns early)"

   dpm_resume+0x128/0x4f8
     device_resume+0xcc/0x1b0
       dpm_run_callback+0x74/0x340
         ravb_resume+0x190/0x1b8
           ravb_open+0x84/0x770
             of_mdiobus_register+0x1e0/0x468
               of_mdiobus_register_phy+0x1b8/0x250
                 of_mdiobus_phy_device_register+0x178/0x1e8
                   phy_device_register+0x114/0x1b8
                     device_add+0x3d4/0x798
                       bus_probe_device+0x98/0xa0
                         device_initial_probe+0x10/0x18
                           __device_attach+0xe4/0x140
                             bus_for_each_drv+0x64/0xc8
                               __device_attach_driver+0xb8/0xe0
                                 driver_probe_device.part.11+0xc4/0xd8
                                   really_probe+0x32c/0x3b8


Hence registering PHY devices from a net_device's ndo_open() call back
must not be done.

I will send a formal revert later today.

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