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
| ||
|
Message-ID: <20220810193825.vq7rdgwx7xua5amj@skbuf> Date: Wed, 10 Aug 2022 22:38:25 +0300 From: Vladimir Oltean <olteanv@...il.com> To: Sergei Antonov <saproj@...il.com> Cc: netdev@...r.kernel.org, Florian Fainelli <f.fainelli@...il.com> Subject: Re: [PATCH] net: dsa: mv88e6060: report max mtu 1536 On Wed, Aug 10, 2022 at 06:56:25PM +0300, Sergei Antonov wrote: > > reg = <16> for switch@0? Something is wrong, probably switch@0. > > Thanks for noticing it. > In my case the device addresses are: > PHY Registers - 0x10-0x14 > Switch Core Registers - 0x18-0x1D > Switch Global Registers - 0x1F > I renamed switch@0 to switch@10 and made reg hexadecimal for clarity: > "reg = <0x10>". It works, see below for more information on testing. > Should I leave it like so? Should be fine like that. I think Marvell switches tend to have this habit of occupying multiple PHY addresses, and our DT bindings want to see the first one. > > The bug seems to have been introduced by commit 0abfd494deef ("net: dsa: > > use dedicated CPU port"), because, although before we'd be uselessly > > programming the port VLAN for a disabled port, now in doing so, we > > dereference a NULL pointer. > > The suggested fix with dsa_is_unused_port() works. I tested it on the > 'netdev/net.git' repo, see below. Should I submit it as a patch > (Fixes: 0abfd494deef)? Yes. See the section that talks about "git log -1 --pretty=fixes" in process/submitting-patches.rst for how the Fixes tag should actually look like. I thought about whether dsa_is_unused_port() is sufficient, since theoretically dsa_is_dsa_port() is also a possibility which isn't covered by the check. But I rechecked and it appears that the Marvell 6060 doesn't support cascade ports, so we should be fine with just that. > So I tested "dsa_is_unused_port()" and "switch@10" fixes with > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git > What I did after system boot-up: > > ~ # dmesg | grep mv88 > [ 7.187296] mv88e6060 92000090.mdio--1-mii:10: switch Marvell 88E6060 (B0) detected > [ 8.325712] mv88e6060 92000090.mdio--1-mii:10: switch Marvell 88E6060 (B0) detected > [ 9.190299] mv88e6060 92000090.mdio--1-mii:10 lan2 (uninitialized): PHY [dsa-0.0:02] driver [Generic PHY] (irq=POLL) > > ~ # ip a > 1: lo: <LOOPBACK> mtu 65536 qdisc noop qlen 1000 > link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00 > 2: eth0: <BROADCAST,MULTICAST> mtu 1504 qdisc noop qlen 1000 > link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff The DSA master is super odd for starting with an all-zero MAC address. What driver handles this part? Normally, drivers are expected to work with a MAC address provided by the firmware (of_get_mac_address or other, perhaps proprietary, means) and fall back to eth_random_addr() if that is missing. > 3: lan2@...0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop qlen 1000 > link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff Here DSA inherits the MAC address of the master. It does this by default in dsa_slave_create() -> eth_hw_addr_inherit(). If the OF node for the DSA port has its own MAC address, that will have priority over the MAC address of the master. > ~ # ip link set dev eth0 address 00:90:e8:00:10:03 up This shouldn't be necessary, neither assigning a MAC address nor putting the master up, see Documentation/networking/dsa/configuration.rst, the master comes up automatically. > ~ # ip a add 192.168.127.254/24 dev lan2 > > ~ # ip link set dev lan2 address 00:90:e8:00:10:03 up > [ 56.383801] DSA: failed to set STP state 3 (-95) errno 95 is EOPNOTSUPP, we shouldn't warn here, I'll submit a patch for that. > [ 56.385491] mv88e6060 92000090.mdio--1-mii:10 lan2: configuring for phy/gmii link mode > [ 58.694319] mv88e6060 92000090.mdio--1-mii:10 lan2: Link is Up - 100Mbps/Full - flow control off The link was negotiated without flow control, so you can't test flow control under these conditions. This depends upon what the internal PHY of the mv88e6060 is advertising, and what the link partner is advertising. What is advertised is a subset of what is supported (and resolved by linkmode_resolve_pause). I'm a bit uncertain as to what happens when the 6060 driver doesn't validate the phylink supported mask at all (it doesn't populate the mac_capabilities structure and it doesn't implement ds->ops->phylink_validate) but I think what happens is that whatever the PHY supports isn't further reduced in any way by the MAC side of things. > [ 58.699244] IPv6: ADDRCONF(NETDEV_CHANGE): lan2: link becomes ready > > ~ # ip a > 1: lo: <LOOPBACK> mtu 65536 qdisc noop qlen 1000 > link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00 > 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1504 qdisc pfifo_fast qlen 1000 > link/ether 00:90:e8:00:10:03 brd ff:ff:ff:ff:ff:ff > inet6 fe80::290:e8ff:fe00:1003/64 scope link > valid_lft forever preferred_lft forever > 3: lan2@...0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue qlen 1000 > link/ether 00:90:e8:00:10:03 brd ff:ff:ff:ff:ff:ff > inet 192.168.127.254/24 scope global lan2 > valid_lft forever preferred_lft forever > inet6 fe80::290:e8ff:fe00:1003/64 scope link > valid_lft forever preferred_lft forever > > Ping, ssh, scp work. > > Is it correct for eth0 and lan2@...0 to have the same MAC? It is not wrong, it's a configuration that many deployed DSA systems use. > I could not make it work with different MACs. That is a problem, and I believe it's a problem with the DSA master driver. See, the reason it should work is this. Switch ports don't really have a MAC address, since they forward everything and not really terminate anything. The MAC address of a switch port is a software construct which means that software L3 termination interfaces (of which we have one per port) should accept packets with some known MAC DA, and drop the rest, and everything should be fine. There are multiple kinds of DSA tags, but 6060 uses a trailer, and this will not shift the 'real' MAC DA of packets compared to where the DSA master expects to see them. So if the MAC address of the DSA master is A, the MAC address of lan2 is B, and you ping lan2 from the outside world, the DSA master will see packets with a MAC DA of B. So the DSA master sees packets with a MAC DA different from its own dev->dev_addr (A) and thinks it's within its right to drop them. Except that it isn't, because we do the following to prevent it: (1) in case the DSA master supports IFF_UNICAST_FLT, we call dev_uc_add() from dsa_slave_set_mac_address() and from dsa_slave_open(), and this informs it of our address B. (2) in case it doesn't support IFF_UNICAST_FLT, we just call dsa_master_set_promiscuity() and that should keep it promiscuous and it should accept packets regardless of MAC DA (that's the definition). So you should run some tcpdump and ethtool -S on the DSA master and see whether it receives any packets or it drops them. It's possible that tcpdump makes packets be accepted, since it puts the interface in promiscuous mode. > I don't know how to test flow control. Ping, ssh, scp work even with > mv88e6060_setup_addr() code removed. Of course, if MAC SA plays some > role in other scenarios, let it be :). I think it's best to leave alone things you don't really care about. The address in mv88e6060_setup_addr() should have nothing to do with what you really seem to want to know, just with flow control.
Powered by blists - more mailing lists