Message-ID: <20201119162826.urnhw4issw2kxens@skbuf>
Date:   Thu, 19 Nov 2020 18:28:26 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Markus Blöchl <Markus.Bloechl@...tronik.com>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Ido Schimmel <idosch@...sch.org>, Andrew Lunn <andrew@...n.ch>,
        Alexander Duyck <alexander.duyck@...il.com>,
        Woojung Huh <woojung.huh@...rochip.com>,
        "David S. Miller" <davem@...emloft.net>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        netdev@...r.kernel.org
Subject: Re: [PATCH net] net: lan78xx: Disable hardware vlan filtering in
 promiscuous mode

On Thu, Nov 19, 2020 at 04:37:51PM +0100, Markus Blöchl wrote:
> > There might be an entire discussion about how promiscuity does _not_
> > mean "deliver all packets to the CPU" that might be of interest to you:
> > https://lkml.org/lkml/2019/8/29/255
>
> If I glanced over this discussion correctly, it is about avoiding
> promiscuity under certain circumstances (while promiscuity was
> only requested for switching and not by userspace) because HW promiscuity
> is not needed in that particular case.
>
> As I currently see it, there are two common use cases for promiscuity:
>
> 1) Applying filtering, switching and other logic on the CPU.
>    This could be due to limited resources in the NIC, e.g. when there
>    are too many unicast addresses configured on that interface or
>    simply an unavailable hardware capability.
>
> 2) Sniffing the wire.
>
> The kernel uses IFF_PROMISC (or `__dev_set_promiscuity()`) for both,
> which obviously can be overkill in the first case.
> The question remains, what does IFF_PROMISC exactly mean for userspace
> (which, I guess, most often uses it for 2)?

And that discussion ended with the conclusion that promiscuity is never
enough to "sniff the wire".

> > > b) - The same as a)
> > >    - Additionally enumerate and disable all available offloading features
> >
> > If said offloading features have to do with the CPU not receiving some
> > frames any longer, and you _do_ want the CPU to see them, then obviously
> > said offloading features should be disabled. This includes not only VLAN
> > filtering, but also bridging offload, IP forwarding offload, etc.
> >
> > I'd say that (b) should be sufficient, but not future-proof in the sense
> > that new offloading features might come every day, and they would need
> > to be disabled on a case-by-case basis.
> >
> > For the forwarding offload, there would be no question whatsoever that
> > you'd need to turn it off, or add a mirroring rule towards the CPU, or
> > do _something_ user-visible, to get that traffic. But as for VLAN
> > filtering offload in particular, there's the (not negligible at all)
> > precedent created by the bridge driver, that Ido pointed out. That would
> > be a decision for the maintainers to make, if the Linux network stack
> > creates its own definition of promiscuity which openly contradicts IEEE's.
> > One might perhaps try to argue that the VLAN ID is an integral part of
> > the station's address (which is true if you look at it from the
> > perspective of an IEEE 802.1Q bridge), and therefore promiscuity should
> > apply on the VLAN ID too, not just the MAC address. Either way, that
> > should be made more clear than it currently is, once a decision is
> > taken.
>
> In that case, maybe new features which could alter user-visible behaviour
> should not be enabled by default?
> If I am not mistaken, bridging offload, IP forwarding offload and
> similar have to be enabled explicitly, at least.

Uhm, no? If L2 and L3 forwarding offload are supported by the hardware,
the way things work is that said driver simply monitors the user commands
and configures itself to seamlessly perform those operations in hardware.
There isn't a knob that says "I don't want to do bridging in hardware
between these two switchdev interfaces", if the driver implements bridging.

> I am not convinced that this definition would indeed contradict the
> IEEE standard. It might just be a stronger one with more guarantees.
> Assuming you have a very stupid NIC without any filtering or offloading
> capabilities, which would always forward all frames to the CPU.
> Would this NIC not comply to the standard?

I don't understand this.

> > > After those are answered, I am open to suggestions on how to fix this
> > > differently (if still needed).
> >
> > Turn off VLAN filtering, or get a commonly accepted definition of
> > promiscuity?
>
>
> Other Drivers
> =============
>
> So I tried to figure out what other existing hardware drivers do
> by grepping for drivers which do something on a change to
> NETIF_F_HW_VLAN_CTAG_FILTER.
>
> Here are the results of me trying to understand the drivers quickly.
> I hope it's somewhat close and helps:
>
> 1) aqc111
>    This controller has a HW register flag for promiscuous mode.
>    I am not sure what it does, but since this is another ASIX
>    device, the documentation from above should apply.
>
> 2) lan78xx
>    Here it began ...
>
> 3) ixgbe
>    This driver disables HW_VLAN_CTAG_FILTER if IFF_PROMISC is set.
>
> 4) ice
>    I don't know.
>
> 5) ocelot_net
>    This driver does not support promiscuous mode with the following
>    explanation:
> 	/* This doesn't handle promiscuous mode because the bridge core is
> 	 * setting IFF_PROMISC on all slave interfaces and all frames would be
> 	 * forwarded to the CPU port.
> 	 */
>    See the already mentioned discussion https://lkml.org/lkml/2019/8/29/255
>    for more.

The explanation for ocelot is bogus. Ocelot is a switch and it doesn't
implement IFF_PROMISC precisely because of that: a switch port is
promiscuous by definition. Also check out mlxsw_sp_set_rx_mode() in the
mlxsw driver. It is deliberately empty for the same reason.

> 6) liquidio
>    This driver also has a HW flag for promiscuous mode.
>    I don't know what it does, though.
>
> 7) mvpp2
>    This driver disables vid filtering if IFF_PROMISC is set.
>
> 8) efx
> 	/* Disable VLAN filtering by default.  It may be enforced if
> 	 * the feature is fixed (i.e. VLAN filters are required to
> 	 * receive VLAN tagged packets due to vPort restrictions).
> 	 */
>    I did not find a variant that really honors this feature.
>
> 9) ef100
>    I don't know.
>
> 10) mlx5
>    This driver sets a HW promisc flag, adds an `ANY` vlan filter rule
>    and ignores further changes to vlan filtering if IFF_PROMISC is set.
>
> 11) nfp
>    This driver uses a HW promisc flag.
>
> 12) atlantic
>    I don't know.
>
> 13) xlgmac
>    This one has an interesting comment in `xlgmac_set_promiscuous_mode`:
> 	/* Hardware will still perform VLAN filtering in promiscuous mode */
> 	if (enable) {
> 		xlgmac_disable_rx_vlan_filtering(pdata);
> 	} else {
> 		if (pdata->netdev->features & NETIF_F_HW_VLAN_CTAG_FILTER)
> 			xlgmac_enable_rx_vlan_filtering(pdata);
> 	}
>
> 14) enetc
>    I think, the feature is overridden everytime in `set_rx_mode` as long
>    as IFF_PROMISC is set.

This one was added by me in 7070eea5e95a ("enetc: permit configuration
of rx-vlan-filter with ethtool"). I'll fully admit that I disabled VLAN
filtering when entering IFF_PROMISC out of stupidity, and the main goal
of that patch was something different anyway. I didn't even think of
checking the IEEE 802.3 standard, but then you had to ask :). I'm sure
many more are in the same situation, and it's what led to this chaos.
But I don't have any users that rely on this behavior of IFF_PROMISC,
and if we were to agree on a definition of promiscuity that does not
assume anything about VLAN filtering, I would be more than happy to
delete that code. In fact I've been waiting for this to reach a conclusion
so that I could do that.

Powered by blists - more mailing lists