From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-24 11:45:04
From: Vladimir Oltean <vladimir.oltean@nxp.com>
This is my second stab at creating a list of unicast and multicast
addresses that the DSA CPU port must trap. I am reusing a lot of
Tobias's work which he submitted here:
https://patchwork.kernel.org/project/netdevbpf/cover/20210116012515.3152-1-tobias@waldekranz.com/
I did not yet have the guts to disable flooding towards the CPU port,
but if feedback for the approach taken here is positive, that will be
next.
Tobias Waldekranz (6):
net: bridge: switchdev: refactor br_switchdev_fdb_notify
net: bridge: switchdev: include local flag in FDB notifications
net: bridge: switchdev: send FDB notifications for host addresses
net: dsa: include bridge addresses which are local in the host fdb
list
net: dsa: sync static FDB entries on foreign interfaces to hardware
net: dsa: mv88e6xxx: Request assisted learning on CPU port
Vladimir Oltean (11):
net: dsa: reference count the host mdb addresses
net: dsa: reference count the host fdb addresses
net: dsa: install the host MDB and FDB entries in the master's RX
filter
net: dsa: install the port MAC addresses as host fdb entries
net: bridge: implement unicast filtering for the bridge device
net: dsa: add addresses obtained from RX filtering to host addresses
net: dsa: include fdb entries pointing to bridge in the host fdb list
net: dsa: replay port and host-joined mdb entries when joining the
bridge
net: dsa: replay port and local fdb entries when joining the bridge
net: bridge: switchdev: let drivers inform which bridge ports are
offloaded
net: bridge: offloaded ports are always promiscuous
drivers/net/dsa/mv88e6xxx/chip.c | 1 +
.../marvell/prestera/prestera_switchdev.c | 4 +-
.../mellanox/mlxsw/spectrum_switchdev.c | 7 +-
drivers/net/ethernet/mscc/ocelot_net.c | 43 +-
drivers/net/ethernet/rocker/rocker_main.c | 4 +-
drivers/net/ethernet/rocker/rocker_ofdpa.c | 2 +
drivers/net/ethernet/ti/am65-cpsw-nuss.c | 2 +
drivers/net/ethernet/ti/am65-cpsw-switchdev.c | 4 +-
drivers/net/ethernet/ti/cpsw_new.c | 1 +
drivers/net/ethernet/ti/cpsw_switchdev.c | 4 +-
drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 6 +-
include/linux/if_bridge.h | 18 +
include/net/dsa.h | 35 ++
include/net/switchdev.h | 19 +
net/bridge/br.c | 8 +-
net/bridge/br_device.c | 23 +-
net/bridge/br_fdb.c | 57 +-
net/bridge/br_if.c | 19 +-
net/bridge/br_mdb.c | 117 ++++
net/bridge/br_private.h | 14 +-
net/bridge/br_switchdev.c | 74 +--
net/dsa/dsa2.c | 30 +-
net/dsa/dsa_priv.h | 23 +-
net/dsa/port.c | 51 ++
net/dsa/slave.c | 589 ++++++++++++++----
net/dsa/switch.c | 123 +++-
net/switchdev/switchdev.c | 18 +
27 files changed, 1047 insertions(+), 249 deletions(-)
--
2.25.1
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-24 11:45:07
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Currently any DSA switch that is strict when implementing the mdb
operations prints these benign errors after the addresses expire, with
at least 2 ports bridged:
[ 286.013814] mscc_felix 0000:00:00.5 swp3: failed (err=-2) to del object (id=3)
The reason has to do with this piece of code:
netdev_for_each_lower_dev(dev, lower_dev, iter)
br_mdb_switchdev_host_port(dev, lower_dev, mp, type);
called from:
br_multicast_group_expired
-> br_multicast_host_leave
-> br_mdb_notify
-> br_mdb_switchdev_host
Basically, the bridge code is correct. It tells each switchdev port that
the host can leave that multicast group. But in the case of DSA, all
user ports are connected to the host through the same pipe. So, because
DSA blindly translates a host MDB to a normal MDB on the CPU port, this
means that when all user ports leave a multicast group, DSA tries to
remove it N times from the CPU port.
We should be reference-counting these addresses. Otherwise, the first
port on which the MDB expires will cause an entry removal from the CPU
port, which will break the host MDB for the remaining ports.
There is more. In expectation of future support for multiple CPUs, the
addresses filtered towards the host should be kept in a list per CPU
port. And cross-chip setups with multiple CPU ports can be really
unconventional, nobody says that the switches must be cascaded. Consider
just this example:
+-------------------------+
| Host system |
| |
| eth0 eth1 |
+-------------------------+
CPU port | | CPU port
+-----------+ +-----------+
| | DSA | |
| |-----| |
| | link| |
+-----------+ +-----------+
| | | | | | | |
sw0pN user sw1pN user
ports ports
It is clear that the host addresses of sw0 should not interfere with the
host addresses of sw1, and one switch should not even respond to the
notifiers emitted for the other's host addresses.
An entirely different thing can be said about the typical cross-chip
setup that DSA works with today:
+-----------------+
| Host system |
| |
| eth0 |
+-----------------+
CPU | port
+-----------+
sw0pN -| |
user -| |
ports -| |
-| |
+-----------+
DSA | link
+-----------+
sw1pN -| |
user -| |
ports -| |
-| |
+-----------+
where a host MDB entry installed on a user port of sw1pN should be
installed both on its upstream DSA link as well as on the CPU port
(but not on the downstream DSA link of sw0pN).
Basically this calls for the introduction of a separate notifier for
host addresses, which matches on all upstream ports of the targeted user
port (be they CPU ports or DSA links). This means we can simplify the
normal MDB notifiers to be identical to the FDB notifiers now.
Note that for switches which don't implement .port_mdb_add and
.port_mdb_del, we don't even attempt to keep the address lists, only to
fail at install time.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
include/net/dsa.h | 34 ++++++++++++++++++
net/dsa/dsa2.c | 24 +++++++++++--
net/dsa/dsa_priv.h | 6 ++++
net/dsa/port.c | 24 +++++++++++++
net/dsa/slave.c | 89 +++++++++++++++++++++++++++++++++++++++++-----
net/dsa/switch.c | 73 +++++++++++++++++++++++++++++--------
6 files changed, 226 insertions(+), 24 deletions(-)
@@ -290,6 +290,11 @@ struct dsa_port {*/conststructdsa_netdevice_ops*netdev_ops;+/* List of MAC addresses that must be extracted from the fabric+*throughthisCPUport.ValidonlyforDSA_PORT_TYPE_CPU.+*/+structlist_headhost_mdb;+boolsetup;};
@@ -481,6 +493,28 @@ static inline unsigned int dsa_upstream_port(struct dsa_switch *ds, int port)returndsa_towards_port(ds,cpu_dp->ds->index,cpu_dp->index);}+staticinlinebooldsa_is_upstream_port(structdsa_switch*ds,intport)+{+if(dsa_is_unused_port(ds,port))+returnfalse;++returnport==dsa_upstream_port(ds,port);+}++/* Return true if @upstream_ds is an upstream switch of @downstream_ds. */+staticinlinebooldsa_switch_is_upstream_of(structdsa_switch*upstream_ds,+structdsa_switch*downstream_ds)+{+introuting_port;++if(upstream_ds==downstream_ds)+returntrue;++routing_port=dsa_routing_port(downstream_ds,upstream_ds->index);++returndsa_is_upstream_port(downstream_ds,routing_port);+}+staticinlinebooldsa_port_is_vlan_filtering(conststructdsa_port*dp){conststructdsa_switch*ds=dp->ds;
@@ -336,6 +341,8 @@ static int dsa_tree_setup_default_cpu(struct dsa_switch_tree *dst)return-EINVAL;}+dsa_setup_cpu_port(cpu_dp);+/* Assign the default CPU port to all ports of the fabric */list_for_each_entry(dp,&dst->ports,list)if(dsa_port_is_user(dp)||dsa_port_is_dsa(dp))
@@ -344,13 +351,26 @@ static int dsa_tree_setup_default_cpu(struct dsa_switch_tree *dst)return0;}+staticvoiddsa_teardown_cpu_port(structdsa_port*cpu_dp)+{+structdsa_host_addr*a,*tmp;++list_for_each_entry_safe(a,tmp,&cpu_dp->host_mdb,list){+list_del(&a->list);+kfree(a);+}+}+staticvoiddsa_tree_teardown_default_cpu(structdsa_switch_tree*dst){structdsa_port*dp;-list_for_each_entry(dp,&dst->ports,list)-if(dsa_port_is_user(dp)||dsa_port_is_dsa(dp))+list_for_each_entry(dp,&dst->ports,list){+if(dsa_port_is_user(dp)||dsa_port_is_dsa(dp)){+dsa_teardown_cpu_port(dp->cpu_dp);dp->cpu_dp=NULL;+}+}}staticintdsa_port_setup(structdsa_port*dp)
@@ -23,6 +23,85 @@#include"dsa_priv.h"+staticstructdsa_host_addr*dsa_host_addr_find(structlist_head*addr_list,+constunsignedchar*addr,+u16vid)+{+structdsa_host_addr*a;++list_for_each_entry(a,addr_list,list)+if(ether_addr_equal(a->addr,addr)&&a->vid==vid)+returna;++returnNULL;+}++/* DSA can directly translate this to a normal MDB add, but on the CPU port.+*Butbecausemultipleuserportscanjointhesamemulticastgroupandthe+*bridgewillemitanotificationforeachport,weneedtoadd/deletethe+*entrytowardsthehostonlyonce,sowereferencecountit.+*/+staticintdsa_host_mdb_add(structdsa_port*dp,+conststructswitchdev_obj_port_mdb*mdb)+{+structdsa_port*cpu_dp=dp->cpu_dp;+structdsa_host_addr*a;+interr;++if(!dp->ds->ops->port_mdb_add||!dp->ds->ops->port_mdb_del)+return-EOPNOTSUPP;++a=dsa_host_addr_find(&cpu_dp->host_mdb,mdb->addr,mdb->vid);+if(a){+refcount_inc(&a->refcount);+return0;+}++a=kzalloc(sizeof(*a),GFP_KERNEL);+if(!a)+return-ENOMEM;++err=dsa_port_host_mdb_add(dp,mdb);+if(err){+kfree(a);+returnerr;+}++ether_addr_copy(a->addr,mdb->addr);+a->vid=mdb->vid;+refcount_set(&a->refcount,1);+list_add_tail(&a->list,&cpu_dp->host_mdb);++return0;+}++staticintdsa_host_mdb_del(structdsa_port*dp,+conststructswitchdev_obj_port_mdb*mdb)+{+structdsa_port*cpu_dp=dp->cpu_dp;+structdsa_host_addr*a;+interr;++if(!dp->ds->ops->port_mdb_add||!dp->ds->ops->port_mdb_del)+return-EOPNOTSUPP;++a=dsa_host_addr_find(&cpu_dp->host_mdb,mdb->addr,mdb->vid);+if(!a)+return-ENOENT;++if(!refcount_dec_and_test(&a->refcount))+return0;++err=dsa_port_host_mdb_del(dp,mdb);+if(err)+returnerr;++list_del(&a->list);+kfree(a);++return0;+}+/* slave mii_bus handling ***************************************************/staticintdsa_slave_phy_read(structmii_bus*bus,intaddr,intreg){
@@ -396,10 +475,7 @@ static int dsa_slave_port_obj_add(struct net_device *dev,err=dsa_port_mdb_add(dp,SWITCHDEV_OBJ_PORT_MDB(obj));break;caseSWITCHDEV_OBJ_ID_HOST_MDB:-/* DSA can directly translate this to a normal MDB add,-*butontheCPUport.-*/-err=dsa_port_mdb_add(dp->cpu_dp,SWITCHDEV_OBJ_PORT_MDB(obj));+err=dsa_host_mdb_add(dp,SWITCHDEV_OBJ_PORT_MDB(obj));break;caseSWITCHDEV_OBJ_ID_PORT_VLAN:err=dsa_slave_vlan_add(dev,obj,extack);
@@ -464,10 +540,7 @@ static int dsa_slave_port_obj_del(struct net_device *dev,err=dsa_port_mdb_del(dp,SWITCHDEV_OBJ_PORT_MDB(obj));break;caseSWITCHDEV_OBJ_ID_HOST_MDB:-/* DSA can directly translate this to a normal MDB add,-*butontheCPUport.-*/-err=dsa_port_mdb_del(dp->cpu_dp,SWITCHDEV_OBJ_PORT_MDB(obj));+err=dsa_host_mdb_del(dp,SWITCHDEV_OBJ_PORT_MDB(obj));break;caseSWITCHDEV_OBJ_ID_PORT_VLAN:err=dsa_slave_vlan_del(dev,obj);
@@ -148,6 +148,25 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds,return0;}+/* Matches for all upstream-facing ports (the CPU port and all upstream-facing+*DSAlinks)thatsitbetweentheportthatemittedthenotificationandthe+*DSAmaster.+*/+staticbooldsa_switch_host_address_match(structdsa_switch*ds,intport,+intinfo_sw_index,intinfo_port)+{+structdsa_switch*downstream_ds;++downstream_ds=dsa_switch_find(ds->dst->index,info_sw_index);+if(WARN_ON(!downstream_ds))+returnfalse;++if(dsa_switch_is_upstream_of(ds,downstream_ds))+returndsa_is_upstream_port(ds,port);++returnfalse;+}+staticintdsa_switch_fdb_add(structdsa_switch*ds,structdsa_notifier_fdb_info*info){
@@ -229,20 +248,30 @@ static int dsa_switch_lag_leave(struct dsa_switch *ds,return0;}-staticbooldsa_switch_mdb_match(structdsa_switch*ds,intport,-structdsa_notifier_mdb_info*info)+staticintdsa_switch_mdb_add(structdsa_switch*ds,+structdsa_notifier_mdb_info*info){-if(ds->index==info->sw_index&&port==info->port)-returntrue;+intport=dsa_towards_port(ds,info->sw_index,info->port);-if(dsa_is_dsa_port(ds,port))-returntrue;+if(!ds->ops->port_mdb_add)+return-EOPNOTSUPP;-returnfalse;+returnds->ops->port_mdb_add(ds,port,info->mdb);}-staticintdsa_switch_mdb_add(structdsa_switch*ds,+staticintdsa_switch_mdb_del(structdsa_switch*ds,structdsa_notifier_mdb_info*info)+{+intport=dsa_towards_port(ds,info->sw_index,info->port);++if(!ds->ops->port_mdb_del)+return-EOPNOTSUPP;++returnds->ops->port_mdb_del(ds,port,info->mdb);+}++staticintdsa_switch_host_mdb_add(structdsa_switch*ds,+structdsa_notifier_mdb_info*info){interr=0;intport;
@@ -251,7 +280,8 @@ static int dsa_switch_mdb_add(struct dsa_switch *ds,return-EOPNOTSUPP;for(port=0;port<ds->num_ports;port++){-if(dsa_switch_mdb_match(ds,port,info)){+if(dsa_switch_host_address_match(ds,port,info->sw_index,+info->port)){err=ds->ops->port_mdb_add(ds,port,info->mdb);if(err)break;
@@ -261,16 +291,25 @@ static int dsa_switch_mdb_add(struct dsa_switch *ds,returnerr;}-staticintdsa_switch_mdb_del(structdsa_switch*ds,-structdsa_notifier_mdb_info*info)+staticintdsa_switch_host_mdb_del(structdsa_switch*ds,+structdsa_notifier_mdb_info*info){+interr=0;+intport;+if(!ds->ops->port_mdb_del)return-EOPNOTSUPP;-if(ds->index==info->sw_index)-returnds->ops->port_mdb_del(ds,info->port,info->mdb);+for(port=0;port<ds->num_ports;port++){+if(dsa_switch_host_address_match(ds,port,info->sw_index,+info->port)){+err=ds->ops->port_mdb_del(ds,info->port,info->mdb);+if(err)+break;+}+}-return0;+returnerr;}staticbooldsa_switch_vlan_match(structdsa_switch*ds,intport,
@@ -508,6 +547,12 @@ static int dsa_switch_event(struct notifier_block *nb,caseDSA_NOTIFIER_MDB_DEL:err=dsa_switch_mdb_del(ds,info);break;+caseDSA_NOTIFIER_HOST_MDB_ADD:+err=dsa_switch_host_mdb_add(ds,info);+break;+caseDSA_NOTIFIER_HOST_MDB_DEL:+err=dsa_switch_host_mdb_del(ds,info);+break;caseDSA_NOTIFIER_VLAN_ADD:err=dsa_switch_vlan_add(ds,info);break;
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-24 11:45:13
From: Vladimir Oltean <vladimir.oltean@nxp.com>
In preparation of unicast filtering towards the CPU, DSA will need to
send to the CPU several classes of addresses:
- local FDB entries from the bridge
- FDB entries pointing to the software bridge itself
- the MAC addresses used for termination in standalone mode
- the MAC addresses of various upper interfaces (8021q, macvlan) that
DSA ports might have
So it will no longer be sufficient to gather the address list from a
single source such as the bridge. Consider the fact that the bridge
currently records the MAC address of every bridge port as a local
('permanent') address placed in the FDB. Sure we could use that as an
indication that the address must be sent to the CPU, but that address
needs to be sent to the CPU anyway, even if we're operating in
standalone mode. So the bridge can't dictate anything, we must keep more
addresses, and if we do that, there is a risk that we might delete an
address when it was still used, if we just listen to the deletion event
emitted by the bridge through switchdev. And that is where the
requirement for reference counting comes from.
Similar to host MDB entries, we should create a new notifier in DSA, and
keep the existing one for out-facing FDB entries untouched. We can also
simplify dsa_slave_switchdev_event_work a little bit now, since we
always schedule the work item for a user port now, we can unconditionally
take the refcount on a net_device.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
include/net/dsa.h | 1 +
net/dsa/dsa2.c | 6 ++++
net/dsa/dsa_priv.h | 7 ++++
net/dsa/port.c | 27 ++++++++++++++
net/dsa/slave.c | 88 +++++++++++++++++++++++++++++++++++++++++-----
net/dsa/switch.c | 50 ++++++++++++++++++++++++++
6 files changed, 170 insertions(+), 9 deletions(-)
@@ -293,6 +293,7 @@ struct dsa_port {/* List of MAC addresses that must be extracted from the fabric*throughthisCPUport.ValidonlyforDSA_PORT_TYPE_CPU.*/+structlist_headhost_fdb;structlist_headhost_mdb;boolsetup;
@@ -2324,6 +2392,7 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,structnet_device*dev=switchdev_notifier_info_to_dev(ptr);conststructswitchdev_notifier_fdb_info*fdb_info;structdsa_switchdev_event_work*switchdev_work;+boolhost_addr=false;structdsa_port*dp;interr;
@@ -2361,7 +2430,8 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,if(!p)returnNOTIFY_DONE;-dp=p->dp->cpu_dp;+dp=p->dp;+host_addr=true;if(!dp->ds->assisted_learning_on_cpu_port)returnNOTIFY_DONE;
@@ -2391,10 +2461,10 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,ether_addr_copy(switchdev_work->addr,fdb_info->addr);switchdev_work->vid=fdb_info->vid;+switchdev_work->host_addr=host_addr;/* Hold a reference on the slave for dsa_fdb_offload_notify */-if(dsa_is_user_port(dp->ds,dp->index))-dev_hold(dev);+dev_hold(dev);dsa_schedule_work(&switchdev_work->work);break;default:
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-24 11:45:59
From: Vladimir Oltean <vladimir.oltean@nxp.com>
If the DSA master implements strict address filtering, then the unicast
and multicast addresses kept by the DSA CPU ports should be synchronized
with the address lists of the DSA master.
Note that we want the synchronization of the master's address lists even
if the DSA switch doesn't support unicast/multicast database operations,
on the premises that the packets will be flooded to the CPU in that
case, and we should still instruct the master to receive them.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/slave.c | 8 ++++++++
1 file changed, 8 insertions(+)
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-24 11:46:22
From: Vladimir Oltean <vladimir.oltean@nxp.com>
In preparation of support for IFF_UNICAST_FLT for DSA standalone ports,
the unicast address of the DSA interfaces should always be known to the
switch and installed as an FDB entry towards the CPU port associated
with that user port.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/slave.c | 32 ++++++++++----------------------
1 file changed, 10 insertions(+), 22 deletions(-)
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-24 11:46:44
From: Vladimir Oltean <vladimir.oltean@nxp.com>
The bridge device currently goes into promiscuous mode when it has an
upper with a different MAC address than itself. But it could do better:
it could sync the MAC addresses of its uppers to the software FDB, as
local entries pointing to the bridge itself. This is compatible with
switchdev, since drivers are now instructed to trap these MAC addresses
to the CPU.
Note that the dev_uc_add API does not propagate VLAN ID, so this only
works for VLAN-unaware bridges.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/bridge/br_device.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-24 11:46:52
From: Tobias Waldekranz <tobias@waldekranz.com>
Instead of having to add more and more arguments to
br_switchdev_fdb_call_notifiers, get rid of it and build the info
struct directly in br_switchdev_fdb_notify.
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/bridge/br_switchdev.c | 41 +++++++++++----------------------------
1 file changed, 11 insertions(+), 30 deletions(-)
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-24 11:47:06
From: Vladimir Oltean <vladimir.oltean@nxp.com>
In case we have ptp4l running on a bridged DSA switch interface, the PTP
traffic is classified as link-local (in the default profile, the MAC
addresses are 01:1b:19:00:00:00 and 01:80:c2:00:00:0e), which means it
isn't the responsibility of the bridge to make sure it gets trapped to
the CPU.
The solution is to implement the standard callbacks for dev_uc_add and
dev_mc_add, and behave just like any other network interface: ensure
that the user space program can see those packets.
Note that since ndo_set_rx_mode runs in atomic context, we must schedule
the dsa_slave_switchdev_event_work in order to install the FDB and MDB
entries to a DSA switch that may sleep. But since the DSA switchdev
event logic only deals with FDB entries, we must fake some events for
MDB ones.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/dsa_priv.h | 10 +-
net/dsa/slave.c | 329 ++++++++++++++++++++++++++++++++-------------
2 files changed, 239 insertions(+), 100 deletions(-)
@@ -171,6 +171,220 @@ static int dsa_host_fdb_del(struct dsa_port *dp, const unsigned char *addr,return0;}+staticvoid+dsa_fdb_offload_notify(structdsa_switchdev_event_work*switchdev_work)+{+structdsa_switch*ds=switchdev_work->ds;+structswitchdev_notifier_fdb_infoinfo;+structdsa_port*dp;++if(!dsa_is_user_port(ds,switchdev_work->port))+return;++info.addr=switchdev_work->addr;+info.vid=switchdev_work->vid;+info.offloaded=true;+dp=dsa_to_port(ds,switchdev_work->port);+call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED,+dp->slave,&info.info,NULL);+}++#define work_to_ctx(w) \+container_of((w),structdsa_switchdev_event_work,work)++staticvoiddsa_slave_switchdev_event_work(structwork_struct*work)+{+structdsa_switchdev_event_work*switchdev_work=work_to_ctx(work);+structdsa_switch*ds=switchdev_work->ds;+structdsa_port*dp;+interr;++dp=dsa_to_port(ds,switchdev_work->port);++rtnl_lock();+switch(switchdev_work->event){+caseDSA_EVENT_FDB_ADD:+if(switchdev_work->host_addr)+err=dsa_host_fdb_add(dp,switchdev_work->addr,+switchdev_work->vid);+else+err=dsa_port_fdb_add(dp,switchdev_work->addr,+switchdev_work->vid);+if(err){+dev_err(ds->dev,+"port %d failed to add %pM vid %d to fdb: %d\n",+dp->index,switchdev_work->addr,+switchdev_work->vid,err);+break;+}+dsa_fdb_offload_notify(switchdev_work);+break;++caseDSA_EVENT_FDB_DEL:+if(switchdev_work->host_addr)+err=dsa_host_fdb_del(dp,switchdev_work->addr,+switchdev_work->vid);+else+err=dsa_port_fdb_del(dp,switchdev_work->addr,+switchdev_work->vid);+if(err){+dev_err(ds->dev,+"port %d failed to delete %pM vid %d from fdb: %d\n",+dp->index,switchdev_work->addr,+switchdev_work->vid,err);+}++break;++caseDSA_EVENT_MDB_ADD:{+structswitchdev_obj_port_mdbmdb;++ether_addr_copy(mdb.addr,switchdev_work->addr);+mdb.vid=switchdev_work->vid;++if(switchdev_work->host_addr)+err=dsa_host_mdb_add(dp,&mdb);+else+err=dsa_port_mdb_add(dp,&mdb);+if(err){+dev_err(ds->dev,+"port %d failed to add %pM vid %d to mdb: %d\n",+dp->index,mdb.addr,mdb.vid,err);+break;+}+dsa_fdb_offload_notify(switchdev_work);+break;+}+caseDSA_EVENT_MDB_DEL:{+structswitchdev_obj_port_mdbmdb;++ether_addr_copy(mdb.addr,switchdev_work->addr);+mdb.vid=switchdev_work->vid;++if(switchdev_work->host_addr)+err=dsa_host_mdb_del(dp,&mdb);+else+err=dsa_port_mdb_del(dp,&mdb);+if(err){+dev_err(ds->dev,+"port %d failed to delete %pM vid %d from mdb: %d\n",+dp->index,mdb.addr,mdb.vid,err);+}+break;+}+default:+break;+}+rtnl_unlock();++kfree(switchdev_work);+dev_put(dp->slave);+}+++staticintdsa_slave_schedule_switchdev_work(structnet_device*dev,+enumdsa_switchdev_eventevent,+constunsignedchar*addr,u16vid,+boolhost_addr)+{+structdsa_switchdev_event_work*switchdev_work;+structdsa_port*dp=dsa_slave_to_port(dev);++if(!dp->ds->ops->port_fdb_add||!dp->ds->ops->port_fdb_del)+return-EOPNOTSUPP;++switchdev_work=kzalloc(sizeof(*switchdev_work),GFP_ATOMIC);+if(!switchdev_work)+return-ENOMEM;++INIT_WORK(&switchdev_work->work,dsa_slave_switchdev_event_work);+switchdev_work->ds=dp->ds;+switchdev_work->port=dp->index;+switchdev_work->event=event;++ether_addr_copy(switchdev_work->addr,addr);+switchdev_work->vid=vid;+switchdev_work->host_addr=host_addr;++/* Hold a reference on the slave for dsa_fdb_offload_notify */+dev_hold(dev);+dsa_schedule_work(&switchdev_work->work);++return0;+}++staticintdsa_slave_sync_uc(structnet_device*dev,+constunsignedchar*addr)+{+interr;++err=dsa_slave_schedule_switchdev_work(dev,DSA_EVENT_FDB_ADD,+addr,0,true);+if(err==-EOPNOTSUPP){+structdsa_port*dp=dsa_slave_to_port(dev);++dev_uc_add(dp->cpu_dp->master,addr);++return0;+}++returnerr;+}++staticintdsa_slave_unsync_uc(structnet_device*dev,+constunsignedchar*addr)+{+interr;++err=dsa_slave_schedule_switchdev_work(dev,DSA_EVENT_FDB_DEL,+addr,0,true);+if(err==-EOPNOTSUPP){+structdsa_port*dp=dsa_slave_to_port(dev);++dev_uc_del(dp->cpu_dp->master,addr);++return0;+}++returnerr;+}++staticintdsa_slave_sync_mc(structnet_device*dev,+constunsignedchar*addr)+{+interr;++err=dsa_slave_schedule_switchdev_work(dev,DSA_EVENT_MDB_ADD,+addr,0,true);+if(err==-EOPNOTSUPP){+structdsa_port*dp=dsa_slave_to_port(dev);++dev_mc_add(dp->cpu_dp->master,addr);++return0;+}++returnerr;+}++staticintdsa_slave_unsync_mc(structnet_device*dev,+constunsignedchar*addr)+{+interr;++err=dsa_slave_schedule_switchdev_work(dev,DSA_EVENT_MDB_DEL,+addr,0,true);+if(err==-EOPNOTSUPP){+structdsa_port*dp=dsa_slave_to_port(dev);++dev_mc_del(dp->cpu_dp->master,addr);++return0;+}++returnerr;+}+/* slave mii_bus handling ***************************************************/staticintdsa_slave_phy_read(structmii_bus*bus,intaddr,intreg){
@@ -263,8 +477,9 @@ static int dsa_slave_close(struct net_device *dev)dsa_port_disable_rt(dp);-dev_mc_unsync(master,dev);-dev_uc_unsync(master,dev);+__dev_uc_sync(dev,dsa_slave_sync_uc,dsa_slave_unsync_uc);+__dev_mc_sync(dev,dsa_slave_sync_mc,dsa_slave_unsync_mc);+if(dev->flags&IFF_ALLMULTI)dev_set_allmulti(master,-1);if(dev->flags&IFF_PROMISC)
@@ -1970,6 +2183,8 @@ int dsa_slave_create(struct dsa_port *port)elseeth_hw_addr_inherit(slave_dev,master);slave_dev->priv_flags|=IFF_NO_QUEUE;+if(ds->ops->port_fdb_add&&ds->ops->port_fdb_del)+slave_dev->priv_flags|=IFF_UNICAST_FLT;slave_dev->netdev_ops=&dsa_slave_netdev_ops;if(ds->ops->port_max_mtu)slave_dev->max_mtu=ds->ops->port_max_mtu(ds,port->index);
@@ -2290,75 +2505,6 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,returnNOTIFY_DONE;}-staticvoid-dsa_fdb_offload_notify(structdsa_switchdev_event_work*switchdev_work)-{-structdsa_switch*ds=switchdev_work->ds;-structswitchdev_notifier_fdb_infoinfo;-structdsa_port*dp;--if(!dsa_is_user_port(ds,switchdev_work->port))-return;--info.addr=switchdev_work->addr;-info.vid=switchdev_work->vid;-info.offloaded=true;-dp=dsa_to_port(ds,switchdev_work->port);-call_switchdev_notifiers(SWITCHDEV_FDB_OFFLOADED,-dp->slave,&info.info,NULL);-}--staticvoiddsa_slave_switchdev_event_work(structwork_struct*work)-{-structdsa_switchdev_event_work*switchdev_work=-container_of(work,structdsa_switchdev_event_work,work);-structdsa_switch*ds=switchdev_work->ds;-structdsa_port*dp;-interr;--dp=dsa_to_port(ds,switchdev_work->port);--rtnl_lock();-switch(switchdev_work->event){-caseSWITCHDEV_FDB_ADD_TO_DEVICE:-if(switchdev_work->host_addr)-err=dsa_host_fdb_add(dp,switchdev_work->addr,-switchdev_work->vid);-else-err=dsa_port_fdb_add(dp,switchdev_work->addr,-switchdev_work->vid);-if(err){-dev_err(ds->dev,-"port %d failed to add %pM vid %d to fdb: %d\n",-dp->index,switchdev_work->addr,-switchdev_work->vid,err);-break;-}-dsa_fdb_offload_notify(switchdev_work);-break;--caseSWITCHDEV_FDB_DEL_TO_DEVICE:-if(switchdev_work->host_addr)-err=dsa_host_fdb_del(dp,switchdev_work->addr,-switchdev_work->vid);-else-err=dsa_port_fdb_del(dp,switchdev_work->addr,-switchdev_work->vid);-if(err){-dev_err(ds->dev,-"port %d failed to delete %pM vid %d from fdb: %d\n",-dp->index,switchdev_work->addr,-switchdev_work->vid,err);-}--break;-}-rtnl_unlock();--kfree(switchdev_work);-dev_put(dp->slave);-}-staticintdsa_lower_dev_walk(structnet_device*lower_dev,structnetdev_nested_priv*priv){
@@ -2387,7 +2533,7 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,{structnet_device*dev=switchdev_notifier_info_to_dev(ptr);conststructswitchdev_notifier_fdb_info*fdb_info;-structdsa_switchdev_event_work*switchdev_work;+enumdsa_switchdev_eventdsa_event;boolhost_addr=false;structdsa_port*dp;interr;
@@ -2441,28 +2587,19 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,returnNOTIFY_DONE;}-if(!dp->ds->ops->port_fdb_add||!dp->ds->ops->port_fdb_del)-returnNOTIFY_DONE;--switchdev_work=kzalloc(sizeof(*switchdev_work),GFP_ATOMIC);-if(!switchdev_work)-returnNOTIFY_BAD;--INIT_WORK(&switchdev_work->work,-dsa_slave_switchdev_event_work);-switchdev_work->ds=dp->ds;-switchdev_work->port=dp->index;-switchdev_work->event=event;+if(event==SWITCHDEV_FDB_ADD_TO_DEVICE)+dsa_event=DSA_EVENT_FDB_ADD;+else+dsa_event=DSA_EVENT_FDB_DEL;-ether_addr_copy(switchdev_work->addr,-fdb_info->addr);-switchdev_work->vid=fdb_info->vid;-switchdev_work->host_addr=host_addr;+err=dsa_slave_schedule_switchdev_work(dp->slave,dsa_event,+fdb_info->addr,+fdb_info->vid,+host_addr);+if(err==-EOPNOTSUPP)+returnNOTIFY_OK;-/* Hold a reference on the slave for dsa_fdb_offload_notify */-dev_hold(dev);-dsa_schedule_work(&switchdev_work->work);-break;+returnnotifier_from_errno(err);default:returnNOTIFY_DONE;}
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-24 11:47:27
From: Vladimir Oltean <vladimir.oltean@nxp.com>
As explained in the discussion on the initial version of this patch:
https://lore.kernel.org/netdev/20210117193009.io3nungdwuzmo5f7@skbuf/
the switchdev notifiers for FDB entries managed to have a zero-day bug.
The bridge would not say that this entry is local:
ip link add br0 type bridge
ip link set swp0 master br0
bridge fdb add dev swp0 00:01:02:03:04:05 master local
and the switchdev driver would be more than happy to offload it as a
normal static FDB entry. This is despite the fact that 'local' and
non-'local' entries have completely opposite directions: a local entry
is locally terminated and not forwarded, whereas a static entry is
forwarded and not locally terminated. So, for example, DSA would install
this entry on swp0 instead of installing it on the CPU port as it should.
There is an even sadder part, which is that the 'local' flag is implicit
if 'static' is not specified, meaning that this command produces the
same result of adding a 'local' entry:
bridge fdb add dev swp0 00:01:02:03:04:05 master
So this patch is a bugfix which does what should have been done from day
one: make the bridge inform switchdev that it's a local entry, and have
all drivers ignore local entries (since none of them currently has any
logic to deal with host entries).
At least we've updated the man pages for 'bridge' now, and we're pretty
explicit that the commands above were broken and should have never worked:
https://patchwork.kernel.org/project/netdevbpf/cover/20210211104502.2081443-1-olteanv@gmail.com/
But we don't do anything to even attempt to keep backwards compatibility
with the broken behavior. If we did that, it would be pretty much game
over for any attempt to really deal with host entries in switchdev drivers.
Co-developed-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
drivers/net/ethernet/marvell/prestera/prestera_switchdev.c | 2 +-
drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c | 5 +++--
drivers/net/ethernet/rocker/rocker_main.c | 4 ++--
drivers/net/ethernet/ti/am65-cpsw-switchdev.c | 4 ++--
drivers/net/ethernet/ti/cpsw_switchdev.c | 4 ++--
drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 4 ++--
include/net/switchdev.h | 1 +
net/bridge/br_switchdev.c | 1 +
net/dsa/slave.c | 2 +-
9 files changed, 15 insertions(+), 12 deletions(-)
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-24 11:47:38
From: Tobias Waldekranz <tobias@waldekranz.com>
Treat addresses added to the bridge itself in the same way as regular
ports and send out a notification so that drivers may sync it down to
the hardware FDB.
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/bridge/br_fdb.c | 4 ++--
net/bridge/br_private.h | 7 ++++---
net/bridge/br_switchdev.c | 11 +++++------
3 files changed, 11 insertions(+), 11 deletions(-)
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-24 11:48:03
From: Tobias Waldekranz <tobias@waldekranz.com>
The bridge automatically creates local (not forwarded) fdb entries
pointing towards physical ports with their interface MAC addresses.
For switchdev, the significance of these fdb entries is the exact
opposite of that of non-local entries: instead of sending these frame
outwards, we must send them inwards (towards the host).
NOTE: The bridge's own MAC address is also "local". If that address is
not shared with any port, the bridge's MAC is not be added by this
functionality - but the following commit takes care of that case.
NOTE 2: We mark these addresses as host-filtered regardless of the value
of ds->assisted_learning_on_cpu_port. This is because, as opposed to the
speculative logic done for dynamic address learning on foreign
interfaces, the local FDB entries are rather fixed, so there isn't any
risk of them migrating from one bridge port to another.
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/slave.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-24 11:48:11
From: Vladimir Oltean <vladimir.oltean@nxp.com>
The bridge supports a legacy way of adding local (non-forwarded) FDB
entries, which works on an individual port basis:
bridge fdb add dev swp0 00:01:02:03:04:05 master local
As well as a new way, added by Roopa Prabhu in commit 3741873b4f73
("bridge: allow adding of fdb entries pointing to the bridge device"):
bridge fdb add dev br0 00:01:02:03:04:05 self local
The two commands are functionally equivalent, except that the first one
produces an entry with fdb->dst == swp0, and the other an entry with
fdb->dst == NULL. The confusing part, though, is that even if fdb->dst
is swp0 for the 'local on port' entry, that destination is not used.
Nonetheless, the idea is that the bridge has reference counting for
local entries, and local entries pointing towards the bridge are still
'as local' as local entries for a port.
The bridge adds the MAC addresses of the interfaces automatically as
FDB entries with is_local=1. For the MAC address of the ports, fdb->dst
will be equal to the port, and for the MAC address of the bridge,
fdb->dst will point towards the bridge (i.e. be NULL). Therefore, if the
MAC address of the bridge is not inherited from either of the physical
ports, then we must explicitly catch local FDB entries emitted towards
the br0, otherwise we'll miss the MAC address of the bridge (and, of
course, any entry with 'bridge add dev br0 ... self local').
Co-developed-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/slave.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-24 11:48:35
From: Vladimir Oltean <vladimir.oltean@nxp.com>
I have udhcpcd in my system and this is configured to bring interfaces
up as soon as they are created.
I create a bridge as follows:
ip link add br0 type bridge
As soon as I create the bridge and udhcpcd brings it up, I have some
other crap (avahi) that starts sending some random IPv6 packets to
advertise some local services, and from there, the br0 bridge joins the
following IPv6 groups:
33:33:ff:6d:c1:9c vid 0
33:33:00:00:00:6a vid 0
33:33:00:00:00:fb vid 0
br_dev_xmit
-> br_multicast_rcv
-> br_ip6_multicast_add_group
-> __br_multicast_add_group
-> br_multicast_host_join
-> br_mdb_notify
This is all fine, but inside br_mdb_notify we have br_mdb_switchdev_host
hooked up, and switchdev will attempt to offload the host joined groups
to an empty list of ports. Of course nobody offloads them.
Then when we add a port to br0:
ip link set swp0 master br0
the bridge doesn't replay the host-joined MDB entries from br_add_if,
and eventually the host joined addresses expire, and a switchdev
notification for deleting it is emitted, but surprise, the original
addition was already completely missed.
The strategy to address this problem is to replay the MDB entries (both
the port ones and the host joined ones) when the new port joins the
bridge, similar to what vxlan_fdb_replay does (in that case, its FDB can
be populated and only then attached to a bridge that you offload).
However there are 2 possibilities: the addresses can be 'pushed' by the
bridge into the port, or the port can 'pull' them from the bridge.
Considering that in the general case, the new port can be really late to
the party, and there may have been many other switchdev ports that
already received the initial notification, we would like to avoid
delivering duplicate events to them, since they might misbehave. And
currently, the bridge calls the entire switchdev notifier chain, whereas
for replaying it should just call the notifier block of the new guy.
But the bridge doesn't know what is the new guy's notifier block, it
just knows where the switchdev notifier chain is. So for simplification,
we make this a driver-initiated pull for now, and the notifier block is
passed as an argument.
To emulate the calling context for mdb objects (deferred and put on the
blocking notifier chain), we must iterate under RCU protection through
the bridge's mdb entries, queue them, and only call them once we're out
of the RCU read-side critical section.
Suggested-by: Ido Schimmel <redacted>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
include/linux/if_bridge.h | 8 +++
include/net/switchdev.h | 1 +
net/bridge/br_mdb.c | 117 ++++++++++++++++++++++++++++++++++++++
net/dsa/slave.c | 17 +++++-
4 files changed, 141 insertions(+), 2 deletions(-)
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-24 11:48:48
From: Tobias Waldekranz <tobias@waldekranz.com>
Reuse the "assisted_learning_on_cpu_port" functionality to always add
entries for user-configured entries on foreign interfaces, even if
assisted_learning_on_cpu_port is not enabled. E.g. in this situation:
br0
/ \
swp0 dummy0
$ bridge fdb add 02:00:de:ad:00:01 dev dummy0 vlan 1 master static
Results in DSA adding an entry in the hardware FDB, pointing this
address towards the CPU port.
The same is true for entries added to the bridge itself, e.g:
$ bridge fdb add 02:00:de:ad:00:01 dev br0 vlan 1 self local
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/dsa/slave.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
@@ -2556,9 +2556,12 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,elseif(!fdb_info->added_by_user)returnNOTIFY_OK;}else{-/* Snoop addresses learnt on foreign interfaces-*bridgedwithus,forswitchesthatdon't-*automaticallylearnSAfromCPU-injectedtraffic+/* Snoop addresses added to foreign interfaces+*bridgedwithus,orthebridge+*itself.Dynamicallylearnedaddressescan+*alsobeaddedforswitchesthatdon't+*automaticallylearnSAfromCPU-injected+*traffic.*/structnet_device*br_dev;structdsa_slave_priv*p;
@@ -2581,7 +2584,8 @@ static int dsa_slave_switchdev_event(struct notifier_block *unused,dp=p->dp;host_addr=true;-if(!dp->ds->assisted_learning_on_cpu_port)+if(!fdb_info->added_by_user&&+!dp->ds->assisted_learning_on_cpu_port)returnNOTIFY_DONE;/* When the bridge learns an address on an offloaded
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-24 11:48:56
From: Tobias Waldekranz <tobias@waldekranz.com>
While the hardware is capable of performing learning on the CPU port,
it requires alot of additions to the bridge's forwarding path in order
to handle multi-destination traffic correctly.
Until that is in place, opt for the next best thing and let DSA sync
the relevant addresses down to the hardware FDB.
Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
drivers/net/dsa/mv88e6xxx/chip.c | 1 +
1 file changed, 1 insertion(+)
@@ -5818,6 +5818,7 @@ static int mv88e6xxx_register_switch(struct mv88e6xxx_chip *chip)ds->ops=&mv88e6xxx_switch_ops;ds->ageing_time_min=chip->info->age_time_coeff;ds->ageing_time_max=chip->info->age_time_coeff*U8_MAX;+ds->assisted_learning_on_cpu_port=true;/* Some chips support up to 32, but that requires enabling the*5-bitportmode,whichwedonotsupport.640k^W16oughtto
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-24 11:49:08
From: Vladimir Oltean <vladimir.oltean@nxp.com>
On reception of an skb, the bridge checks if it was marked as 'already
forwarded in hardware' (checks if skb->offload_fwd_mark == 1), and if it
is, it puts a mark of its own on that skb, with the switchdev mark of
the ingress port. Then during forwarding, it enforces that the egress
port must have a different switchdev mark than the ingress one (this is
done in nbp_switchdev_allowed_egress).
Non-switchdev drivers don't report any physical switch id (neither
through devlink nor .ndo_get_port_parent_id), therefore the bridge
assigns them a switchdev mark of 0, and packets coming from them will
always have skb->offload_fwd_mark = 0. So there aren't any restrictions.
Problems appear due to the fact that DSA would like to perform software
fallback for bonding and team interfaces that the physical switch cannot
offload.
+-- br0 -+
/ / | \
/ / | \
/ / | \
/ / | \
/ / | \
/ | | bond0
/ | | / \
swp0 swp1 swp2 swp3 swp4
There, it is desirable that the presence of swp3 and swp4 under a
non-offloaded LAG does not preclude us from doing hardware bridging
beteen swp0, swp1 and swp2. The bandwidth of the CPU is often times high
enough that software bridging between {swp0,swp1,swp2} and bond0 is not
impractical.
But this creates an impossible paradox given the current way in which
port switchdev marks are assigned. When the driver receives a packet
from swp0 (say, due to flooding), it must set skb->offload_fwd_mark to
something.
- If we set it to 0, then the bridge will forward it towards swp1, swp2
and bond0. But the switch has already forwarded it towards swp1 and
swp2 (not to bond0, remember, that isn't offloaded, so as far as the
switch is concerned, ports swp3 and swp4 are not looking up the FDB,
and the entire bond0 is a destination that is strictly behind the
CPU). But we don't want duplicated traffic towards swp1 and swp2, so
it's not ok to set skb->offload_fwd_mark = 0.
- If we set it to 1, then the bridge will not forward the skb towards
the ports with the same switchdev mark, i.e. not to swp1, swp2 and
bond0. Towards swp1 and swp2 that's ok, but towards bond0? It should
have forwarded the skb there.
So the real issue is that bond0 will be assigned the same switchdev mark
as {swp0,swp1,swp2}, because the function that assigns switchdev marks
to bridge ports, nbp_switchdev_mark_set, recurses through bond0's lower
interfaces until it finds something that implements devlink.
A solution is to give the bridge explicit hints as to what switchdev
mark it should use for each port.
Currently, the bridging offload is very 'silent': a driver registers a
netdevice notifier, which is put on the netns's notifier chain, and
which sniffs around for NETDEV_CHANGEUPPER events where the upper is a
bridge, and the lower is an interface it knows about (one registered by
this driver, normally). Then, from within that notifier, it does a bunch
of stuff behind the bridge's back, without the bridge necessarily
knowing that there's somebody offloading that port. It looks like this:
ip link set swp0 master br0
|
v
bridge calls netdev_master_upper_dev_link
|
v
call_netdevice_notifiers
|
v
dsa_slave_netdevice_event
|
v
oh, hey! it's for me!
|
v
.port_bridge_join
What we do to solve the conundrum is to be less silent, and emit a
notification back. Something like this:
ip link set swp0 master br0
|
v
bridge calls netdev_master_upper_dev_link
|
v bridge: Aye! I'll use this
call_netdevice_notifiers ^ ppid as the
| | switchdev mark for
v | this port, and zero
dsa_slave_netdevice_event | if I got nothing.
| |
v |
oh, hey! it's for me! |
| |
v |
.port_bridge_join |
| |
+------------------------+
call_switchdev_notifiers(swp0, SWITCHDEV_BRPORT_OFFLOADED, ppid)
Then stacked interfaces (like bond0 on top of swp3/swp4) would be
treated differently in DSA, depending on whether we can or cannot
offload them.
The offload case:
ip link set bond0 master br0
|
v
bridge calls netdev_master_upper_dev_link
|
v bridge: Aye! I'll use this
call_netdevice_notifiers ^ ppid as the
| | switchdev mark for
v | bond0.
dsa_slave_netdevice_event | Coincidentally (or not),
| | bond0 and swp0, swp1, swp2
v | all have the same switchdev
hmm, it's not quite for me, | mark now, since the ASIC
but my driver has already | is able to forward towards
called .port_lag_join | all these ports in hw.
for it, because I have |
a port with dp->lag_dev == bond0. |
| |
v |
.port_bridge_join |
for swp3 and swp4 |
| |
+------------------------+
call_switchdev_notifiers(bond0, SWITCHDEV_BRPORT_OFFLOADED, ppid)
And the non-offload case:
ip link set bond0 master br0
|
v
bridge calls netdev_master_upper_dev_link
|
v bridge waiting:
call_netdevice_notifiers ^ huh, no SWITCHDEV_BRPORT_OFFLOADED
| | event, okay, I'll use a switchdev
v | mark of zero for this one.
dsa_slave_netdevice_event : Then packets received on swp0 will
| : not be forwarded towards swp1, but
v : they will towards bond0.
it's not for me, but
bond0 is an upper of swp3
and swp4, but their dp->lag_dev
is NULL because they couldn't
offload it.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
.../marvell/prestera/prestera_switchdev.c | 2 +
.../mellanox/mlxsw/spectrum_switchdev.c | 2 +
drivers/net/ethernet/mscc/ocelot_net.c | 43 +++++++++++++++----
drivers/net/ethernet/rocker/rocker_ofdpa.c | 2 +
drivers/net/ethernet/ti/am65-cpsw-nuss.c | 2 +
drivers/net/ethernet/ti/cpsw_new.c | 1 +
drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 2 +
include/net/switchdev.h | 16 +++++++
net/bridge/br.c | 8 +++-
net/bridge/br_if.c | 11 ++---
net/bridge/br_private.h | 7 +--
net/bridge/br_switchdev.c | 27 +++++-------
net/dsa/slave.c | 20 +++++----
net/switchdev/switchdev.c | 18 ++++++++
14 files changed, 116 insertions(+), 45 deletions(-)
@@ -1111,10 +1111,14 @@ static int ocelot_port_obj_del(struct net_device *dev,returnret;}-staticintocelot_netdevice_bridge_join(structocelot*ocelot,intport,+staticintocelot_netdevice_bridge_join(structnet_device*dev,structnet_device*bridge){+structocelot_port_private*priv=netdev_priv(dev);+structocelot_port*ocelot_port=&priv->port;+structocelot*ocelot=ocelot_port->ocelot;structswitchdev_brport_flagsflags;+intport=priv->chip_port;interr;flags.mask=BR_LEARNING|BR_FLOOD|BR_MCAST_FLOOD|BR_BCAST_FLOOD;
@@ -1124,15 +1128,20 @@ static int ocelot_netdevice_bridge_join(struct ocelot *ocelot, int port,if(err)returnerr;+switchdev_bridge_port_offload_notify(dev);ocelot_port_bridge_flags(ocelot,port,flags);return0;}-staticintocelot_netdevice_bridge_leave(structocelot*ocelot,intport,+staticintocelot_netdevice_bridge_leave(structnet_device*dev,structnet_device*bridge){+structocelot_port_private*priv=netdev_priv(dev);+structocelot_port*ocelot_port=&priv->port;+structocelot*ocelot=ocelot_port->ocelot;structswitchdev_brport_flagsflags;+intport=priv->chip_port;interr;flags.mask=BR_LEARNING|BR_FLOOD|BR_MCAST_FLOOD|BR_BCAST_FLOOD;
@@ -1146,7 +1155,8 @@ static int ocelot_netdevice_bridge_leave(struct ocelot *ocelot, int port,}staticintocelot_netdevice_changeupper(structnet_device*dev,-structnetdev_notifier_changeupper_info*info)+structnetdev_notifier_changeupper_info*info,+boolnotify){structocelot_port_private*priv=netdev_priv(dev);structocelot_port*ocelot_port=&priv->port;
@@ -1156,11 +1166,11 @@ static int ocelot_netdevice_changeupper(struct net_device *dev,if(netif_is_bridge_master(info->upper_dev)){if(info->linking){-err=ocelot_netdevice_bridge_join(ocelot,port,-info->upper_dev);+err=ocelot_netdevice_bridge_join(dev,info->upper_dev);+if(!err&¬ify)+switchdev_bridge_port_offload_notify(dev);}else{-err=ocelot_netdevice_bridge_leave(ocelot,port,-info->upper_dev);+err=ocelot_netdevice_bridge_leave(dev,info->upper_dev);}}if(netif_is_lag_master(info->upper_dev)){
@@ -1182,6 +1192,12 @@ static int ocelot_netdevice_changeupper(struct net_device *dev,returnnotifier_from_errno(err);}+/* Treat CHANGEUPPER events on an offloaded LAG as individual CHANGEUPPER+*eventsforthelowerphysicalportsoftheLAG.+*IftheLAGupperisn'toffloaded,ignoreitsCHANGEUPPERevents.+*IncasetheLAGjoinedabridge,notifythatweareoffloadingitandcando+*forwardinginhardwaretowardsit.+*/staticintocelot_netdevice_lag_changeupper(structnet_device*dev,structnetdev_notifier_changeupper_info*info)
@@ -204,6 +206,11 @@ struct switchdev_notifier_info {structnetlink_ext_ack*extack;};+structswitchdev_notifier_brport_info{+structswitchdev_notifier_infoinfo;/* must be first */+structnetdev_phys_item_idppid;+};+structswitchdev_notifier_fdb_info{structswitchdev_notifier_infoinfo;/* must be first */structlist_headlist;
@@ -284,6 +291,9 @@ int switchdev_handle_port_attr_set(struct net_device *dev,int(*set_cb)(structnet_device*dev,conststructswitchdev_attr*attr,structnetlink_ext_ack*extack));++intswitchdev_bridge_port_offload_notify(structnet_device*dev);+#elsestaticinlinevoidswitchdev_deferred_process(void)
@@ -671,13 +669,13 @@ int br_add_if(struct net_bridge *br, struct net_device *dev,*/err=dev_pre_changeaddr_notify(br->dev,dev->dev_addr,extack);if(err)-gotoerr7;+gotoerr6;}err=nbp_vlan_init(p,extack);if(err){netdev_err(dev,"failed to initialize vlan filtering on this port\n");-gotoerr7;+gotoerr6;}spin_lock_bh(&br->lock);
@@ -8,36 +8,29 @@#include"br_private.h"-staticintbr_switchdev_mark_get(structnet_bridge*br,structnet_device*dev)+staticintbr_switchdev_mark_get(structnet_bridge*br,+structnet_bridge_port*new_nbp){structnet_bridge_port*p;/* dev is yet to be added to the port list. */list_for_each_entry(p,&br->port_list,list){-if(netdev_port_same_parent_id(dev,p->dev))+if(!p->offloaded)+continue;++if(netdev_phys_item_id_same(&p->ppid,&new_nbp->ppid))returnp->offload_fwd_mark;}return++br->offload_fwd_mark;}-intnbp_switchdev_mark_set(structnet_bridge_port*p)+voidnbp_switchdev_mark_set(structnet_bridge_port*p){-structnetdev_phys_item_idppid={};-interr;--ASSERT_RTNL();+if(!p->offloaded)+return;-err=dev_get_port_parent_id(p->dev,&ppid,true);-if(err){-if(err==-EOPNOTSUPP)-return0;-returnerr;-}--p->offload_fwd_mark=br_switchdev_mark_get(p->br,p->dev);--return0;+p->offload_fwd_mark=br_switchdev_mark_get(p->br,p);}voidnbp_switchdev_frame_mark(conststructnet_bridge_port*p,
@@ -546,3 +546,21 @@ int switchdev_handle_port_attr_set(struct net_device *dev,returnerr;}EXPORT_SYMBOL_GPL(switchdev_handle_port_attr_set);++/* Let the bridge know that this port is offloaded, so that it can use the+*portparentidobtainedbyrecursiontodeterminethebridgeport's+*switchdevmark.+*/+intswitchdev_bridge_port_offload_notify(structnet_device*dev)+{+structswitchdev_notifier_brport_infoinfo;+interr;++err=dev_get_port_parent_id(dev,&info.ppid,true);+if(err)+returnerr;++returncall_switchdev_notifiers(SWITCHDEV_BRPORT_OFFLOADED,dev,+&info.info,NULL);+}+EXPORT_SYMBOL(switchdev_bridge_port_offload_notify);
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-24 11:49:40
From: Vladimir Oltean <vladimir.oltean@nxp.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 <redacted>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
include/linux/if_bridge.h | 10 ++++++++
include/net/switchdev.h | 1 +
net/bridge/br_fdb.c | 53 +++++++++++++++++++++++++++++++++++++++
net/dsa/slave.c | 7 +++++-
4 files changed, 70 insertions(+), 1 deletion(-)
@@ -206,6 +206,7 @@ struct switchdev_notifier_info {structswitchdev_notifier_fdb_info{structswitchdev_notifier_infoinfo;/* must be first */+structlist_headlist;constunsignedchar*addr;u16vid;u8added_by_user:1,
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-24 11:50:40
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Automatic bridge ports are ones with source address learning or unicast
flooding enabled (corollary: non-automatic ports have learning and
flooding disabled).
The bridge driver has an optimization which says that if all ports are
non-automatic, they don't need to operate in promiscuous mode (i.e. they
don't need to receive all packets). Instead, if a non-automatic port
supports unicast filtering, it can be made to receive only the packets
which have a static FDB entry towards another port in the same forwarding
domain. The logic is that, if a packet is received and does not have a
static FDB entry for routing, it would be dropped anyway because all the
other ports have flooding disabled. So it makes sense to not accept the
packet on RX in the first place.
When a non-automatic port switches to non-promiscuous mode, the static
FDB entries towards the other bridge ports are synced to the RX filtering
list of this ingress port using dev_uc_add.
However, this optimization doesn't bring any benefit for switchdev ports
that offload the bridge. Their hardware data path is promiscuous by its
very definition, i.e. they accept packets regardless of destination MAC
address, because they need to forward them towards the correct station.
Not only is the optimization not necessary, it is also detrimential.
The promise of switchdev is that it looks just like a regular interface
to user space, and it offers extra offloading functionality for stacked
virtual interfaces that can benefit from it. Therefore, it is imaginable
that a switchdev driver might want to implement IFF_UNICAST_FLT too.
When not offloading the bridge, a switchdev interface should really be
indistinguishable from a normal port from user space's perspective,
which means that addresses installed with dev_uc_add and dev_mc_add
should be accepted, and those which aren't should be dropped.
So a switchdev driver might implement dev_uc_add and dev_mc_add by
extracting these addresses from the hardware data path and delivering
them to the CPU, and drop the rest by disabling flooding to the CPU,
and this makes perfect sense when there is no bridge involved on top.
However, things get complicated when the bridge ports are non-automatic
and enter non-promiscuous mode. The bridge will then panic 'oh no, I
need to do something in order for my packets to not get dropped', and
will do the dev_uc_add procedure mentioned above. This will result in
the undesirable behavior that the switchdev driver will extract those
MAC addresses to the CPU, when in fact all that the bridge wanted was
for the packets to not be dropped.
To avoid this situation, the switchdev driver would need to conditionally
accept an address added through dev_uc_add and extract it to the CPU
only if it wasn't added by the bridge, which is both complicated,
strange and counterproductive. It is already unfortunate enough that the
bridge uses its own notification mechanisms for addresses which need to
be extracted (SWITCHDEV_OBJ_ID_HOST_MDB, SWITCHDEV_FDB_ADD_TO_DEVICE
with dev=br0). It shouldn't monopolize the switchdev driver's
functionality and instead it should allow it to offer its services to
other layers which are unaware of switchdev.
So this patch's premise is that the bridge, which is fully aware of
switchdev anyway, is the one that needs to compromise, and must not do
something which isn't needed if switchdev is being used to offload a
port. This way, dev_uc_add and dev_mc_add can be used as a valid mechanism
for address filtering towards the CPU requested by switchdev-unaware
layers ('towards the CPU' because switchdev-unaware will not benefit
from the hardware offload datapath anyway, that's effectively the only
destination which is relevant for them).
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
net/bridge/br_if.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
@@ -111,9 +111,13 @@ static void br_port_clear_promisc(struct net_bridge_port *p)/* Check if the port is already non-promisc or if it doesn't*supportUNICASTfiltering.Withoutunicastfilteringsupport*we'llendupre-enablingpromiscmodeanyway,sojustcheckfor-*ithere.+*ithere.Also,aswitchdevoffloadingthisportneedstobe+*promiscuousbydefinition,sodon'tevenattempttogetitoutof+*promiscuousmodeorsyncunicastFDBentriestoit,sincethatis+*pointlessandnotnecessary.*/-if(!br_promisc_port(p)||!(p->dev->priv_flags&IFF_UNICAST_FLT))+if(!br_promisc_port(p)||!(p->dev->priv_flags&IFF_UNICAST_FLT)||+p->offloaded)return;/* Since we'll be clearing the promisc mode, program the port
On Wed, Feb 24, 2021 at 13:43, Vladimir Oltean [off-list ref] wrote:
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Currently any DSA switch that is strict when implementing the mdb
operations prints these benign errors after the addresses expire, with
at least 2 ports bridged:
[ 286.013814] mscc_felix 0000:00:00.5 swp3: failed (err=-2) to del object (id=3)
The reason has to do with this piece of code:
netdev_for_each_lower_dev(dev, lower_dev, iter)
br_mdb_switchdev_host_port(dev, lower_dev, mp, type);
called from:
br_multicast_group_expired
-> br_multicast_host_leave
-> br_mdb_notify
-> br_mdb_switchdev_host
Basically, the bridge code is correct.
How about "the bridge code is not wrong"? Is there any reason why we
could not get rid of the duplicated messages at the source
(br_mdb_switchdev_host)?
The forward offloading we have talked about before requires that the
bridge-internal port OFMs are bounded to some low value
(e.g. BITS_PER_LONG) such that they can be tracked in a small
bitfield. I have a patch that does this, it is tiny.
With that in place, imagine a `br_switchdev_for_each_distinct_dev`
helper that would wrap `netdev_for_each_lower_dev`, keeping track of
OFMs it had already visited.
For all host related switchdev calls, we should be able to use that
instead of the full iterator to only contact each switchdev driver once.
On Wed, Feb 24, 2021 at 13:43, Vladimir Oltean [off-list ref] wrote:
From: Vladimir Oltean <vladimir.oltean@nxp.com>
In case we have ptp4l running on a bridged DSA switch interface, the PTP
traffic is classified as link-local (in the default profile, the MAC
addresses are 01:1b:19:00:00:00 and 01:80:c2:00:00:0e), which means it
isn't the responsibility of the bridge to make sure it gets trapped to
the CPU.
The solution is to implement the standard callbacks for dev_uc_add and
dev_mc_add, and behave just like any other network interface: ensure
that the user space program can see those packets.
So presumably the application would use PACKET_ADD_MEMBERSHIP to set
this up?
This is a really elegant way of solving this problem I think!
One problem I see is that this will not result in packets getting
trapped to the CPU, rather they will simply be forwarded. I.e. with
this patch applied, once ptp4l adds the groups it is interested in, my
HW FDB will look like this:
ADDR VID DST TYPE
01:1b:19:00:00:00 0 cpu0 static
01:80:c2:00:00:0e 0 cpu0 static
But this will not allow these groups to ingress on (STP) blocked
ports. AFAIK, PTP (certainly LLDP which also uses the latter group)
should be able to do that.
For mv88e6xxx (but I think this applies to most switches), there are
roughly three ways a given multicast group can reach the CPU:
1. Trap: Packet is unconditionally redirected to the CPU, independent
of things like 802.1X or STP state on the ingressing port.
2. Mirror: Send a copy of packets that pass all other ingress policy to
the CPU.
3. Forward: Forward packets that pass all other ingress policy to the
CPU.
Entries are now added as "Forward", which means that the group will no
longer reach the other local ports. But the command from the application
is "I want to see these packets", it says nothing about preventing the
group from being forwarded. So I think the default ought to be
"Mirror". Additionally, we probably need some way of specifying "Trap"
to those applications that need it. E.g. ptp4l could specify
PACKET_MR_MULTICAST_TRAP in mr_action or something if it does not want
the bridge (or the switch) to forward it.
If "Forward" is desired, the existing "bridge mdb" interface seems like
the proper one, since it also affects other ports.
On Wed, Feb 24, 2021 at 13:43, Vladimir Oltean [off-list ref] wrote:
quoted hunk
From: Vladimir Oltean <vladimir.oltean@nxp.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 <redacted>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
include/linux/if_bridge.h | 10 ++++++++
include/net/switchdev.h | 1 +
net/bridge/br_fdb.c | 53 +++++++++++++++++++++++++++++++++++++++
net/dsa/slave.c | 7 +++++-
4 files changed, 70 insertions(+), 1 deletion(-)
@@ -206,6 +206,7 @@ struct switchdev_notifier_info {structswitchdev_notifier_fdb_info{structswitchdev_notifier_infoinfo;/* must be first */+structlist_headlist;constunsignedchar*addr;u16vid;u8added_by_user:1,
@@ -2306,6 +2307,8 @@ static int dsa_slave_changeupper(struct net_device *dev,err=dsa_port_bridge_join(dp,bridge_dev);if(!err){dsa_bridge_mtu_normalization(dp);+br_fdb_replay(bridge_dev,dev,+&dsa_slave_switchdev_notifier);br_mdb_replay(bridge_dev,dev,&dsa_slave_switchdev_blocking_notifier);
If VLAN filtering is enabled, we would also have to replay that. Port
attributes also, right?
I like the pull model, because it saves the bridge from doing lots of
dumpster diving. However, should there be a single `bridge_replay` that
takes care of everything?
Rather than this kit-car approarch which outsources ordering etc to each
switchdev driver, you issue a single call saying: "bring me up to
speed". It seems right that that knowledge should reside in the bridge
since it was the one who sent the original events that are being
replayed.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-26 13:29:25
On Fri, Feb 26, 2021 at 11:59:36AM +0100, Tobias Waldekranz wrote:
On Wed, Feb 24, 2021 at 13:43, Vladimir Oltean [off-list ref] wrote:
quoted
From: Vladimir Oltean <vladimir.oltean@nxp.com>
In case we have ptp4l running on a bridged DSA switch interface, the PTP
traffic is classified as link-local (in the default profile, the MAC
addresses are 01:1b:19:00:00:00 and 01:80:c2:00:00:0e), which means it
isn't the responsibility of the bridge to make sure it gets trapped to
the CPU.
The solution is to implement the standard callbacks for dev_uc_add and
dev_mc_add, and behave just like any other network interface: ensure
that the user space program can see those packets.
So presumably the application would use PACKET_ADD_MEMBERSHIP to set
this up?
This is a really elegant way of solving this problem I think!
Yes, using the unmodified *_ADD_MEMBERSHIP UAPI was the intention.
If that is not possible, the whole idea kinda loses its appeal and we'd
be better off starting from scratch and figuring out how we'd prefer for
user space to request (exclusive) address membership.
One problem I see is that this will not result in packets getting
trapped to the CPU, rather they will simply be forwarded. I.e. with
this patch applied, once ptp4l adds the groups it is interested in, my
HW FDB will look like this:
ADDR VID DST TYPE
01:1b:19:00:00:00 0 cpu0 static
01:80:c2:00:00:0e 0 cpu0 static
But this will not allow these groups to ingress on (STP) blocked
ports. AFAIK, PTP (certainly LLDP which also uses the latter group)
should be able to do that.
For mv88e6xxx (but I think this applies to most switches), there are
roughly three ways a given multicast group can reach the CPU:
1. Trap: Packet is unconditionally redirected to the CPU, independent
of things like 802.1X or STP state on the ingressing port.
2. Mirror: Send a copy of packets that pass all other ingress policy to
the CPU.
3. Forward: Forward packets that pass all other ingress policy to the
CPU.
Entries are now added as "Forward", which means that the group will no
longer reach the other local ports. But the command from the application
is "I want to see these packets", it says nothing about preventing the
group from being forwarded. So I think the default ought to be
"Mirror". Additionally, we probably need some way of specifying "Trap"
to those applications that need it. E.g. ptp4l could specify
PACKET_MR_MULTICAST_TRAP in mr_action or something if it does not want
the bridge (or the switch) to forward it.
If "Forward" is desired, the existing "bridge mdb" interface seems like
the proper one, since it also affects other ports.
I'm not sure I understand your exact requirement.
Let me try to quote from IEEE 802.1Q-2018 clause 8.13.9 "Points of
attachment and connectivity for Higher Layer Entities". For context,
this talks about about Higher Layer Entities, aka applications such as
STP, MRP, ISIS-SPB, whose world view is as though they are connected
directly to the LAN that the bridge port is connected to, i.e. they
bypass the MAC relay (forwarding) function.
The spec says:
Controls placed in the forwarding path have no effect on the ability
of a Higher Layer Entity to transmit and receive frames to or from a
given LAN using a direct attachment to that LAN (e.g., from entity A
to LAN A); they only affect the path taken by any indirect
transmission or reception (e.g., from entity A to or from LAN B).
Then there's this drawing:
+-----------------------+ +-----------------------+
| Higher Layer Entity A | | Higher Layer Entity B |
+-----------------------+ +-----------------------+
| |
| +---------------------------------------+ |
| | MAC Relay Entity | |
| | | |
| | Port Filtering Database Port | |
| | State Information State | |
| | | |
| | / / / | |
+-----|---x/ x---------x/ x---------x/ x---+-----+
| | | |
| +---------------------------------------+ |
| |
+----------------------------+ +----------------------------+
| | | | | |
| x | | x |
| MAC / | | MAC / |
| Entity / MAC_Operational | | Entity / MAC_Operational |
| x | | x |
| | | | | |
+----------------------------+ +----------------------------+
| |
| |
| |
---------------------------- ----------------------------
/ LAN A / / LAN B /
/ / / /
/---------------------------/ /---------------------------/
Figure 8-20: Effect of control information on the forwarding path
A one phrase conclusion from the above is that a Higher Layer Entity
should be able to accept packets from the bridge port regardless of its
STP state, as long as the MAC is operational.
In my world view, anything sent and received through the swpN DSA
interfaces, and therefore by injection/extraction through the CPU port,
represents a Higher Layer Entity.
So the fact that your Marvell switch treats the CPU port as just another
destination for forwarding should be hidden away from the externally
observable behavior. DSA does not really support the switched endpoint
use case, we should deny attaching IP interfaces to it.
But to your point: we are currently using the .port_fdb_add and
.port_mdb_add API in DSA to install the host-filtered addresses. This is
true, and potentially incorrect, since indeed it assumes that the
underlying mechanism to trap these addresses to the CPU is through the
FDB/MDB, which will violate the expectation of Higher Layer Entities
since it goes through the forwarding process. I think we could add a new
set of APIs in DSA, something like .host_uc_add and .host_mc_add. Then,
drivers that can't do any better can go ahead and internally call their
.port_fdb_add and .port_mdb_add. Question: If we add .host_uc_add and
.host_mc_add, should these APIs be per front port? And if they should,
should we leave the switch driver the task of reference counting them?
As to why does IEEE 1588 use 01-80-C2-00-00-0E for peer delay in its
default profile for the L2 transport, I could only find this (quoting
clause "F.3 Multicast MAC Addresses"):
To ensure peer delay measurements on ports blocked by (Rapid/Multiple)
Spanning Tree Protocols, a reserved address, 01-80-C2-00-00-0E, shall be
used as a Destination MAC Address for PTP peer delay mechanism messages.
Basically, unlike end-to-end delay measurements, peer delay is by
definition point-to-point, which in an L2 network mean link-local.
So if your LAN uses the peer delay measurement protocol, then all your
switches must have some sort of PTP awareness. There are 2 cases really:
- You are a Peer-to-peer Transparent Clock, so you must speak the Peer
Delay protocol yourself. Therefore you must have a Higher Layer Entity
that consumes the PTP PDUs. So for this case, it doesn't really make
any difference that the reserved bridge MAC address range is used.
- You are an End-to-end Transparent Clock. These are an oddity of the
1588 standard which do not speak the Peer Delay protocol, but are
allowed to forward Peer Delay messages, and update the correctionField
of those Peer Delay messages that are Events (contain timestamps).
I would think that this is where having a reserved address for the
Peer Delay messages would make a difference.
Then, there seems to be the second part of your request, that of address
exclusivity. I think that for addresses that should explicitly be
removed from the hardware data path, the tc-trap action was created for
that exact purpose. However, the question really becomes: does PTP care
enough to know it's running on top of a switchdev interface, so that it
should claim exclusive ownership of that multicast group, or should it
just care enough to say "hey, I'm here, I'm interested in it too"?
For one thing, the 01-80-c2-00-00-0e multicast group is shared, even
IEEE 1588 says so:
NOTE 2: At its July 17−20, 2006 meeting the IEEE 802.1 Working Group
approved a motion that included the following text: "re-designate the
reserved address currently identified for use by 802.1AB as an address
that can be used by protocols that require the scope of the address to
be limited to an individual LAN" and "The reserved multicast address
that IEEE 1588 should use is 01-80-C2-00-00-0E." This address is not
reserved exclusively for PTP, but rather it is a shared address.
and the bridge driver also supports this odd attribute:
group_fwd_mask MASK - set the group forward mask. This is the bitmask
that is applied to decide whether to forward incoming frames destined
to link-local addresses, ie addresses of the form 01:80:C2:00:00:0X
(defaults to 0, ie the bridge does not forward any link-local frames).
which by the way seems to be in blatant contradiction of clause
8.13.4 Reserved MAC addresses: "Any frame with a destination address
that is a reserved MAC address shall not be forwarded by a Bridge".
So if ptp4l was to install a trap action by itself, it would potentially
interact in unexpected ways with other uses of that address on the same
system.
Thoughts?
From: Vladimir Oltean <olteanv@gmail.com> Date: 2021-02-26 18:08:55
On Fri, Feb 26, 2021 at 01:23:23PM +0100, Tobias Waldekranz wrote:
If VLAN filtering is enabled, we would also have to replay that. Port
attributes also, right?
I like the pull model, because it saves the bridge from doing lots of
dumpster diving. However, should there be a single `bridge_replay` that
takes care of everything?
Rather than this kit-car approarch which outsources ordering etc to each
switchdev driver, you issue a single call saying: "bring me up to
speed". It seems right that that knowledge should reside in the bridge
since it was the one who sent the original events that are being
replayed.
Yes, in the non-RFC version I'm going to do that.
I'm also thinking I could just pass the blocking and atomic switchdev
notifiers as an argument to the switchdev_bridge_port_offload_notify()
call, such that the drivers need to do one thing and one thing only.
For the purposes of this RFC I just wanted to have something that works
for address filtering.
On Fri, Feb 26, 2021 at 15:28, Vladimir Oltean [off-list ref] wrote:
On Fri, Feb 26, 2021 at 11:59:36AM +0100, Tobias Waldekranz wrote:
quoted
On Wed, Feb 24, 2021 at 13:43, Vladimir Oltean [off-list ref] wrote:
quoted
From: Vladimir Oltean <vladimir.oltean@nxp.com>
In case we have ptp4l running on a bridged DSA switch interface, the PTP
traffic is classified as link-local (in the default profile, the MAC
addresses are 01:1b:19:00:00:00 and 01:80:c2:00:00:0e), which means it
isn't the responsibility of the bridge to make sure it gets trapped to
the CPU.
The solution is to implement the standard callbacks for dev_uc_add and
dev_mc_add, and behave just like any other network interface: ensure
that the user space program can see those packets.
So presumably the application would use PACKET_ADD_MEMBERSHIP to set
this up?
This is a really elegant way of solving this problem I think!
Yes, using the unmodified *_ADD_MEMBERSHIP UAPI was the intention.
If that is not possible, the whole idea kinda loses its appeal and we'd
be better off starting from scratch and figuring out how we'd prefer for
user space to request (exclusive) address membership.
quoted
One problem I see is that this will not result in packets getting
trapped to the CPU, rather they will simply be forwarded. I.e. with
this patch applied, once ptp4l adds the groups it is interested in, my
HW FDB will look like this:
ADDR VID DST TYPE
01:1b:19:00:00:00 0 cpu0 static
01:80:c2:00:00:0e 0 cpu0 static
But this will not allow these groups to ingress on (STP) blocked
ports. AFAIK, PTP (certainly LLDP which also uses the latter group)
should be able to do that.
For mv88e6xxx (but I think this applies to most switches), there are
roughly three ways a given multicast group can reach the CPU:
1. Trap: Packet is unconditionally redirected to the CPU, independent
of things like 802.1X or STP state on the ingressing port.
2. Mirror: Send a copy of packets that pass all other ingress policy to
the CPU.
3. Forward: Forward packets that pass all other ingress policy to the
CPU.
Entries are now added as "Forward", which means that the group will no
longer reach the other local ports. But the command from the application
is "I want to see these packets", it says nothing about preventing the
group from being forwarded. So I think the default ought to be
"Mirror". Additionally, we probably need some way of specifying "Trap"
to those applications that need it. E.g. ptp4l could specify
PACKET_MR_MULTICAST_TRAP in mr_action or something if it does not want
the bridge (or the switch) to forward it.
If "Forward" is desired, the existing "bridge mdb" interface seems like
the proper one, since it also affects other ports.
I'm not sure I understand your exact requirement.
Let me try to quote from IEEE 802.1Q-2018 clause 8.13.9 "Points of
attachment and connectivity for Higher Layer Entities". For context,
this talks about about Higher Layer Entities, aka applications such as
STP, MRP, ISIS-SPB, whose world view is as though they are connected
directly to the LAN that the bridge port is connected to, i.e. they
bypass the MAC relay (forwarding) function.
The spec says:
Controls placed in the forwarding path have no effect on the ability
of a Higher Layer Entity to transmit and receive frames to or from a
given LAN using a direct attachment to that LAN (e.g., from entity A
to LAN A); they only affect the path taken by any indirect
transmission or reception (e.g., from entity A to or from LAN B).
Then there's this drawing:
+-----------------------+ +-----------------------+
| Higher Layer Entity A | | Higher Layer Entity B |
+-----------------------+ +-----------------------+
| |
| +---------------------------------------+ |
| | MAC Relay Entity | |
| | | |
| | Port Filtering Database Port | |
| | State Information State | |
| | | |
| | / / / | |
+-----|---x/ x---------x/ x---------x/ x---+-----+
| | | |
| +---------------------------------------+ |
| |
+----------------------------+ +----------------------------+
| | | | | |
| x | | x |
| MAC / | | MAC / |
| Entity / MAC_Operational | | Entity / MAC_Operational |
| x | | x |
| | | | | |
+----------------------------+ +----------------------------+
| |
| |
| |
---------------------------- ----------------------------
/ LAN A / / LAN B /
/ / / /
/---------------------------/ /---------------------------/
Figure 8-20: Effect of control information on the forwarding path
A one phrase conclusion from the above is that a Higher Layer Entity
should be able to accept packets from the bridge port regardless of its
STP state, as long as the MAC is operational.
In my world view, anything sent and received through the swpN DSA
interfaces, and therefore by injection/extraction through the CPU port,
represents a Higher Layer Entity.
Yes, I agree.
So the fact that your Marvell switch treats the CPU port as just another
destination for forwarding should be hidden away from the externally
observable behavior. DSA does not really support the switched endpoint
use case, we should deny attaching IP interfaces to it.
But to your point: we are currently using the .port_fdb_add and
.port_mdb_add API in DSA to install the host-filtered addresses. This is
true, and potentially incorrect, since indeed it assumes that the
underlying mechanism to trap these addresses to the CPU is through the
FDB/MDB, which will violate the expectation of Higher Layer Entities
since it goes through the forwarding process.
Well, at least the way it is implemented now. E.g. mv88e6xxx can setup
mirrors/traps via the FDB (ATU), I imagine that many smaller devices
reuse their FDBs for this purpose. So it might be possible to just
extend the API a bit to signal the type of entry required.
I think we could add a new
set of APIs in DSA, something like .host_uc_add and .host_mc_add. Then,
drivers that can't do any better can go ahead and internally call their
.port_fdb_add and .port_mdb_add.
That might be an ever better way of managing it. That would also make it
easier to design in the difference between trap/mirror from the start.
Question: If we add .host_uc_add and
.host_mc_add, should these APIs be per front port?
I think they would have to be per port in order to work in the
standalone case. You need some context to about which FID (or
equivalent) to tie the entry to.
And if they should,
should we leave the switch driver the task of reference counting them?
Well, I know you do not like the DSA layer "trying to be helpful", but I
can not imagine a case where some hardware would be interested in having
N callbacks for the same address when N ports are attached to the same
bridge. So I think it would be a service that DSA can provide.
As to why does IEEE 1588 use 01-80-C2-00-00-0E for peer delay in its
default profile for the L2 transport, I could only find this (quoting
clause "F.3 Multicast MAC Addresses"):
To ensure peer delay measurements on ports blocked by (Rapid/Multiple)
Spanning Tree Protocols, a reserved address, 01-80-C2-00-00-0E, shall be
used as a Destination MAC Address for PTP peer delay mechanism messages.
Yeah that makes sense. You want to make sure that you are already synced
with your neighbor when a topology change occurs.
Basically, unlike end-to-end delay measurements, peer delay is by
definition point-to-point, which in an L2 network mean link-local.
So if your LAN uses the peer delay measurement protocol, then all your
switches must have some sort of PTP awareness. There are 2 cases really:
- You are a Peer-to-peer Transparent Clock, so you must speak the Peer
Delay protocol yourself. Therefore you must have a Higher Layer Entity
that consumes the PTP PDUs. So for this case, it doesn't really make
any difference that the reserved bridge MAC address range is used.
- You are an End-to-end Transparent Clock. These are an oddity of the
1588 standard which do not speak the Peer Delay protocol, but are
allowed to forward Peer Delay messages, and update the correctionField
of those Peer Delay messages that are Events (contain timestamps).
I would think that this is where having a reserved address for the
Peer Delay messages would make a difference.
Then, there seems to be the second part of your request, that of address
exclusivity. I think that for addresses that should explicitly be
removed from the hardware data path, the tc-trap action was created for
that exact purpose.
Maybe. Tc-trap is a weird creature in that it seems to violate the rule
that in order for an offload to be accepted into the kernel, a software
implementation needs to be in place first. The exception, as I
understand it, is for things that do not make sense in software
(e.g. cut-through switching).
Trapping a packet, blocking bridging, certainly seems like something
that can be done in software. The fact that this was not done means that
it is hard to add it now without potentially breaking existing
use-cases.
Now when a tc trap filter is added, the switch will trap it to the CPU,
the switchdev driver will _not_ set OFM. The result is that the frame is
software forwarded by the bridge. Even if you hack the driver to pretend
that it was HW forwarded (setting OFM), that will not stop the bridge
from forwarding it to foreign interfaces.
One idea could be to implement the software version of "trap" to mean
that sch_handle_ingress could return a signal to
__netif_receive_skb_core to skip any rx handlers and only consider
device specific protocol handlers. A kind of "RX_HANDLER_EXACT with a
twist". Again, this is hard to add after the fact, but you should at
least be able to get the old behavior with "skip_sw".
However, the question really becomes: does PTP care
enough to know it's running on top of a switchdev interface, so that it
should claim exclusive ownership of that multicast group, or should it
just care enough to say "hey, I'm here, I'm interested in it too"?
Ideally it would not know about switchdev, but maybe it should care
whether it is running on top of a bridge, since that has implications
for how PTP frames should be forwarded (or not).
For one thing, the 01-80-c2-00-00-0e multicast group is shared, even
IEEE 1588 says so:
NOTE 2: At its July 17−20, 2006 meeting the IEEE 802.1 Working Group
approved a motion that included the following text: "re-designate the
reserved address currently identified for use by 802.1AB as an address
that can be used by protocols that require the scope of the address to
be limited to an individual LAN" and "The reserved multicast address
that IEEE 1588 should use is 01-80-C2-00-00-0E." This address is not
reserved exclusively for PTP, but rather it is a shared address.
and the bridge driver also supports this odd attribute:
group_fwd_mask MASK - set the group forward mask. This is the bitmask
that is applied to decide whether to forward incoming frames destined
to link-local addresses, ie addresses of the form 01:80:C2:00:00:0X
(defaults to 0, ie the bridge does not forward any link-local frames).
which by the way seems to be in blatant contradiction of clause
8.13.4 Reserved MAC addresses: "Any frame with a destination address
that is a reserved MAC address shall not be forwarded by a Bridge".
Well, the set of addresses that are reserved vary based on the type of
bridge (Tables 8-1, 8-2, and 8-3). So this setting allows you to choose
the applicable set.
Additionally, there are cases where you want to emulate a $10 switch (or
even hub) which does _no_ kind of filtering at all.
So if ptp4l was to install a trap action by itself, it would potentially
interact in unexpected ways with other uses of that address on the same
system.
Yeah that is a problem. The easy way out is probably "that is the
admin's problem". A middle way could be to warn/abort if the trap is not
in place. You could also go all the way and reference count filters I
suppose.
On Wed, Feb 24, 2021 at 01:43:38PM +0200, Vladimir Oltean wrote:
From: Vladimir Oltean <vladimir.oltean@nxp.com>
The bridge device currently goes into promiscuous mode when it has an
upper with a different MAC address than itself. But it could do better:
it could sync the MAC addresses of its uppers to the software FDB, as
local entries pointing to the bridge itself. This is compatible with
switchdev, since drivers are now instructed to trap these MAC addresses
to the CPU.
Note that the dev_uc_add API does not propagate VLAN ID, so this only
works for VLAN-unaware bridges.
IOW, it breaks VLAN-aware bridges...
I understand that you do not want to track bridge uppers, but once you
look beyond L2 you will need to do it anyway.
Currently, you only care about getting packets with specific DMACs to
the CPU. With L3 offload you will need to send these packets to your
router block instead and track other attributes of these uppers such as
their MTU so that the hardware will know to generate MTU exceptions. In
addition, the hardware needs to know the MAC addresses of these uppers
so that it will rewrite the SMAC of forwarded packets.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2022-02-22 11:22:09
Hi Ido,
On Mon, 1 Mar 2021 at 17:22, Ido Schimmel [off-list ref] wrote:
On Wed, Feb 24, 2021 at 01:43:38PM +0200, Vladimir Oltean wrote:
quoted
From: Vladimir Oltean <vladimir.oltean@nxp.com>
The bridge device currently goes into promiscuous mode when it has an
upper with a different MAC address than itself. But it could do better:
it could sync the MAC addresses of its uppers to the software FDB, as
local entries pointing to the bridge itself. This is compatible with
switchdev, since drivers are now instructed to trap these MAC addresses
to the CPU.
Note that the dev_uc_add API does not propagate VLAN ID, so this only
works for VLAN-unaware bridges.
IOW, it breaks VLAN-aware bridges...
I understand that you do not want to track bridge uppers, but once you
look beyond L2 you will need to do it anyway.
Currently, you only care about getting packets with specific DMACs to
the CPU. With L3 offload you will need to send these packets to your
router block instead and track other attributes of these uppers such as
their MTU so that the hardware will know to generate MTU exceptions. In
addition, the hardware needs to know the MAC addresses of these uppers
so that it will rewrite the SMAC of forwarded packets.
Ok, let's say I want to track bridge uppers. How can I track the changes to
those interfaces' secondary addresses, in a way that keeps the association
with their VLAN ID, if those uppers are VLAN interfaces?
On Tue, Feb 22, 2022 at 01:21:53PM +0200, Vladimir Oltean wrote:
Hi Ido,
On Mon, 1 Mar 2021 at 17:22, Ido Schimmel [off-list ref] wrote:
quoted
On Wed, Feb 24, 2021 at 01:43:38PM +0200, Vladimir Oltean wrote:
quoted
From: Vladimir Oltean <vladimir.oltean@nxp.com>
The bridge device currently goes into promiscuous mode when it has an
upper with a different MAC address than itself. But it could do better:
it could sync the MAC addresses of its uppers to the software FDB, as
local entries pointing to the bridge itself. This is compatible with
switchdev, since drivers are now instructed to trap these MAC addresses
to the CPU.
Note that the dev_uc_add API does not propagate VLAN ID, so this only
works for VLAN-unaware bridges.
IOW, it breaks VLAN-aware bridges...
I understand that you do not want to track bridge uppers, but once you
look beyond L2 you will need to do it anyway.
Currently, you only care about getting packets with specific DMACs to
the CPU. With L3 offload you will need to send these packets to your
router block instead and track other attributes of these uppers such as
their MTU so that the hardware will know to generate MTU exceptions. In
addition, the hardware needs to know the MAC addresses of these uppers
so that it will rewrite the SMAC of forwarded packets.
Ok, let's say I want to track bridge uppers. How can I track the changes to
those interfaces' secondary addresses, in a way that keeps the association
with their VLAN ID, if those uppers are VLAN interfaces?
Hi,
I'm not sure what you mean by "secondary addresses", but the canonical
way that I'm familiar with of adding MAC addresses to a netdev is to use
macvlan uppers. For example:
# ip link add name br0 up type bridge vlan_filtering 1
# ip link add link br0 name br0.10 type vlan id 10
# ip link add link br0.10 name br0.10-v address 00:11:22:33:44:55 type macvlan mode private
keepalived uses it in VRRP virtual MAC mode (for example):
https://github.com/acassen/keepalived/blob/master/doc/NOTE_vrrp_vmac.txt
In the software data path, this will result in br0 transitioning to
promisc mode and passing all the packets to upper devices that will
filter them.
In the hardware data path, you can apply promisc mode by flooding to
your CPU port (I believe this is what you are trying to avoid) or
install an FDB entry <00:11:22:33:44:55,10> that points to your CPU
port.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2022-02-22 17:18:17
On Tue, Feb 22, 2022 at 06:54:13PM +0200, Ido Schimmel wrote:
On Tue, Feb 22, 2022 at 01:21:53PM +0200, Vladimir Oltean wrote:
quoted
Hi Ido,
On Mon, 1 Mar 2021 at 17:22, Ido Schimmel [off-list ref] wrote:
quoted
On Wed, Feb 24, 2021 at 01:43:38PM +0200, Vladimir Oltean wrote:
quoted
From: Vladimir Oltean <vladimir.oltean@nxp.com>
The bridge device currently goes into promiscuous mode when it has an
upper with a different MAC address than itself. But it could do better:
it could sync the MAC addresses of its uppers to the software FDB, as
local entries pointing to the bridge itself. This is compatible with
switchdev, since drivers are now instructed to trap these MAC addresses
to the CPU.
Note that the dev_uc_add API does not propagate VLAN ID, so this only
works for VLAN-unaware bridges.
IOW, it breaks VLAN-aware bridges...
I understand that you do not want to track bridge uppers, but once you
look beyond L2 you will need to do it anyway.
Currently, you only care about getting packets with specific DMACs to
the CPU. With L3 offload you will need to send these packets to your
router block instead and track other attributes of these uppers such as
their MTU so that the hardware will know to generate MTU exceptions. In
addition, the hardware needs to know the MAC addresses of these uppers
so that it will rewrite the SMAC of forwarded packets.
Ok, let's say I want to track bridge uppers. How can I track the changes to
those interfaces' secondary addresses, in a way that keeps the association
with their VLAN ID, if those uppers are VLAN interfaces?
Hi,
I'm not sure what you mean by "secondary addresses", but the canonical
way that I'm familiar with of adding MAC addresses to a netdev is to use
macvlan uppers. For example:
# ip link add name br0 up type bridge vlan_filtering 1
# ip link add link br0 name br0.10 type vlan id 10
# ip link add link br0.10 name br0.10-v address 00:11:22:33:44:55 type macvlan mode private
keepalived uses it in VRRP virtual MAC mode (for example):
https://github.com/acassen/keepalived/blob/master/doc/NOTE_vrrp_vmac.txt
In the software data path, this will result in br0 transitioning to
promisc mode and passing all the packets to upper devices that will
filter them.
In the hardware data path, you can apply promisc mode by flooding to
your CPU port (I believe this is what you are trying to avoid) or
install an FDB entry <00:11:22:33:44:55,10> that points to your CPU
port.
Maybe the terminology is not the best, but by secondary addresses I mean
struct net_device :: uc and mc. To my knowledge, the MAC address of
vlan/macvlan uppers is not the only way in which these lists can be
populated. There is also AF_PACKET UAPI for PACKET_MR_MULTICAST and
PACKET_MR_UNICAST, and this ends up calling dev_mc_add() and
dev_uc_add(). User space may use this API to add a secondary address to
a VLAN upper interface of a bridge.
The question was how can the bridge get notified of changes to those 2
lists of its upper interfaces?
If it monitors NETDEV_CHANGEUPPER it has access to those lists only when
an upper joins or leaves.
If it monitors NETDEV_CHANGEADDR, it gets notified only to changes on
the primary addresses of the uppers (dev_addr and dev_addrs).
If it implements ndo_set_rx_mode (this patch), it has all the addresses
synced to it, but they lack a VLAN ID, because every address lacks
further information about which device added it.
If there's logic in the mlxsw driver that does this, unfortunately I
haven't found it.
On Tue, Feb 22, 2022 at 07:18:10PM +0200, Vladimir Oltean wrote:
On Tue, Feb 22, 2022 at 06:54:13PM +0200, Ido Schimmel wrote:
quoted
On Tue, Feb 22, 2022 at 01:21:53PM +0200, Vladimir Oltean wrote:
quoted
Hi Ido,
On Mon, 1 Mar 2021 at 17:22, Ido Schimmel [off-list ref] wrote:
quoted
On Wed, Feb 24, 2021 at 01:43:38PM +0200, Vladimir Oltean wrote:
quoted
From: Vladimir Oltean <vladimir.oltean@nxp.com>
The bridge device currently goes into promiscuous mode when it has an
upper with a different MAC address than itself. But it could do better:
it could sync the MAC addresses of its uppers to the software FDB, as
local entries pointing to the bridge itself. This is compatible with
switchdev, since drivers are now instructed to trap these MAC addresses
to the CPU.
Note that the dev_uc_add API does not propagate VLAN ID, so this only
works for VLAN-unaware bridges.
IOW, it breaks VLAN-aware bridges...
I understand that you do not want to track bridge uppers, but once you
look beyond L2 you will need to do it anyway.
Currently, you only care about getting packets with specific DMACs to
the CPU. With L3 offload you will need to send these packets to your
router block instead and track other attributes of these uppers such as
their MTU so that the hardware will know to generate MTU exceptions. In
addition, the hardware needs to know the MAC addresses of these uppers
so that it will rewrite the SMAC of forwarded packets.
Ok, let's say I want to track bridge uppers. How can I track the changes to
those interfaces' secondary addresses, in a way that keeps the association
with their VLAN ID, if those uppers are VLAN interfaces?
Hi,
I'm not sure what you mean by "secondary addresses", but the canonical
way that I'm familiar with of adding MAC addresses to a netdev is to use
macvlan uppers. For example:
# ip link add name br0 up type bridge vlan_filtering 1
# ip link add link br0 name br0.10 type vlan id 10
# ip link add link br0.10 name br0.10-v address 00:11:22:33:44:55 type macvlan mode private
keepalived uses it in VRRP virtual MAC mode (for example):
https://github.com/acassen/keepalived/blob/master/doc/NOTE_vrrp_vmac.txt
In the software data path, this will result in br0 transitioning to
promisc mode and passing all the packets to upper devices that will
filter them.
In the hardware data path, you can apply promisc mode by flooding to
your CPU port (I believe this is what you are trying to avoid) or
install an FDB entry <00:11:22:33:44:55,10> that points to your CPU
port.
Maybe the terminology is not the best, but by secondary addresses I mean
struct net_device :: uc and mc. To my knowledge, the MAC address of
vlan/macvlan uppers is not the only way in which these lists can be
populated. There is also AF_PACKET UAPI for PACKET_MR_MULTICAST and
PACKET_MR_UNICAST, and this ends up calling dev_mc_add() and
dev_uc_add(). User space may use this API to add a secondary address to
a VLAN upper interface of a bridge.
OK, I see the problem... So you want the bridge to support
'IFF_UNICAST_FLT' by installing local FDB entries? I see two potential
problems:
1. For VLAN-unaware bridges this is trivial as VLAN information is of no
use. For VLAN-aware bridges we either need to communicate VLAN
information from upper layers or install a local FDB entry per each
configured VLAN (wasteful...). Note that VLAN information will not
always be available (in PACKET_MR_UNICAST, for example), in which case a
local FDB entry will need to be configured per each existing VLAN in
order to maintain existing behavior. Which lead to me think about the
second problem...
2. The bigger problem that I see is that if the bridge starts supporting
'IFF_UNICAST_FLT' by installing local FDB entries, then packets that
were previously locally received and flooded will only be locally
received. Only locally receiving them makes sense, but I don't know what
will break if we change the existing behavior... Maybe this needs to be
guarded by a new bridge option?
The question was how can the bridge get notified of changes to those 2
lists of its upper interfaces?
If it monitors NETDEV_CHANGEUPPER it has access to those lists only when
an upper joins or leaves.
If it monitors NETDEV_CHANGEADDR, it gets notified only to changes on
the primary addresses of the uppers (dev_addr and dev_addrs).
If it implements ndo_set_rx_mode (this patch), it has all the addresses
synced to it, but they lack a VLAN ID, because every address lacks
further information about which device added it.
If there's logic in the mlxsw driver that does this, unfortunately I
haven't found it.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2022-02-24 13:52:52
On Thu, Feb 24, 2022 at 03:22:31PM +0200, Ido Schimmel wrote:
On Tue, Feb 22, 2022 at 07:18:10PM +0200, Vladimir Oltean wrote:
quoted
On Tue, Feb 22, 2022 at 06:54:13PM +0200, Ido Schimmel wrote:
quoted
On Tue, Feb 22, 2022 at 01:21:53PM +0200, Vladimir Oltean wrote:
quoted
Hi Ido,
On Mon, 1 Mar 2021 at 17:22, Ido Schimmel [off-list ref] wrote:
quoted
On Wed, Feb 24, 2021 at 01:43:38PM +0200, Vladimir Oltean wrote:
quoted
From: Vladimir Oltean <vladimir.oltean@nxp.com>
The bridge device currently goes into promiscuous mode when it has an
upper with a different MAC address than itself. But it could do better:
it could sync the MAC addresses of its uppers to the software FDB, as
local entries pointing to the bridge itself. This is compatible with
switchdev, since drivers are now instructed to trap these MAC addresses
to the CPU.
Note that the dev_uc_add API does not propagate VLAN ID, so this only
works for VLAN-unaware bridges.
IOW, it breaks VLAN-aware bridges...
I understand that you do not want to track bridge uppers, but once you
look beyond L2 you will need to do it anyway.
Currently, you only care about getting packets with specific DMACs to
the CPU. With L3 offload you will need to send these packets to your
router block instead and track other attributes of these uppers such as
their MTU so that the hardware will know to generate MTU exceptions. In
addition, the hardware needs to know the MAC addresses of these uppers
so that it will rewrite the SMAC of forwarded packets.
Ok, let's say I want to track bridge uppers. How can I track the changes to
those interfaces' secondary addresses, in a way that keeps the association
with their VLAN ID, if those uppers are VLAN interfaces?
Hi,
I'm not sure what you mean by "secondary addresses", but the canonical
way that I'm familiar with of adding MAC addresses to a netdev is to use
macvlan uppers. For example:
# ip link add name br0 up type bridge vlan_filtering 1
# ip link add link br0 name br0.10 type vlan id 10
# ip link add link br0.10 name br0.10-v address 00:11:22:33:44:55 type macvlan mode private
keepalived uses it in VRRP virtual MAC mode (for example):
https://github.com/acassen/keepalived/blob/master/doc/NOTE_vrrp_vmac.txt
In the software data path, this will result in br0 transitioning to
promisc mode and passing all the packets to upper devices that will
filter them.
In the hardware data path, you can apply promisc mode by flooding to
your CPU port (I believe this is what you are trying to avoid) or
install an FDB entry <00:11:22:33:44:55,10> that points to your CPU
port.
Maybe the terminology is not the best, but by secondary addresses I mean
struct net_device :: uc and mc. To my knowledge, the MAC address of
vlan/macvlan uppers is not the only way in which these lists can be
populated. There is also AF_PACKET UAPI for PACKET_MR_MULTICAST and
PACKET_MR_UNICAST, and this ends up calling dev_mc_add() and
dev_uc_add(). User space may use this API to add a secondary address to
a VLAN upper interface of a bridge.
OK, I see the problem... So you want the bridge to support
'IFF_UNICAST_FLT' by installing local FDB entries? I see two potential
problems:
1. For VLAN-unaware bridges this is trivial as VLAN information is of no
use. For VLAN-aware bridges we either need to communicate VLAN
information from upper layers or install a local FDB entry per each
configured VLAN (wasteful...). Note that VLAN information will not
always be available (in PACKET_MR_UNICAST, for example), in which case a
local FDB entry will need to be configured per each existing VLAN in
order to maintain existing behavior. Which lead to me think about the
second problem...
2. The bigger problem that I see is that if the bridge starts supporting
'IFF_UNICAST_FLT' by installing local FDB entries, then packets that
were previously locally received and flooded will only be locally
received. Only locally receiving them makes sense, but I don't know what
will break if we change the existing behavior... Maybe this needs to be
guarded by a new bridge option?
I think it boils down to whether PACKET_MR_UNICAST on br0 is equivalent to
'bridge fdb add dev br0 self permanent' or not. Theoretically, the
former means "if a packet enters the local termination path of br0,
don't drop it", while the other means "direct this MAC DA only towards
the local termination path of br0". I.o.w. the difference between "copy
to CPU" and "trap to CPU".
If we agree they aren't equivalent, and we also agree that a macvlan on
top of a bridge wants "trap to CPU" instead of "copy to CPU", I think
the only logical conclusion is that the communication mechanism between
the bridge and the macvlan that we're looking for doesn't exist -
dev_uc_add() does something slightly different.
Which is why I want to better understand your idea of having the bridge
track upper interfaces.
Essentially, it isn't the bridge local FDB entries that I have a problem with.
"Locally terminated packets that are also flooded on other bridge ports"
is a problem that DSA users have tried to get rid of for years, I didn't
hear a single complaint after we started fixing that. To me, a bridge
VLAN is by definition an L2 broadcast domain and MAC addresses should be
unique. I can't imagine what would break if we'd make the bridge deliver
the packets only to their known destination.
On Thu, Feb 24, 2022 at 03:52:41PM +0200, Vladimir Oltean wrote:
On Thu, Feb 24, 2022 at 03:22:31PM +0200, Ido Schimmel wrote:
quoted
On Tue, Feb 22, 2022 at 07:18:10PM +0200, Vladimir Oltean wrote:
quoted
On Tue, Feb 22, 2022 at 06:54:13PM +0200, Ido Schimmel wrote:
quoted
On Tue, Feb 22, 2022 at 01:21:53PM +0200, Vladimir Oltean wrote:
quoted
Hi Ido,
On Mon, 1 Mar 2021 at 17:22, Ido Schimmel [off-list ref] wrote:
quoted
On Wed, Feb 24, 2021 at 01:43:38PM +0200, Vladimir Oltean wrote:
quoted
From: Vladimir Oltean <vladimir.oltean@nxp.com>
The bridge device currently goes into promiscuous mode when it has an
upper with a different MAC address than itself. But it could do better:
it could sync the MAC addresses of its uppers to the software FDB, as
local entries pointing to the bridge itself. This is compatible with
switchdev, since drivers are now instructed to trap these MAC addresses
to the CPU.
Note that the dev_uc_add API does not propagate VLAN ID, so this only
works for VLAN-unaware bridges.
IOW, it breaks VLAN-aware bridges...
I understand that you do not want to track bridge uppers, but once you
look beyond L2 you will need to do it anyway.
Currently, you only care about getting packets with specific DMACs to
the CPU. With L3 offload you will need to send these packets to your
router block instead and track other attributes of these uppers such as
their MTU so that the hardware will know to generate MTU exceptions. In
addition, the hardware needs to know the MAC addresses of these uppers
so that it will rewrite the SMAC of forwarded packets.
Ok, let's say I want to track bridge uppers. How can I track the changes to
those interfaces' secondary addresses, in a way that keeps the association
with their VLAN ID, if those uppers are VLAN interfaces?
Hi,
I'm not sure what you mean by "secondary addresses", but the canonical
way that I'm familiar with of adding MAC addresses to a netdev is to use
macvlan uppers. For example:
# ip link add name br0 up type bridge vlan_filtering 1
# ip link add link br0 name br0.10 type vlan id 10
# ip link add link br0.10 name br0.10-v address 00:11:22:33:44:55 type macvlan mode private
keepalived uses it in VRRP virtual MAC mode (for example):
https://github.com/acassen/keepalived/blob/master/doc/NOTE_vrrp_vmac.txt
In the software data path, this will result in br0 transitioning to
promisc mode and passing all the packets to upper devices that will
filter them.
In the hardware data path, you can apply promisc mode by flooding to
your CPU port (I believe this is what you are trying to avoid) or
install an FDB entry <00:11:22:33:44:55,10> that points to your CPU
port.
Maybe the terminology is not the best, but by secondary addresses I mean
struct net_device :: uc and mc. To my knowledge, the MAC address of
vlan/macvlan uppers is not the only way in which these lists can be
populated. There is also AF_PACKET UAPI for PACKET_MR_MULTICAST and
PACKET_MR_UNICAST, and this ends up calling dev_mc_add() and
dev_uc_add(). User space may use this API to add a secondary address to
a VLAN upper interface of a bridge.
OK, I see the problem... So you want the bridge to support
'IFF_UNICAST_FLT' by installing local FDB entries? I see two potential
problems:
1. For VLAN-unaware bridges this is trivial as VLAN information is of no
use. For VLAN-aware bridges we either need to communicate VLAN
information from upper layers or install a local FDB entry per each
configured VLAN (wasteful...). Note that VLAN information will not
always be available (in PACKET_MR_UNICAST, for example), in which case a
local FDB entry will need to be configured per each existing VLAN in
order to maintain existing behavior. Which lead to me think about the
second problem...
2. The bigger problem that I see is that if the bridge starts supporting
'IFF_UNICAST_FLT' by installing local FDB entries, then packets that
were previously locally received and flooded will only be locally
received. Only locally receiving them makes sense, but I don't know what
will break if we change the existing behavior... Maybe this needs to be
guarded by a new bridge option?
I think it boils down to whether PACKET_MR_UNICAST on br0 is equivalent to
'bridge fdb add dev br0 self permanent' or not. Theoretically, the
former means "if a packet enters the local termination path of br0,
don't drop it",
Trying to understand the first part of the sentence, are you saying that
if user space decides to use this interface, then it is up to it to
ensure that packets with the given unicast address are terminated on the
bridge? That is, it is up to user space to install the necessary
permanent FDB record? I think that is fair, it is just that right now
this operation does something else and causes all the packets forwarded
via the bridge to be locally terminated. Most of them will then be
dropped by upper layers. I don't think this was the author's intention,
it seems like an unfortunate side effect of current implementation. This
behavior is even more ridiculous when you take hardware offload into
account, as usually the CPU is unable to handle all these packets.
while the other means "direct this MAC DA only towards
the local termination path of br0".
This I agree with.
I.o.w. the difference between "copy to CPU" and "trap to CPU".
If we agree they aren't equivalent, and we also agree that a macvlan on
top of a bridge wants "trap to CPU" instead of "copy to CPU", I think
the only logical conclusion is that the communication mechanism between
the bridge and the macvlan that we're looking for doesn't exist -
dev_uc_add() does something slightly different.
Which is why I want to better understand your idea of having the bridge
track upper interfaces.
In my case these upper interfaces are actually router interfaces and I'm
interested in their MAC (in addition to other attributes) to know which
FDB entry to program towards the router port (your CPU port) on ingress
and which SA to use on egress (the hardware has limitations on SAs).
I'm pretty sure bridge maintainers will not agree to have this code in
the bridge driver in which case you can implement this in DSA. Should be
quite simple as I guess most configurations use VLANs/MACVLANs uppers.
Essentially, it isn't the bridge local FDB entries that I have a problem with.
"Locally terminated packets that are also flooded on other bridge ports"
is a problem that DSA users have tried to get rid of for years, I didn't
hear a single complaint after we started fixing that. To me, a bridge
VLAN is by definition an L2 broadcast domain and MAC addresses should be
unique. I can't imagine what would break if we'd make the bridge deliver
the packets only to their known destination.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2022-03-02 11:19:50
On Tue, Mar 01, 2022 at 06:20:39PM +0200, Ido Schimmel wrote:
quoted
quoted
OK, I see the problem... So you want the bridge to support
'IFF_UNICAST_FLT' by installing local FDB entries? I see two potential
problems:
1. For VLAN-unaware bridges this is trivial as VLAN information is of no
use. For VLAN-aware bridges we either need to communicate VLAN
information from upper layers or install a local FDB entry per each
configured VLAN (wasteful...). Note that VLAN information will not
always be available (in PACKET_MR_UNICAST, for example), in which case a
local FDB entry will need to be configured per each existing VLAN in
order to maintain existing behavior. Which lead to me think about the
second problem...
2. The bigger problem that I see is that if the bridge starts supporting
'IFF_UNICAST_FLT' by installing local FDB entries, then packets that
were previously locally received and flooded will only be locally
received. Only locally receiving them makes sense, but I don't know what
will break if we change the existing behavior... Maybe this needs to be
guarded by a new bridge option?
I think it boils down to whether PACKET_MR_UNICAST on br0 is equivalent to
'bridge fdb add dev br0 self permanent' or not. Theoretically, the
former means "if a packet enters the local termination path of br0,
don't drop it",
Trying to understand the first part of the sentence, are you saying that
if user space decides to use this interface, then it is up to it to
ensure that packets with the given unicast address are terminated on the
bridge? That is, it is up to user space to install the necessary
permanent FDB record?
This first part of the sentence is just wondering whether it is even
sane to make the bridge driver essentially provide an implementation for
PACKET_MR_UNICAST, and translate that into a local FDB entry which means
something else. User space can already install a local FDB entry with
the MAC address of the upper interface, and this will behave closer to
what is expected.
If the bridge ever implements the support for PACKET_MR_UNICAST, a new
FDB entry flag is probably needed, for local reception. If the MAC
address added with PACKET_MR_UNICAST is new, the bridge would create an
entry with fdb->dst = NULL. If it already exists, it would keep the
existing fdb->dst and just mark the local reception flag as true.
This is to comply with the "copy to CPU" semantics instead of altering
the forwarding destination. I'm not sure whether there are real use
cases beyond just complying to expected semantics.
I think that is fair, it is just that right now this operation does
something else and causes all the packets forwarded via the bridge to
be locally terminated. Most of them will then be dropped by upper
layers. I don't think this was the author's intention, it seems like
an unfortunate side effect of current implementation.
Do you mean here that the "something else" is to turn on promiscuous
mode for the bridge, and this makes local_rcv = true for every packet in
br_handle_frame_finish?
Yes, that is a problem. The dev_uc_add() calls will keep the bridge's
promiscuity at 1, with no way to turn it back to 0 from user space.
To get rid of this we'd need to declare IFF_UNICAST_FLT at the very
least.
This behavior is even more ridiculous when you take hardware offload
into account, as usually the CPU is unable to handle all these
packets.
If we keep the analogy that a PACKET_MR_UNICAST means "copy MAC address
X to CPU", then IFF_PROMISC means "copy all packets to CPU", no?
So I wouldn't say the behavior is even more ridiculous, it is just as
ridiculous, just on a different level. And maybe not even "ridiculous",
just "highly sub-optimal". Ridiculous would be to not comply to the
expected behavior.
quoted
while the other means "direct this MAC DA only towards
the local termination path of br0".
This I agree with.
quoted
I.o.w. the difference between "copy to CPU" and "trap to CPU".
If we agree they aren't equivalent, and we also agree that a macvlan on
top of a bridge wants "trap to CPU" instead of "copy to CPU", I think
the only logical conclusion is that the communication mechanism between
the bridge and the macvlan that we're looking for doesn't exist -
dev_uc_add() does something slightly different.
Which is why I want to better understand your idea of having the bridge
track upper interfaces.
In my case these upper interfaces are actually router interfaces and I'm
interested in their MAC (in addition to other attributes) to know which
FDB entry to program towards the router port (your CPU port) on ingress
and which SA to use on egress (the hardware has limitations on SAs).
I'm pretty sure bridge maintainers will not agree to have this code in
the bridge driver in which case you can implement this in DSA. Should be
quite simple as I guess most configurations use VLANs/MACVLANs uppers.
Yes, but this will record only the dev_addr of those upper interfaces.
It would not be fully compliant with what user space can ask for.