| Message ID | 20220707152930.1789437-4-netdev@kapio-technology.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Extend locked port feature with FDB locked flag (MAC-Auth/MAB) | expand |
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/202207081529.riNiUWOf-lkp@intel.com/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 <lkp@intel.com>
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
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 <netdev@kapio-technology.com> > --- 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 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 <netdev@kapio-technology.com> >> --- > > 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 Fri, Jul 08, 2022 at 11:06:24AM +0200, netdev@kapio-technology.com 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 <netdev@kapio-technology.com> > > > --- > > > > 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 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, netdev@kapio-technology.com 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, netdev@kapio-technology.com > 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/202207090438.PlGIeA4G-lkp@intel.com/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 <lkp@intel.com>
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
On Fri, Jul 08, 2022 at 02:34:25PM +0200, netdev@kapio-technology.com wrote: > On 2022-07-08 13:56, Vladimir Oltean wrote: > > On Fri, Jul 08, 2022 at 11:50:33AM +0200, netdev@kapio-technology.com > > 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, netdev@kapio-technology.com > wrote: >> On 2022-07-08 13:56, Vladimir Oltean wrote: >> > On Fri, Jul 08, 2022 at 11:50:33AM +0200, netdev@kapio-technology.com >> > 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, netdev@kapio-technology.com wrote: > On 2022-07-10 10:35, Ido Schimmel wrote: > > On Fri, Jul 08, 2022 at 02:34:25PM +0200, netdev@kapio-technology.com > > wrote: > > > On 2022-07-08 13:56, Vladimir Oltean wrote: > > > > On Fri, Jul 08, 2022 at 11:50:33AM +0200, netdev@kapio-technology.com > > > > 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, netdev@kapio-technology.com > 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, netdev@kapio-technology.com wrote: > On 2022-07-13 14:39, Ido Schimmel wrote: > > On Wed, Jul 13, 2022 at 09:09:58AM +0200, netdev@kapio-technology.com > > 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, netdev@kapio-technology.com > wrote: >> On 2022-07-13 14:39, Ido Schimmel wrote: >> > On Wed, Jul 13, 2022 at 09:09:58AM +0200, netdev@kapio-technology.com >> > 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, netdev@kapio-technology.com wrote: > On 2022-07-17 14:57, Vladimir Oltean wrote: > > On Sun, Jul 17, 2022 at 02:21:47PM +0200, netdev@kapio-technology.com > > wrote: > > > On 2022-07-13 14:39, Ido Schimmel wrote: > > > > On Wed, Jul 13, 2022 at 09:09:58AM +0200, netdev@kapio-technology.com > > > > 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, netdev@kapio-technology.com > wrote: >> On 2022-07-17 14:57, Vladimir Oltean wrote: >> > On Sun, Jul 17, 2022 at 02:21:47PM +0200, netdev@kapio-technology.com >> > wrote: >> > > On 2022-07-13 14:39, Ido Schimmel wrote: >> > > > On Wed, Jul 13, 2022 at 09:09:58AM +0200, netdev@kapio-technology.com >> > > > 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, netdev@kapio-technology.com 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, netdev@kapio-technology.com wrote: > On 2022-07-13 14:39, Ido Schimmel wrote: > > On Wed, Jul 13, 2022 at 09:09:58AM +0200, netdev@kapio-technology.com > > 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:20, Ido Schimmel wrote: > On Sun, Jul 17, 2022 at 02:21:47PM +0200, netdev@kapio-technology.com > wrote: >> On 2022-07-13 14:39, Ido Schimmel wrote: >> > On Wed, Jul 13, 2022 at 09:09:58AM +0200, netdev@kapio-technology.com >> > 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 2022-07-17 17:08, Vladimir Oltean wrote: > On Sun, Jul 17, 2022 at 04:57:50PM +0200, netdev@kapio-technology.com > 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 Fri, Jul 08, 2022 at 11:50:33AM +0200, netdev@kapio-technology.com 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, netdev@kapio-technology.com 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 Sun, Jul 17, 2022 at 05:53:22PM +0200, netdev@kapio-technology.com 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 Thu, Jul 21, 2022 at 02:59:35PM +0300, Vladimir Oltean wrote: > On Sun, Jul 17, 2022 at 05:53:22PM +0200, netdev@kapio-technology.com 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, netdev@kapio-technology.com > 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, netdev@kapio-technology.com 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, netdev@kapio-technology.com > 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, netdev@kapio-technology.com wrote: > On 2022-08-09 11:20, Ido Schimmel wrote: > > On Mon, Aug 01, 2022 at 05:33:49PM +0200, netdev@kapio-technology.com > > 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, netdev@kapio-technology.com > wrote: >> On 2022-08-09 11:20, Ido Schimmel wrote: >> > On Mon, Aug 01, 2022 at 05:33:49PM +0200, netdev@kapio-technology.com >> > 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, netdev@kapio-technology.com wrote: > On 2022-08-10 09:21, Ido Schimmel wrote: > > On Tue, Aug 09, 2022 at 10:00:49PM +0200, netdev@kapio-technology.com > > wrote: > > > On 2022-08-09 11:20, Ido Schimmel wrote: > > > > On Mon, Aug 01, 2022 at 05:33:49PM +0200, netdev@kapio-technology.com > > > > 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. >> > 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 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, netdev@kapio-technology.com 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.
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;
Ignore locked fdb entries coming in on all drivers. Signed-off-by: Hans Schultz <netdev@kapio-technology.com> --- 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(-)