netdev - Re: [PATCH v4 4/8] PCI: replace pci_dev::driver usage that gets the driver name

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: <20210929080541.GA13506@corigine.com>
Date:   Wed, 29 Sep 2021 10:05:42 +0200
From:   Simon Horman <simon.horman@...igine.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Uwe Kleine-König <uwe@...ine-koenig.org>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        linux-pci@...r.kernel.org, Alexander Duyck <alexanderduyck@...com>,
        oss-drivers@...igine.com, Paul Mackerras <paulus@...ba.org>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Michael Ellerman <mpe@...erman.id.au>,
        Rafał Miłecki <zajec5@...il.com>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Bjorn Helgaas <helgaas@...nel.org>,
        Ido Schimmel <idosch@...dia.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Yisen Zhuang <yisen.zhuang@...wei.com>,
        Vadym Kochan <vkochan@...vell.com>, Michael Buesch <m@...s.ch>,
        Jiri Pirko <jiri@...dia.com>,
        Salil Mehta <salil.mehta@...wei.com>, netdev@...r.kernel.org,
        linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
        Taras Chornyi <tchornyi@...vell.com>,
        Zhou Wang <wangzhou1@...ilicon.com>,
        linux-crypto@...r.kernel.org, kernel@...gutronix.de,
        Oliver O'Halloran <oohall@...il.com>,
        linuxppc-dev@...ts.ozlabs.org,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH v4 4/8] PCI: replace pci_dev::driver usage that gets the
 driver name

On Tue, Sep 28, 2021 at 12:31:29PM +0200, Uwe Kleine-König wrote:
> On Tue, Sep 28, 2021 at 12:01:28PM +0200, Simon Horman wrote:
> > On Mon, Sep 27, 2021 at 10:43:22PM +0200, Uwe Kleine-König wrote:
> > > From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
> > > 
> > > struct pci_dev::driver holds (apart from a constant offset) the same
> > > data as struct pci_dev::dev->driver. With the goal to remove struct
> > > pci_dev::driver to get rid of data duplication replace getting the
> > > driver name by dev_driver_string() which implicitly makes use of struct
> > > pci_dev::dev->driver.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
> > 
> > ...
> > 
> > > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
> > > index 0685ece1f155..23dfb599c828 100644
> > > --- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
> > > +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
> > > @@ -202,7 +202,7 @@ nfp_get_drvinfo(struct nfp_app *app, struct pci_dev *pdev,
> > >  {
> > >  	char nsp_version[ETHTOOL_FWVERS_LEN] = {};
> > >  
> > > -	strlcpy(drvinfo->driver, pdev->driver->name, sizeof(drvinfo->driver));
> > > +	strlcpy(drvinfo->driver, dev_driver_string(&pdev->dev), sizeof(drvinfo->driver));
> > 
> > I'd slightly prefer to maintain lines under 80 columns wide.
> > But not nearly strongly enough to engage in a long debate about it.
> 
> :-)
> 
> Looking at the output of
> 
> 	git grep strlcpy.\*sizeof
> 
> I wonder if it would be sensible to introduce something like
> 
> 	#define strlcpy_array(arr, src) (strlcpy(arr, src, sizeof(arr)) + __must_be_array(arr))
> 
> but not sure this is possible without a long debate either (and this
> line is over 80 chars wide, too :-).

My main motivation for the 80 char limit in nfp_net_ethtool.c is
not that I think 80 char is universally a good limit (although that is true),
but rather that I expect that is the prevailing style in nfp_net_ethtool.c.

So a macro more than 80 car wide somewhere else is fine by me.

However, when running checkpatch --strict over the patch it told me:

    WARNING: Prefer strscpy over strlcpy - see: https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/
    #276: FILE: drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c:205:
    +	strlcpy(drvinfo->driver, dev_driver_string(&pdev->dev), sizeof(drvinfo->driver));

    total: 0 errors, 1 warnings, 0 checks, 80 lines checked

(Amusingly, more text wider than 80 column, perhaps suggesting the folly of
 my original comment, but lets move on from that.)

As your patch doesn't introduce the usage of strlcpy() I was considering a
follow-up patch to change it to strscpy(). And in general the email at the
link above suggests all usages of strlcpy() should do so. So perhaps
creating strscpy_array is a better idea?

I have not thought about this much, and probably this just leads us to a
deeper part of the rabbit hole.