netdev - Re: [PATCH net-next] net: dsa: Some cleanups in remove code

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:   Wed, 10 Nov 2021 15:15:40 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        kernel@...gutronix.de, Jakub Kicinski <kuba@...nel.org>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH net-next] net: dsa: Some cleanups in remove code

On Tue, Nov 09, 2021 at 06:50:55PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> Cc += gregkh, maybe he has something to say on this matter
> 
> On Tue, Nov 09, 2021 at 01:54:34PM +0200, Vladimir Oltean wrote:
> > Your commit prefix does not reflect the fact that you are touching the
> > vsc73xx driver. Try "net: dsa: vsc73xx: ".
> 
> Oh, I missed that indeed.
> 
> > On Tue, Nov 09, 2021 at 12:39:21PM +0100, Uwe Kleine-König wrote:
> > > vsc73xx_remove() returns zero unconditionally and no caller checks the
> > > returned value. So convert the function to return no value.
> > 
> > This I agree with.
> > 
> > > For both the platform and the spi variant ..._get_drvdata() will never
> > > return NULL in .remove() because the remove callback is only called after
> > > the probe callback returned successfully and in this case driver data was
> > > set to a non-NULL value.
> > 
> > Have you read the commit message of 0650bf52b31f ("net: dsa: be
> > compatible with masters which unregister on shutdown")?
> 
> No. But I did now. I consider it very surprising that .shutdown() calls
> the .remove() callback and would recommend to not do this. The commit
> log seems to prove this being difficult.

Why do you consider it surprising?

Many drivers implement ->shutdown by calling ->remove for the simple
reason that ->remove provides for a well-tested code path already, and
leaves the hardware in a known state, workable for kexec and others.

Many drivers have buses beneath them. Those buses go away when these
drivers unregister, and so do their children.

==============================================

=> some drivers do both => children of these buses should expect to be
potentially unregistered after they've been shut down.

> > To remove the check for dev_get_drvdata == NULL in ->remove, you need to
> > prove that ->remove will never be called after ->shutdown. For platform
> > devices this is pretty easy to prove, for SPI devices not so much.
> > I intentionally kept the code structure the same because code gets
> > copied around a lot, it is easy to copy from the wrong place.
> 
> Alternatively remove spi_set_drvdata(spi, NULL); from
> vsc73xx_spi_shutdown()?

What is the end goal exactly?

> Also I'm not aware how platform devices are
> different to spi devices that the ordering of .remove and shutdown() is
> more or less obvious than on the other bus?!

Not sure what you mean. See the explanation above. For the "platform"
bus, there simply isn't any code path that unregisters children on the
->shutdown callback. For other buses, there is.

> > > Also setting driver data to NULL is not necessary, this is already done
> > > in the driver core in __device_release_driver(), so drop this from the
> > > remove callback, too.
> > 
> > And this was also intentional, for visibility more or less. I would like
> > you to ack that you understand the problems surrounding ->remove/->shutdown
> > ordering for devices on buses, prior to making seemingly trivial cleanups.
> 
> I see that the change is not so obviously correct as I thought. I'll
> have to think about this and will respin if and when I find a sane way
> forward.

A way forward towards what? This is literally a cosmetic patch that
would happen to break some stuff, were it to be applied.