netdev - Re: Regression introduced by "r8169: simplify rtl_set_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] [day] [month] [year] [list]

Message-ID: <c969c5f7-b613-7a1b-774d-22a7dce4a51d@gmail.com>
Date:   Mon, 2 Jul 2018 22:35:18 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Corinna Vinschen <vinschen@...hat.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: Regression introduced by "r8169: simplify rtl_set_mac_address"

On 02.07.2018 21:48, Corinna Vinschen wrote:
> Hi,
> 
> the patch 1f7aa2bc268e, "r8169: simplify rtl_set_mac_address",
> introduced a regression found by trying to team a r8169 NIC.
> 
Thanks for reporting!

> Try the following (assuming the r8169 NIC is eth0):
> 
> $ nmcli con add type team con-name team0 ifname nm-team config \
>   '{"runner": {"name": "lacp"}, "link_watch": {"name": "ethtool"}}' \
>   ipv4.method disable ipv6.method ignore
> $ nmcli con add type ethernet ifname eth0 con-name team0.0 master nm-team
> $ nmcli con up team0.0
> $ teamdctl nm-team port present eth0
> command call failed (No such device)
> 
> Bisecting turned up commit 1f7aa2bc268e, "r8169: simplify
> rtl_set_mac_address" as the culprit.  Reverting this patch fixes
> the issue and the teamdctl call succeeds.
> 
> The reason is apparently the usage of eth_mac_addr here.  eth_mac_addr
> calls eth_prepare_mac_addr_change which checks for IFF_LIVE_ADDR_CHANGE.
> Debugging shows this flag not being set on r8169, thus
> eth_prepare_mac_addr_change returns -EBUSY (no idea why userspace claims
> "No such device", rather than "Device or resource busy", but that's not
> the point here).
> 
> Note that other devices like igb, don't call eth_mac_addr either, but
> rather call memcpy by themselves to copy the new MAC, just as the
> original r8169 code did, too.  Consequentially this problem is not
> present on igb.
> 
Doing the memcpy directly in the driver (like it was before) would be
a solution, however I'd consider this more a workaround because purpose
of the core functions is to encapsulate such details.

> I suggest to revert this change in the first place, but I wonder if
> we're not just missing to set IFF_LIVE_ADDR_CHANGE in a lot of drivers.
> 
I'd prefer to keep the code and set flag IFF_LIVE_ADDR_CHANGE in the
driver. Setting this flag via ethtool --set-priv-flags in theory would
also be an option, however the r8169 driver doesn't implement the
ethtool_ops set_priv_flags callback.

> 
> Thanks,
> Corinna
> 
Rgds, Heiner