netdev - Re: [RFC net-next 3/6] ethernet: prestera: use eth_hw_addr_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] [thread-next>] [day] [month] [year] [list]

Message-ID: <443e4671-bc84-b4a9-7198-7de301a03d52@pensando.io>
Date:   Mon, 18 Oct 2021 10:54:56 -0700
From:   Shannon Nelson <snelson@...sando.io>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        olteanv@...il.com, andrew@...n.ch, f.fainelli@...il.com,
        vkochan@...vell.com, tchornyi@...vell.com
Subject: Re: [RFC net-next 3/6] ethernet: prestera: use eth_hw_addr_set_port()

On 10/18/21 10:33 AM, Ido Schimmel wrote:
> On Mon, Oct 18, 2021 at 09:26:21AM -0700, Shannon Nelson wrote:
>> On 10/18/21 7:19 AM, Jakub Kicinski wrote:
>>> On Sat, 16 Oct 2021 14:19:18 -0700 Shannon Nelson wrote:
>>>> As a potential consumer of these helpers, I'd rather do my own mac
>>>> address byte twiddling and then use eth_hw_addr_set() to put it into place.
>>> This is disproved by many upstream drivers, I only converted the ones
>>> that jumped out at me on Friday, but I'm sure there is more. If your
>>> driver is _really_ doing something questionable^W I mean "special"
>>> nothing is stopping you from open coding it. For others the helper will
>>> be useful.
>>>
>>> IOW I don't understand your comment.
>> To try to answer your RFC more clearly: I feel that this particular helper
>> obfuscates the operation more than it helps.
> FWIW, it at least helped me realize that we are going to have a bug with
> Spectrum-4. Currently we have:
>
> ether_addr_copy(addr, mlxsw_sp->base_mac);
> addr[ETH_ALEN - 1] += mlxsw_sp_port->local_port;
>
> As a preparation for Spectrum-4 we are promoting 'local_port' to u16
> since at least 257 and 258 are valid local port values.
>
> With the current code, the netdev corresponding to local port 1 will
> have the same MAC as the netdev corresponding to local port 257.
>
> After Jakub's conversion and changing the 'id' argument to 'unsigned
> int' [1], it should work correctly.
>
> [1] https://lore.kernel.org/netdev/20211018070845.68ba815d@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/

I would think that it might be clearer to do something like

     u64 addr64;

     addr64 = ether_addr_to_64(mlxsw_sp->base_mac);
     addr64 += mlxsw_sp_port->local_port;
     u64_to_ether_addr(addr64, addr);
     eth_hw_addr_set(dev, addr);

This uses our helpers to keep common actions safe, but also keeps the 
vendor specific logic (add N to the base_mac) in the driver.

sln