| 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
| ||
|
Message-ID: <87wntzmbva.fsf@waldekranz.com>
Date: Mon, 22 Mar 2021 16:44:41 +0100
From: Tobias Waldekranz <tobias@...dekranz.com>
To: Vladimir Oltean <olteanv@...il.com>,
Jakub Kicinski <kuba@...nel.org>,
"David S. Miller" <davem@...emloft.net>
Cc: Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
Roopa Prabhu <roopa@...dia.com>,
Nikolay Aleksandrov <nikolay@...dia.com>,
Jiri Pirko <jiri@...nulli.us>,
Ido Schimmel <idosch@...sch.org>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
UNGLinuxDriver@...rochip.com, Vadym Kochan <vkochan@...vell.com>,
Taras Chornyi <tchornyi@...vell.com>,
Grygorii Strashko <grygorii.strashko@...com>,
Vignesh Raghavendra <vigneshr@...com>,
Ioana Ciornei <ioana.ciornei@....com>,
Ivan Vecera <ivecera@...hat.com>, linux-omap@...r.kernel.org,
Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [RFC PATCH v2 net-next 09/16] net: dsa: replay port and local fdb entries when joining the bridge
On Fri, Mar 19, 2021 at 01:18, Vladimir Oltean <olteanv@...il.com> wrote:
> From: Vladimir Oltean <vladimir.oltean@....com>
>
> When a DSA port joins a LAG that already had an FDB entry pointing to it:
>
> ip link set bond0 master br0
> bridge fdb add dev bond0 00:01:02:03:04:05 master static
> ip link set swp0 master bond0
>
> the DSA port will have no idea that this FDB entry is there, because it
> missed the switchdev event emitted at its creation.
>
> Ido Schimmel pointed this out during a discussion about challenges with
> switchdev offloading of stacked interfaces between the physical port and
> the bridge, and recommended to just catch that condition and deny the
> CHANGEUPPER event:
> https://lore.kernel.org/netdev/20210210105949.GB287766@shredder.lan/
>
> But in fact, we might need to deal with the hard thing anyway, which is
> to replay all FDB addresses relevant to this port, because it isn't just
> static FDB entries, but also local addresses (ones that are not
> forwarded but terminated by the bridge). There, we can't just say 'oh
> yeah, there was an upper already so I'm not joining that'.
>
> So, similar to the logic for replaying MDB entries, add a function that
> must be called by individual switchdev drivers and replays local FDB
> entries as well as ones pointing towards a bridge port. This time, we
> use the atomic switchdev notifier block, since that's what FDB entries
> expect for some reason.
>
> Reported-by: Ido Schimmel <idosch@...sch.org>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> include/linux/if_bridge.h | 9 +++++++
> include/net/switchdev.h | 1 +
> net/bridge/br_fdb.c | 52 +++++++++++++++++++++++++++++++++++++++
> net/dsa/dsa_priv.h | 1 +
> net/dsa/port.c | 4 +++
> net/dsa/slave.c | 2 +-
> 6 files changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index 4c25dafb013d..89596134e88f 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -147,6 +147,8 @@ void br_fdb_clear_offload(const struct net_device *dev, u16 vid);
> bool br_port_flag_is_set(const struct net_device *dev, unsigned long flag);
> u8 br_port_get_stp_state(const struct net_device *dev);
> clock_t br_get_ageing_time(struct net_device *br_dev);
> +int br_fdb_replay(struct net_device *br_dev, struct net_device *dev,
> + struct notifier_block *nb);
> #else
> static inline struct net_device *
> br_fdb_find_port(const struct net_device *br_dev,
> @@ -175,6 +177,13 @@ static inline clock_t br_get_ageing_time(struct net_device *br_dev)
> {
> return 0;
> }
> +
> +static inline int br_fdb_replay(struct net_device *br_dev,
> + struct net_device *dev,
> + struct notifier_block *nb)
> +{
> + return -EINVAL;
> +}
> #endif
>
> #endif
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index b7fc7d0f54e2..7688ec572757 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -205,6 +205,7 @@ struct switchdev_notifier_info {
>
> struct switchdev_notifier_fdb_info {
> struct switchdev_notifier_info info; /* must be first */
> + struct list_head list;
> const unsigned char *addr;
> u16 vid;
> u8 added_by_user:1,
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index b7490237f3fc..49125cc196ac 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -726,6 +726,58 @@ static inline size_t fdb_nlmsg_size(void)
> + nla_total_size(sizeof(u8)); /* NFEA_ACTIVITY_NOTIFY */
> }
>
> +static int br_fdb_replay_one(struct notifier_block *nb,
> + struct net_bridge_fdb_entry *fdb,
> + struct net_device *dev)
> +{
> + struct switchdev_notifier_fdb_info item;
> + int err;
> +
> + item.addr = fdb->key.addr.addr;
> + item.vid = fdb->key.vlan_id;
> + item.added_by_user = test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
> + item.offloaded = test_bit(BR_FDB_OFFLOADED, &fdb->flags);
> + item.info.dev = dev;
> +
> + err = nb->notifier_call(nb, SWITCHDEV_FDB_ADD_TO_DEVICE, &item);
> + return notifier_to_errno(err);
> +}
> +
> +int br_fdb_replay(struct net_device *br_dev, struct net_device *dev,
> + struct notifier_block *nb)
> +{
> + struct net_bridge_fdb_entry *fdb;
> + struct net_bridge *br;
> + int err = 0;
> +
> + if (!netif_is_bridge_master(br_dev))
> + return -EINVAL;
> +
> + if (!netif_is_bridge_port(dev))
> + return -EINVAL;
> +
> + br = netdev_priv(br_dev);
> +
> + rcu_read_lock();
> +
> + hlist_for_each_entry_rcu(fdb, &br->fdb_list, fdb_node) {
> + struct net_device *dst_dev;
> +
> + dst_dev = fdb->dst ? fdb->dst->dev : br->dev;
> + if (dst_dev != br_dev && dst_dev != dev)
> + continue;
> +
I do not know if it is a problem or not, more of an observation: This is
not guaranteed to be an exact replay of the events that the bridge port
(i.e. bond0 or whatever) has received since, in fdb_insert, we exit
early when adding local entries if that address is already in the
database.
Do we have to guard against this somehow? Or maybe we should consider
the current behavior a bug and make sure to always send the event in the
first place?
> + err = br_fdb_replay_one(nb, fdb, dst_dev);
> + if (err)
> + break;
> + }
> +
> + rcu_read_unlock();
> +
> + return err;
> +}
> +EXPORT_SYMBOL(br_fdb_replay);
> +
> static void fdb_notify(struct net_bridge *br,
> const struct net_bridge_fdb_entry *fdb, int type,
> bool swdev_notify)
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index b14c43cb88bb..92282de54230 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -262,6 +262,7 @@ static inline bool dsa_tree_offloads_bridge_port(struct dsa_switch_tree *dst,
>
> /* slave.c */
> extern const struct dsa_device_ops notag_netdev_ops;
> +extern struct notifier_block dsa_slave_switchdev_notifier;
> extern struct notifier_block dsa_slave_switchdev_blocking_notifier;
>
> void dsa_slave_mii_bus_init(struct dsa_switch *ds);
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index 6670612f96c6..9850051071f2 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -205,6 +205,10 @@ static int dsa_port_switchdev_sync(struct dsa_port *dp,
> if (err && err != -EOPNOTSUPP)
> return err;
>
> + err = br_fdb_replay(br, brport_dev, &dsa_slave_switchdev_notifier);
> + if (err && err != -EOPNOTSUPP)
> + return err;
> +
> return 0;
> }
>
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index b974d8f84a2e..c51e52418a62 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -2392,7 +2392,7 @@ static struct notifier_block dsa_slave_nb __read_mostly = {
> .notifier_call = dsa_slave_netdevice_event,
> };
>
> -static struct notifier_block dsa_slave_switchdev_notifier = {
> +struct notifier_block dsa_slave_switchdev_notifier = {
> .notifier_call = dsa_slave_switchdev_event,
> };
>
> --
> 2.25.1
Powered by blists - more mailing lists