Re: [RFC PATCH v2 net-next 05/17] net: bridge: implement unicast filtering for the bridge device

From: Vladimir Oltean <olteanv@gmail.com>
To: Ido Schimmel <idosch@idosch.org>
Cc: netdev@vger.kernel.org, Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Jiri Pirko <jiri@resnulli.us>, DENG Qingfang <dqfext@gmail.com>,
	Tobias Waldekranz <tobias@waldekranz.com>,
	George McCollister <george.mccollister@gmail.com>,
	Vlad Yasevich <vyasevich@gmail.com>,
	Roopa Prabhu <roopa@nvidia.com>,
	Nikolay Aleksandrov <nikolay@nvidia.com>
Subject: Re: [RFC PATCH v2 net-next 05/17] net: bridge: implement unicast filtering for the bridge device
Date: Wed, 2 Mar 2022 13:17:59 +0200	[thread overview]
Message-ID: <20220302111759.xylbcwkiev6igmqg@skbuf> (raw)
In-Reply-To: <Yh5H1zexT0/Q2bc4@shredder>

On Tue, Mar 01, 2022 at 06:20:39PM +0200, Ido Schimmel wrote:
> > > OK, I see the problem... So you want the bridge to support
> > > 'IFF_UNICAST_FLT' by installing local FDB entries? I see two potential
> > > problems:
> > > 
> > > 1. For VLAN-unaware bridges this is trivial as VLAN information is of no
> > > use. For VLAN-aware bridges we either need to communicate VLAN
> > > information from upper layers or install a local FDB entry per each
> > > configured VLAN (wasteful...). Note that VLAN information will not
> > > always be available (in PACKET_MR_UNICAST, for example), in which case a
> > > local FDB entry will need to be configured per each existing VLAN in
> > > order to maintain existing behavior. Which lead to me think about the
> > > second problem...
> > >
> > > 2. The bigger problem that I see is that if the bridge starts supporting
> > > 'IFF_UNICAST_FLT' by installing local FDB entries, then packets that
> > > were previously locally received and flooded will only be locally
> > > received. Only locally receiving them makes sense, but I don't know what
> > > will break if we change the existing behavior... Maybe this needs to be
> > > guarded by a new bridge option?
> > 
> > I think it boils down to whether PACKET_MR_UNICAST on br0 is equivalent to
> > 'bridge fdb add dev br0 self permanent' or not. Theoretically, the
> > former means "if a packet enters the local termination path of br0,
> > don't drop it", 
> 
> Trying to understand the first part of the sentence, are you saying that
> if user space decides to use this interface, then it is up to it to
> ensure that packets with the given unicast address are terminated on the
> bridge? That is, it is up to user space to install the necessary
> permanent FDB record?

This first part of the sentence is just wondering whether it is even
sane to make the bridge driver essentially provide an implementation for
PACKET_MR_UNICAST, and translate that into a local FDB entry which means
something else. User space can already install a local FDB entry with
the MAC address of the upper interface, and this will behave closer to
what is expected.

If the bridge ever implements the support for PACKET_MR_UNICAST, a new
FDB entry flag is probably needed, for local reception. If the MAC
address added with PACKET_MR_UNICAST is new, the bridge would create an
entry with fdb->dst = NULL. If it already exists, it would keep the
existing fdb->dst and just mark the local reception flag as true.
This is to comply with the "copy to CPU" semantics instead of altering
the forwarding destination. I'm not sure whether there are real use
cases beyond just complying to expected semantics.

> I think that is fair, it is just that right now this operation does
> something else and causes all the packets forwarded via the bridge to
> be locally terminated. Most of them will then be dropped by upper
> layers. I don't think this was the author's intention, it seems like
> an unfortunate side effect of current implementation.

Do you mean here that the "something else" is to turn on promiscuous
mode for the bridge, and this makes local_rcv = true for every packet in
br_handle_frame_finish?

Yes, that is a problem. The dev_uc_add() calls will keep the bridge's
promiscuity at 1, with no way to turn it back to 0 from user space.
To get rid of this we'd need to declare IFF_UNICAST_FLT at the very
least.

> This behavior is even more ridiculous when you take hardware offload
> into account, as usually the CPU is unable to handle all these
> packets.

If we keep the analogy that a PACKET_MR_UNICAST means "copy MAC address
X to CPU", then IFF_PROMISC means "copy all packets to CPU", no?

So I wouldn't say the behavior is even more ridiculous, it is just as
ridiculous, just on a different level. And maybe not even "ridiculous",
just "highly sub-optimal". Ridiculous would be to not comply to the
expected behavior.

> > while the other means "direct this MAC DA only towards
> > the local termination path of br0".
> 
> This I agree with.
> 
> > I.o.w. the difference between "copy to CPU" and "trap to CPU".
> > 
> > If we agree they aren't equivalent, and we also agree that a macvlan on
> > top of a bridge wants "trap to CPU" instead of "copy to CPU", I think
> > the only logical conclusion is that the communication mechanism between
> > the bridge and the macvlan that we're looking for doesn't exist -
> > dev_uc_add() does something slightly different.
> > 
> > Which is why I want to better understand your idea of having the bridge
> > track upper interfaces.
> 
> In my case these upper interfaces are actually router interfaces and I'm
> interested in their MAC (in addition to other attributes) to know which
> FDB entry to program towards the router port (your CPU port) on ingress
> and which SA to use on egress (the hardware has limitations on SAs).
> 
> I'm pretty sure bridge maintainers will not agree to have this code in
> the bridge driver in which case you can implement this in DSA. Should be
> quite simple as I guess most configurations use VLANs/MACVLANs uppers.

Yes, but this will record only the dev_addr of those upper interfaces.
It would not be fully compliant with what user space can ask for.