linux-kernel - Re: [PATCH] Do not disable driver and bus shutdown hook when class shutdown hook is set.

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]

Message-ID: <20170811190102.1e26c51a@kitsune.suse.cz>
Date:   Fri, 11 Aug 2017 19:01:02 +0200
From:   Michal Suchánek <msuchanek@...e.de>
To:     Henrique de Moraes Holschuh <hmh@....eng.br>
Cc:     Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Peter Huewe <peterhuewe@....de>,
        Marcel Selhorst <tpmdd@...horst.net>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        linux-kernel@...r.kernel.org, tpmdd-devel@...ts.sourceforge.net
Subject: Re: [PATCH] Do not disable driver and bus shutdown hook when class
 shutdown hook is set.

On Fri, 11 Aug 2017 12:28:57 -0300
Henrique de Moraes Holschuh <hmh@....eng.br> wrote:

> On Fri, 11 Aug 2017, Michal Suchánek wrote:
> > On 2017-08-10 18:30, Jason Gunthorpe wrote:  
> > > On Thu, Aug 10, 2017 at 12:18:11PM +0200, Michal Suchánek wrote:  
> > > > > Existing bus implementations do properly chain to driver
> > > > > shutdown (eg look at mmc_bus_shutdown) and it appears to have
> > > > > been written like  
> > > > 
> > > > Neither isa nor ibmebus does. These are two random buses I
> > > > tried to look at.  
> > > 
> > > I'm not following, I see this:
> > > 
> > > static void ibmebus_bus_device_shutdown(struct device *dev)
> > > {
> > >         struct platform_device *of_dev = to_platform_device(dev);
> > >         struct platform_driver *drv =
> > > to_platform_driver(dev->driver);
> > > 
> > >         if (dev->driver && drv->shutdown)
> > >                 drv->shutdown(of_dev);
> > > }
> > > 
> > > It looks to me like in this case the struct device_driver
> > > shutdown is not used, and instead the struct platform_driver
> > > shutdown is called.  
> > 
> > And it is not used even if a device driver sets it and expects it
> > to run.  
> 
> Which is the kind of landmine it is best avoided in drivers/, so it
> would be nice to get WARN_ON() during device register when
> dev->shutdown() methods *that are going to be ignored* because of
> class/bus handlers are non-NULL...

We actually have the warning:

int driver_register(struct device_driver *drv)
{
        int ret;
        struct device_driver *other;

        BUG_ON(!drv->bus->p);

        if ((drv->bus->probe && drv->probe) ||
            (drv->bus->remove && drv->remove) ||
            (drv->bus->shutdown && drv->shutdown))
                printk(KERN_WARNING "Driver '%s' needs updating -
        please use " "bus_type methods\n", drv->name);

So presumably setting both bus and driver shutdown is bogus while
setting both class and bus shutdown is fine and expected. So the logic
in the initial class shutdown addition that disables driver and
bus shutdown is bogus - there is no reason to disable them.

Given that nobody pointed out this shows how well understood this
interface is :/

Thanks

Michal