netdev at kapio-technology.com
2022-Aug-10 08:40 UTC
[Bridge] [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers
On 2022-08-10 09:21, Ido Schimmel wrote:> On Tue, Aug 09, 2022 at 10:00:49PM +0200, netdev at kapio-technology.com > wrote: >> On 2022-08-09 11:20, Ido Schimmel wrote: >> > On Mon, Aug 01, 2022 at 05:33:49PM +0200, netdev at kapio-technology.com >> > wrote: >> > > On 2022-07-13 14:39, Ido Schimmel wrote: >> > > >> > > > >> > > > What are "Storm Prevention" and "zero-DPV" FDB entries? >> > > > >> > > >> > > For the zero-DPV entries, I can summarize: >> > > >> > > Since a CPU can become saturated from constant SA Miss Violations >> > > from a >> > > denied source, source MAC address are masked by loading a zero-DPV >> > > (Destination Port Vector) entry in the ATU. As the address now >> > > appears in >> > > the database it will not cause more Miss Violations. ANY port trying >> > > to send >> > > a frame to this unauthorized address is discarded. Any locked port >> > > trying to >> > > use this unauthorized address has its frames discarded too (as the >> > > ports SA >> > > bit is not set in the ATU entry). >> > >> > What happens to unlocked ports that have learning enabled and are trying >> > to use this address as SMAC? AFAICT, at least in the bridge driver, the >> > locked entry will roam, but will keep the "locked" flag, which is >> > probably not what we want. Let's see if we can agree on these semantics >> > for a "locked" entry: >> >> The next version of this will block forwarding to locked entries in >> the >> bridge, so they will behave like the zero-DPV entries. > > I'm talking about roaming, not forwarding. Let's say you have a locked > entry with MAC X pointing to port Y. Now you get a packet with SMAC X > from port Z which is unlocked. Will the FDB entry roam to port Z? I > think it should, but at least in current implementation it seems that > the "locked" flag will not be reset and having locked entries pointing > to an unlocked port looks like a bug. >Remember that zero-DPV entries blackhole (mask) the MAC, so whenever a packet appears with the same MAC on another port it is just dropped in the HW, so there is no possibility of doing any CPU processing in this case. Only after the timeout (5 min) can the MAC get a normal ATU on an open port. For the bridge to do what you suggest, a FDB search would be needed afaics, and this might be in a process sensitive part of the code, thus leading to too heavy a cost.>> >> > >> > 1. It discards packets with matching DMAC, regardless of ingress port. I >> > read the document [1] you linked to in a different reply and could not >> > find anything against this approach, so this might be fine or at least >> > not very significant. >> > >> > Note that this means that "locked" entries need to be notified to device >> > drivers so that they will install a matching entry in the HW FDB. >> >> Okay, so as V4 does (just without the error noted). >> >> > >> > 2. It is not refreshed and has ageing enabled. That is, after initial >> > installation it will be removed by the bridge driver after configured >> > ageing time unless converted to a regular (unlocked) entry. >> > >> > I assume this allows you to remove the timer implementation from your >> > driver and let the bridge driver notify you about the removal of this >> > entry. >> >> Okay, but only if the scheme is not so that the driver creates the >> locked >> entries itself, unless you indicate that the driver notifies the >> bridge, >> which then notifies back to the driver and installs the zero-DPV >> entry? If >> not I think the current implementation for the mv88e6xxx is fine. > > I don't see a problem in having the driver notifying the bridge about > the installation of this entry and the bridge notifying the driver that > the entry needs to be removed. It removes complexity from device > drivers > like mv88e6xxx and doesn't add extra complexity to the bridge driver. > > Actually, there is one complication, 'SWITCHDEV_FDB_ADD_TO_BRIDGE' will > add the locked entry as externally learned, which means the bridge will > not age it. Might need something like this: > > diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c > index e7f4fccb6adb..5f73d0b44ed9 100644 > --- a/net/bridge/br_fdb.c > +++ b/net/bridge/br_fdb.c > @@ -530,7 +530,8 @@ void br_fdb_cleanup(struct work_struct *work) > unsigned long this_timer = f->updated + delay; > > if (test_bit(BR_FDB_STATIC, &f->flags) || > - test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags)) { > + (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags) && > + !test_bit(BR_FDB_ENTRY_LOCKED, &f->flags))) { > if (test_bit(BR_FDB_NOTIFY, &f->flags)) { > if (time_after(this_timer, now)) > work_delay = min(work_delay, >There is a case of ownership of the FDB/ATU entry, which if I remember correctly, will point to the current implementation being the right way to do it, thus having the driver keeping ownership of the entry and thereby also ageing it, but I think Vladimir should have his say here.>> >> > >> > 3. With regards to roaming, the entry cannot roam between locked ports >> > (they need to have learning disabled anyway), but can roam to an >> > unlocked port, in which case it becomes a regular entry that can roam >> > and age. >> > >> > If we agree on these semantics, then I can try to verify that at least >> > Spectrum can support them (it seems mv88e6xxx can). >> >> The consensus here is that at least for the mv88e6xxx, learning should >> be on >> and link local learning should be blocked by the userspace setting you >> pointed to earlier. > > Why learning needs to be on in the bridge (not mv88e6xxx) driver?I think it is seen as 'cheating' to enable learning only in the driver behind the scenes, so kind of hackish. E.g. 'bridge -d link show' will then report 'learning off', while learning is on in the driver. And learning needs to be on for the driver as discussed earlier, which only gives rise to the link local learning problem, which is then solved by the user space setting.
Ido Schimmel
2022-Aug-11 11:28 UTC
[Bridge] [PATCH v4 net-next 3/6] drivers: net: dsa: add locked fdb entry flag to drivers
On Wed, Aug 10, 2022 at 10:40:45AM +0200, netdev at kapio-technology.com wrote:> On 2022-08-10 09:21, Ido Schimmel wrote: > > On Tue, Aug 09, 2022 at 10:00:49PM +0200, netdev at kapio-technology.com > > wrote: > > > On 2022-08-09 11:20, Ido Schimmel wrote: > > > > On Mon, Aug 01, 2022 at 05:33:49PM +0200, netdev at kapio-technology.com > > > > wrote: > > > > > On 2022-07-13 14:39, Ido Schimmel wrote: > > > > > > > > > > > > > > > > > What are "Storm Prevention" and "zero-DPV" FDB entries? > > > > > > > > > > > > > > > > For the zero-DPV entries, I can summarize: > > > > > > > > > > Since a CPU can become saturated from constant SA Miss Violations > > > > > from a > > > > > denied source, source MAC address are masked by loading a zero-DPV > > > > > (Destination Port Vector) entry in the ATU. As the address now > > > > > appears in > > > > > the database it will not cause more Miss Violations. ANY port trying > > > > > to send > > > > > a frame to this unauthorized address is discarded. Any locked port > > > > > trying to > > > > > use this unauthorized address has its frames discarded too (as the > > > > > ports SA > > > > > bit is not set in the ATU entry). > > > > > > > > What happens to unlocked ports that have learning enabled and are trying > > > > to use this address as SMAC? AFAICT, at least in the bridge driver, the > > > > locked entry will roam, but will keep the "locked" flag, which is > > > > probably not what we want. Let's see if we can agree on these semantics > > > > for a "locked" entry: > > > > > > The next version of this will block forwarding to locked entries in > > > the > > > bridge, so they will behave like the zero-DPV entries. > > > > I'm talking about roaming, not forwarding. Let's say you have a locked > > entry with MAC X pointing to port Y. Now you get a packet with SMAC X > > from port Z which is unlocked. Will the FDB entry roam to port Z? I > > think it should, but at least in current implementation it seems that > > the "locked" flag will not be reset and having locked entries pointing > > to an unlocked port looks like a bug. > > > > Remember that zero-DPV entries blackhole (mask) the MAC, so whenever a > packet appears with the same MAC on another port it is just dropped in theWhat do you mean by "same MAC"? As SMAC or DMAC? I'm talking about SMAC and when the packet is received via an unlocked port. Why would such a packet be dropped?> HW, so there is no possibility of doing any CPU processing in this case. > Only after the timeout (5 min) can the MAC get a normal ATU on an open port. > For the bridge to do what you suggest, a FDB search would be needed afaics, > and this might be in a process sensitive part of the code, thus leading to > too heavy a cost.When learning is enabled the bridge already performs a lookup on the SMAC. TBH, I don't think this is progressing well because there is too much discrepancy between how this feature works in the bridge driver and in the hardware you work with. I suggest to first define the model in the bridge driver and then take care of the offload. My suggestion is to send another RFC with only the bridge changes with emphasize on the following aspects: * Forwarding rules for "locked" entries. Do they drop matching packets? Forward them? Or not considered at all for forwarding? * Roaming rules for "locked" entries. Can they roam between locked ports? Can they roam from a locked port to an unlocked port and vice versa? Or are they completely sticky? * Ageing rule for "locked" entries. Are these entries subject to the ageing time or are they static? If they are not static, are they refreshed by incoming traffic from a locked port or not? * MAB enablement. New option? Overload an existing one? No option? The commit messages should explain these design choices and new tests cases should verify the desired behavior. Once we have an agreement we can work out the switchdev/mv88e6xxx parts and eventually the entire thing can be merged together. Fair?