netdev - Re: [RFC net-next 2/7] net: bridge: switchdev: Include local flag in FDB notifications

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210118192757.xpb4ad2af2xpetx3@skbuf>
Date:   Mon, 18 Jan 2021 21:27:57 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Tobias Waldekranz <tobias@...dekranz.com>
Cc:     davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
        vivien.didelot@...il.com, f.fainelli@...il.com, roopa@...dia.com,
        nikolay@...dia.com, netdev@...r.kernel.org, jiri@...nulli.us,
        idosch@...sch.org, stephen@...workplumber.org
Subject: Re: [RFC net-next 2/7] net: bridge: switchdev: Include local flag in
 FDB notifications

On Mon, Jan 18, 2021 at 07:58:59PM +0100, Tobias Waldekranz wrote:
> Ah I see, no I was not aware of that. I just saw that the entry towards
> the CPU was added to the ATU, which it would in both cases. I.e. from
> the switch's POV, in this setup:
> 
>    br0
>    / \ (A)
> swp0 dummy0
>        (B)
> 
> A "local" entry like (A), or a "static" entry like (B) means the same
> thing to the switch: "it is somewhere behind my CPU-port".

Yes, except that if dummy0 was a real and non-switchdev interface, then
the "local" entry would probably break your traffic if what you meant
was "static".

> > So I think there is a very real issue in that the FDB entries with the
> > is_local bit was never specified to switchdev thus far, and now suddenly
> > is. I'm sorry, but what you're saying in the commit message, that
> > "!added_by_user has so far been indistinguishable from is_local" is
> > simply false.
> 
> Alright, so how do you do it? Here is the struct:
> 
>     struct switchdev_notifier_fdb_info {
> 	struct switchdev_notifier_info info; /* must be first */
> 	const unsigned char *addr;
> 	u16 vid;
> 	u8 added_by_user:1,
> 	   offloaded:1;
>     };
> 
> Which field separates a local address on swp0 from a dynamically learned
> address on swp0?

None, that's the problem. Local addresses are already presented to
switchdev without saying that they're local. Which is the entire reason
that users are misled into thinking that the addresses are not local.

I may have misread what you said, but to me, "!added_by_user has so far
been indistinguishable from is_local" means that:
- every struct switchdev_notifier_fdb_info with added_by_user == true
  also had an implicit is_local == false
- every struct switchdev_notifier_fdb_info with added_by_user == false
  also had an implicit is_local == true
It is _this_ that I deemed as clearly untrue.

The is_local flag is not indistinguishable from !added_by_user, it is
indistinguishable full stop. Which makes it hard to work with in a
backwards-compatible way.

> Ok, so just to see if I understand this correctly:
> 
> The situation today it that `bridge fdb add ADDR dev DEV master` results
> in flows towards ADDR being sent to:
> 
> 1. DEV if DEV belongs to a DSA switch.
> 2. To the host if DEV was a non-offloaded interface.

Not quite. In the bridge software FDB, the entry is marked as is_local
in both cases, doesn't matter if the interface is offloaded or not.
Just that switchdev does not propagate the is_local flag, which makes
the switchdev listeners think it is not local. The interpretation of
that will probably vary among switchdev drivers.

The subtlety is that for a non-offloading interface, the
misconfiguration (when you mean static but use local) is easy to catch.
Since only the entry from the software FDB will be hit, this means that
the frame will never be forwarded, so traffic will break.
But in the case of a switchdev offloading interface, the frames will hit
the hardware FDB entry more often than the software FDB entry. So
everything will work just fine and dandy even though it shouldn't.

> With this series applied both would result in (2) which, while
> idiosyncratic, is as intended. But this of course runs the risk of
> breaking existing scripts which rely on the current behavior.

Yes.

My only hope is that we could just offload the entries pointing towards
br0, and ignore the local ones. But for that I would need the bridge
maintainers to clarify what is the difference between then, as I asked
in your other patch.