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
| ||
|
Date: Thu, 9 Sep 2021 09:00:21 -0700 From: Florian Fainelli <f.fainelli@...il.com> To: Vladimir Oltean <olteanv@...il.com>, Lino Sanfilippo <LinoSanfilippo@....de>, Saravana Kannan <saravanak@...gle.com> Cc: p.rosenberger@...bus.com, woojung.huh@...rochip.com, UNGLinuxDriver@...rochip.com, andrew@...n.ch, vivien.didelot@...il.com, davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH 0/3] Fix for KSZ DSA switch shutdown +Saravana, On 9/9/2021 8:47 AM, Vladimir Oltean wrote: > On Thu, Sep 09, 2021 at 03:19:52PM +0200, Lino Sanfilippo wrote: >>> Do you see similar things on your 5.10 kernel? >> >> For the master device is see >> >> lrwxrwxrwx 1 root root 0 Sep 9 14:10 /sys/class/net/eth0/device/consumer:spi:spi3.0 -> ../../../virtual/devlink/platform:fd580000.ethernet--spi:spi3.0 > > So this is the worst of the worst, we have a device link but it doesn't help. > > Where the device link helps is here: > > __device_release_driver > while (device_links_busy(dev)) > device_links_unbind_consumers(dev); > > but during dev_shutdown, device_links_unbind_consumers does not get called > (actually I am not even sure whether it should). > > I've reproduced your issue by making this very simple change: > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c > index 60d94e0a07d6..ec00f34cac47 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c > @@ -1372,6 +1372,7 @@ static struct pci_driver enetc_pf_driver = { > .id_table = enetc_pf_id_table, > .probe = enetc_pf_probe, > .remove = enetc_pf_remove, > + .shutdown = enetc_pf_remove, > #ifdef CONFIG_PCI_IOV > .sriov_configure = enetc_sriov_configure, > #endif > > on my DSA master driver. This is what the genet driver has "special". > > I was led into grave error by Documentation/driver-api/device_link.rst, > which I've based my patch on, where it clearly says that device links > are supposed to help with shutdown ordering (how?!). I was also under the impression that device links were supposed to help with shutdown ordering, because it does matter a lot. One thing that I had to work before (and seems like it came back recently) is the shutdown ordering between gpio_keys.c and the GPIO controller. If you suspend the GPIO controller first, gpio_keys.c never gets a chance to keep the GPIO pin configured for a wake-up interrupt, therefore no wake-up event happens on key presses, whoops. > > So the question is, why did my DSA trees get torn down on shutdown? > Basically the short answer is that my SPI controller driver does > implement .shutdown, and calls the same code path as the .remove code, > which calls spi_unregister_controller which removes all SPI children.. > > When I added this device link, one of the main objectives was to not > modify all DSA drivers. I was certain based on the documentation that > device links would help, now I'm not so sure anymore. > > So what happens is that the DSA master attempts to unregister its net > device on .shutdown, but DSA does not implement .shutdown, so it just > sits there holding a reference (supposedly via dev_hold, but where from?!) > to the master, which makes netdev_wait_allrefs to wait and wait. It's not coming from of_find_net_device_by_node() that's for sure and with OF we don't go through the code path calling dsa_dev_to_net_device() which does call dev_hold() and then shortly thereafter the caller calls dev_put() anyway. > > I need more time for the denial phase to pass, and to understand what > can actually be done. I will also be away from the keyboard for the next > few days, so it might take a while. Your patches obviously offer a > solution only for KSZ switches, we need something more general. If I > understand your solution, it works not by virtue of there being any > shutdown ordering guarantee at all, but simply due to the fact that > DSA's .shutdown hook gets called eventually, and the reference to the > master gets freed eventually, which unblocks the unregister_netdevice > call from the master. I don't yet understand why DSA holds a long-term > reference to the master, that's one thing I need to figure out. > Agreed. -- Florian
Powered by blists - more mailing lists