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]

Message-ID: <20211109175055.46rytrdejv56hkxv@pengutronix.de>
Date:   Tue, 9 Nov 2021 18:50:55 +0100
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Vladimir Oltean <olteanv@...il.com>
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

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.

> 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()? 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?!

> > 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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)