netdev - Re: [PATCH v2 1/4] of_net: Add NVMEM support to of_get_mac

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:   Tue, 7 May 2019 11:06:43 -0500
From:   Rob Herring <robh@...nel.org>
To:     Petr Štetiar <ynezz@...e.cz>
Cc:     netdev <netdev@...r.kernel.org>, devicetree@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Frank Rowand <frowand.list@...il.com>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Maxime Ripard <maxime.ripard@...tlin.com>,
        Alban Bedel <albeu@...e.fr>, Felix Fietkau <nbd@....name>,
        John Crispin <john@...ozen.org>
Subject: Re: [PATCH v2 1/4] of_net: Add NVMEM support to of_get_mac_address

On Thu, May 2, 2019 at 4:05 AM Petr Štetiar <ynezz@...e.cz> wrote:
>
> Rob Herring <robh@...nel.org> [2019-05-01 15:19:25]:
>
> Hi Rob,
>
> > > +   struct property *pp;
>
> ...
>
> > > +   pp = kzalloc(sizeof(*pp), GFP_KERNEL);
> > > +   if (!pp)
> > > +           return NULL;
> > > +
> > > +   pp->name = "nvmem-mac-address";
> > > +   pp->length = ETH_ALEN;
> > > +   pp->value = kmemdup(mac, ETH_ALEN, GFP_KERNEL);
> > > +   if (!pp->value || of_add_property(np, pp))
> > > +           goto free;
> >
> > Why add this to the DT?
>
> I've just carried it over from v1 ("of_net: add mtd-mac-address support to
> of_get_mac_address()")[1] as nobody objected about this so far.

That's not really a reason...

> Honestly I don't know if it's necessary to have it, but so far address,
> mac-address and local-mac-address properties provide this DT nodes, so I've
> simply thought, that it would be good to have it for MAC address from NVMEM as
> well in order to stay consistent.

If you want to be consistent, then fill in 'local-mac-address' with
the value from nvmem. We don't need the same thing with a new name
added to DT. (TBC, I'm not suggesting you do that here.)

But really, my point with using devm_kzalloc() is just return the
data, not store in DT and free it when the driver unbinds. Allocating
it with devm_kzalloc AND adding it to DT as you've done in v4 leads to
2 entities refcounting the allocation. If the driver unbinds, the
buffer is freed, but DT code is still referencing that memory.

Also, what happens the 2 time a driver binds? The property would
already be in the DT.

>
> Just FYI, my testing ar9331_8dev_carambola2.dts[2] currently produces
> following runtime DT content:
>
>  root@...nWrt:/# find /sys/firmware/devicetree/ -name *nvmem* -o -name *addr@*
>  /sys/firmware/devicetree/base/ahb/spi@...00000/flash@...artitions/partition@...000/nvmem-cells
>  /sys/firmware/devicetree/base/ahb/spi@...00000/flash@...artitions/partition@...000/nvmem-cells/eth-mac-addr@0
>  /sys/firmware/devicetree/base/ahb/spi@...00000/flash@...artitions/partition@...000/nvmem-cells/eth-mac-addr@6
>  /sys/firmware/devicetree/base/ahb/spi@...00000/flash@...artitions/partition@...000/nvmem-cells/wifi-mac-addr@...2
>  /sys/firmware/devicetree/base/ahb/wmac@...00000/nvmem-cells
>  /sys/firmware/devicetree/base/ahb/wmac@...00000/nvmem-mac-address
>  /sys/firmware/devicetree/base/ahb/wmac@...00000/nvmem-cell-names
>  /sys/firmware/devicetree/base/ahb/eth@...00000/nvmem-cells
>  /sys/firmware/devicetree/base/ahb/eth@...00000/nvmem-mac-address
>  /sys/firmware/devicetree/base/ahb/eth@...00000/nvmem-cell-names
>  /sys/firmware/devicetree/base/ahb/eth@...00000/nvmem-cells
>  /sys/firmware/devicetree/base/ahb/eth@...00000/nvmem-mac-address
>  /sys/firmware/devicetree/base/ahb/eth@...00000/nvmem-cell-names

'nvmem-mac-address' is not a documented property. That would need to
be documented before using upstream. Though, for reasons above, I
don't think it should be.

Rob