Message-ID: <20220410220324.4c3l3idubwi3w6if@skbuf>
Date:   Sun, 10 Apr 2022 22:03:25 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Tobias Waldekranz <tobias@...dekranz.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Roopa Prabhu <roopa@...dia.com>,
        Nikolay Aleksandrov <nikolay@...dia.com>,
        Jiri Pirko <jiri@...dia.com>, Ido Schimmel <idosch@...dia.com>,
        Mattias Forsblad <mattias.forsblad@...il.com>,
        Joachim Wiberg <troglobit@...il.com>
Subject: Re: [PATCH net-next 0/6] Disable host flooding for DSA ports under a
 bridge

On Sun, Apr 10, 2022 at 08:02:13PM +0200, Tobias Waldekranz wrote:
> On Sat, Apr 09, 2022 at 20:45, Vladimir Oltean <vladimir.oltean@....com> wrote:
> > Hi Tobias,
> >
> > On Sat, Apr 09, 2022 at 09:46:54PM +0200, Tobias Waldekranz wrote:
> >> On Fri, Apr 08, 2022 at 23:03, Vladimir Oltean <vladimir.oltean@....com> wrote:
> >> > For this patch series to make more sense, it should be reviewed from the
> >> > last patch to the first. Changes were made in the order that they were
> >> > just to preserve patch-with-patch functionality.
> >> >
> >> > A little while ago, some DSA switch drivers gained support for
> >> > IFF_UNICAST_FLT, a mechanism through which they are notified of the
> >> > MAC addresses required for local standalone termination.
> >> > A bit longer ago, DSA also gained support for offloading BR_FDB_LOCAL
> >> > bridge FDB entries, which are the MAC addresses required for local
> >> > termination when under a bridge.
> >> >
> >> > So we have come one step closer to removing the CPU from the list of
> >> > destinations for packets with unknown MAC DA.What remains is to check
> >> > whether any software L2 forwarding is enabled, and that is accomplished
> >> > by monitoring the neighbor bridge ports that DSA switches have.
> >> >
> >> > With these changes, DSA drivers that fulfill the requirements for
> >> > dsa_switch_supports_uc_filtering() and dsa_switch_supports_mc_filtering()
> >> > will keep flooding towards the CPU disabled for as long as no port is
> >> > promiscuous. The bridge won't attempt to make its ports promiscuous
> >> > anymore either if said ports are offloaded by switchdev (this series
> >> > changes that behavior). Instead, DSA will fall back by its own will to
> >> > promiscuous mode on bridge ports when the bridge itself becomes
> >> > promiscuous, or a foreign interface is detected under the same bridge.
> >> 
> >> Hi Vladimir,
> >> 
> >> Great stuff! I've added Joachim to Cc. He has been working on a series
> >> to add support for configuring the equivalent of BR_FLOOD,
> >> BR_MCAST_FLOOD, and BR_BCAST_FLOOD on the bridge itself. I.e. allowing
> >> the user to specify how local_rcv is managed in br_handle_frame_finish.
> >> 
> >> For switchdev drivers, being able to query whether a bridge will ingress
> >> unknown unicast to the host or not seems like the missing piece that
> >> makes this bullet proof. I.e. if you have...
> >> 
> >> - No foreign interfaces
> >> - No promisc
> >> _and_
> >> - No BR_FLOOD on the bridge itself
> >> 
> >> ..._then_ you can safely disable unicast flooding towards the CPU
> >> port. The same would hold for multicast and BR_MCAST_FLOOD of course.
> >> 
> >> Not sure how close Joachim is to publishing his work. But I just thought
> >> you two should know about the other one's work :)
> >
> > I haven't seen Joachim's work and I sure hope he can clarify.
> 
> If you want to get a feel for it, it is available here (the branch name
> is just where he started out I think :))
> 
> https://github.com/westermo/linux/tree/bridge-always-flood-unknown-mcast
> 
> > It seems
> > like there is some overlap that I don't currently know what to make of.
> > The way I see things, BR_FLOOD and BR_MCAST_FLOOD are egress settings,
> > so I'm not sure how to interpret them when applied to the bridge device
> > itself.
> 
> They are egress settings, yes. But from the view of the forwarding
> mechanism in the bridge, I would argue that the host interface is (or at
> least should be) as much an egress port as any of the lower devices.
> 
> - It can be the target of an FDB entry
> - It can be a member of an MDB entry
> 
> I.e. it can be chosen as a _destination_ => egress. This is analogous to
> the CPU port, which from the ASICs point of view is an egress port, but
> from a system POV it is receiving frames.
> 
> > On the other hand, treating IFF_PROMISC/IFF_ALLMULTI on the
> > bridge device as the knob that decides whether the software bridge wants
> > to ingress unknown MAC DA packets seems the more appropriate thing to do.
> 
> Maybe. I think it depends on how exact we want to be in our
> classification. Fundementally, I think the problem is that a bridge
> deals with one thing that other netdevs do not:
> 
>   Whether the destination in known/registered or not.
> 
> A NIC's unicast/multicast filters are not quite the same thing, because
> a they only deal with a single endpoint. I.e. if an address isn't in a
> NIC's list of known DA's, then it is "not mine". But in a bridge
> scenario, although it is not associated with the host (i.e. "not mine"),
> it can still be "known" (i.e. associated with some lower port).
> 
> AFAIK, promisc means "receive all the things!", whereas BR_FLOOD would
> just select the subset of frames for which the destination is unknown.
> 
> Likewise for multicast, IFF_ALLMULTI means "receive _all_ multicast" -
> it does not discriminate between registered and unregistered
> flows. BR_MCAST_FLOOD OTOH would only target unregistered flows.
> 
> Here is my understanding of how the two solutions would differ in the
> types of flows that they would affect:
> 
>                     .--------------------.-----------------.
>                     |       IFF_*        |    BR_*FLOOD    |
> .-------------------|---------.----------|-----.-----.-----|
> | Type              | Promisc | Allmulti | BC  | UC  | MC  |
> |-------------------|---------|----------|-----|-----|-----|
> | Broadcast         | Yes     | No       | Yes | No  | No  |
> | Unknown unicast   | Yes     | No       | No  | Yes | No  |
> | Unknown multicast | Yes     | Yes      | No  | No  | Yes |
> | Known unicast     | Yes     | No       | No  | No  | No  |
                        ~~~
                        To what degree does IFF_PROMISC affect known
                        unicast traffic?

