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

[Nikolay Aleksandrov <nikolay@nvidia.com>]

On 18/01/2021 22:19, Tobias Waldekranz wrote:
> On Mon, Jan 18, 2021 at 21:27, Vladimir Oltean <olteanv@gmail.com> wrote:
>> 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".
> 
> Agreed.
> 
>>>> 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.
> 
> This was probably a semantic mistake on my part, we meant the same
> thing.
> 
>>> 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.
> 
> Quite right.
> 
>>> 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
> 
> That was my initial approach. Unfortunately that breaks down when the
> bridge inherits its address from a port, i.e. the default case.
> 
> When the address is added to the bridge (fdb->dst == NULL), fdb_insert
> will find the previous local entry that is set on the port and bail out
> before sending a notification:
> 
> 	if (fdb) {
> 		/* it is okay to have multiple ports with same
> 		 * address, just use the first one.
> 		 */
> 		if (test_bit(BR_FDB_LOCAL, &fdb->flags))
> 			return 0;
> 		br_warn(br, "adding interface %s with same address as a received packet (addr:%pM, vlan:%u)\n",
> 		       source ? source->dev->name : br->dev->name, addr, vid);
> 		fdb_delete(br, fdb, true);
> 	}
> 
> You could change this so that a notification always is sent out. Or you
> could give precedence to !fdb->dst and update the existing entry.
> 
>> maintainers to clarify what is the difference between then, as I asked
>> in your other patch.
> 
> I am pretty sure they mean the same thing, I believe that !fdb->dst
> implies is_local. It is just that "bridge fdb add ADDR dev br0 self" is
> a new(er) thing, and before that there was "local" entries on ports.
> 
> Maybe I should try to get rid of the local flag in the bridge first, and
> then come back to this problem once that is done? Either way, I agree
> that 5/7 is all we want to add to DSA to get this working.
> 

BR_FDB_LOCAL and !fdb->dst are not the same thing, check fdb_add_entry().
You cannot get rid of it, !fdb->dst implies BR_FDB_LOCAL, but it's not
symmetrical.

Cheers,
 Nik