Ignore locked fdb entries coming in on all drivers.
Signed-off-by: Hans Schultz <[email protected]>
---
drivers/net/dsa/b53/b53_common.c | 5 +++++
drivers/net/dsa/b53/b53_priv.h | 1 +
drivers/net/dsa/hirschmann/hellcreek.c | 5 +++++
drivers/net/dsa/lan9303-core.c | 5 +++++
drivers/net/dsa/lantiq_gswip.c | 5 +++++
drivers/net/dsa/microchip/ksz9477.c | 5 +++++
drivers/net/dsa/mt7530.c | 5 +++++
drivers/net/dsa/mv88e6xxx/chip.c | 5 +++++
drivers/net/dsa/ocelot/felix.c | 5 +++++
drivers/net/dsa/qca8k.c | 5 +++++
drivers/net/dsa/sja1105/sja1105_main.c | 5 +++++
include/net/dsa.h | 1 +
net/dsa/switch.c | 4 ++--
13 files changed, 54 insertions(+), 2 deletions(-)
diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index fbb32aa49b24..3567d4fcbaa7 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1688,11 +1688,16 @@ static int b53_arl_op(struct b53_device *dev, int op, int port,
int b53_fdb_add(struct dsa_switch *ds, int port,
const unsigned char *addr, u16 vid,
+ bool is_locked,
struct dsa_db db)
{
struct b53_device *priv = ds->priv;
int ret;
+ /* Ignore locked entries */
+ if (is_locked)
+ return 0;
+
/* 5325 and 5365 require some more massaging, but could
* be supported eventually
*/
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index 795cbffd5c2b..19501b76b9df 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -362,6 +362,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port,
const struct switchdev_obj_port_vlan *vlan);
int b53_fdb_add(struct dsa_switch *ds, int port,
const unsigned char *addr, u16 vid,
+ bool is_locked,
struct dsa_db db);
int b53_fdb_del(struct dsa_switch *ds, int port,
const unsigned char *addr, u16 vid,
diff --git a/drivers/net/dsa/hirschmann/hellcreek.c b/drivers/net/dsa/hirschmann/hellcreek.c
index ac1f3b3a7040..0125d901c988 100644
--- a/drivers/net/dsa/hirschmann/hellcreek.c
+++ b/drivers/net/dsa/hirschmann/hellcreek.c
@@ -829,12 +829,17 @@ static int hellcreek_fdb_get(struct hellcreek *hellcreek,
static int hellcreek_fdb_add(struct dsa_switch *ds, int port,
const unsigned char *addr, u16 vid,
+ bool is_locked,
struct dsa_db db)
{
struct hellcreek_fdb_entry entry = { 0 };
struct hellcreek *hellcreek = ds->priv;
int ret;
+ /* Ignore locked entries */
+ if (is_locked)
+ return 0;
+
dev_dbg(hellcreek->dev, "Add FDB entry for MAC=%pM\n", addr);
mutex_lock(&hellcreek->reg_lock);
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index e03ff1f267bb..426b9ea668da 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -1190,10 +1190,15 @@ static void lan9303_port_fast_age(struct dsa_switch *ds, int port)
static int lan9303_port_fdb_add(struct dsa_switch *ds, int port,
const unsigned char *addr, u16 vid,
+ bool is_locked,
struct dsa_db db)
{
struct lan9303 *chip = ds->priv;
+ /* Ignore locked entries */
+ if (is_locked)
+ return 0;
+
dev_dbg(chip->dev, "%s(%d, %pM, %d)\n", __func__, port, addr, vid);
if (vid)
return -EOPNOTSUPP;
diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 8af4def38a98..4da7186935f4 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -1399,8 +1399,13 @@ static int gswip_port_fdb(struct dsa_switch *ds, int port,
static int gswip_port_fdb_add(struct dsa_switch *ds, int port,
const unsigned char *addr, u16 vid,
+ bool is_locked,
struct dsa_db db)
{
+ /* Ignore locked entries */
+ if (is_locked)
+ return 0;
+
return gswip_port_fdb(ds, port, addr, vid, true);
}
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index ab40b700cf1a..c177eb97a072 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -481,6 +481,7 @@ static int ksz9477_port_vlan_del(struct dsa_switch *ds, int port,
static int ksz9477_port_fdb_add(struct dsa_switch *ds, int port,
const unsigned char *addr, u16 vid,
+ bool is_locked,
struct dsa_db db)
{
struct ksz_device *dev = ds->priv;
@@ -488,6 +489,10 @@ static int ksz9477_port_fdb_add(struct dsa_switch *ds, int port,
u32 data;
int ret = 0;
+ /* Ignore locked entries */
+ if (is_locked)
+ return ret;
+
mutex_lock(&dev->alu_mutex);
/* find any entry with mac & vid */
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 2b02d823d497..cbf922cf425e 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1356,12 +1356,17 @@ mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
static int
mt7530_port_fdb_add(struct dsa_switch *ds, int port,
const unsigned char *addr, u16 vid,
+ bool is_locked,
struct dsa_db db)
{
struct mt7530_priv *priv = ds->priv;
int ret;
u8 port_mask = BIT(port);
+ /* Ignore locked entries */
+ if (is_locked)
+ return 0;
+
mutex_lock(&priv->reg_mutex);
mt7530_fdb_write(priv, vid, port_mask, addr, -1, STATIC_ENT);
ret = mt7530_fdb_cmd(priv, MT7530_FDB_WRITE, NULL);
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 5d2c57a7c708..d134153ef023 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2717,11 +2717,16 @@ static int mv88e6xxx_vlan_msti_set(struct dsa_switch *ds,
static int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
const unsigned char *addr, u16 vid,
+ bool is_locked,
struct dsa_db db)
{
struct mv88e6xxx_chip *chip = ds->priv;
int err;
+ /* Ignore locked entries */
+ if (is_locked)
+ return 0;
+
mv88e6xxx_reg_lock(chip);
err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid,
MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC);
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 3e07dc39007a..00ba495a7ab5 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -695,12 +695,17 @@ static int felix_fdb_dump(struct dsa_switch *ds, int port,
static int felix_fdb_add(struct dsa_switch *ds, int port,
const unsigned char *addr, u16 vid,
+ bool is_locked,
struct dsa_db db)
{
struct net_device *bridge_dev = felix_classify_db(db);
struct dsa_port *dp = dsa_to_port(ds, port);
struct ocelot *ocelot = ds->priv;
+ /* Ignore locked entries */
+ if (is_locked)
+ return 0;
+
if (IS_ERR(bridge_dev))
return PTR_ERR(bridge_dev);
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 2727d3169c25..6267e94af485 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -2369,11 +2369,16 @@ qca8k_port_fdb_insert(struct qca8k_priv *priv, const u8 *addr,
static int
qca8k_port_fdb_add(struct dsa_switch *ds, int port,
const unsigned char *addr, u16 vid,
+ bool is_locked,
struct dsa_db db)
{
struct qca8k_priv *priv = (struct qca8k_priv *)ds->priv;
u16 port_mask = BIT(port);
+ /* Ignore locked entries */
+ if (is_locked)
+ return 0;
+
return qca8k_port_fdb_insert(priv, addr, port_mask, vid);
}
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 72b6fc1932b5..9b5c2b56d06d 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1802,10 +1802,15 @@ int sja1105pqrs_fdb_del(struct dsa_switch *ds, int port,
static int sja1105_fdb_add(struct dsa_switch *ds, int port,
const unsigned char *addr, u16 vid,
+ bool is_locked,
struct dsa_db db)
{
struct sja1105_private *priv = ds->priv;
+ /* Ignore locked entries */
+ if (is_locked)
+ return 0;
+
if (!vid) {
switch (db.type) {
case DSA_DB_PORT:
diff --git a/include/net/dsa.h b/include/net/dsa.h
index a5a843b2d67d..ebae403ce734 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -1010,6 +1010,7 @@ struct dsa_switch_ops {
*/
int (*port_fdb_add)(struct dsa_switch *ds, int port,
const unsigned char *addr, u16 vid,
+ bool is_locked,
struct dsa_db db);
int (*port_fdb_del)(struct dsa_switch *ds, int port,
const unsigned char *addr, u16 vid,
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 32b1e7ac6373..ea3b9c1acb8c 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -243,7 +243,7 @@ static int dsa_port_do_fdb_add(struct dsa_port *dp, const unsigned char *addr,
/* No need to bother with refcounting for user ports */
if (!(dsa_port_is_cpu(dp) || dsa_port_is_dsa(dp)))
- return ds->ops->port_fdb_add(ds, port, addr, vid, db);
+ return ds->ops->port_fdb_add(ds, port, addr, vid, is_locked, db);
mutex_lock(&dp->addr_lists_lock);
@@ -259,7 +259,7 @@ static int dsa_port_do_fdb_add(struct dsa_port *dp, const unsigned char *addr,
goto out;
}
- err = ds->ops->port_fdb_add(ds, port, addr, vid, db);
+ err = ds->ops->port_fdb_add(ds, port, addr, vid, is_locked, db);
if (err) {
kfree(a);
goto out;
--
2.30.2
Hi Hans,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net/master]
[also build test ERROR on shuah-kselftest/next linus/master v5.19-rc5]
[cannot apply to net-next/master next-20220707]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Hans-Schultz/Extend-locked-port-feature-with-FDB-locked-flag-MAC-Auth-MAB/20220707-233246
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 07266d066301b97ad56a693f81b29b7ced429b27
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220708/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/ebd598d7ea6c015001489c4293da887763491086
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Hans-Schultz/Extend-locked-port-feature-with-FDB-locked-flag-MAC-Auth-MAB/20220707-233246
git checkout ebd598d7ea6c015001489c4293da887763491086
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/net/dsa/sja1105/sja1105_main.c: In function 'sja1105_mdb_add':
>> drivers/net/dsa/sja1105/sja1105_main.c:1952:63: error: incompatible type for argument 5 of 'sja1105_fdb_add'
1952 | return sja1105_fdb_add(ds, port, mdb->addr, mdb->vid, db);
| ^~
| |
| struct dsa_db
drivers/net/dsa/sja1105/sja1105_main.c:1805:33: note: expected 'bool' {aka '_Bool'} but argument is of type 'struct dsa_db'
1805 | bool is_locked,
| ~~~~~^~~~~~~~~
>> drivers/net/dsa/sja1105/sja1105_main.c:1952:16: error: too few arguments to function 'sja1105_fdb_add'
1952 | return sja1105_fdb_add(ds, port, mdb->addr, mdb->vid, db);
| ^~~~~~~~~~~~~~~
drivers/net/dsa/sja1105/sja1105_main.c:1803:12: note: declared here
1803 | static int sja1105_fdb_add(struct dsa_switch *ds, int port,
| ^~~~~~~~~~~~~~~
drivers/net/dsa/sja1105/sja1105_main.c:1953:1: error: control reaches end of non-void function [-Werror=return-type]
1953 | }
| ^
cc1: some warnings being treated as errors
vim +/sja1105_fdb_add +1952 drivers/net/dsa/sja1105/sja1105_main.c
5126ec72a094bd3 Vladimir Oltean 2021-08-08 1947
a52b2da778fc93e Vladimir Oltean 2021-01-09 1948 static int sja1105_mdb_add(struct dsa_switch *ds, int port,
c26933639b5402c Vladimir Oltean 2022-02-25 1949 const struct switchdev_obj_port_mdb *mdb,
c26933639b5402c Vladimir Oltean 2022-02-25 1950 struct dsa_db db)
291d1e72b756424 Vladimir Oltean 2019-05-02 1951 {
c26933639b5402c Vladimir Oltean 2022-02-25 @1952 return sja1105_fdb_add(ds, port, mdb->addr, mdb->vid, db);
291d1e72b756424 Vladimir Oltean 2019-05-02 1953 }
291d1e72b756424 Vladimir Oltean 2019-05-02 1954
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Hans,
On Thu, Jul 07, 2022 at 05:29:27PM +0200, Hans Schultz wrote:
> Ignore locked fdb entries coming in on all drivers.
>
> Signed-off-by: Hans Schultz <[email protected]>
> ---
A good patch should have a reason for the change in the commit message.
This has no reason because there is no reason.
Think about it, you've said it yourself in patch 1:
| Only the kernel can set this FDB entry flag, while userspace can read
| the flag and remove it by replacing or deleting the FDB entry.
So if user space will never add locked FDB entries to the bridge,
then FDB entries with is_locked=true are never transported using
SWITCHDEV_FDB_ADD_TO_DEVICE to drivers, and so, there is no reason at
all to pass is_locked to drivers, just for them to ignore something that
won't appear.
You just need this for SWITCHDEV_FDB_ADD_TO_BRIDGE, so please keep it
only in those code paths, and remove it from net/dsa/slave.c as well.
> drivers/net/dsa/b53/b53_common.c | 5 +++++
> drivers/net/dsa/b53/b53_priv.h | 1 +
> drivers/net/dsa/hirschmann/hellcreek.c | 5 +++++
> drivers/net/dsa/lan9303-core.c | 5 +++++
> drivers/net/dsa/lantiq_gswip.c | 5 +++++
> drivers/net/dsa/microchip/ksz9477.c | 5 +++++
> drivers/net/dsa/mt7530.c | 5 +++++
> drivers/net/dsa/mv88e6xxx/chip.c | 5 +++++
> drivers/net/dsa/ocelot/felix.c | 5 +++++
> drivers/net/dsa/qca8k.c | 5 +++++
> drivers/net/dsa/sja1105/sja1105_main.c | 5 +++++
> include/net/dsa.h | 1 +
> net/dsa/switch.c | 4 ++--
> 13 files changed, 54 insertions(+), 2 deletions(-)
On Fri, Jul 08, 2022 at 11:06:24AM +0200, [email protected] wrote:
> On 2022-07-08 10:49, Vladimir Oltean wrote:
> > Hi Hans,
> >
> > On Thu, Jul 07, 2022 at 05:29:27PM +0200, Hans Schultz wrote:
> > > Ignore locked fdb entries coming in on all drivers.
> > >
> > > Signed-off-by: Hans Schultz <[email protected]>
> > > ---
> >
> > A good patch should have a reason for the change in the commit message.
> > This has no reason because there is no reason.
> >
> > Think about it, you've said it yourself in patch 1:
> >
> > | Only the kernel can set this FDB entry flag, while userspace can read
> > | the flag and remove it by replacing or deleting the FDB entry.
> >
> > So if user space will never add locked FDB entries to the bridge,
> > then FDB entries with is_locked=true are never transported using
> > SWITCHDEV_FDB_ADD_TO_DEVICE to drivers, and so, there is no reason at
> > all to pass is_locked to drivers, just for them to ignore something that
> > won't appear.
>
> Correct me if I am wrong, but since the bridge can add locked entries, and
> the ensuring fdb update will create a SWITCHDEV_FDB_ADD_TO_DEVICE, those
> entries
> should reach the driver. The policy to ignore those in the driver can be
> seen as either the right thing to do, or not yet implemented.
>
> I remember Ido wrote at a point that the scheme they use is to trap various
> packets to the CPU and let the bridge add the locked entry, which I then
> understand is sent to the driver with a SWITCHDEV_FDB_ADD_TO_DEVICE event.
Well, yes, but if I'm correct, the bridge right now can't create locked
FDB entries, so is_locked will always be false in the ADD_TO_DEVICE
direction.
When the possibility for it to be true will exist, _all_ switchdev
drivers will need to be updated to ignore that (mlxsw, cpss, ocelot,
rocker, prestera, etc etc), not just DSA. And you don't need to
propagate the is_locked flag to all individual DSA sub-drivers when none
care about is_locked in the ADD_TO_DEVICE direction, you can just ignore
within DSA until needed otherwise.
> >
> > You just need this for SWITCHDEV_FDB_ADD_TO_BRIDGE, so please keep it
> > only in those code paths, and remove it from net/dsa/slave.c as well.
> >
> > > drivers/net/dsa/b53/b53_common.c | 5 +++++
> > > drivers/net/dsa/b53/b53_priv.h | 1 +
> > > drivers/net/dsa/hirschmann/hellcreek.c | 5 +++++
> > > drivers/net/dsa/lan9303-core.c | 5 +++++
> > > drivers/net/dsa/lantiq_gswip.c | 5 +++++
> > > drivers/net/dsa/microchip/ksz9477.c | 5 +++++
> > > drivers/net/dsa/mt7530.c | 5 +++++
> > > drivers/net/dsa/mv88e6xxx/chip.c | 5 +++++
> > > drivers/net/dsa/ocelot/felix.c | 5 +++++
> > > drivers/net/dsa/qca8k.c | 5 +++++
> > > drivers/net/dsa/sja1105/sja1105_main.c | 5 +++++
> > > include/net/dsa.h | 1 +
> > > net/dsa/switch.c | 4 ++--
> > > 13 files changed, 54 insertions(+), 2 deletions(-)
On 2022-07-08 11:15, Vladimir Oltean wrote:
>
> Well, yes, but if I'm correct, the bridge right now can't create locked
> FDB entries, so is_locked will always be false in the ADD_TO_DEVICE
> direction.
The bridge can create locked FDB entries with this patch set. That is
part of the idea that a SW bridge will have the same features as the HW
version, so first I made it in the bridge.
>
> When the possibility for it to be true will exist, _all_ switchdev
> drivers will need to be updated to ignore that (mlxsw, cpss, ocelot,
> rocker, prestera, etc etc), not just DSA. And you don't need to
> propagate the is_locked flag to all individual DSA sub-drivers when
> none
> care about is_locked in the ADD_TO_DEVICE direction, you can just
> ignore
> within DSA until needed otherwise.
>
On 2022-07-08 10:49, Vladimir Oltean wrote:
> Hi Hans,
>
> On Thu, Jul 07, 2022 at 05:29:27PM +0200, Hans Schultz wrote:
>> Ignore locked fdb entries coming in on all drivers.
>>
>> Signed-off-by: Hans Schultz <[email protected]>
>> ---
>
> A good patch should have a reason for the change in the commit message.
> This has no reason because there is no reason.
>
> Think about it, you've said it yourself in patch 1:
>
> | Only the kernel can set this FDB entry flag, while userspace can read
> | the flag and remove it by replacing or deleting the FDB entry.
>
> So if user space will never add locked FDB entries to the bridge,
> then FDB entries with is_locked=true are never transported using
> SWITCHDEV_FDB_ADD_TO_DEVICE to drivers, and so, there is no reason at
> all to pass is_locked to drivers, just for them to ignore something
> that
> won't appear.
Correct me if I am wrong, but since the bridge can add locked entries,
and
the ensuring fdb update will create a SWITCHDEV_FDB_ADD_TO_DEVICE, those
entries
should reach the driver. The policy to ignore those in the driver can be
seen as either the right thing to do, or not yet implemented.
I remember Ido wrote at a point that the scheme they use is to trap
various
packets to the CPU and let the bridge add the locked entry, which I then
understand is sent to the driver with a SWITCHDEV_FDB_ADD_TO_DEVICE
event.
>
> You just need this for SWITCHDEV_FDB_ADD_TO_BRIDGE, so please keep it
> only in those code paths, and remove it from net/dsa/slave.c as well.
>
>> drivers/net/dsa/b53/b53_common.c | 5 +++++
>> drivers/net/dsa/b53/b53_priv.h | 1 +
>> drivers/net/dsa/hirschmann/hellcreek.c | 5 +++++
>> drivers/net/dsa/lan9303-core.c | 5 +++++
>> drivers/net/dsa/lantiq_gswip.c | 5 +++++
>> drivers/net/dsa/microchip/ksz9477.c | 5 +++++
>> drivers/net/dsa/mt7530.c | 5 +++++
>> drivers/net/dsa/mv88e6xxx/chip.c | 5 +++++
>> drivers/net/dsa/ocelot/felix.c | 5 +++++
>> drivers/net/dsa/qca8k.c | 5 +++++
>> drivers/net/dsa/sja1105/sja1105_main.c | 5 +++++
>> include/net/dsa.h | 1 +
>> net/dsa/switch.c | 4 ++--
>> 13 files changed, 54 insertions(+), 2 deletions(-)
On 2022-07-08 11:15, Vladimir Oltean wrote:
> When the possibility for it to be true will exist, _all_ switchdev
> drivers will need to be updated to ignore that (mlxsw, cpss, ocelot,
> rocker, prestera, etc etc), not just DSA. And you don't need to
> propagate the is_locked flag to all individual DSA sub-drivers when
> none
> care about is_locked in the ADD_TO_DEVICE direction, you can just
> ignore
> within DSA until needed otherwise.
>
Maybe I have it wrong, but I think that Ido requested me to send it to
all the drivers, and have them ignore entries with is_locked=true ...
On Fri, Jul 08, 2022 at 11:50:33AM +0200, [email protected] wrote:
> On 2022-07-08 11:15, Vladimir Oltean wrote:
> > When the possibility for it to be true will exist, _all_ switchdev
> > drivers will need to be updated to ignore that (mlxsw, cpss, ocelot,
> > rocker, prestera, etc etc), not just DSA. And you don't need to
> > propagate the is_locked flag to all individual DSA sub-drivers when none
> > care about is_locked in the ADD_TO_DEVICE direction, you can just ignore
> > within DSA until needed otherwise.
> >
>
> Maybe I have it wrong, but I think that Ido requested me to send it to all
> the drivers, and have them ignore entries with is_locked=true ...
I don't think Ido requested you to ignore is_locked from all DSA
drivers, but instead from all switchdev drivers maybe. Quite different.
In any case I'm going to take a look at this patch set more closely and
run the selftest on my Marvell switch, but I can't do this today
unfortunately. I'll return with more comments.
On 2022-07-08 13:56, Vladimir Oltean wrote:
> On Fri, Jul 08, 2022 at 11:50:33AM +0200, [email protected]
> wrote:
>> On 2022-07-08 11:15, Vladimir Oltean wrote:
>> > When the possibility for it to be true will exist, _all_ switchdev
>> > drivers will need to be updated to ignore that (mlxsw, cpss, ocelot,
>> > rocker, prestera, etc etc), not just DSA. And you don't need to
>> > propagate the is_locked flag to all individual DSA sub-drivers when none
>> > care about is_locked in the ADD_TO_DEVICE direction, you can just ignore
>> > within DSA until needed otherwise.
>> >
>>
>> Maybe I have it wrong, but I think that Ido requested me to send it to
>> all
>> the drivers, and have them ignore entries with is_locked=true ...
>
> I don't think Ido requested you to ignore is_locked from all DSA
> drivers, but instead from all switchdev drivers maybe. Quite different.
So without changing the signature on port_fdb_add(). If that is to avoid
changing that signature, which needs to be changed anyhow for any
switchcore driver to act on it, then my next patch set will change the
signarure also as it is needed for creating dynamic ATU entries from
userspace, which is needed to make the whole thing complete.
As it is already done (with the is_locked to the drivers) and needed for
future application, I would like Ido to comment on it before I take
action.
>
> In any case I'm going to take a look at this patch set more closely and
> run the selftest on my Marvell switch, but I can't do this today
> unfortunately. I'll return with more comments.
Yes :-)
Hi Hans,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net/master]
[also build test ERROR on shuah-kselftest/next linus/master v5.19-rc5]
[cannot apply to net-next/master next-20220708]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Hans-Schultz/Extend-locked-port-feature-with-FDB-locked-flag-MAC-Auth-MAB/20220707-233246
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 07266d066301b97ad56a693f81b29b7ced429b27
config: i386-randconfig-a013 (https://download.01.org/0day-ci/archive/20220709/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 562c3467a6738aa89203f72fc1d1343e5baadf3c)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/ebd598d7ea6c015001489c4293da887763491086
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Hans-Schultz/Extend-locked-port-feature-with-FDB-locked-flag-MAC-Auth-MAB/20220707-233246
git checkout ebd598d7ea6c015001489c4293da887763491086
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/net/dsa/sja1105/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/net/dsa/sja1105/sja1105_main.c:1952:58: error: too few arguments to function call, expected 6, have 5
return sja1105_fdb_add(ds, port, mdb->addr, mdb->vid, db);
~~~~~~~~~~~~~~~ ^
drivers/net/dsa/sja1105/sja1105_main.c:1803:12: note: 'sja1105_fdb_add' declared here
static int sja1105_fdb_add(struct dsa_switch *ds, int port,
^
1 error generated.
vim +1952 drivers/net/dsa/sja1105/sja1105_main.c
5126ec72a094bd3 Vladimir Oltean 2021-08-08 1947
a52b2da778fc93e Vladimir Oltean 2021-01-09 1948 static int sja1105_mdb_add(struct dsa_switch *ds, int port,
c26933639b5402c Vladimir Oltean 2022-02-25 1949 const struct switchdev_obj_port_mdb *mdb,
c26933639b5402c Vladimir Oltean 2022-02-25 1950 struct dsa_db db)
291d1e72b756424 Vladimir Oltean 2019-05-02 1951 {
c26933639b5402c Vladimir Oltean 2022-02-25 @1952 return sja1105_fdb_add(ds, port, mdb->addr, mdb->vid, db);
291d1e72b756424 Vladimir Oltean 2019-05-02 1953 }
291d1e72b756424 Vladimir Oltean 2019-05-02 1954
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Fri, Jul 08, 2022 at 02:34:25PM +0200, [email protected] wrote:
> On 2022-07-08 13:56, Vladimir Oltean wrote:
> > On Fri, Jul 08, 2022 at 11:50:33AM +0200, [email protected]
> > wrote:
> > > On 2022-07-08 11:15, Vladimir Oltean wrote:
> > > > When the possibility for it to be true will exist, _all_ switchdev
> > > > drivers will need to be updated to ignore that (mlxsw, cpss, ocelot,
> > > > rocker, prestera, etc etc), not just DSA. And you don't need to
> > > > propagate the is_locked flag to all individual DSA sub-drivers when none
> > > > care about is_locked in the ADD_TO_DEVICE direction, you can just ignore
> > > > within DSA until needed otherwise.
> > > >
> > >
> > > Maybe I have it wrong, but I think that Ido requested me to send it
> > > to all
> > > the drivers, and have them ignore entries with is_locked=true ...
> >
> > I don't think Ido requested you to ignore is_locked from all DSA
> > drivers, but instead from all switchdev drivers maybe. Quite different.
>
> So without changing the signature on port_fdb_add(). If that is to avoid
> changing that signature, which needs to be changed anyhow for any switchcore
> driver to act on it, then my next patch set will change the signarure also
> as it is needed for creating dynamic ATU entries from userspace, which is
> needed to make the whole thing complete.
>
> As it is already done (with the is_locked to the drivers) and needed for
> future application, I would like Ido to comment on it before I take action.
It's related to my reply here [1]. AFAICT, we have two classes of device
drivers:
1. Drivers like mv88e6xxx that report locked entries to the bridge
driver via 'SWITCHDEV_FDB_ADD_TO_BRIDGE'.
2. Drivers like mlxsw that trap packets that incurred an FDB miss to the
bridge driver. These packets will cause the bridge driver to emit
'SWITCHDEV_FDB_ADD_TO_DEVICE' notifications with the locked flag.
If we can agree that locked entries are only meant to signal to user
space that a certain MAC tried to gain authorization and that the bridge
should ignore them while forwarding, then there is no point in
generating the 'SWITCHDEV_FDB_ADD_TO_DEVICE' notifications. We should
teach the bridge driver to suppress these so that there is no need to
patch all the device drivers.
[1] https://lore.kernel.org/netdev/YsqLyxTRtUjzDj6D@shredder/
>
> >
> > In any case I'm going to take a look at this patch set more closely and
> > run the selftest on my Marvell switch, but I can't do this today
> > unfortunately. I'll return with more comments.
>
> Yes :-)
On 2022-07-10 10:35, Ido Schimmel wrote:
> On Fri, Jul 08, 2022 at 02:34:25PM +0200, [email protected]
> wrote:
>> On 2022-07-08 13:56, Vladimir Oltean wrote:
>> > On Fri, Jul 08, 2022 at 11:50:33AM +0200, [email protected]
>> > wrote:
>> > > On 2022-07-08 11:15, Vladimir Oltean wrote:
>> > > > When the possibility for it to be true will exist, _all_ switchdev
>> > > > drivers will need to be updated to ignore that (mlxsw, cpss, ocelot,
>> > > > rocker, prestera, etc etc), not just DSA. And you don't need to
>> > > > propagate the is_locked flag to all individual DSA sub-drivers when none
>> > > > care about is_locked in the ADD_TO_DEVICE direction, you can just ignore
>> > > > within DSA until needed otherwise.
>> > > >
>> > >
>> > > Maybe I have it wrong, but I think that Ido requested me to send it
>> > > to all
>> > > the drivers, and have them ignore entries with is_locked=true ...
>> >
>> > I don't think Ido requested you to ignore is_locked from all DSA
>> > drivers, but instead from all switchdev drivers maybe. Quite different.
>>
>> So without changing the signature on port_fdb_add(). If that is to
>> avoid
>> changing that signature, which needs to be changed anyhow for any
>> switchcore
>> driver to act on it, then my next patch set will change the signarure
>> also
>> as it is needed for creating dynamic ATU entries from userspace, which
>> is
>> needed to make the whole thing complete.
>>
>> As it is already done (with the is_locked to the drivers) and needed
>> for
>> future application, I would like Ido to comment on it before I take
>> action.
>
> It's related to my reply here [1]. AFAICT, we have two classes of
> device
> drivers:
>
> 1. Drivers like mv88e6xxx that report locked entries to the bridge
> driver via 'SWITCHDEV_FDB_ADD_TO_BRIDGE'.
>
> 2. Drivers like mlxsw that trap packets that incurred an FDB miss to
> the
> bridge driver. These packets will cause the bridge driver to emit
> 'SWITCHDEV_FDB_ADD_TO_DEVICE' notifications with the locked flag.
>
> If we can agree that locked entries are only meant to signal to user
> space that a certain MAC tried to gain authorization and that the
> bridge
> should ignore them while forwarding, then there is no point in
> generating the 'SWITCHDEV_FDB_ADD_TO_DEVICE' notifications. We should
> teach the bridge driver to suppress these so that there is no need to
> patch all the device drivers.
I do not know of all about what other switchcores there are and how they
work, but those that I have knowledge of, it has been prudent in
connection with the locked port feature to install Storm Prevention or
zero-DPV (Destination Port Vector) FDB entries. I would think that that
should be the case for other switchcores too.
Those entries cannot normally be installed from userspace (of course
special tools can do anything).
But if the decision is to drop locked entries at the DSA layer, I can do
that. I just want to ensure that all considerations have been taken.
>
> [1] https://lore.kernel.org/netdev/YsqLyxTRtUjzDj6D@shredder/
>
>>
>> >
>> > In any case I'm going to take a look at this patch set more closely and
>> > run the selftest on my Marvell switch, but I can't do this today
>> > unfortunately. I'll return with more comments.
>>
>> Yes :-)
On Wed, Jul 13, 2022 at 09:09:58AM +0200, [email protected] wrote:
> On 2022-07-10 10:35, Ido Schimmel wrote:
> > On Fri, Jul 08, 2022 at 02:34:25PM +0200, [email protected]
> > wrote:
> > > On 2022-07-08 13:56, Vladimir Oltean wrote:
> > > > On Fri, Jul 08, 2022 at 11:50:33AM +0200, [email protected]
> > > > wrote:
> > > > > On 2022-07-08 11:15, Vladimir Oltean wrote:
> > > > > > When the possibility for it to be true will exist, _all_ switchdev
> > > > > > drivers will need to be updated to ignore that (mlxsw, cpss, ocelot,
> > > > > > rocker, prestera, etc etc), not just DSA. And you don't need to
> > > > > > propagate the is_locked flag to all individual DSA sub-drivers when none
> > > > > > care about is_locked in the ADD_TO_DEVICE direction, you can just ignore
> > > > > > within DSA until needed otherwise.
> > > > > >
> > > > >
> > > > > Maybe I have it wrong, but I think that Ido requested me to send it
> > > > > to all
> > > > > the drivers, and have them ignore entries with is_locked=true ...
> > > >
> > > > I don't think Ido requested you to ignore is_locked from all DSA
> > > > drivers, but instead from all switchdev drivers maybe. Quite different.
> > >
> > > So without changing the signature on port_fdb_add(). If that is to
> > > avoid
> > > changing that signature, which needs to be changed anyhow for any
> > > switchcore
> > > driver to act on it, then my next patch set will change the
> > > signarure also
> > > as it is needed for creating dynamic ATU entries from userspace,
> > > which is
> > > needed to make the whole thing complete.
> > >
> > > As it is already done (with the is_locked to the drivers) and needed
> > > for
> > > future application, I would like Ido to comment on it before I take
> > > action.
> >
> > It's related to my reply here [1]. AFAICT, we have two classes of device
> > drivers:
> >
> > 1. Drivers like mv88e6xxx that report locked entries to the bridge
> > driver via 'SWITCHDEV_FDB_ADD_TO_BRIDGE'.
> >
> > 2. Drivers like mlxsw that trap packets that incurred an FDB miss to the
> > bridge driver. These packets will cause the bridge driver to emit
> > 'SWITCHDEV_FDB_ADD_TO_DEVICE' notifications with the locked flag.
> >
> > If we can agree that locked entries are only meant to signal to user
> > space that a certain MAC tried to gain authorization and that the bridge
> > should ignore them while forwarding, then there is no point in
> > generating the 'SWITCHDEV_FDB_ADD_TO_DEVICE' notifications. We should
> > teach the bridge driver to suppress these so that there is no need to
> > patch all the device drivers.
>
> I do not know of all about what other switchcores there are and how they
> work, but those that I have knowledge of, it has been prudent in connection
> with the locked port feature to install Storm Prevention or zero-DPV
> (Destination Port Vector) FDB entries.
What are "Storm Prevention" and "zero-DPV" FDB entries?
> I would think that that should be the case for other switchcores too.
> Those entries cannot normally be installed from userspace (of course special
> tools can do anything).
>
> But if the decision is to drop locked entries at the DSA layer, I can do
> that. I just want to ensure that all considerations have been taken.
There is no decision that I'm aware of. I'm simply trying to understand
how FDB entries that have 'BR_FDB_ENTRY_LOCKED' set are handled in
mv88e6xxx and other devices in this class. We have at least three
different implementations to consolidate:
1. The bridge driver, pure software forwarding. The locked entry is
dynamically created by the bridge. Packets received via the locked port
with a SA corresponding to the locked entry will be dropped, but will
refresh the entry. On the other hand, packets with a DA corresponding to
the locked entry will be forwarded as known unicast through the locked
port.
2. Hardware implementations like Spectrum that can be programmed to trap
packets that incurred an FDB miss. Like in the first case, the locked
entry is dynamically created by the bridge driver and also aged by it.
Unlike in the first case, since this entry is not present in hardware,
packets with a DA corresponding to the locked entry will be flooded as
unknown unicast.
3. Hardware implementations like mv88e6xxx that fire an interrupt upon
FDB miss. Need your help to understand how the above works there and
why. Specifically, how locked entries are represented in hardware (if at
all) and what is the significance of not installing corresponding
entries in hardware.
>
> >
> > [1] https://lore.kernel.org/netdev/YsqLyxTRtUjzDj6D@shredder/
> >
> > >
> > > >
> > > > In any case I'm going to take a look at this patch set more closely and
> > > > run the selftest on my Marvell switch, but I can't do this today
> > > > unfortunately. I'll return with more comments.
> > >
> > > Yes :-)
On 2022-07-13 14:39, Ido Schimmel wrote:
> On Wed, Jul 13, 2022 at 09:09:58AM +0200, [email protected]
> wrote:
>
> What are "Storm Prevention" and "zero-DPV" FDB entries?
They are both FDB entries that at the HW level drops all packets having
a specific SA, thus using minimum resources.
(thus the name "Storm Prevention" aka, protection against DOS attacks.
We must remember that we operate with CPU based learning.)
>
> There is no decision that I'm aware of. I'm simply trying to understand
> how FDB entries that have 'BR_FDB_ENTRY_LOCKED' set are handled in
> mv88e6xxx and other devices in this class. We have at least three
> different implementations to consolidate:
>
> 1. The bridge driver, pure software forwarding. The locked entry is
> dynamically created by the bridge. Packets received via the locked port
> with a SA corresponding to the locked entry will be dropped, but will
> refresh the entry. On the other hand, packets with a DA corresponding
> to
> the locked entry will be forwarded as known unicast through the locked
> port.
>
> 2. Hardware implementations like Spectrum that can be programmed to
> trap
> packets that incurred an FDB miss. Like in the first case, the locked
> entry is dynamically created by the bridge driver and also aged by it.
> Unlike in the first case, since this entry is not present in hardware,
> packets with a DA corresponding to the locked entry will be flooded as
> unknown unicast.
>
> 3. Hardware implementations like mv88e6xxx that fire an interrupt upon
> FDB miss. Need your help to understand how the above works there and
> why. Specifically, how locked entries are represented in hardware (if
> at
> all) and what is the significance of not installing corresponding
> entries in hardware.
>
With the mv88e6xxx, a miss violation with the SA occurs when there is no
entry. If you then add a normal entry with the SA, the port is open for
that SA of course. The zero-DPV entry is an entry that ensures that
there is no more miss violation interrupts from that SA, while dropping
all entries with the SA.
On Sun, Jul 17, 2022 at 02:21:47PM +0200, [email protected] wrote:
> On 2022-07-13 14:39, Ido Schimmel wrote:
> > On Wed, Jul 13, 2022 at 09:09:58AM +0200, [email protected]
> > wrote:
>
> >
> > What are "Storm Prevention" and "zero-DPV" FDB entries?
>
> They are both FDB entries that at the HW level drops all packets having a
> specific SA, thus using minimum resources.
> (thus the name "Storm Prevention" aka, protection against DOS attacks. We
> must remember that we operate with CPU based learning.)
DPV means Destination Port Vector, and an ATU entry with a DPV of 0
essentially means a FDB entry pointing nowhere, so it will drop the
packet. That's a slight problem with Hans' implementation, the bridge
thinks that the locked FDB entry belongs to port X, but in reality it
matches on all bridged ports (since it matches by FID). FID allocation
in mv88e6xxx is slightly strange, all VLAN-unaware bridge ports,
belonging to any bridge, share the same FID, so the FDB databases are
not exactly isolated from each other.
On 2022-07-17 14:57, Vladimir Oltean wrote:
> On Sun, Jul 17, 2022 at 02:21:47PM +0200, [email protected]
> wrote:
>> On 2022-07-13 14:39, Ido Schimmel wrote:
>> > On Wed, Jul 13, 2022 at 09:09:58AM +0200, [email protected]
>> > wrote:
>>
>> >
>> > What are "Storm Prevention" and "zero-DPV" FDB entries?
>>
>> They are both FDB entries that at the HW level drops all packets
>> having a
>> specific SA, thus using minimum resources.
>> (thus the name "Storm Prevention" aka, protection against DOS attacks.
>> We
>> must remember that we operate with CPU based learning.)
>
> DPV means Destination Port Vector, and an ATU entry with a DPV of 0
> essentially means a FDB entry pointing nowhere, so it will drop the
> packet. That's a slight problem with Hans' implementation, the bridge
> thinks that the locked FDB entry belongs to port X, but in reality it
> matches on all bridged ports (since it matches by FID). FID allocation
> in mv88e6xxx is slightly strange, all VLAN-unaware bridge ports,
> belonging to any bridge, share the same FID, so the FDB databases are
> not exactly isolated from each other.
But if the locked port is vlan aware and has a pvid, it should not block
other ports. Besides the fid will be zero with vlan unaware afaik, and
all with zero fid do not create locked entries.
On Sun, Jul 17, 2022 at 03:09:10PM +0200, [email protected] wrote:
> On 2022-07-17 14:57, Vladimir Oltean wrote:
> > On Sun, Jul 17, 2022 at 02:21:47PM +0200, [email protected]
> > wrote:
> > > On 2022-07-13 14:39, Ido Schimmel wrote:
> > > > On Wed, Jul 13, 2022 at 09:09:58AM +0200, [email protected]
> > > > wrote:
> > >
> > > >
> > > > What are "Storm Prevention" and "zero-DPV" FDB entries?
> > >
> > > They are both FDB entries that at the HW level drops all packets
> > > having a
> > > specific SA, thus using minimum resources.
> > > (thus the name "Storm Prevention" aka, protection against DOS
> > > attacks. We
> > > must remember that we operate with CPU based learning.)
> >
> > DPV means Destination Port Vector, and an ATU entry with a DPV of 0
> > essentially means a FDB entry pointing nowhere, so it will drop the
> > packet. That's a slight problem with Hans' implementation, the bridge
> > thinks that the locked FDB entry belongs to port X, but in reality it
> > matches on all bridged ports (since it matches by FID). FID allocation
> > in mv88e6xxx is slightly strange, all VLAN-unaware bridge ports,
> > belonging to any bridge, share the same FID, so the FDB databases are
> > not exactly isolated from each other.
>
> But if the locked port is vlan aware and has a pvid, it should not block
> other ports.
I don't understand what you want to say by that. It will block all other
packets with the same MAC SA that are classified to the same FID.
In case of VLAN-aware bridges, the mv88e6xxx driver allocates a new FID
for each VID (see mv88e6xxx_atu_new). In other words, if a locked port
is VLAN-aware and has a pvid, then whatever the PVID may be, all ports
in that same VLAN are still blocked in the same way.
> Besides the fid will be zero with vlan unaware afaik, and all with
> zero fid do not create locked entries.
If by 0 you mean 1 (MV88E6XXX_FID_BRIDGED), then you are correct: ports
with FID 0 (MV88E6XXX_FID_STANDALONE) should not create locked FDB
entries, because they are, well, standalone and not bridged.
Again I don't exactly see the relevance though.
On 2022-07-17 15:59, Vladimir Oltean wrote:
> On Sun, Jul 17, 2022 at 03:09:10PM +0200, [email protected]
> wrote:
>> On 2022-07-17 14:57, Vladimir Oltean wrote:
>> > On Sun, Jul 17, 2022 at 02:21:47PM +0200, [email protected]
>> > wrote:
>> > > On 2022-07-13 14:39, Ido Schimmel wrote:
>> > > > On Wed, Jul 13, 2022 at 09:09:58AM +0200, [email protected]
>> > > > wrote:
>> > >
>> > > >
>> > > > What are "Storm Prevention" and "zero-DPV" FDB entries?
>> > >
>> > > They are both FDB entries that at the HW level drops all packets
>> > > having a
>> > > specific SA, thus using minimum resources.
>> > > (thus the name "Storm Prevention" aka, protection against DOS
>> > > attacks. We
>> > > must remember that we operate with CPU based learning.)
>> >
>> > DPV means Destination Port Vector, and an ATU entry with a DPV of 0
>> > essentially means a FDB entry pointing nowhere, so it will drop the
>> > packet. That's a slight problem with Hans' implementation, the bridge
>> > thinks that the locked FDB entry belongs to port X, but in reality it
>> > matches on all bridged ports (since it matches by FID). FID allocation
>> > in mv88e6xxx is slightly strange, all VLAN-unaware bridge ports,
>> > belonging to any bridge, share the same FID, so the FDB databases are
>> > not exactly isolated from each other.
>>
>> But if the locked port is vlan aware and has a pvid, it should not
>> block
>> other ports.
>
> I don't understand what you want to say by that. It will block all
> other
> packets with the same MAC SA that are classified to the same FID.
> In case of VLAN-aware bridges, the mv88e6xxx driver allocates a new FID
> for each VID (see mv88e6xxx_atu_new). In other words, if a locked port
> is VLAN-aware and has a pvid, then whatever the PVID may be, all ports
> in that same VLAN are still blocked in the same way.
Maybe I am just trying to understand the problem you are posing, so
afaics MAC addresses should be unique and having the same MAC address
behind a locked port and a not-locked port seems like a
mis-configuration regardless of vlan setup? As the zero-DPV entry only
blocks the specific SA MAC on a specific vlan, which is behind a locked
port, there shouldn't be any problem...?
If the host behind a locked port starts sending on another vlan than
where it got the first locked entry, another locked entry will occur, as
the locked entries are MAC + vlan.
>
>> Besides the fid will be zero with vlan unaware afaik, and all with
>> zero fid do not create locked entries.
>
> If by 0 you mean 1 (MV88E6XXX_FID_BRIDGED), then you are correct: ports
> with FID 0 (MV88E6XXX_FID_STANDALONE) should not create locked FDB
> entries, because they are, well, standalone and not bridged.
> Again I don't exactly see the relevance though.
On Sun, Jul 17, 2022 at 04:57:50PM +0200, [email protected] wrote:
>
> Maybe I am just trying to understand the problem you are posing, so afaics
> MAC addresses should be unique and having the same MAC address behind a
> locked port and a not-locked port seems like a mis-configuration regardless
> of vlan setup? As the zero-DPV entry only blocks the specific SA MAC on a
> specific vlan, which is behind a locked port, there shouldn't be any
> problem...?
>
> If the host behind a locked port starts sending on another vlan than where
> it got the first locked entry, another locked entry will occur, as the
> locked entries are MAC + vlan.
I don't think it's an invalid configuration, I have a 17-port Marvell
switch which I use as infrastructure to connect my PC with my board farm
and to the Internet. I've cropped 4 out of those 17 ports for use in
selftests, effectively now having 2 bridges (br0 used by the selftests
and br-lan for systemd-networkd).
Currently all the traffic sent and received by the selftests is done
through lan1-lan4, but if I wanted to run some bridge locked port tests
with traffic from my PC, what I'd do is I'd connect a (locked) port from br0
to a port from br-lan, and my PC would thus gain indirect connectivity to the
locked port.
Then I'd send a packet and the switch would create a locked FDB entry
for my PC's MAC address, but that FDB entry would span across the entire
MV88E6XXX_FID_BRIDGED, so practically speaking, it would block my PC's
MAC address from doing anything, including accessing the Internet, i.e.
traffic that has nothing at all to do with the locked port in br0.
That isn't quite ok.
On Sun, Jul 17, 2022 at 02:21:47PM +0200, [email protected] wrote:
> On 2022-07-13 14:39, Ido Schimmel wrote:
> > On Wed, Jul 13, 2022 at 09:09:58AM +0200, [email protected]
> > wrote:
>
> >
> > What are "Storm Prevention" and "zero-DPV" FDB entries?
>
> They are both FDB entries that at the HW level drops all packets having a
> specific SA, thus using minimum resources.
> (thus the name "Storm Prevention" aka, protection against DOS attacks. We
> must remember that we operate with CPU based learning.)
>
> >
> > There is no decision that I'm aware of. I'm simply trying to understand
> > how FDB entries that have 'BR_FDB_ENTRY_LOCKED' set are handled in
> > mv88e6xxx and other devices in this class. We have at least three
> > different implementations to consolidate:
> >
> > 1. The bridge driver, pure software forwarding. The locked entry is
> > dynamically created by the bridge. Packets received via the locked port
> > with a SA corresponding to the locked entry will be dropped, but will
> > refresh the entry. On the other hand, packets with a DA corresponding to
> > the locked entry will be forwarded as known unicast through the locked
> > port.
> >
> > 2. Hardware implementations like Spectrum that can be programmed to trap
> > packets that incurred an FDB miss. Like in the first case, the locked
> > entry is dynamically created by the bridge driver and also aged by it.
> > Unlike in the first case, since this entry is not present in hardware,
> > packets with a DA corresponding to the locked entry will be flooded as
> > unknown unicast.
> >
> > 3. Hardware implementations like mv88e6xxx that fire an interrupt upon
> > FDB miss. Need your help to understand how the above works there and
> > why. Specifically, how locked entries are represented in hardware (if at
> > all) and what is the significance of not installing corresponding
> > entries in hardware.
> >
>
> With the mv88e6xxx, a miss violation with the SA occurs when there is no
> entry. If you then add a normal entry with the SA, the port is open for that
> SA of course.
Good
> The zero-DPV entry is an entry that ensures that there is no more miss
> violation interrupts from that SA, while dropping all entries with the
> SA.
Few questions:
1. Is it correct to think of this entry as an entry pointing to a
special /dev/null port?
2. After installing this entry, you no longer get miss violation
interrupts because packets with this SA incur a mismatch violation
(src_port != /dev/null) and therefore discarded in hardware?
3. What happens to packets with a DA matching the zero-DPV entry, are
they also discarded in hardware? If so, here we differ from the bridge
driver implementation where such packets will be forwarded according to
the locked entry and egress the locked port
4. The reason for installing this entry is to suppress further miss
violation interrupts?
5. If not replaced, will this entry always age out after the ageing
time? Not sure what can refresh it given that traffic does not ingress
from the /dev/null port.
Thanks
On 2022-07-17 17:08, Vladimir Oltean wrote:
> On Sun, Jul 17, 2022 at 04:57:50PM +0200, [email protected]
> wrote:
>>
>> Maybe I am just trying to understand the problem you are posing, so
>> afaics
>> MAC addresses should be unique and having the same MAC address behind
>> a
>> locked port and a not-locked port seems like a mis-configuration
>> regardless
>> of vlan setup? As the zero-DPV entry only blocks the specific SA MAC
>> on a
>> specific vlan, which is behind a locked port, there shouldn't be any
>> problem...?
>>
>> If the host behind a locked port starts sending on another vlan than
>> where
>> it got the first locked entry, another locked entry will occur, as the
>> locked entries are MAC + vlan.
>
> I don't think it's an invalid configuration, I have a 17-port Marvell
> switch which I use as infrastructure to connect my PC with my board
> farm
> and to the Internet. I've cropped 4 out of those 17 ports for use in
> selftests, effectively now having 2 bridges (br0 used by the selftests
> and br-lan for systemd-networkd).
>
> Currently all the traffic sent and received by the selftests is done
> through lan1-lan4, but if I wanted to run some bridge locked port tests
> with traffic from my PC, what I'd do is I'd connect a (locked) port
> from br0
> to a port from br-lan, and my PC would thus gain indirect connectivity
> to the
> locked port.
>
> Then I'd send a packet and the switch would create a locked FDB entry
> for my PC's MAC address, but that FDB entry would span across the
> entire
> MV88E6XXX_FID_BRIDGED, so practically speaking, it would block my PC's
> MAC address from doing anything, including accessing the Internet, i.e.
> traffic that has nothing at all to do with the locked port in br0.
> That isn't quite ok.
Okay, I see the problem you refer to. I think that we have to accept
some limitations unless you think that just zeroing the specific port
bit in the DPV would be a better solution, and there wouldn't be caveats
with that besides having to do a FDB search etc to get the correct DPV
if I am not too mistaken.
Also trunk ports is a limitation as that is not supported in this
implementation.
On 2022-07-17 17:20, Ido Schimmel wrote:
> On Sun, Jul 17, 2022 at 02:21:47PM +0200, [email protected]
> wrote:
>> On 2022-07-13 14:39, Ido Schimmel wrote:
>> > On Wed, Jul 13, 2022 at 09:09:58AM +0200, [email protected]
>> > wrote:
>>
>> >
>> > What are "Storm Prevention" and "zero-DPV" FDB entries?
>>
>> They are both FDB entries that at the HW level drops all packets
>> having a
>> specific SA, thus using minimum resources.
>> (thus the name "Storm Prevention" aka, protection against DOS attacks.
>> We
>> must remember that we operate with CPU based learning.)
>>
>> >
>> > There is no decision that I'm aware of. I'm simply trying to understand
>> > how FDB entries that have 'BR_FDB_ENTRY_LOCKED' set are handled in
>> > mv88e6xxx and other devices in this class. We have at least three
>> > different implementations to consolidate:
>> >
>> > 1. The bridge driver, pure software forwarding. The locked entry is
>> > dynamically created by the bridge. Packets received via the locked port
>> > with a SA corresponding to the locked entry will be dropped, but will
>> > refresh the entry. On the other hand, packets with a DA corresponding to
>> > the locked entry will be forwarded as known unicast through the locked
>> > port.
>> >
>> > 2. Hardware implementations like Spectrum that can be programmed to trap
>> > packets that incurred an FDB miss. Like in the first case, the locked
>> > entry is dynamically created by the bridge driver and also aged by it.
>> > Unlike in the first case, since this entry is not present in hardware,
>> > packets with a DA corresponding to the locked entry will be flooded as
>> > unknown unicast.
>> >
>> > 3. Hardware implementations like mv88e6xxx that fire an interrupt upon
>> > FDB miss. Need your help to understand how the above works there and
>> > why. Specifically, how locked entries are represented in hardware (if at
>> > all) and what is the significance of not installing corresponding
>> > entries in hardware.
>> >
>>
>> With the mv88e6xxx, a miss violation with the SA occurs when there is
>> no
>> entry. If you then add a normal entry with the SA, the port is open
>> for that
>> SA of course.
>
> Good
>
>> The zero-DPV entry is an entry that ensures that there is no more miss
>> violation interrupts from that SA, while dropping all entries with the
>> SA.
>
> Few questions:
>
> 1. Is it correct to think of this entry as an entry pointing to a
> special /dev/null port?
I guess you can think of it like that. It's internal to the chipset how
it does it.
>
> 2. After installing this entry, you no longer get miss violation
> interrupts because packets with this SA incur a mismatch violation
> (src_port != /dev/null) and therefore discarded in hardware?
Yes, and mismatch violations are suppressed in this implementation when
locking the port.
>
> 3. What happens to packets with a DA matching the zero-DPV entry, are
> they also discarded in hardware? If so, here we differ from the bridge
> driver implementation where such packets will be forwarded according to
> the locked entry and egress the locked port
I understand that egress will follow what is setup with regard to UC, MC
and BC, though I haven't tested that. But no replies will get through of
course as long as the port hasn't been opened for the iface behind the
locked port.
>
> 4. The reason for installing this entry is to suppress further miss
> violation interrupts?
Yes, while still HW dropping all ingress packets with the same (SA-mac,
vlan) on the port.
>
> 5. If not replaced, will this entry always age out after the ageing
> time? Not sure what can refresh it given that traffic does not ingress
> from the /dev/null port.
That is where my implementation keeps the entries in a list and removes
them after the bridge timeout using a kernel worker and jiffies.
So by default they age out after approx. 5 min.
>
> Thanks
On Sun, Jul 17, 2022 at 05:53:22PM +0200, [email protected] wrote:
> > 3. What happens to packets with a DA matching the zero-DPV entry, are
> > they also discarded in hardware? If so, here we differ from the bridge
> > driver implementation where such packets will be forwarded according to
> > the locked entry and egress the locked port
>
> I understand that egress will follow what is setup with regard to UC, MC and
> BC, though I haven't tested that. But no replies will get through of course
> as long as the port hasn't been opened for the iface behind the locked port.
Here, should we be rather fixing the software bridge, if the current
behavior is to forward packets towards locked FDB entries?
On Fri, Jul 08, 2022 at 11:50:33AM +0200, [email protected] wrote:
> On 2022-07-08 11:15, Vladimir Oltean wrote:
> > When the possibility for it to be true will exist, _all_ switchdev
> > drivers will need to be updated to ignore that (mlxsw, cpss, ocelot,
> > rocker, prestera, etc etc), not just DSA. And you don't need to
> > propagate the is_locked flag to all individual DSA sub-drivers when none
> > care about is_locked in the ADD_TO_DEVICE direction, you can just ignore
> > within DSA until needed otherwise.
> >
>
> Maybe I have it wrong, but I think that Ido requested me to send it to all
> the drivers, and have them ignore entries with is_locked=true ...
Yes, but re-read my message about what "all the drivers" means.
On Sun, Jul 17, 2022 at 06:10:22PM +0200, [email protected] wrote:
> Okay, I see the problem you refer to. I think that we have to accept some
> limitations unless you think that just zeroing the specific port bit in the
> DPV would be a better solution, and there wouldn't be caveats with that
> besides having to do a FDB search etc to get the correct DPV if I am not too
> mistaken.
No, honestly I believe that what we should do to improve the limitation
is to have proper ATU database separation between one VLAN-unaware
bridge and another (i.e. what is now MV88E6XXX_FID_BRIDGED to be one FID
per bridge).
On Thu, Jul 21, 2022 at 02:59:35PM +0300, Vladimir Oltean wrote:
> On Sun, Jul 17, 2022 at 05:53:22PM +0200, [email protected] wrote:
> > > 3. What happens to packets with a DA matching the zero-DPV entry, are
> > > they also discarded in hardware? If so, here we differ from the bridge
> > > driver implementation where such packets will be forwarded according to
> > > the locked entry and egress the locked port
> >
> > I understand that egress will follow what is setup with regard to UC, MC and
> > BC, though I haven't tested that. But no replies will get through of course
> > as long as the port hasn't been opened for the iface behind the locked port.
>
> Here, should we be rather fixing the software bridge, if the current
> behavior is to forward packets towards locked FDB entries?
I think the bridge needs to be fixed, but not to discard packets. If I
decided to lock a port, it means I do not blindly trust whoever who
is behind the port, but instead want to authorize them first. Since an
unauthorized user is able to create locked FDB entries we need to
carefully define what they mean. I tried looking information about MAB
online, but couldn't find detailed material that answers my questions,
so my answers are based on what I believe is logical, which might be
wrong.
Currently, the bridge will forward packets to a locked entry which
effectively means that an unauthorized host can cause the bridge to
direct packets to it and sniff them. Yes, the host can't send any
packets through the port (while locked) and can't overtake an existing
(unlocked) FDB entry, but it still seems like an odd decision. IMO, the
situation in mv88e6xxx is even worse because there an unauthorized host
can cause packets to a certain DMAC to be blackholed via its zero-DPV
entry.
Another (minor?) issue is that locked entries cannot roam between locked
ports. Lets say that my user space MAB policy is to authorize MAC X if
it appears behind one of the locked ports swp1-swp4. An unauthorized
host behind locked port swp5 can generate packets with SMAC X,
preventing the true owner of this MAC behind swp1 from ever being
authorized.
It seems like the main purpose of these locked entries is to signal to
user space the presence of a certain MAC behind a locked port, but they
should not be able to affect packet forwarding in the bridge, unlike
regular entries.
Regarding a separate knob for MAB, I tend to agree we need it. Otherwise
we cannot control which locked ports are able to populate the FDB with
locked entries. I don't particularly like the fact that we overload an
existing flag ("learning") for that. Any reason not to add an explicit
flag ("mab")? At least with the current implementation, locked entries
cannot roam between locked ports and cannot be refreshed, which differs
from regular learning.
On Thu, Jul 21, 2022 at 04:27:52PM +0300, Ido Schimmel wrote:
> I tried looking information about MAB online, but couldn't find
> detailed material that answers my questions, so my answers are based
> on what I believe is logical, which might be wrong.
I'm kind of in the same situation here.
> Currently, the bridge will forward packets to a locked entry which
> effectively means that an unauthorized host can cause the bridge to
> direct packets to it and sniff them. Yes, the host can't send any
> packets through the port (while locked) and can't overtake an existing
> (unlocked) FDB entry, but it still seems like an odd decision. IMO, the
> situation in mv88e6xxx is even worse because there an unauthorized host
> can cause packets to a certain DMAC to be blackholed via its zero-DPV
> entry.
>
> Another (minor?) issue is that locked entries cannot roam between locked
> ports. Lets say that my user space MAB policy is to authorize MAC X if
> it appears behind one of the locked ports swp1-swp4. An unauthorized
> host behind locked port swp5 can generate packets with SMAC X,
> preventing the true owner of this MAC behind swp1 from ever being
> authorized.
In the mv88e6xxx offload implementation, the locked entries eventually
age out from time to time, practically giving the true owner of the MAC
address another chance every 5 minutes or so. In the pure software
implementation of locked FDB entries I'm not quite sure. It wouldn't
make much sense for the behavior to differ significantly though.
> It seems like the main purpose of these locked entries is to signal to
> user space the presence of a certain MAC behind a locked port, but they
> should not be able to affect packet forwarding in the bridge, unlike
> regular entries.
So essentially what you want is for br_handle_frame_finish() to treat
"dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);" as NULL if
test_bit(BR_FDB_LOCKED, &dst->flags) is true?
> Regarding a separate knob for MAB, I tend to agree we need it. Otherwise
> we cannot control which locked ports are able to populate the FDB with
> locked entries. I don't particularly like the fact that we overload an
> existing flag ("learning") for that. Any reason not to add an explicit
> flag ("mab")? At least with the current implementation, locked entries
> cannot roam between locked ports and cannot be refreshed, which differs
> from regular learning.
Well, assuming we model the software bridge closer to mv88e6xxx (where
locked FDB entries can roam after a certain time), does this change things?
In the software implementation I think it would make sense for them to
be able to roam right away (the age-out interval in mv88e6xxx is just a
compromise between responsiveness to roaming and resistance to DoS).
On Thu, Jul 21, 2022 at 05:20:01PM +0300, Vladimir Oltean wrote:
> On Thu, Jul 21, 2022 at 04:27:52PM +0300, Ido Schimmel wrote:
> > I tried looking information about MAB online, but couldn't find
> > detailed material that answers my questions, so my answers are based
> > on what I believe is logical, which might be wrong.
>
> I'm kind of in the same situation here.
:(
>
> > Currently, the bridge will forward packets to a locked entry which
> > effectively means that an unauthorized host can cause the bridge to
> > direct packets to it and sniff them. Yes, the host can't send any
> > packets through the port (while locked) and can't overtake an existing
> > (unlocked) FDB entry, but it still seems like an odd decision. IMO, the
> > situation in mv88e6xxx is even worse because there an unauthorized host
> > can cause packets to a certain DMAC to be blackholed via its zero-DPV
> > entry.
> >
> > Another (minor?) issue is that locked entries cannot roam between locked
> > ports. Lets say that my user space MAB policy is to authorize MAC X if
> > it appears behind one of the locked ports swp1-swp4. An unauthorized
> > host behind locked port swp5 can generate packets with SMAC X,
> > preventing the true owner of this MAC behind swp1 from ever being
> > authorized.
>
> In the mv88e6xxx offload implementation, the locked entries eventually
> age out from time to time, practically giving the true owner of the MAC
> address another chance every 5 minutes or so. In the pure software
> implementation of locked FDB entries I'm not quite sure. It wouldn't
> make much sense for the behavior to differ significantly though.
From what I can tell, the same happens in software, but this behavior
does not really make sense to me. It differs from how other learned
entries age/roam and can lead to problems such as the one described
above. It is also not documented anywhere, so I can't tell if it's
intentional or an oversight. We need to have a good reason for such a
behavior other than the fact that it appears to conform to the quirks of
one hardware implementation.
>
> > It seems like the main purpose of these locked entries is to signal to
> > user space the presence of a certain MAC behind a locked port, but they
> > should not be able to affect packet forwarding in the bridge, unlike
> > regular entries.
>
> So essentially what you want is for br_handle_frame_finish() to treat
> "dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);" as NULL if
> test_bit(BR_FDB_LOCKED, &dst->flags) is true?
Yes. It's not clear to me why unauthorized hosts should be given the
ability to affect packet forwarding in the bridge through these locked
entries when their primary purpose seems to be notifying user space
about the presence of the MAC. At the very least this should be
explained in the commit message, to indicate that some thought went into
this decision.
>
> > Regarding a separate knob for MAB, I tend to agree we need it. Otherwise
> > we cannot control which locked ports are able to populate the FDB with
> > locked entries. I don't particularly like the fact that we overload an
> > existing flag ("learning") for that. Any reason not to add an explicit
> > flag ("mab")? At least with the current implementation, locked entries
> > cannot roam between locked ports and cannot be refreshed, which differs
> > from regular learning.
>
> Well, assuming we model the software bridge closer to mv88e6xxx (where
> locked FDB entries can roam after a certain time), does this change things?
> In the software implementation I think it would make sense for them to
> be able to roam right away (the age-out interval in mv88e6xxx is just a
> compromise between responsiveness to roaming and resistance to DoS).
Exactly. If this is the best that we can do with mv88e6xxx, then so be
it, but other implementations (software/hardware) do not have the same
limitations and I don't see a reason to bend them.
Regarding "learning" vs. "mab" (or something else), the former is a
well-defined flag available since forever. In 5.18 and 5.19 it can also
be enabled together with "locked" and packets from an unauthorized host
(modulo link-local ones) will not populate the FDB. I prefer not to
change an existing behavior.
From usability point of view, I think a new flag would be easier to
explain than explaining that "learning on" behaves like A or B, based on
whether "locked on" is set. The bridge can also be taught to forbid the
new flag from being set when "locked" is not set.
A user space daemon that wants to try 802.1x and fallback to MAB can
enable both flags or enable "mab" after some timer expires.
On 2022-07-24 13:10, Ido Schimmel wrote:
> On Thu, Jul 21, 2022 at 05:20:01PM +0300, Vladimir Oltean wrote:
>> On Thu, Jul 21, 2022 at 04:27:52PM +0300, Ido Schimmel wrote:
>> > I tried looking information about MAB online, but couldn't find
>> > detailed material that answers my questions, so my answers are based
>> > on what I believe is logical, which might be wrong.
>>
>> I'm kind of in the same situation here.
>
> :(
>
Sorry for being off here a while... here is my best link regarding MAB.
Maybe it can answer some of your questions...
https://www.cisco.com/c/en/us/td/docs/solutions/Enterprise/Security/TrustSec_1-99/MAB/MAB_Dep_Guide.html#wp392522
On 2022-07-24 13:10, Ido Schimmel wrote:
>
>> In the mv88e6xxx offload implementation, the locked entries eventually
>> age out from time to time, practically giving the true owner of the
>> MAC
>> address another chance every 5 minutes or so. In the pure software
>> implementation of locked FDB entries I'm not quite sure. It wouldn't
>> make much sense for the behavior to differ significantly though.
>
> From what I can tell, the same happens in software, but this behavior
> does not really make sense to me. It differs from how other learned
> entries age/roam and can lead to problems such as the one described
> above. It is also not documented anywhere, so I can't tell if it's
> intentional or an oversight. We need to have a good reason for such a
> behavior other than the fact that it appears to conform to the quirks
> of
> one hardware implementation.
>
>>
>> > It seems like the main purpose of these locked entries is to signal to
>> > user space the presence of a certain MAC behind a locked port, but they
>> > should not be able to affect packet forwarding in the bridge, unlike
>> > regular entries.
>>
>> So essentially what you want is for br_handle_frame_finish() to treat
>> "dst = br_fdb_find_rcu(br, eth_hdr(skb)->h_dest, vid);" as NULL if
>> test_bit(BR_FDB_LOCKED, &dst->flags) is true?
>
> Yes. It's not clear to me why unauthorized hosts should be given the
> ability to affect packet forwarding in the bridge through these locked
> entries when their primary purpose seems to be notifying user space
> about the presence of the MAC. At the very least this should be
> explained in the commit message, to indicate that some thought went
> into
> this decision.
>
I guess you are right that the SW setup locked entries can be used to
gain uni-directional traffic through a switch, which should really not
be the case.
In this case I expect the zero-DPV entries to not give this ability,
which is the correct behaviour with MAB IMHO.
>>
>> > Regarding a separate knob for MAB, I tend to agree we need it. Otherwise
>> > we cannot control which locked ports are able to populate the FDB with
>> > locked entries. I don't particularly like the fact that we overload an
>> > existing flag ("learning") for that. Any reason not to add an explicit
>> > flag ("mab")? At least with the current implementation, locked entries
>> > cannot roam between locked ports and cannot be refreshed, which differs
>> > from regular learning.
>>
>> Well, assuming we model the software bridge closer to mv88e6xxx (where
>> locked FDB entries can roam after a certain time), does this change
>> things?
>> In the software implementation I think it would make sense for them to
>> be able to roam right away (the age-out interval in mv88e6xxx is just
>> a
>> compromise between responsiveness to roaming and resistance to DoS).
>
> Exactly. If this is the best that we can do with mv88e6xxx, then so be
> it, but other implementations (software/hardware) do not have the same
> limitations and I don't see a reason to bend them.
>
> Regarding "learning" vs. "mab" (or something else), the former is a
> well-defined flag available since forever. In 5.18 and 5.19 it can also
> be enabled together with "locked" and packets from an unauthorized host
> (modulo link-local ones) will not populate the FDB. I prefer not to
> change an existing behavior.
>
> From usability point of view, I think a new flag would be easier to
> explain than explaining that "learning on" behaves like A or B, based
> on
> whether "locked on" is set. The bridge can also be taught to forbid the
> new flag from being set when "locked" is not set.
>
> A user space daemon that wants to try 802.1x and fallback to MAB can
> enable both flags or enable "mab" after some timer expires.
With this driver it is not really an option to use +learning for a
opt-in for MAB. I think locked port should always have -learning before
locking the port. In fact there is a problem in this implementation with
MAB if -learning is applied after locking the port, as that will disable
MAB, but also refresh and all other violation interrupts.
So I guess I need to disable the learning flag to the driver when the
port is locked, or even from the bridge?
On 2022-07-13 14:39, Ido Schimmel wrote:
>
> What are "Storm Prevention" and "zero-DPV" FDB entries?
>
For the zero-DPV entries, I can summarize:
Since a CPU can become saturated from constant SA Miss Violations from a
denied source, source MAC address are masked by loading a zero-DPV
(Destination Port Vector) entry in the ATU. As the address now appears
in the database it will not cause more Miss Violations. ANY port trying
to send a frame to this unauthorized address is discarded. Any locked
port trying to use this unauthorized address has its frames discarded
too (as the ports SA bit is not set in the ATU entry).
On 2022-07-21 13:59, Vladimir Oltean wrote:
> On Sun, Jul 17, 2022 at 05:53:22PM +0200, [email protected]
> wrote:
>> > 3. What happens to packets with a DA matching the zero-DPV entry, are
>> > they also discarded in hardware? If so, here we differ from the bridge
>> > driver implementation where such packets will be forwarded according to
>> > the locked entry and egress the locked port
>>
>> I understand that egress will follow what is setup with regard to UC,
>> MC and
>> BC, though I haven't tested that. But no replies will get through of
>> course
>> as long as the port hasn't been opened for the iface behind the locked
>> port.
>
> Here, should we be rather fixing the software bridge, if the current
> behavior is to forward packets towards locked FDB entries?
Yes, I think that locked entries should block egress to the respective
hosts behind the locked port, which should be fixed in the bridge.
On Mon, Aug 01, 2022 at 05:33:49PM +0200, [email protected] wrote:
> On 2022-07-13 14:39, Ido Schimmel wrote:
>
> >
> > What are "Storm Prevention" and "zero-DPV" FDB entries?
> >
>
> For the zero-DPV entries, I can summarize:
>
> Since a CPU can become saturated from constant SA Miss Violations from a
> denied source, source MAC address are masked by loading a zero-DPV
> (Destination Port Vector) entry in the ATU. As the address now appears in
> the database it will not cause more Miss Violations. ANY port trying to send
> a frame to this unauthorized address is discarded. Any locked port trying to
> use this unauthorized address has its frames discarded too (as the ports SA
> bit is not set in the ATU entry).
What happens to unlocked ports that have learning enabled and are trying
to use this address as SMAC? AFAICT, at least in the bridge driver, the
locked entry will roam, but will keep the "locked" flag, which is
probably not what we want. Let's see if we can agree on these semantics
for a "locked" entry:
1. It discards packets with matching DMAC, regardless of ingress port. I
read the document [1] you linked to in a different reply and could not
find anything against this approach, so this might be fine or at least
not very significant.
Note that this means that "locked" entries need to be notified to device
drivers so that they will install a matching entry in the HW FDB.
2. It is not refreshed and has ageing enabled. That is, after initial
installation it will be removed by the bridge driver after configured
ageing time unless converted to a regular (unlocked) entry.
I assume this allows you to remove the timer implementation from your
driver and let the bridge driver notify you about the removal of this
entry.
3. With regards to roaming, the entry cannot roam between locked ports
(they need to have learning disabled anyway), but can roam to an
unlocked port, in which case it becomes a regular entry that can roam
and age.
If we agree on these semantics, then I can try to verify that at least
Spectrum can support them (it seems mv88e6xxx can).
P.S. Sorry for the delay, I'm busy with other tasks at the moment.
[1] https://www.cisco.com/c/en/us/td/docs/solutions/Enterprise/Security/TrustSec_1-99/MAB/MAB_Dep_Guide.html#wp392522
On 2022-08-09 11:20, Ido Schimmel wrote:
> On Mon, Aug 01, 2022 at 05:33:49PM +0200, [email protected]
> wrote:
>> On 2022-07-13 14:39, Ido Schimmel wrote:
>>
>> >
>> > What are "Storm Prevention" and "zero-DPV" FDB entries?
>> >
>>
>> For the zero-DPV entries, I can summarize:
>>
>> Since a CPU can become saturated from constant SA Miss Violations from
>> a
>> denied source, source MAC address are masked by loading a zero-DPV
>> (Destination Port Vector) entry in the ATU. As the address now appears
>> in
>> the database it will not cause more Miss Violations. ANY port trying
>> to send
>> a frame to this unauthorized address is discarded. Any locked port
>> trying to
>> use this unauthorized address has its frames discarded too (as the
>> ports SA
>> bit is not set in the ATU entry).
>
> What happens to unlocked ports that have learning enabled and are
> trying
> to use this address as SMAC? AFAICT, at least in the bridge driver, the
> locked entry will roam, but will keep the "locked" flag, which is
> probably not what we want. Let's see if we can agree on these semantics
> for a "locked" entry:
The next version of this will block forwarding to locked entries in the
bridge, so they will behave like the zero-DPV entries.
>
> 1. It discards packets with matching DMAC, regardless of ingress port.
> I
> read the document [1] you linked to in a different reply and could not
> find anything against this approach, so this might be fine or at least
> not very significant.
>
> Note that this means that "locked" entries need to be notified to
> device
> drivers so that they will install a matching entry in the HW FDB.
Okay, so as V4 does (just without the error noted).
>
> 2. It is not refreshed and has ageing enabled. That is, after initial
> installation it will be removed by the bridge driver after configured
> ageing time unless converted to a regular (unlocked) entry.
>
> I assume this allows you to remove the timer implementation from your
> driver and let the bridge driver notify you about the removal of this
> entry.
Okay, but only if the scheme is not so that the driver creates the
locked entries itself, unless you indicate that the driver notifies the
bridge, which then notifies back to the driver and installs the zero-DPV
entry? If not I think the current implementation for the mv88e6xxx is
fine.
>
> 3. With regards to roaming, the entry cannot roam between locked ports
> (they need to have learning disabled anyway), but can roam to an
> unlocked port, in which case it becomes a regular entry that can roam
> and age.
>
> If we agree on these semantics, then I can try to verify that at least
> Spectrum can support them (it seems mv88e6xxx can).
The consensus here is that at least for the mv88e6xxx, learning should
be on and link local learning should be blocked by the userspace setting
you pointed to earlier.
>
> P.S. Sorry for the delay, I'm busy with other tasks at the moment.
I understand :-)
>
> [1]
> https://www.cisco.com/c/en/us/td/docs/solutions/Enterprise/Security/TrustSec_1-99/MAB/MAB_Dep_Guide.html#wp392522
On Tue, Aug 09, 2022 at 10:00:49PM +0200, [email protected] wrote:
> On 2022-08-09 11:20, Ido Schimmel wrote:
> > On Mon, Aug 01, 2022 at 05:33:49PM +0200, [email protected]
> > wrote:
> > > On 2022-07-13 14:39, Ido Schimmel wrote:
> > >
> > > >
> > > > What are "Storm Prevention" and "zero-DPV" FDB entries?
> > > >
> > >
> > > For the zero-DPV entries, I can summarize:
> > >
> > > Since a CPU can become saturated from constant SA Miss Violations
> > > from a
> > > denied source, source MAC address are masked by loading a zero-DPV
> > > (Destination Port Vector) entry in the ATU. As the address now
> > > appears in
> > > the database it will not cause more Miss Violations. ANY port trying
> > > to send
> > > a frame to this unauthorized address is discarded. Any locked port
> > > trying to
> > > use this unauthorized address has its frames discarded too (as the
> > > ports SA
> > > bit is not set in the ATU entry).
> >
> > What happens to unlocked ports that have learning enabled and are trying
> > to use this address as SMAC? AFAICT, at least in the bridge driver, the
> > locked entry will roam, but will keep the "locked" flag, which is
> > probably not what we want. Let's see if we can agree on these semantics
> > for a "locked" entry:
>
> The next version of this will block forwarding to locked entries in the
> bridge, so they will behave like the zero-DPV entries.
I'm talking about roaming, not forwarding. Let's say you have a locked
entry with MAC X pointing to port Y. Now you get a packet with SMAC X
from port Z which is unlocked. Will the FDB entry roam to port Z? I
think it should, but at least in current implementation it seems that
the "locked" flag will not be reset and having locked entries pointing
to an unlocked port looks like a bug.
>
> >
> > 1. It discards packets with matching DMAC, regardless of ingress port. I
> > read the document [1] you linked to in a different reply and could not
> > find anything against this approach, so this might be fine or at least
> > not very significant.
> >
> > Note that this means that "locked" entries need to be notified to device
> > drivers so that they will install a matching entry in the HW FDB.
>
> Okay, so as V4 does (just without the error noted).
>
> >
> > 2. It is not refreshed and has ageing enabled. That is, after initial
> > installation it will be removed by the bridge driver after configured
> > ageing time unless converted to a regular (unlocked) entry.
> >
> > I assume this allows you to remove the timer implementation from your
> > driver and let the bridge driver notify you about the removal of this
> > entry.
>
> Okay, but only if the scheme is not so that the driver creates the locked
> entries itself, unless you indicate that the driver notifies the bridge,
> which then notifies back to the driver and installs the zero-DPV entry? If
> not I think the current implementation for the mv88e6xxx is fine.
I don't see a problem in having the driver notifying the bridge about
the installation of this entry and the bridge notifying the driver that
the entry needs to be removed. It removes complexity from device drivers
like mv88e6xxx and doesn't add extra complexity to the bridge driver.
Actually, there is one complication, 'SWITCHDEV_FDB_ADD_TO_BRIDGE' will
add the locked entry as externally learned, which means the bridge will
not age it. Might need something like this:
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index e7f4fccb6adb..5f73d0b44ed9 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -530,7 +530,8 @@ void br_fdb_cleanup(struct work_struct *work)
unsigned long this_timer = f->updated + delay;
if (test_bit(BR_FDB_STATIC, &f->flags) ||
- test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags)) {
+ (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags) &&
+ !test_bit(BR_FDB_ENTRY_LOCKED, &f->flags))) {
if (test_bit(BR_FDB_NOTIFY, &f->flags)) {
if (time_after(this_timer, now))
work_delay = min(work_delay,
>
> >
> > 3. With regards to roaming, the entry cannot roam between locked ports
> > (they need to have learning disabled anyway), but can roam to an
> > unlocked port, in which case it becomes a regular entry that can roam
> > and age.
> >
> > If we agree on these semantics, then I can try to verify that at least
> > Spectrum can support them (it seems mv88e6xxx can).
>
> The consensus here is that at least for the mv88e6xxx, learning should be on
> and link local learning should be blocked by the userspace setting you
> pointed to earlier.
Why learning needs to be on in the bridge (not mv88e6xxx) driver?
On 2022-08-10 09:21, Ido Schimmel wrote:
> On Tue, Aug 09, 2022 at 10:00:49PM +0200, [email protected]
> wrote:
>> On 2022-08-09 11:20, Ido Schimmel wrote:
>> > On Mon, Aug 01, 2022 at 05:33:49PM +0200, [email protected]
>> > wrote:
>> > > On 2022-07-13 14:39, Ido Schimmel wrote:
>> > >
>> > > >
>> > > > What are "Storm Prevention" and "zero-DPV" FDB entries?
>> > > >
>> > >
>> > > For the zero-DPV entries, I can summarize:
>> > >
>> > > Since a CPU can become saturated from constant SA Miss Violations
>> > > from a
>> > > denied source, source MAC address are masked by loading a zero-DPV
>> > > (Destination Port Vector) entry in the ATU. As the address now
>> > > appears in
>> > > the database it will not cause more Miss Violations. ANY port trying
>> > > to send
>> > > a frame to this unauthorized address is discarded. Any locked port
>> > > trying to
>> > > use this unauthorized address has its frames discarded too (as the
>> > > ports SA
>> > > bit is not set in the ATU entry).
>> >
>> > What happens to unlocked ports that have learning enabled and are trying
>> > to use this address as SMAC? AFAICT, at least in the bridge driver, the
>> > locked entry will roam, but will keep the "locked" flag, which is
>> > probably not what we want. Let's see if we can agree on these semantics
>> > for a "locked" entry:
>>
>> The next version of this will block forwarding to locked entries in
>> the
>> bridge, so they will behave like the zero-DPV entries.
>
> I'm talking about roaming, not forwarding. Let's say you have a locked
> entry with MAC X pointing to port Y. Now you get a packet with SMAC X
> from port Z which is unlocked. Will the FDB entry roam to port Z? I
> think it should, but at least in current implementation it seems that
> the "locked" flag will not be reset and having locked entries pointing
> to an unlocked port looks like a bug.
>
Remember that zero-DPV entries blackhole (mask) the MAC, so whenever a
packet appears with the same MAC on another port it is just dropped in
the HW, so there is no possibility of doing any CPU processing in this
case. Only after the timeout (5 min) can the MAC get a normal ATU on an
open port.
For the bridge to do what you suggest, a FDB search would be needed
afaics, and this might be in a process sensitive part of the code, thus
leading to too heavy a cost.
>>
>> >
>> > 1. It discards packets with matching DMAC, regardless of ingress port. I
>> > read the document [1] you linked to in a different reply and could not
>> > find anything against this approach, so this might be fine or at least
>> > not very significant.
>> >
>> > Note that this means that "locked" entries need to be notified to device
>> > drivers so that they will install a matching entry in the HW FDB.
>>
>> Okay, so as V4 does (just without the error noted).
>>
>> >
>> > 2. It is not refreshed and has ageing enabled. That is, after initial
>> > installation it will be removed by the bridge driver after configured
>> > ageing time unless converted to a regular (unlocked) entry.
>> >
>> > I assume this allows you to remove the timer implementation from your
>> > driver and let the bridge driver notify you about the removal of this
>> > entry.
>>
>> Okay, but only if the scheme is not so that the driver creates the
>> locked
>> entries itself, unless you indicate that the driver notifies the
>> bridge,
>> which then notifies back to the driver and installs the zero-DPV
>> entry? If
>> not I think the current implementation for the mv88e6xxx is fine.
>
> I don't see a problem in having the driver notifying the bridge about
> the installation of this entry and the bridge notifying the driver that
> the entry needs to be removed. It removes complexity from device
> drivers
> like mv88e6xxx and doesn't add extra complexity to the bridge driver.
>
> Actually, there is one complication, 'SWITCHDEV_FDB_ADD_TO_BRIDGE' will
> add the locked entry as externally learned, which means the bridge will
> not age it. Might need something like this:
>
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index e7f4fccb6adb..5f73d0b44ed9 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -530,7 +530,8 @@ void br_fdb_cleanup(struct work_struct *work)
> unsigned long this_timer = f->updated + delay;
>
> if (test_bit(BR_FDB_STATIC, &f->flags) ||
> - test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags)) {
> + (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags) &&
> + !test_bit(BR_FDB_ENTRY_LOCKED, &f->flags))) {
> if (test_bit(BR_FDB_NOTIFY, &f->flags)) {
> if (time_after(this_timer, now))
> work_delay = min(work_delay,
>
There is a case of ownership of the FDB/ATU entry, which if I remember
correctly, will point to the current implementation being the right way
to do it, thus having the driver keeping ownership of the entry and
thereby also ageing it, but I think Vladimir should have his say here.
>>
>> >
>> > 3. With regards to roaming, the entry cannot roam between locked ports
>> > (they need to have learning disabled anyway), but can roam to an
>> > unlocked port, in which case it becomes a regular entry that can roam
>> > and age.
>> >
>> > If we agree on these semantics, then I can try to verify that at least
>> > Spectrum can support them (it seems mv88e6xxx can).
>>
>> The consensus here is that at least for the mv88e6xxx, learning should
>> be on
>> and link local learning should be blocked by the userspace setting you
>> pointed to earlier.
>
> Why learning needs to be on in the bridge (not mv88e6xxx) driver?
I think it is seen as 'cheating' to enable learning only in the driver
behind the scenes, so kind of hackish. E.g. 'bridge -d link show' will
then report 'learning off', while learning is on in the driver.
And learning needs to be on for the driver as discussed earlier, which
only gives rise to the link local learning problem, which is then solved
by the user space setting.
On Wed, Aug 10, 2022 at 10:40:45AM +0200, [email protected] wrote:
> On 2022-08-10 09:21, Ido Schimmel wrote:
> > On Tue, Aug 09, 2022 at 10:00:49PM +0200, [email protected]
> > wrote:
> > > On 2022-08-09 11:20, Ido Schimmel wrote:
> > > > On Mon, Aug 01, 2022 at 05:33:49PM +0200, [email protected]
> > > > wrote:
> > > > > On 2022-07-13 14:39, Ido Schimmel wrote:
> > > > >
> > > > > >
> > > > > > What are "Storm Prevention" and "zero-DPV" FDB entries?
> > > > > >
> > > > >
> > > > > For the zero-DPV entries, I can summarize:
> > > > >
> > > > > Since a CPU can become saturated from constant SA Miss Violations
> > > > > from a
> > > > > denied source, source MAC address are masked by loading a zero-DPV
> > > > > (Destination Port Vector) entry in the ATU. As the address now
> > > > > appears in
> > > > > the database it will not cause more Miss Violations. ANY port trying
> > > > > to send
> > > > > a frame to this unauthorized address is discarded. Any locked port
> > > > > trying to
> > > > > use this unauthorized address has its frames discarded too (as the
> > > > > ports SA
> > > > > bit is not set in the ATU entry).
> > > >
> > > > What happens to unlocked ports that have learning enabled and are trying
> > > > to use this address as SMAC? AFAICT, at least in the bridge driver, the
> > > > locked entry will roam, but will keep the "locked" flag, which is
> > > > probably not what we want. Let's see if we can agree on these semantics
> > > > for a "locked" entry:
> > >
> > > The next version of this will block forwarding to locked entries in
> > > the
> > > bridge, so they will behave like the zero-DPV entries.
> >
> > I'm talking about roaming, not forwarding. Let's say you have a locked
> > entry with MAC X pointing to port Y. Now you get a packet with SMAC X
> > from port Z which is unlocked. Will the FDB entry roam to port Z? I
> > think it should, but at least in current implementation it seems that
> > the "locked" flag will not be reset and having locked entries pointing
> > to an unlocked port looks like a bug.
> >
>
> Remember that zero-DPV entries blackhole (mask) the MAC, so whenever a
> packet appears with the same MAC on another port it is just dropped in the
What do you mean by "same MAC"? As SMAC or DMAC? I'm talking about SMAC
and when the packet is received via an unlocked port. Why would such a
packet be dropped?
> HW, so there is no possibility of doing any CPU processing in this case.
> Only after the timeout (5 min) can the MAC get a normal ATU on an open port.
> For the bridge to do what you suggest, a FDB search would be needed afaics,
> and this might be in a process sensitive part of the code, thus leading to
> too heavy a cost.
When learning is enabled the bridge already performs a lookup on the
SMAC.
TBH, I don't think this is progressing well because there is too much
discrepancy between how this feature works in the bridge driver and in
the hardware you work with.
I suggest to first define the model in the bridge driver and then take
care of the offload. My suggestion is to send another RFC with only the
bridge changes with emphasize on the following aspects:
* Forwarding rules for "locked" entries. Do they drop matching packets?
Forward them? Or not considered at all for forwarding?
* Roaming rules for "locked" entries. Can they roam between locked
ports? Can they roam from a locked port to an unlocked port and
vice versa? Or are they completely sticky?
* Ageing rule for "locked" entries. Are these entries subject to the
ageing time or are they static? If they are not static, are they
refreshed by incoming traffic from a locked port or not?
* MAB enablement. New option? Overload an existing one? No option?
The commit messages should explain these design choices and new tests
cases should verify the desired behavior.
Once we have an agreement we can work out the switchdev/mv88e6xxx parts
and eventually the entire thing can be merged together. Fair?
On 2022-08-11 13:28, Ido Schimmel wrote:
>> > I'm talking about roaming, not forwarding. Let's say you have a locked
>> > entry with MAC X pointing to port Y. Now you get a packet with SMAC X
>> > from port Z which is unlocked. Will the FDB entry roam to port Z? I
>> > think it should, but at least in current implementation it seems that
>> > the "locked" flag will not be reset and having locked entries pointing
>> > to an unlocked port looks like a bug.
>> >
>>
In general I have been thinking that the said setup is a network
configuration error as I was arguing in an earlier conversation with
Vladimir. In this setup we must remember that SMAC X becomes DMAC X in
the return traffic on the open port. But the question arises to me why
MAC X would be behind the locked port without getting authed while being
behind an open port too?
In a real life setup, I don't think you would want random hosts behind a
locked port in the MAB case, but only the hosts you will let through.
Other hosts should be regarded as intruders.
If we are talking about a station move, then the locked entry will age
out and MAC X will function normally on the open port after the timeout,
which was a case that was taken up in earlier discussions.
But I will anyhow do some testing with this 'edge case' (of being behind
both a locked and an unlocked port) if I may call it so, and see to that
the offloaded and non-offloaded cases correspond to each other, and will
work satisfactory.
I think it will be good to have a flag to enable the mac-auth/MAB
feature, and I suggest just calling the flag 'mab', as it is short.
Otherwise I don't see any major issues with the whole feature as it is.
On 2022-08-11 13:28, Ido Schimmel wrote:
>> >
>> > I'm talking about roaming, not forwarding. Let's say you have a locked
>> > entry with MAC X pointing to port Y. Now you get a packet with SMAC X
>> > from port Z which is unlocked. Will the FDB entry roam to port Z? I
>> > think it should, but at least in current implementation it seems that
>> > the "locked" flag will not be reset and having locked entries pointing
>> > to an unlocked port looks like a bug.
>> >
Yes, now I have tried to test with a case like this using the bridge and
have verified the locked entry pointing to an unlocked port, which I
agree seems to be a bug, which I will get fixed.
On Fri, Aug 12, 2022 at 02:29:48PM +0200, [email protected] wrote:
> On 2022-08-11 13:28, Ido Schimmel wrote:
>
> > > > I'm talking about roaming, not forwarding. Let's say you have a locked
> > > > entry with MAC X pointing to port Y. Now you get a packet with SMAC X
> > > > from port Z which is unlocked. Will the FDB entry roam to port Z? I
> > > > think it should, but at least in current implementation it seems that
> > > > the "locked" flag will not be reset and having locked entries pointing
> > > > to an unlocked port looks like a bug.
> > > >
> > >
>
> In general I have been thinking that the said setup is a network
> configuration error as I was arguing in an earlier conversation with
> Vladimir. In this setup we must remember that SMAC X becomes DMAC X in the
> return traffic on the open port. But the question arises to me why MAC X
> would be behind the locked port without getting authed while being behind an
> open port too?
> In a real life setup, I don't think you would want random hosts behind a
> locked port in the MAB case, but only the hosts you will let through. Other
> hosts should be regarded as intruders.
>
> If we are talking about a station move, then the locked entry will age out
> and MAC X will function normally on the open port after the timeout, which
> was a case that was taken up in earlier discussions.
>
> But I will anyhow do some testing with this 'edge case' (of being behind
> both a locked and an unlocked port) if I may call it so, and see to that the
> offloaded and non-offloaded cases correspond to each other, and will work
> satisfactory.
It would be best to implement these as additional test cases in the
current selftest. Then you can easily test with both veth pairs and
loopbacks and see that the hardware and software data paths behave the
same.
>
> I think it will be good to have a flag to enable the mac-auth/MAB feature,
> and I suggest just calling the flag 'mab', as it is short.
Fine by me, but I'm not sure everyone agrees.
>
> Otherwise I don't see any major issues with the whole feature as it is.
Will review and test the next version.
Thanks
On 2022-08-10 09:21, Ido Schimmel wrote:
>> >
>> > 1. It discards packets with matching DMAC, regardless of ingress port. I
>> > read the document [1] you linked to in a different reply and could not
>> > find anything against this approach, so this might be fine or at least
>> > not very significant.
>> >
>> > Note that this means that "locked" entries need to be notified to device
>> > drivers so that they will install a matching entry in the HW FDB.
>>
I just want to be completely sure as what should be done in version 5
with locked entries from the bridge, as - if I should implement it so
that they are sent to all the drivers, and the drivers then ignore them
if they don't need to take action? (for the mv88e6xxx driver, it does
not need them and can ignore but other drivers might need.)
On Tue, Aug 16, 2022 at 09:51:32AM +0200, [email protected] wrote:
> On 2022-08-10 09:21, Ido Schimmel wrote:
> > > >
> > > > 1. It discards packets with matching DMAC, regardless of ingress port. I
> > > > read the document [1] you linked to in a different reply and could not
> > > > find anything against this approach, so this might be fine or at least
> > > > not very significant.
> > > >
> > > > Note that this means that "locked" entries need to be notified to device
> > > > drivers so that they will install a matching entry in the HW FDB.
> > >
>
> I just want to be completely sure as what should be done in version 5 with
> locked entries from the bridge, as - if I should implement it so that they
> are sent to all the drivers, and the drivers then ignore them if they don't
> need to take action? (for the mv88e6xxx driver, it does not need them and
> can ignore but other drivers might need.)
Yes, I think that would be best. At least when mlxsw starts supporting
MAB it will need to program the locked entry as an FDB with discard
action.
To be clear, I'm aware that all drivers other than mv88e6xxx currently
forbid a port from being locked, making it unlikely that they will
receive such notifications, but if you do it now then we will not need
more changes in the bridge when other drivers gain support for
802.1X/MAB.
On 2022-08-14 16:55, Ido Schimmel wrote:
> On Fri, Aug 12, 2022 at 02:29:48PM +0200, [email protected]
> wrote:
>> On 2022-08-11 13:28, Ido Schimmel wrote:
>>
>> > > > I'm talking about roaming, not forwarding. Let's say you have a locked
>> > > > entry with MAC X pointing to port Y. Now you get a packet with SMAC X
>> > > > from port Z which is unlocked. Will the FDB entry roam to port Z? I
>> > > > think it should, but at least in current implementation it seems that
>> > > > the "locked" flag will not be reset and having locked entries pointing
>> > > > to an unlocked port looks like a bug.
I have made the locked entries sticky in the bridge, so that they don't
move to other ports.
>> > > >
>> > >
>>
>> In general I have been thinking that the said setup is a network
>> configuration error as I was arguing in an earlier conversation with
>> Vladimir. In this setup we must remember that SMAC X becomes DMAC X in
>> the
>> return traffic on the open port. But the question arises to me why MAC
>> X
>> would be behind the locked port without getting authed while being
>> behind an
>> open port too?
>> In a real life setup, I don't think you would want random hosts behind
>> a
>> locked port in the MAB case, but only the hosts you will let through.
>> Other
>> hosts should be regarded as intruders.
>>
>> If we are talking about a station move, then the locked entry will age
>> out
>> and MAC X will function normally on the open port after the timeout,
>> which
>> was a case that was taken up in earlier discussions.
>>
>> But I will anyhow do some testing with this 'edge case' (of being
>> behind
>> both a locked and an unlocked port) if I may call it so, and see to
>> that the
>> offloaded and non-offloaded cases correspond to each other, and will
>> work
>> satisfactory.
>
> It would be best to implement these as additional test cases in the
> current selftest. Then you can easily test with both veth pairs and
> loopbacks and see that the hardware and software data paths behave the
> same.
>
How many loops would be needed to have a selftest with a HUB and a MAC
on both a locked and an unlocked port?
>>
>> I think it will be good to have a flag to enable the mac-auth/MAB
>> feature,
>> and I suggest just calling the flag 'mab', as it is short.
I have now created the flag to enable Mac-Auth/MAB with iproute2:
bridge link set dev DEV macauth on|off
with the example output from 'bridge -d link show dev DEV' when macauth
is enabled:
1: ethX: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 master br0 state
forwarding priority 32 cost 19
hairpin off guard off root_block off fastleave off learning on flood
off mcast_flood on bcast_flood on mcast_router 1 mcast_to_unicast off
neigh_suppress off vlan_tunnel off isolated off locked mab on
The flag itself in the code is called BR_PORT_MACAUTH.
>
> Fine by me, but I'm not sure everyone agrees.
On Fri, Aug 19, 2022 at 11:51:11AM +0200, [email protected] wrote:
> On 2022-08-14 16:55, Ido Schimmel wrote:
> > On Fri, Aug 12, 2022 at 02:29:48PM +0200, [email protected]
> > wrote:
> > > On 2022-08-11 13:28, Ido Schimmel wrote:
> > >
> > > > > > I'm talking about roaming, not forwarding. Let's say you have a locked
> > > > > > entry with MAC X pointing to port Y. Now you get a packet with SMAC X
> > > > > > from port Z which is unlocked. Will the FDB entry roam to port Z? I
> > > > > > think it should, but at least in current implementation it seems that
> > > > > > the "locked" flag will not be reset and having locked entries pointing
> > > > > > to an unlocked port looks like a bug.
>
> I have made the locked entries sticky in the bridge, so that they don't move
> to other ports.
Please make sure that this design choice is explained in the commit
message. To be clear, it cannot be "this is how device X happens to
work".
>
> > > > > >
> > > > >
> > >
> > > In general I have been thinking that the said setup is a network
> > > configuration error as I was arguing in an earlier conversation with
> > > Vladimir. In this setup we must remember that SMAC X becomes DMAC X
> > > in the
> > > return traffic on the open port. But the question arises to me why
> > > MAC X
> > > would be behind the locked port without getting authed while being
> > > behind an
> > > open port too?
> > > In a real life setup, I don't think you would want random hosts
> > > behind a
> > > locked port in the MAB case, but only the hosts you will let
> > > through. Other
> > > hosts should be regarded as intruders.
> > >
> > > If we are talking about a station move, then the locked entry will
> > > age out
> > > and MAC X will function normally on the open port after the timeout,
> > > which
> > > was a case that was taken up in earlier discussions.
> > >
> > > But I will anyhow do some testing with this 'edge case' (of being
> > > behind
> > > both a locked and an unlocked port) if I may call it so, and see to
> > > that the
> > > offloaded and non-offloaded cases correspond to each other, and will
> > > work
> > > satisfactory.
> >
> > It would be best to implement these as additional test cases in the
> > current selftest. Then you can easily test with both veth pairs and
> > loopbacks and see that the hardware and software data paths behave the
> > same.
> >
>
> How many loops would be needed to have a selftest with a HUB and a MAC on
> both a locked and an unlocked port?
I assume you want a hub to simulate multiple MACs behind the same port.
You don't need a hub for that. You can set the MAC using mausezahn. See
'-a' option:
"
-a <src-mac|keyword>
Use specified source MAC address with hexadecimal notation such as 00:00:aa:bb:cc:dd. By default the interface MAC address will be used. The keywords ''rand''
and ''own'' refer to a random MAC address (only unicast addresses are created) and the own address, respectively. You can also use the keywords mentioned below
although broadcast-type source addresses are officially invalid.
"
>
> > >
> > > I think it will be good to have a flag to enable the mac-auth/MAB
> > > feature,
> > > and I suggest just calling the flag 'mab', as it is short.
>
> I have now created the flag to enable Mac-Auth/MAB with iproute2:
> bridge link set dev DEV macauth on|off
You have 'macauth' here, but 'mab' in the output below. They need to
match. I prefer the latter unless you have a good reason to use
'macauth'.
>
> with the example output from 'bridge -d link show dev DEV' when macauth is
> enabled:
> 1: ethX: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 master br0 state
> forwarding priority 32 cost 19
> hairpin off guard off root_block off fastleave off learning on flood off
> mcast_flood on bcast_flood on mcast_router 1 mcast_to_unicast off
> neigh_suppress off vlan_tunnel off isolated off locked mab on
>
> The flag itself in the code is called BR_PORT_MACAUTH.
>
> >
> > Fine by me, but I'm not sure everyone agrees.
On 2022-08-21 09:08, Ido Schimmel wrote:
> On Fri, Aug 19, 2022 at 11:51:11AM +0200, [email protected]
> wrote:
>> On 2022-08-14 16:55, Ido Schimmel wrote:
>> > On Fri, Aug 12, 2022 at 02:29:48PM +0200, [email protected]
>> > wrote:
>> > > On 2022-08-11 13:28, Ido Schimmel wrote:
>> > >
>> > > > > > I'm talking about roaming, not forwarding. Let's say you have a locked
>> > > > > > entry with MAC X pointing to port Y. Now you get a packet with SMAC X
>> > > > > > from port Z which is unlocked. Will the FDB entry roam to port Z? I
>> > > > > > think it should, but at least in current implementation it seems that
>> > > > > > the "locked" flag will not be reset and having locked entries pointing
>> > > > > > to an unlocked port looks like a bug.
>>
>> I have made the locked entries sticky in the bridge, so that they
>> don't move
>> to other ports.
>
> Please make sure that this design choice is explained in the commit
> message. To be clear, it cannot be "this is how device X happens to
> work".
>
The real issue I think is that the locked entry should mask the MAC
address involved (as the description I gave for zero-DPV entries and
actually also storm prevention entries ensure), so that there is no
forwarding to the address on any port, otherwise it will allow one-way
traffic to a host that is not trusted. Thus flooding of unknown unicast
on a locked port should of course be disabled ('flood off'), so that
there is no way of sending to an unauthorized silent host behind the
locked port.
The issue with the locked entry appearing on another SW bridge port from
where it originated, I think is more of a cosmetic bug, though I could
be mistaken. But adding the sticky flag to locked entries ensures that
they do not move to another port.
This of course does that instant roaming is not possible, but I think
that the right approach is to use the ageing out of entries to allow the
station move/roaming.
The case of unwanted traffic to a MAC behind a locked port with a locked
entry is what I would regard as more worthy of a selftest. The sticky
flag I know will ensure that the locked entries do not move to other
ports, and since it is only in the bridge this can be tested (e.g. using
'bridge fdb show dev DEV'), I think that the test would be superfluos.
What do you think of that and my other consideration for a test?
>> I have now created the flag to enable Mac-Auth/MAB with iproute2:
>> bridge link set dev DEV macauth on|off
>
> You have 'macauth' here, but 'mab' in the output below. They need to
> match. I prefer the latter unless you have a good reason to use
> 'macauth'.
>
>>
>> with the example output from 'bridge -d link show dev DEV' when
>> macauth is
>> enabled:
>> 1: ethX: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 master br0 state
>> forwarding priority 32 cost 19
>> hairpin off guard off root_block off fastleave off learning on
>> flood off
>> mcast_flood on bcast_flood on mcast_router 1 mcast_to_unicast off
>> neigh_suppress off vlan_tunnel off isolated off locked mab on
>>
>> The flag itself in the code is called BR_PORT_MACAUTH.
>>
>> >
>> > Fine by me, but I'm not sure everyone agrees.
I will change it in iproute2 to:
bridge link set dev DEV mab on|off
On Sun, Aug 21, 2022 at 03:43:04PM +0200, [email protected] wrote:
> On 2022-08-21 09:08, Ido Schimmel wrote:
> > On Fri, Aug 19, 2022 at 11:51:11AM +0200, [email protected]
> > wrote:
> > > On 2022-08-14 16:55, Ido Schimmel wrote:
> > > > On Fri, Aug 12, 2022 at 02:29:48PM +0200, [email protected]
> > > > wrote:
> > > > > On 2022-08-11 13:28, Ido Schimmel wrote:
> > > > >
> > > > > > > > I'm talking about roaming, not forwarding. Let's say you have a locked
> > > > > > > > entry with MAC X pointing to port Y. Now you get a packet with SMAC X
> > > > > > > > from port Z which is unlocked. Will the FDB entry roam to port Z? I
> > > > > > > > think it should, but at least in current implementation it seems that
> > > > > > > > the "locked" flag will not be reset and having locked entries pointing
> > > > > > > > to an unlocked port looks like a bug.
> > >
> > > I have made the locked entries sticky in the bridge, so that they
> > > don't move
> > > to other ports.
> >
> > Please make sure that this design choice is explained in the commit
> > message. To be clear, it cannot be "this is how device X happens to
> > work".
> >
>
> The real issue I think is that the locked entry should mask the MAC address
> involved (as the description I gave for zero-DPV entries and actually also
> storm prevention entries ensure), so that there is no forwarding to the
> address on any port, otherwise it will allow one-way traffic to a host that
> is not trusted. Thus flooding of unknown unicast on a locked port should of
> course be disabled ('flood off'), so that there is no way of sending to an
> unauthorized silent host behind the locked port.
>
> The issue with the locked entry appearing on another SW bridge port from
> where it originated, I think is more of a cosmetic bug, though I could be
> mistaken. But adding the sticky flag to locked entries ensures that they do
> not move to another port.
>
> This of course does that instant roaming is not possible, but I think that
> the right approach is to use the ageing out of entries to allow the station
> move/roaming.
I personally think that the mv88e6xxx semantics are very weird (e.g., no
roaming, traffic blackhole) and I don't want them to determine how the
feature works in the pure software bridge or other hardware
implementations. On the other hand, I understand your constraints and I
don't want to create a situation where user space is unable to
understand how the data path works from the bridge FDB dump with
mv88e6xxx.
My suggestion is to have mv88e6xxx report the "locked" entry to the
bridge driver with additional flags that describe its behavior in terms
of roaming, ageing and forwarding.
In terms of roaming, since in mv88e6xxx the entry can't roam you should
report the entry with the "sticky" flag. In terms of ageing, since
mv88e6xxx is the one doing the ageing and not the bridge driver, report
the entry with the "extern_learn" flag. In terms of forwarding, in
mv88e6xxx the entry discards all matching packets. We can introduce a
new FDB flag that instructs the entry to silently discard all matching
packets. Like we have with blackhole routes and nexthops.
I believe that the above suggestion allows you to fully describe how
these entries work in mv88e6xxx while keeping the bridge driver in sync
with complete visibility towards user space.
It also frees the pure software implementation from the constraints of
mv88e6xxx, allowing "locked" entries to behave like any other
dynamically learned entries modulo the fact that they cannot "unlock" a
locked port.
Yes, it does mean that user space will get a bit different behavior with
mv88e6xxx compared to a pure software solution, but a) It's only the
corner cases that act a bit differently. As a whole, the feature works
largely the same. b) User space has complete visibility to understand
the behavior of the offloaded data path.
>
> The case of unwanted traffic to a MAC behind a locked port with a locked
> entry is what I would regard as more worthy of a selftest. The sticky flag I
> know will ensure that the locked entries do not move to other ports, and
> since it is only in the bridge this can be tested (e.g. using 'bridge fdb
> show dev DEV'), I think that the test would be superfluos. What do you think
> of that and my other consideration for a test?
If we go with the above suggestion, then you can have a
mv88e6xxx-specific selftest that tests the corner cases where mv88e6xxx
acts a bit differently from the pure software bridge.
>
>
> > > I have now created the flag to enable Mac-Auth/MAB with iproute2:
> > > bridge link set dev DEV macauth on|off
> >
> > You have 'macauth' here, but 'mab' in the output below. They need to
> > match. I prefer the latter unless you have a good reason to use
> > 'macauth'.
> >
> > >
> > > with the example output from 'bridge -d link show dev DEV' when
> > > macauth is
> > > enabled:
> > > 1: ethX: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 master br0 state
> > > forwarding priority 32 cost 19
> > > hairpin off guard off root_block off fastleave off learning on
> > > flood off
> > > mcast_flood on bcast_flood on mcast_router 1 mcast_to_unicast off
> > > neigh_suppress off vlan_tunnel off isolated off locked mab on
> > >
> > > The flag itself in the code is called BR_PORT_MACAUTH.
> > >
> > > >
> > > > Fine by me, but I'm not sure everyone agrees.
>
> I will change it in iproute2 to:
> bridge link set dev DEV mab on|off
And s/BR_PORT_MACAUTH/BR_PORT_MAB/ ?
On 2022-08-22 07:40, Ido Schimmel wrote:
> On Sun, Aug 21, 2022 at 03:43:04PM +0200, [email protected]
> wrote:
>
> I personally think that the mv88e6xxx semantics are very weird (e.g.,
> no
> roaming, traffic blackhole) and I don't want them to determine how the
> feature works in the pure software bridge or other hardware
> implementations. On the other hand, I understand your constraints and I
> don't want to create a situation where user space is unable to
> understand how the data path works from the bridge FDB dump with
> mv88e6xxx.
>
> My suggestion is to have mv88e6xxx report the "locked" entry to the
> bridge driver with additional flags that describe its behavior in terms
> of roaming, ageing and forwarding.
>
> In terms of roaming, since in mv88e6xxx the entry can't roam you should
> report the entry with the "sticky" flag.
As I am not familiar with roaming in this context, I need to know how
the SW bridge should behave in this case. In this I am assuming that
roaming is regarding unauthorized entries.
In this case, is the roaming only between locked ports or does the
roaming include that the entry can move to a unlocked port, resulting in
the locked flag getting removed?
> In terms of ageing, since
> mv88e6xxx is the one doing the ageing and not the bridge driver, report
> the entry with the "extern_learn" flag.
Just for the record, I see that entries coming from the driver to the
bridge will always have the "extern learn" flag set as can be seen from
the SWITCHDEV_FDB_ADD_TO_BRIDGE events handling in br_switchdev_event()
in br.c, which I think is the correct behavior.
> In terms of forwarding, in
> mv88e6xxx the entry discards all matching packets. We can introduce a
> new FDB flag that instructs the entry to silently discard all matching
> packets. Like we have with blackhole routes and nexthops.
Any suggestions to the name of this flag?
>
> I believe that the above suggestion allows you to fully describe how
> these entries work in mv88e6xxx while keeping the bridge driver in sync
> with complete visibility towards user space.
>
> It also frees the pure software implementation from the constraints of
> mv88e6xxx, allowing "locked" entries to behave like any other
> dynamically learned entries modulo the fact that they cannot "unlock" a
> locked port.
>
> Yes, it does mean that user space will get a bit different behavior
> with
> mv88e6xxx compared to a pure software solution, but a) It's only the
> corner cases that act a bit differently. As a whole, the feature works
> largely the same. b) User space has complete visibility to understand
> the behavior of the offloaded data path.
>
>>
>> I will change it in iproute2 to:
>> bridge link set dev DEV mab on|off
>
> And s/BR_PORT_MACAUTH/BR_PORT_MAB/ ?
Sure, I will do that. :-)
On Mon, Aug 22, 2022 at 09:49:28AM +0200, [email protected] wrote:
> On 2022-08-22 07:40, Ido Schimmel wrote:
> > On Sun, Aug 21, 2022 at 03:43:04PM +0200, [email protected]
> > wrote:
> >
> > I personally think that the mv88e6xxx semantics are very weird (e.g., no
> > roaming, traffic blackhole) and I don't want them to determine how the
> > feature works in the pure software bridge or other hardware
> > implementations. On the other hand, I understand your constraints and I
> > don't want to create a situation where user space is unable to
> > understand how the data path works from the bridge FDB dump with
> > mv88e6xxx.
> >
> > My suggestion is to have mv88e6xxx report the "locked" entry to the
> > bridge driver with additional flags that describe its behavior in terms
> > of roaming, ageing and forwarding.
> >
> > In terms of roaming, since in mv88e6xxx the entry can't roam you should
> > report the entry with the "sticky" flag.
>
> As I am not familiar with roaming in this context, I need to know how the SW
> bridge should behave in this case.
I think I wasn't clear enough. The idea is to make the bridge compatible
with mv88e6xxx in a way that is discoverable by user space by having
mv88e6xxx add the locked entry with flags that describe the hardware
behavior. Therefore, it's not a matter of "how the SW bridge should
behave", but having it behave in a way that matches the offloaded data
path.
From what I was able to understand from you, the "locked" entry cannot
roam at all in mv88e6xxx, which can be described by the "sticky" flag.
> In this I am assuming that roaming is regarding unauthorized entries.
Yes, talking about "locked" entries that are notified by mv88e6xxx to
the bridge.
> In this case, is the roaming only between locked ports or does the
> roaming include that the entry can move to a unlocked port, resulting
> in the locked flag getting removed?
Any two ports. If the "locked" entry in mv88e6xxx cannot move once
installed, then the "sticky" flag accurately describes it.
>
> > In terms of ageing, since
> > mv88e6xxx is the one doing the ageing and not the bridge driver, report
> > the entry with the "extern_learn" flag.
>
> Just for the record, I see that entries coming from the driver to the bridge
> will always have the "extern learn" flag set as can be seen from the
> SWITCHDEV_FDB_ADD_TO_BRIDGE events handling in br_switchdev_event() in br.c,
> which I think is the correct behavior.
Yes.
>
> > In terms of forwarding, in
> > mv88e6xxx the entry discards all matching packets. We can introduce a
> > new FDB flag that instructs the entry to silently discard all matching
> > packets. Like we have with blackhole routes and nexthops.
>
> Any suggestions to the name of this flag?
I'm not good at naming, but "blackhole" is at least consistent with what
we already have for routes and nexthop objects.
>
> >
> > I believe that the above suggestion allows you to fully describe how
> > these entries work in mv88e6xxx while keeping the bridge driver in sync
> > with complete visibility towards user space.
> >
> > It also frees the pure software implementation from the constraints of
> > mv88e6xxx, allowing "locked" entries to behave like any other
> > dynamically learned entries modulo the fact that they cannot "unlock" a
> > locked port.
> >
> > Yes, it does mean that user space will get a bit different behavior with
> > mv88e6xxx compared to a pure software solution, but a) It's only the
> > corner cases that act a bit differently. As a whole, the feature works
> > largely the same. b) User space has complete visibility to understand
> > the behavior of the offloaded data path.
> >
>
> > >
> > > I will change it in iproute2 to:
> > > bridge link set dev DEV mab on|off
> >
> > And s/BR_PORT_MACAUTH/BR_PORT_MAB/ ?
>
> Sure, I will do that. :-)
Thanks
On 2022-08-23 08:48, Ido Schimmel wrote:
> On Mon, Aug 22, 2022 at 09:49:28AM +0200, [email protected]
> wrote:
>> As I am not familiar with roaming in this context, I need to know how
>> the SW
>> bridge should behave in this case.
>
>> In this case, is the roaming only between locked ports or does the
>> roaming include that the entry can move to a unlocked port, resulting
>> in the locked flag getting removed?
>
> Any two ports. If the "locked" entry in mv88e6xxx cannot move once
> installed, then the "sticky" flag accurately describes it.
>
But since I am also doing the SW bridge implementation without mv88e6xxx
I need it to function according to needs.
Thus the locked entries created in the bridge I shall not put the sticky
flag on, but there will be the situation where a locked entry can move
to an unlocked port, which we regarded as a bug. In that case there is
two possibilities, the locked entry can move to an unlocked port with
the locked flag being removed or the locked entry can only move to
another locked port?
On Tue, Aug 23, 2022 at 09:13:54AM +0200, [email protected] wrote:
> On 2022-08-23 08:48, Ido Schimmel wrote:
> > On Mon, Aug 22, 2022 at 09:49:28AM +0200, [email protected]
> > wrote:
>
> > > As I am not familiar with roaming in this context, I need to know
> > > how the SW
> > > bridge should behave in this case.
> >
>
> > > In this case, is the roaming only between locked ports or does the
> > > roaming include that the entry can move to a unlocked port, resulting
> > > in the locked flag getting removed?
> >
> > Any two ports. If the "locked" entry in mv88e6xxx cannot move once
> > installed, then the "sticky" flag accurately describes it.
> >
>
> But since I am also doing the SW bridge implementation without mv88e6xxx I
> need it to function according to needs.
> Thus the locked entries created in the bridge I shall not put the sticky
> flag on, but there will be the situation where a locked entry can move to an
> unlocked port, which we regarded as a bug.
I do not regard this as a bug. It makes sense to me that an authorized
port can cause an entry pointing to an unauthorized port to roam to
itself. Just like normal learned entries. What I considered as a bug is
the fact that the "locked" flag is not cleared when roaming to an
authorized port.
> In that case there is two possibilities, the locked entry can move to
> an unlocked port with the locked flag being removed or the locked
> entry can only move to another locked port?
My suggestion is to allow roaming and maintain / clear the "locked" flag
based on whether the new destination port is locked or not.
On 2022-08-23 09:24, Ido Schimmel wrote:
> On Tue, Aug 23, 2022 at 09:13:54AM +0200, [email protected]
> wrote:
>> On 2022-08-23 08:48, Ido Schimmel wrote:
>> > On Mon, Aug 22, 2022 at 09:49:28AM +0200, [email protected]
>> > wrote:
>>
>> > > As I am not familiar with roaming in this context, I need to know
>> > > how the SW
>> > > bridge should behave in this case.
>> >
>>
>> > > In this case, is the roaming only between locked ports or does the
>> > > roaming include that the entry can move to a unlocked port, resulting
>> > > in the locked flag getting removed?
>> >
>> > Any two ports. If the "locked" entry in mv88e6xxx cannot move once
>> > installed, then the "sticky" flag accurately describes it.
>> >
>>
>> But since I am also doing the SW bridge implementation without
>> mv88e6xxx I
>> need it to function according to needs.
>> Thus the locked entries created in the bridge I shall not put the
>> sticky
>> flag on, but there will be the situation where a locked entry can move
>> to an
>> unlocked port, which we regarded as a bug.
>
> I do not regard this as a bug. It makes sense to me that an authorized
> port can cause an entry pointing to an unauthorized port to roam to
> itself. Just like normal learned entries. What I considered as a bug is
> the fact that the "locked" flag is not cleared when roaming to an
> authorized port.
>
>> In that case there is two possibilities, the locked entry can move to
>> an unlocked port with the locked flag being removed or the locked
>> entry can only move to another locked port?
>
> My suggestion is to allow roaming and maintain / clear the "locked"
> flag
> based on whether the new destination port is locked or not.
Thus I understand it as saying that the "locked" flag can also be set
when roaming from an unlocked port to a locked port?
On Tue, Aug 23, 2022 at 09:37:54AM +0200, [email protected] wrote:
> On 2022-08-23 09:24, Ido Schimmel wrote:
> > On Tue, Aug 23, 2022 at 09:13:54AM +0200, [email protected]
> > wrote:
> > > On 2022-08-23 08:48, Ido Schimmel wrote:
> > > > On Mon, Aug 22, 2022 at 09:49:28AM +0200, [email protected]
> > > > wrote:
> > >
> > > > > As I am not familiar with roaming in this context, I need to know
> > > > > how the SW
> > > > > bridge should behave in this case.
> > > >
> > >
> > > > > In this case, is the roaming only between locked ports or does the
> > > > > roaming include that the entry can move to a unlocked port, resulting
> > > > > in the locked flag getting removed?
> > > >
> > > > Any two ports. If the "locked" entry in mv88e6xxx cannot move once
> > > > installed, then the "sticky" flag accurately describes it.
> > > >
> > >
> > > But since I am also doing the SW bridge implementation without
> > > mv88e6xxx I
> > > need it to function according to needs.
> > > Thus the locked entries created in the bridge I shall not put the
> > > sticky
> > > flag on, but there will be the situation where a locked entry can
> > > move to an
> > > unlocked port, which we regarded as a bug.
> >
> > I do not regard this as a bug. It makes sense to me that an authorized
> > port can cause an entry pointing to an unauthorized port to roam to
> > itself. Just like normal learned entries. What I considered as a bug is
> > the fact that the "locked" flag is not cleared when roaming to an
> > authorized port.
> >
> > > In that case there is two possibilities, the locked entry can move to
> > > an unlocked port with the locked flag being removed or the locked
> > > entry can only move to another locked port?
> >
> > My suggestion is to allow roaming and maintain / clear the "locked" flag
> > based on whether the new destination port is locked or not.
>
> Thus I understand it as saying that the "locked" flag can also be set when
> roaming from an unlocked port to a locked port?
"learning on locked on" is really a misconfiguration, but it can also
happen today and entries do not roam with the "locked" flag for the
simple reason that it does not exist. I see two options:
1. Do not clear / set "locked" flag during roaming. Given learning
should be disabled on locked ports, then the only half interesting case
is roaming to an unlocked port. Keeping the "locked" flag basically
means "if you were to lock the port, then the presence of this entry is
not enough to let traffic with the SA be forwarded by the bridge".
Unlikely that anyone will do that.
2. Always set "locked" flag for learned entries (new & roamed) on locked
ports and clear it for learned entries on unlocked ports.
Both options are consistent in how they treat the "locked" flag (either
always do nothing or always set/clear) and both do not impact the
integrity of the solution when configured correctly (disabling learning
on locked ports). I guess users will find option 2 easier to understand
/ work with.
On 2022-08-23 08:48, Ido Schimmel wrote:
>
> I'm not good at naming, but "blackhole" is at least consistent with
> what
> we already have for routes and nexthop objects.
>
I have changed it the name "masked", as that is also the term used in
the documentation for the zero-DPV entries, and I think that it will
generally be a more accepted term.
Thus the name of the flag is now "BR_FDB_ENTRY_MASKED".
I hope that is fine with you?
On 2022-08-23 14:36, Ido Schimmel wrote:
> On Tue, Aug 23, 2022 at 09:37:54AM +0200, [email protected]
> wrote:
>
> "learning on locked on" is really a misconfiguration, but it can also
> happen today and entries do not roam with the "locked" flag for the
> simple reason that it does not exist. I see two options:
>
> 1. Do not clear / set "locked" flag during roaming. Given learning
> should be disabled on locked ports, then the only half interesting case
> is roaming to an unlocked port. Keeping the "locked" flag basically
> means "if you were to lock the port, then the presence of this entry is
> not enough to let traffic with the SA be forwarded by the bridge".
> Unlikely that anyone will do that.
>
> 2. Always set "locked" flag for learned entries (new & roamed) on
> locked
> ports and clear it for learned entries on unlocked ports.
>
> Both options are consistent in how they treat the "locked" flag (either
> always do nothing or always set/clear) and both do not impact the
> integrity of the solution when configured correctly (disabling learning
> on locked ports). I guess users will find option 2 easier to understand
> / work with.
Roaming to a locked port with an entry without the locked bit set would
open the port for said MAC without necessary authorization. Thus I think
that the only real option is the 2. case.
On 2022-08-21 09:08, Ido Schimmel wrote:
>
> I assume you want a hub to simulate multiple MACs behind the same port.
> You don't need a hub for that. You can set the MAC using mausezahn. See
> '-a' option:
>
> "
> -a <src-mac|keyword>
> Use specified source MAC address with hexadecimal notation such
> as 00:00:aa:bb:cc:dd. By default the interface MAC address will be
> used. The keywords ''rand''
> and ''own'' refer to a random MAC address (only unicast
> addresses are created) and the own address, respectively. You can also
> use the keywords mentioned below
> although broadcast-type source addresses are officially invalid.
> "
>
Ido, I am not so known to the selftests, so I am wondering why I don't
see either check_err or check_fail fail, whichever I use, when I think
they should and then they are not really checking...
local mac=10:20:30:30:20:10
$MZ $h1 -t udp -a $mac -b rand
bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0
locked"
check_err $? "MAB station move: no locked entry on first
injection"
$MZ $h2 -t udp -a $mac -b rand
bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0
locked"
check_err $? "MAB station move: locked entry did not move"
What is wrong here?
For a mv88e6xxx test I guess I can make a check to verify that this
driver is in use?
On Wed, Aug 24, 2022 at 10:29:20PM +0200, [email protected] wrote:
> On 2022-08-21 09:08, Ido Schimmel wrote:
> >
> > I assume you want a hub to simulate multiple MACs behind the same port.
> > You don't need a hub for that. You can set the MAC using mausezahn. See
> > '-a' option:
> >
> > "
> > -a <src-mac|keyword>
> > Use specified source MAC address with hexadecimal notation such
> > as 00:00:aa:bb:cc:dd. By default the interface MAC address will be
> > used. The keywords ''rand''
> > and ''own'' refer to a random MAC address (only unicast
> > addresses are created) and the own address, respectively. You can also
> > use the keywords mentioned below
> > although broadcast-type source addresses are officially invalid.
> > "
> >
>
>
> Ido, I am not so known to the selftests, so I am wondering why I don't see
> either check_err or check_fail fail, whichever I use, when I think they
> should and then they are not really checking...
>
>
> local mac=10:20:30:30:20:10
>
>
> $MZ $h1 -t udp -a $mac -b rand
> bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0 locked"
> check_err $? "MAB station move: no locked entry on first injection"
>
> $MZ $h2 -t udp -a $mac -b rand
> bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0 locked"
> check_err $? "MAB station move: locked entry did not move"
>
> What is wrong here?
Did you try adding a sleep between mausezahn and the FDB dump? At least
that is what learning_test() is doing. It is possible that the packet is
not sent / processed fast enough for the bridge to learn it before the
dump.
>
> For a mv88e6xxx test I guess I can make a check to verify that this driver
> is in use?
Not in a generic forwarding test. Maybe in
tools/testing/selftests/drivers/net/dsa/
My preference would be to get as much tests as possible in
tools/testing/selftests/net/forwarding/bridge_locked_port.sh.
I'm not sure which tests you are planning for mv88e6xxx, but we can pass
/ fail test cases based on the flags we observe in the FDB dump. For
example, if the entry has the "sticky" flag, then the expectation is
that the roaming test will fail. Otherwise, it should pass.
On Tue, Aug 23, 2022 at 01:41:51PM +0200, [email protected] wrote:
> On 2022-08-23 08:48, Ido Schimmel wrote:
> >
> > I'm not good at naming, but "blackhole" is at least consistent with what
> > we already have for routes and nexthop objects.
> >
>
> I have changed it the name "masked", as that is also the term used in the
> documentation for the zero-DPV entries, and I think that it will generally
> be a more accepted term.
"blackhole" is an already accepted term and at least to me it is much
more clear than "masked". Keep in mind that both L2 neighbours (FDB) and
L3 neighbours share the same uAPI and eventually we might want to extend
the use of this flag for L3 neighbours (at least Spectrum supports it),
so it needs to make sense for both.
On 2022-08-25 11:36, Ido Schimmel wrote:
> On Tue, Aug 23, 2022 at 01:41:51PM +0200, [email protected]
> wrote:
>> On 2022-08-23 08:48, Ido Schimmel wrote:
>> >
>> > I'm not good at naming, but "blackhole" is at least consistent with what
>> > we already have for routes and nexthop objects.
>> >
>>
>> I have changed it the name "masked", as that is also the term used in
>> the
>> documentation for the zero-DPV entries, and I think that it will
>> generally
>> be a more accepted term.
>
> "blackhole" is an already accepted term and at least to me it is much
> more clear than "masked". Keep in mind that both L2 neighbours (FDB)
> and
> L3 neighbours share the same uAPI and eventually we might want to
> extend
> the use of this flag for L3 neighbours (at least Spectrum supports it),
> so it needs to make sense for both.
Okay, I will do that then...
On 2022-08-25 11:23, Ido Schimmel wrote:
>>
>>
>> Ido, I am not so known to the selftests, so I am wondering why I don't
>> see
>> either check_err or check_fail fail, whichever I use, when I think
>> they
>> should and then they are not really checking...
>>
>>
>> local mac=10:20:30:30:20:10
>>
>>
>> $MZ $h1 -t udp -a $mac -b rand
>> bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0
>> locked"
>> check_err $? "MAB station move: no locked entry on first
>> injection"
>>
>> $MZ $h2 -t udp -a $mac -b rand
>> bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0
>> locked"
>> check_err $? "MAB station move: locked entry did not move"
>>
>> What is wrong here?
>
> Did you try adding a sleep between mausezahn and the FDB dump? At least
> that is what learning_test() is doing. It is possible that the packet
> is
> not sent / processed fast enough for the bridge to learn it before the
> dump.
>
I missed the call to log_test at the end of the test.
>>
>> For a mv88e6xxx test I guess I can make a check to verify that this
>> driver
>> is in use?
>
> Not in a generic forwarding test. Maybe in
> tools/testing/selftests/drivers/net/dsa/
>
> My preference would be to get as much tests as possible in
> tools/testing/selftests/net/forwarding/bridge_locked_port.sh.
I now have a roaming test in
tools/testing/selftests/net/forwarding/bridge_locked_port.sh, but it
will not pass with mv88e6xxx as it is meant for the SW bridge.
I can check if the sticky flag is set on the locked entry and then skip
the test if it is.
The bridge_locked_port.sh test is linked in
tools/testing/selftests/drivers/net/dsa/, but if I cannot check if the
mv88e6xxx driver or other switchcores are in use, I cannot do more.
>
> I'm not sure which tests you are planning for mv88e6xxx, but we can
> pass
> / fail test cases based on the flags we observe in the FDB dump. For
> example, if the entry has the "sticky" flag, then the expectation is
> that the roaming test will fail. Otherwise, it should pass.
On Thu, Aug 25, 2022 at 12:27:01PM +0200, [email protected] wrote:
> On 2022-08-25 11:23, Ido Schimmel wrote:
> > >
> > >
> > > Ido, I am not so known to the selftests, so I am wondering why I
> > > don't see
> > > either check_err or check_fail fail, whichever I use, when I think
> > > they
> > > should and then they are not really checking...
> > >
> > >
> > > local mac=10:20:30:30:20:10
> > >
> > >
> > > $MZ $h1 -t udp -a $mac -b rand
> > > bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0
> > > locked"
> > > check_err $? "MAB station move: no locked entry on first
> > > injection"
> > >
> > > $MZ $h2 -t udp -a $mac -b rand
> > > bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0
> > > locked"
> > > check_err $? "MAB station move: locked entry did not move"
> > >
> > > What is wrong here?
> >
> > Did you try adding a sleep between mausezahn and the FDB dump? At least
> > that is what learning_test() is doing. It is possible that the packet is
> > not sent / processed fast enough for the bridge to learn it before the
> > dump.
> >
>
> I missed the call to log_test at the end of the test.
>
> > >
> > > For a mv88e6xxx test I guess I can make a check to verify that this
> > > driver
> > > is in use?
> >
> > Not in a generic forwarding test. Maybe in
> > tools/testing/selftests/drivers/net/dsa/
> >
> > My preference would be to get as much tests as possible in
> > tools/testing/selftests/net/forwarding/bridge_locked_port.sh.
>
> I now have a roaming test in
> tools/testing/selftests/net/forwarding/bridge_locked_port.sh, but it will
> not pass with mv88e6xxx as it is meant for the SW bridge.
>
> I can check if the sticky flag is set on the locked entry and then skip the
> test if it is.
Instead of skipping it you can check that roaming fails when "sticky" is
set.
>
> The bridge_locked_port.sh test is linked in
> tools/testing/selftests/drivers/net/dsa/, but if I cannot check if the
> mv88e6xxx driver or other switchcores are in use, I cannot do more.
Since the behavior of the HW data path is reflected to the software
bridge and user space via "sticky" / "blackhole" / "extern_learn", you
should be able to add test cases to the generic selftest. For example,
if "blackhole" is set, then simple ping is expected to fail. Otherwise
it is expected to pass.
On 2022-08-25 13:58, Ido Schimmel wrote:
> On Thu, Aug 25, 2022 at 12:27:01PM +0200, [email protected]
> wrote:
>
> Instead of skipping it you can check that roaming fails when "sticky"
> is
> set.
>
I think that the sticky flag topic generally is beyond the MAB feature,
and it doesn't really fit into the bridge_locked_port.sh.
But anyhow I guess I can add it to the bridge_sticky_fdb.sh tests.
>>
>> The bridge_locked_port.sh test is linked in
>> tools/testing/selftests/drivers/net/dsa/, but if I cannot check if the
>> mv88e6xxx driver or other switchcores are in use, I cannot do more.
>
> Since the behavior of the HW data path is reflected to the software
> bridge and user space via "sticky" / "blackhole" / "extern_learn", you
> should be able to add test cases to the generic selftest. For example,
> if "blackhole" is set, then simple ping is expected to fail. Otherwise
> it is expected to pass.
The problem here is that the "blackhole" flag can only be set now from
the mv88e6xxx driver under a locked port, and the locked port itself
will not allow ping to work anyhow without a FDB entry free of the
"locked" flag, as the MAB tests verify.
And disabling MAB on the locked port on the mv88e6xxx will clean the
locked entries.
So I see it as a flag for future use, otherwise I will have to add a
userspace command to enable the "blackhole" flag.
I have now made station move tests for both the locked port and MAB
cases.
On 2022-08-25 11:36, Ido Schimmel wrote:
> On Tue, Aug 23, 2022 at 01:41:51PM +0200, [email protected]
> wrote:
>> On 2022-08-23 08:48, Ido Schimmel wrote:
>> >
>> > I'm not good at naming, but "blackhole" is at least consistent with what
>> > we already have for routes and nexthop objects.
>> >
>>
>> I have changed it the name "masked", as that is also the term used in
>> the
>> documentation for the zero-DPV entries, and I think that it will
>> generally
>> be a more accepted term.
>
> "blackhole" is an already accepted term and at least to me it is much
> more clear than "masked". Keep in mind that both L2 neighbours (FDB)
> and
> L3 neighbours share the same uAPI and eventually we might want to
> extend
> the use of this flag for L3 neighbours (at least Spectrum supports it),
> so it needs to make sense for both.
I have changed the name of the flag to 'blackhole', but the struct entry
in switchdev_notifier_fdb_info and a function input parameter is still
named 'masked'. If that should be changed before I send out V5, please
let me know as I hope to get this patch set accepted.