> | Known multicast   | Yes     | Yes      | No  | No  | No  |
> '-------------------'--------------------'-----------------'

So to summarize what you're trying to say. You see two planes back to back.

 +-------------------------------+
 |    User space, other uppers   |
 +-------------------------------+
 |   Termination plane governed  |
 |     by dev->uc and dev->mc    |
 |            +-----+            |
 |            | br0 |            |
 +------------+-----+------------+
 |            | br0 |            |
 |            +-----+            |
 |  Forwarding plane governed    |
 |        by FDB and MDB         |
 | +------+------+------+------+ |
 | | swp0 | swp1 | swp2 | swp3 | |
 +-+------+------+------+------+-+

For a packet to be locally received on the br0 interface, it needs to
pass the filters from both the forwarding plane and the termination
plane.

Considering a unicast packet:
- if a local/permanent entry exists in the FDB, it is known to the
  forwarding plane, and its destination is the bridge device seen as a
  bridge port
- if the FDB doesn't contain this MAC DA, it is unknown to the
  forwarding plane, but the bridge device is still a destination for it,
  since the bridge seen as a bridge port has a non-configurable BR_FLOOD
  port flag which allows unknown unicast to exit the forwarding plane
  towards the termination plane implicitly on
- if the MAC DA exists in the RX filter of the bridge device
  (dev->dev_addr or dev->uc), the packet is known to the termination
  plane, so it is not filtered out.
- if the MAC DA doesn't exist in the RX filter, the packet is filtered
  out, unless the RX filter is disabled via IFF_PROMISC, case in which
  it is still accepted

