linux-kernel - Re: [PATCH v2 net-next 1/6] net: bridge: notify switchdev of disappearance of old FDB entry upon migration

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]

Date:   Sun, 13 Dec 2020 13:55:58 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Nikolay Aleksandrov <nikolay@...dia.com>
CC:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "bridge@...ts.linux-foundation.org" 
        <bridge@...ts.linux-foundation.org>,
        Roopa Prabhu <roopa@...dia.com>,
        "David S. Miller" <davem@...emloft.net>,
        DENG Qingfang <dqfext@...il.com>,
        Tobias Waldekranz <tobias@...dekranz.com>,
        Marek Behun <marek.behun@....cz>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Alexandra Winter <wintera@...ux.ibm.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Ido Schimmel <idosch@...sch.org>,
        Claudiu Manoil <claudiu.manoil@....com>
Subject: Re: [PATCH v2 net-next 1/6] net: bridge: notify switchdev of
 disappearance of old FDB entry upon migration

Hi Nik,

On Sun, Dec 13, 2020 at 03:22:16PM +0200, Nikolay Aleksandrov wrote:
> Hi Vladimir,
> Thank you for the good explanation, it really helps a lot to understand the issue.
> Even though it's deceptively simple, that call adds another lock/unlock for everyone
> when moving or learning (due to notifier lock)

This unlikely code path is just on movement, as far as I understand it.
How often do we expect that to happen? Is there any practical use case
where an FDB entry ping pongs between ports?

> , but I do like how simple the solution
> becomes with this change, so I'm not strictly against it. I think I'll add a "refcnt"-like
> check in the switchdev fn which would process the chain only when there are registered users
> to avoid any locks when moving fdbs on pure software bridges (like it was before swdev).

That makes sense.

> I get that the alternative is to track these within DSA, I'm tempted to say that's not such
> a bad alternative as this change would make moving fdbs slower in general.

I deliberately said "rule" instead of "static FDB entry" and "control
interface" instead of "CPU port" because this is not only about DSA.
I know of at least one other switchdev device which doesn't support
source address learning for host-injected traffic. It isn't even so much
of a quirk as it is the way that the hardware works. If you think of it
as a "switch with queues", there would be little reason for a hardware
designer to not just provide you the means to inject directly into the
queues of the egress port, therefore bypassing the normal analyzer and
forwarding logic.

Everything we do in DSA must be copied sooner or later in other similar
drivers, to get the same functionality. So I would really like to keep
this interface simple, and not inflict unnecessary complications if
possible.

> Have you thought about another way to find out, e.g. if more fdb
> information is passed to the notifications ?

Like what, keep emitting just the ADD notification, but put some
metadata in it letting listeners know that it was actually migrated from
a different bridge port, in order to save one notification? That would
mean that we would need to:

	case SWITCHDEV_FDB_ADD_TO_DEVICE:
		fdb_info = ptr;

		if (dsa_slave_dev_check(dev)) {
			if (!fdb_info->migrated_from_dev || dsa_slave_dev_check(fdb_info->migrated_from_dev)) {
				if (!fdb_info->added_by_user)
					return NOTIFY_OK;

				dp = dsa_slave_to_port(dev);

				add = true;
			} else if (fdb_info->migrated_from_dev && !dsa_slave_dev_check(fdb_info->migrated_from_dev)) {
				/* An address has migrated from a non-DSA port
				 * to a DSA port. Check if that non-DSA port was
				 * bridged with us, aka if we previously had that
				 * address installed towards the CPU.
				 */
				struct net_device *br_dev;
				struct dsa_slave_priv *p;

				br_dev = netdev_master_upper_dev_get_rcu(dev);
				if (!br_dev)
					return NOTIFY_DONE;

				if (!netif_is_bridge_master(br_dev))
					return NOTIFY_DONE;

				p = dsa_slave_dev_lower_find(br_dev);
				if (!p)
					return NOTIFY_DONE;

				delete = true;
			}
		} else {
			/* Snoop addresses learnt on foreign interfaces
			 * bridged with us, for switches that don't
			 * automatically learn SA from CPU-injected traffic
			 */
			struct net_device *br_dev;
			struct dsa_slave_priv *p;

			br_dev = netdev_master_upper_dev_get_rcu(dev);
			if (!br_dev)
				return NOTIFY_DONE;

			if (!netif_is_bridge_master(br_dev))
				return NOTIFY_DONE;

			p = dsa_slave_dev_lower_find(br_dev);
			if (!p)
				return NOTIFY_DONE;

			dp = p->dp->cpu_dp;

			if (!dp->ds->assisted_learning_on_cpu_port)
				return NOTIFY_DONE;
		}
	case SWITCHDEV_FDB_DEL_TO_DEVICE:
		not shown here

I probably didn't even get it right. We would need to delete an entry
from the notification of a FDB insertion, which is a bit counter-intuitive,
and the logic is a bit mind-boggling. I don't know, it is all much
simpler if we just emit an insertion notification on insertion and a
deletion notification on deletion. Which way you prefer, really.