Message-ID: <ee159769-4359-86ce-3dca-78dff9d8366a@nvidia.com>
Date:   Mon, 18 Jan 2021 23:39:27 +0200
From:   Nikolay Aleksandrov <nikolay@...dia.com>
To:     Tobias Waldekranz <tobias@...dekranz.com>,
        Vladimir Oltean <olteanv@...il.com>
CC:     <davem@...emloft.net>, <kuba@...nel.org>, <andrew@...n.ch>,
        <vivien.didelot@...il.com>, <f.fainelli@...il.com>,
        <roopa@...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 18/01/2021 23:22, Nikolay Aleksandrov wrote:
> On 18/01/2021 23:17, Nikolay Aleksandrov wrote:
>> On 18/01/2021 22:19, Tobias Waldekranz wrote:
>>> On Mon, Jan 18, 2021 at 21:27, Vladimir Oltean <olteanv@...il.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.
>>
> 
> Scratch that, I spoke too soon. You can get rid of it internally, just need
> to be careful not to break user-visible behaviour as Vladimir mentioned.
> 
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nvidia.com; s=n1;
	t=1611005977; bh=bdfYBvMa8LNyHkRyEtaHStOZr794nuxZw02BF6Zfg5c=;
	h=ARC-Seal:ARC-Message-Signature:ARC-Authentication-Results:
	 Authentication-Results:Subject:From:To:CC:References:Message-ID:
	 Date:User-Agent:In-Reply-To:Content-Type:Content-Language:
	 Content-Transfer-Encoding:X-Originating-IP:X-ClientProxiedBy:
	 MIME-Version:X-MS-Exchange-MessageSentRepresentingType:
	 X-MS-PublicTrafficType:X-MS-Office365-Filtering-Correlation-Id:
	 X-MS-TrafficTypeDiagnostic:X-MS-Exchange-Transport-Forked:
	 X-Microsoft-Antispam-PRVS:X-MS-Oob-TLC-OOBClassifiers:
	 X-MS-Exchange-SenderADCheck:X-Microsoft-Antispam:
	 X-Microsoft-Antispam-Message-Info:X-Forefront-Antispam-Report:
	 X-MS-Exchange-AntiSpam-MessageData:
	 X-MS-Exchange-CrossTenant-Network-Message-Id:
	 X-MS-Exchange-CrossTenant-AuthSource:
	 X-MS-Exchange-CrossTenant-AuthAs:
	 X-MS-Exchange-CrossTenant-OriginalArrivalTime:
	 X-MS-Exchange-CrossTenant-FromEntityHeader:
	 X-MS-Exchange-CrossTenant-Id:X-MS-Exchange-CrossTenant-MailboxType:
	 X-MS-Exchange-CrossTenant-UserPrincipalName:
	 X-MS-Exchange-Transport-CrossTenantHeadersStamped:X-OriginatorOrg;
	b=lOH2ClNGfg5hhcLMJSlOtsM9K7JoVYkrv1v5FBtgFxxiiWBcOGYir2uqdIfXvMTD/
	 LUTYZ9tVzvZJ/tKymoPhlR+V27URN1nOwkEdz3k1u46QcB+3eQa1blAz8c8bU/HMAP
	 joO4AKJ9BIuG/sc3ZTAX7jdOE3JUSOmCdfhqTCNc6sGmaVFBAwPrrhGPVth3niCkc7
	 JwfTXir+8JtBC0XV3Vw2DiYs8RCX22S/48evhzu6O3PNsmLTFaOZaDb0Ep76MquFPu
	 rjCjXH2ZfG9J/D9YchY/hybtGMRK4aruos1La9mEVi6WzUeW+PhR0/FiXzfW/6fef7
	 cswqoYtddosLw==

Apologies for the multiple emails, but wanted to leave an example:

00:11:22:33:44:55 dev ens16 master bridge permanent

This must always exist and user-space must be able to create it, which
might be against what you want to achieve (no BR_FDB_LOCAL entries with
fdb->dst != NULL).

Powered by blists - more mailing lists