Considering a multicast packet, things are mostly the same, except for
the fact that having a local/permanent MDB entry towards the bridge
device does not necessarily 'steal' it from the forwarding plane as it
does in case of unicast, since multicast traffic can have multiple
destinations which don't exclude each other.

Needless to say that what we have today is a very limited piece of the
bigger puzzle you've presented above, and perhaps does not even behave
the same as the larger puzzle would, restricted to the same operating
conditions as the current code.

Here are some practical questions so I can make sure I understand the
model you're proposing.

1. You or Joachim add support for BR_FLOOD on the bridge device itself.
   The bridge device is promiscuous, and a packet with a MAC DA unknown
   to the forwarding plane is received. Will this packet be seen in
   tcpdump or not? I assume not, because it never even reached the RX
   filtering lists of the bridge device, it didn't exit the forwarding
   plane.

2. Somebody comes later and adds support for IFF_ALLMULTI. This means,
   the RX filtering for unknown multicast can be toggled on or off.
   Be there a bridge with mcast_router enabled. Should the bridge device
   receive unknown multicast traffic or not, when IFF_ALLMULTI isn't
   enabled? Right now, I think IFF_ALLMULTI isn't necessary.

3. The bridge device does not implement IFF_UNICAST_FLT today, so any
   addition to the bridge_dev->uc list will turn on bridge_dev->flags &
   IFF_PROMISC. Do you see this as a problem on the RX filtering side of
   things, or from a system administration PoV, would you just like to
   disable BR_FLOOD on the forwarding side of the bridge device and call
   it a day, expect the applications to just continue working?

4. Let's say we implement IFF_UNICAST_FLT for the bridge device.
   How do we do it? Add one more lookup in br_handle_frame_finish()
   which didn't exist before, to search for this MAC DA in bridge_dev->uc?

5. Should the implementation of IFF_UNICAST_FLT influence the forwarding
   plane in any way? Think of a user space application emitting a
   PACKET_MR_UNICAST request to ensure it sees the packets with the MAC
   DA it needs. Should said application have awareness of the fact that
   the interface it's speaking to is a bridge device? If not, three
   things may happen. The admin (you) has turned off BR_FLOOD towards
   the bridge device, effectively cutting it off from the (unknown to
   the forwarding plane) packets it wants to see. Or the admin hasn't
   turned off BR_FLOOD, but the MAC DA, while present in the RX
   filtering lists, is still absent from the FDB, so while it is
   received locally by the application, is also flooded to all other
   bridge ports. Or the kernel may automatically add a BR_FDB_LOCAL entry,
   considering that it knows that if there's a destination for this
   MAC DA in this broadcast domain, for certain there isn't more than
   one, so it could just as well guide the forwarding plane towards it.
   Or you may choose completely the other side, "hey, the bridge device
   really is that special, and user space should put up with it and know
   it has to configure both its forwarding and termination plane before
   things work in a reasonable manner on it". But if this is the path
   you choose, what about the uppers a bridge may have, like a VLAN with
   a MAC address different from the bridge's? Should the 8021q driver
   also be aware of the fact that dev_uc_add() may not be sufficient
   when applied to the bridge as a real_dev?

6. For a switchdev driver like DSA, what condition do you propose it
   should monitor for deciding whether to enable flooding towards the
   host? IFF_PROMISC, BR_FLOOD towards the bridge, or both? You may say
   "hey, switchdev offloads just the forwarding plane, I don't really
   care if the bridge software path is going to accept the packet or
   not". Or you may say that unicast flooding is to be enabled if
   IFF_PROMISC || BR_FLOOD, and multicast flooding if IFF_PROMISC ||
   IFF_ALLMULTI || BR_MCAST_FLOOD, and broadcast flooding if
   BR_BCAST_FLOOD. Now take into consideration the additional checks for
   foreign interfaces. Does this complexity scale to the number of
   switchdev drivers we have? If not, can we do something about it?

Powered by blists - more mailing lists