diff mbox series

[v5,net-next,6/6] selftests: forwarding: add test of MAC-Auth Bypass to locked port tests

Message ID 20220826114538.705433-7-netdev@kapio-technology.com (mailing list archive)
State New, archived
Headers show
Series Extend locked port feature with FDB locked flag (MAC-Auth/MAB) | expand

Commit Message

Hans Schultz Aug. 26, 2022, 11:45 a.m. UTC
Verify that the MAC-Auth mechanism works by adding a FDB entry with the
locked flag set, denying access until the FDB entry is replaced with a
FDB entry without the locked flag set.

Also add a test that verifies that sticky FDB entries cannot roam.

Signed-off-by: Hans Schultz <netdev@kapio-technology.com>
---
 .../net/forwarding/bridge_locked_port.sh      | 107 +++++++++++++++++-
 .../net/forwarding/bridge_sticky_fdb.sh       |  21 +++-
 2 files changed, 126 insertions(+), 2 deletions(-)

Comments

Ido Schimmel Aug. 27, 2022, 6:21 p.m. UTC | #1
On Fri, Aug 26, 2022 at 01:45:38PM +0200, Hans Schultz wrote:
> -ALL_TESTS="locked_port_ipv4 locked_port_ipv6 locked_port_vlan"
> +ALL_TESTS="
> +	locked_port_ipv4
> +	locked_port_ipv6
> +	locked_port_vlan
> +	locked_port_mab
> +	locked_port_station_move
> +	locked_port_mab_station_move
> +"
> +
>  NUM_NETIFS=4
>  CHECK_TC="no"
>  source lib.sh
> @@ -166,6 +174,103 @@ locked_port_ipv6()
>  	log_test "Locked port ipv6"
>  }
>  
> +locked_port_mab()
> +{
> +	RET=0
> +	check_locked_port_support || return 0
> +
> +	ping_do $h1 192.0.2.2
> +	check_err $? "MAB: Ping did not work before locking port"
> +
> +	bridge link set dev $swp1 locked on
> +	bridge link set dev $swp1 learning on

"locked on learning on" is counter intuitive and IMO very much a
misconfiguration that we should have disallowed when the "locked" option
was introduced. It is my understanding that the only reason we are even
talking about it is because mv88e6xxx needs it for MAB for some reason.
Please avoid leaking this implementation detail to user space and
instead use the "MAB" flag to enable learning if you need it in
mv88e6xxx.

> +	if ! bridge link set dev $swp1 mab on 2>/dev/null; then
> +		echo "SKIP: iproute2 too old; MacAuth feature not supported."
> +		return $ksft_skip
> +	fi

Please add a similar function to check_locked_port_support() and invoke
it next to it.

> +
> +	ping_do $h1 192.0.2.2
> +	check_fail $? "MAB: Ping worked on locked port without FDB entry"
> +
> +	bridge fdb show | grep `mac_get $h1` | grep -q "locked"
> +	check_err $? "MAB: No locked fdb entry after ping on locked port"
> +
> +	bridge fdb replace `mac_get $h1` dev $swp1 master static
> +
> +	ping_do $h1 192.0.2.2
> +	check_err $? "MAB: Ping did not work with fdb entry without locked flag"
> +
> +	bridge fdb del `mac_get $h1` dev $swp1 master

Missing:

bridge link set dev $swp1 mab off

> +	bridge link set dev $swp1 learning off

Can be removed assuming we get rid of "learning on" above.

> +	bridge link set dev $swp1 locked off
> +
> +	log_test "Locked port MAB"
> +}
> +
> +# No roaming allowed to a simple locked port
> +locked_port_station_move()
> +{
> +	local mac=a0:b0:c0:c0:b0:a0
> +
> +	RET=0
> +	check_locked_port_support || return 0
> +
> +	bridge link set dev $swp1 locked on
> +	bridge link set dev $swp1 learning on

Same comment as above. 

> +
> +	$MZ $h1 -q -t udp -a $mac -b rand
> +	bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0"
> +	check_fail $? "Locked port station move: FDB entry on first injection"
> +
> +	$MZ $h2 -q -t udp -a $mac -b rand
> +	bridge fdb show dev $swp2 | grep -q "$mac vlan 1 master br0"
> +	check_err $? "Locked port station move: Entry not found on unlocked port"

Looks like this is going to fail with offloaded data path as according
to fdb_print_flags() in iproute2 both the "extern_learn" and "offload"
flags will be printed before "master".

I suggest using "bridge fdb get" instead (didn't test, might need small
tweaks, but you will figure it):

bridge fdb get $mac br br0 vlan 1 master 2> /dev/null | grep -q "$swp2"

Same in other places where "bridge fdb show" is used.

> +
> +	$MZ $h1 -q -t udp -a $mac -b rand
> +	bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0"
> +	check_fail $? "Locked port station move: entry roamed to locked port"

Missing:

bridge link set dev $swp1 locked off
bridge fdb del $mac dev $swp1 master vlan 1

> +
> +	log_test "Locked port station move"
> +}
> +
> +# Roaming to and from a MAB enabled port should work if sticky flag is not set
> +locked_port_mab_station_move()
> +{
> +	local mac=10:20:30:30:20:10
> +
> +	RET=0
> +	check_locked_port_support || return 0
> +
> +	bridge link set dev $swp1 locked on
> +	bridge link set dev $swp1 learning on

Same comment as above.

> +	if ! bridge link set dev $swp1 mab on 2>/dev/null; then

Same comment as above.

> +		echo "SKIP: iproute2 too old; MacAuth feature not supported."
> +		return $ksft_skip
> +	fi
> +
> +	$MZ $h1 -q -t udp -a $mac -b rand
> +	if bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0" | grep -q sticky; then

Will need to change to "permanent" instead of "sticky".

> +		echo "SKIP: Roaming not possible with sticky flag, run sticky flag roaming test"
> +		return $ksft_skip

Missing cleanup before the return.

> +	fi
> +
> +	bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0 locked"
> +	check_err $? "MAB station move: no locked entry on first injection"
> +
> +	$MZ $h2 -q -t udp -a $mac -b rand
> +	bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0 locked"
> +	check_fail $? "MAB station move: locked entry did not move"
> +
> +	bridge fdb show dev $swp2 | grep -q "$mac vlan 1 master br0"

Need to check that it does not roam with the "locked" flag set.

> +	check_err $? "MAB station move: roamed entry not found"
> +
> +	$MZ $h1 -q -t udp -a $mac -b rand
> +	bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0 locked"
> +	check_err $? "MAB station move: entry did not roam back to locked port"

This will need to change to "check_fail" assuming we don't allow roaming
from an authorized port to an unauthorized port, which I believe makes
sense.

> +

Missing cleanup.

> +	log_test "Locked port MAB station move"
> +}
> +
>  trap cleanup EXIT
>  
>  setup_prepare
Hans Schultz Aug. 28, 2022, noon UTC | #2
On 2022-08-27 20:21, Ido Schimmel wrote:
> On Fri, Aug 26, 2022 at 01:45:38PM +0200, Hans Schultz wrote:
>> +locked_port_mab()
>> +{
>> +	RET=0
>> +	check_locked_port_support || return 0
>> +
>> +	ping_do $h1 192.0.2.2
>> +	check_err $? "MAB: Ping did not work before locking port"
>> +
>> +	bridge link set dev $swp1 locked on
>> +	bridge link set dev $swp1 learning on
> 
> "locked on learning on" is counter intuitive and IMO very much a
> misconfiguration that we should have disallowed when the "locked" 
> option
> was introduced. It is my understanding that the only reason we are even
> talking about it is because mv88e6xxx needs it for MAB for some reason.

As the way mv88e6xxx implements "learning off" is to remove port 
association for ingress packets on a port, but that breaks many other 
things such as refreshing ATU entries and violation interrupts, so it is 
needed and the question is then what is the worst to have 'learning on' 
on a locked port or to have the locked port enabling learning in the 
driver silently?

Opinions seem to differ. Note that even on locked ports without MAB, 
port association on ingress is still needed in future as I have a 
dynamic ATU patch set coming, that uses age out violation and hardware 
refreshing to let the hardware keep the dynamic entries as long as the 
authorized station is sending, but will age the entry out if the station 
keeps silent for the ageing time. But that patch set is dependent on 
this patch set, and I don't think I can send it before this is 
accepted...

> Please avoid leaking this implementation detail to user space and
> instead use the "MAB" flag to enable learning if you need it in
> mv88e6xxx.
>
Ido Schimmel Aug. 29, 2022, 7:40 a.m. UTC | #3
On Sun, Aug 28, 2022 at 02:00:29PM +0200, netdev@kapio-technology.com wrote:
> On 2022-08-27 20:21, Ido Schimmel wrote:
> > On Fri, Aug 26, 2022 at 01:45:38PM +0200, Hans Schultz wrote:
> > > +locked_port_mab()
> > > +{
> > > +	RET=0
> > > +	check_locked_port_support || return 0
> > > +
> > > +	ping_do $h1 192.0.2.2
> > > +	check_err $? "MAB: Ping did not work before locking port"
> > > +
> > > +	bridge link set dev $swp1 locked on
> > > +	bridge link set dev $swp1 learning on
> > 
> > "locked on learning on" is counter intuitive and IMO very much a
> > misconfiguration that we should have disallowed when the "locked" option
> > was introduced. It is my understanding that the only reason we are even
> > talking about it is because mv88e6xxx needs it for MAB for some reason.
> 
> As the way mv88e6xxx implements "learning off" is to remove port association
> for ingress packets on a port, but that breaks many other things such as
> refreshing ATU entries and violation interrupts, so it is needed and the
> question is then what is the worst to have 'learning on' on a locked port or
> to have the locked port enabling learning in the driver silently?
> 
> Opinions seem to differ. Note that even on locked ports without MAB, port
> association on ingress is still needed in future as I have a dynamic ATU
> patch set coming, that uses age out violation and hardware refreshing to let
> the hardware keep the dynamic entries as long as the authorized station is
> sending, but will age the entry out if the station keeps silent for the
> ageing time. But that patch set is dependent on this patch set, and I don't
> think I can send it before this is accepted...

Can you explain how you envision user space to work once everything is
merged? I want to make sure we have the full picture before more stuff
is merged. From what you describe, I expect the following:

1. Create topology, assuming two unauthorized ports:

# ip link add name br0 type bridge no_linklocal_learn 1 (*)
# ip link set dev swp1 master br0
# ip link set dev swp2 master br0
# bridge link set dev swp1 learning on locked on
# bridge link set dev swp2 learning on locked on
# ip link set dev swp1 up
# ip link set dev swp2 up
# ip link set dev br0 up

2. Assuming h1 behind swp1 was authorized using 802.1X:

# bridge fdb replace $H1_MAC dev swp1 master dynamic

3. Assuming 802.1X authentication failed for h2 behind swp2, enable MAB:

# bridge link set dev swp2 mab on

4. Assuming $H2_MAC is in our allow list:

# bridge fdb replace $H2_MAC dev swp2 master dynamic

Learning is on in order to refresh the dynamic entries that user space
installed.

(*) Need to add support for this option in iproute2. Already exposed
over netlink (see 'IFLA_BR_MULTI_BOOLOPT').
Hans Schultz Aug. 29, 2022, 8:01 a.m. UTC | #4
On 2022-08-29 09:40, Ido Schimmel wrote:
> On Sun, Aug 28, 2022 at 02:00:29PM +0200, netdev@kapio-technology.com 
> wrote:
>> On 2022-08-27 20:21, Ido Schimmel wrote:
>> > On Fri, Aug 26, 2022 at 01:45:38PM +0200, Hans Schultz wrote:
>> > > +locked_port_mab()
>> > > +{
>> > > +	RET=0
>> > > +	check_locked_port_support || return 0
>> > > +
>> > > +	ping_do $h1 192.0.2.2
>> > > +	check_err $? "MAB: Ping did not work before locking port"
>> > > +
>> > > +	bridge link set dev $swp1 locked on
>> > > +	bridge link set dev $swp1 learning on
>> >
>> > "locked on learning on" is counter intuitive and IMO very much a
>> > misconfiguration that we should have disallowed when the "locked" option
>> > was introduced. It is my understanding that the only reason we are even
>> > talking about it is because mv88e6xxx needs it for MAB for some reason.
>> 
>> As the way mv88e6xxx implements "learning off" is to remove port 
>> association
>> for ingress packets on a port, but that breaks many other things such 
>> as
>> refreshing ATU entries and violation interrupts, so it is needed and 
>> the
>> question is then what is the worst to have 'learning on' on a locked 
>> port or
>> to have the locked port enabling learning in the driver silently?
>> 
>> Opinions seem to differ. Note that even on locked ports without MAB, 
>> port
>> association on ingress is still needed in future as I have a dynamic 
>> ATU
>> patch set coming, that uses age out violation and hardware refreshing 
>> to let
>> the hardware keep the dynamic entries as long as the authorized 
>> station is
>> sending, but will age the entry out if the station keeps silent for 
>> the
>> ageing time. But that patch set is dependent on this patch set, and I 
>> don't
>> think I can send it before this is accepted...
> 
> Can you explain how you envision user space to work once everything is
> merged? I want to make sure we have the full picture before more stuff
> is merged. From what you describe, I expect the following:
> 
> 1. Create topology, assuming two unauthorized ports:
> 
> # ip link add name br0 type bridge no_linklocal_learn 1 (*)
> # ip link set dev swp1 master br0
> # ip link set dev swp2 master br0
> # bridge link set dev swp1 learning on locked on
> # bridge link set dev swp2 learning on locked on

The final decision on this rests with you I would say. Actually I forgot 
to remove the port association in the driver in this version.

> # ip link set dev swp1 up
> # ip link set dev swp2 up
> # ip link set dev br0 up
> 
> 2. Assuming h1 behind swp1 was authorized using 802.1X:
> 
> # bridge fdb replace $H1_MAC dev swp1 master dynamic

With the new MAB flag 'replace' is not needed when MAB is not enabled.

> 
> 3. Assuming 802.1X authentication failed for h2 behind swp2, enable 
> MAB:
> 
> # bridge link set dev swp2 mab on
> 
> 4. Assuming $H2_MAC is in our allow list:
> 
> # bridge fdb replace $H2_MAC dev swp2 master dynamic
> 
> Learning is on in order to refresh the dynamic entries that user space
> installed.

Yes, port association is needed for those reasons. :-)

> 
> (*) Need to add support for this option in iproute2. Already exposed
> over netlink (see 'IFLA_BR_MULTI_BOOLOPT').

Should I do that in this patch set?
Hans Schultz Aug. 29, 2022, 8:55 a.m. UTC | #5
On 2022-08-29 09:40, Ido Schimmel wrote:
> On Sun, Aug 28, 2022 at 02:00:29PM +0200, netdev@kapio-technology.com 
> wrote:
>> On 2022-08-27 20:21, Ido Schimmel wrote:
>> > "locked on learning on" is counter intuitive and IMO very much a
>> > misconfiguration that we should have disallowed when the "locked" option
>> > was introduced. It is my understanding that the only reason we are even
>> > talking about it is because mv88e6xxx needs it for MAB for some reason.
>> 
>> As the way mv88e6xxx implements "learning off" is to remove port 
>> association
>> for ingress packets on a port, but that breaks many other things such 
>> as
>> refreshing ATU entries and violation interrupts, so it is needed and 
>> the
>> question is then what is the worst to have 'learning on' on a locked 
>> port or
>> to have the locked port enabling learning in the driver silently?
>> 
>> Opinions seem to differ. Note that even on locked ports without MAB, 
>> port
>> association on ingress is still needed in future as I have a dynamic 
>> ATU
>> patch set coming, that uses age out violation and hardware refreshing 
>> to let
>> the hardware keep the dynamic entries as long as the authorized 
>> station is
>> sending, but will age the entry out if the station keeps silent for 
>> the
>> ageing time. But that patch set is dependent on this patch set, and I 
>> don't
>> think I can send it before this is accepted...
> 

> # bridge link set dev swp1 learning on locked on
> # bridge link set dev swp2 learning on locked on

As we must think in how most drivers work, which I am not knowledgeable 
of, I think that it is probably the best to think of the way mv88e6xxx 
works as an outlier.
If that is true, then I think the best option is to go with:
#bridge link set dev $swp1 learning off locked on
#bridge link set dev $swp2 learning off locked on

Then the cleanup side will just be:
#bridge link set dev $swp1 locked off
#bridge link set dev $swp2 locked off

The state 'learning off' is then consistent with the behavior of both 
the bridge and driver after the cleanup.
Ido Schimmel Aug. 29, 2022, 11:32 a.m. UTC | #6
On Mon, Aug 29, 2022 at 10:01:18AM +0200, netdev@kapio-technology.com wrote:
> On 2022-08-29 09:40, Ido Schimmel wrote:
> > On Sun, Aug 28, 2022 at 02:00:29PM +0200, netdev@kapio-technology.com
> > wrote:
> > > On 2022-08-27 20:21, Ido Schimmel wrote:
> > > > On Fri, Aug 26, 2022 at 01:45:38PM +0200, Hans Schultz wrote:
> > > > > +locked_port_mab()
> > > > > +{
> > > > > +	RET=0
> > > > > +	check_locked_port_support || return 0
> > > > > +
> > > > > +	ping_do $h1 192.0.2.2
> > > > > +	check_err $? "MAB: Ping did not work before locking port"
> > > > > +
> > > > > +	bridge link set dev $swp1 locked on
> > > > > +	bridge link set dev $swp1 learning on
> > > >
> > > > "locked on learning on" is counter intuitive and IMO very much a
> > > > misconfiguration that we should have disallowed when the "locked" option
> > > > was introduced. It is my understanding that the only reason we are even
> > > > talking about it is because mv88e6xxx needs it for MAB for some reason.
> > > 
> > > As the way mv88e6xxx implements "learning off" is to remove port
> > > association
> > > for ingress packets on a port, but that breaks many other things
> > > such as
> > > refreshing ATU entries and violation interrupts, so it is needed and
> > > the
> > > question is then what is the worst to have 'learning on' on a locked
> > > port or
> > > to have the locked port enabling learning in the driver silently?
> > > 
> > > Opinions seem to differ. Note that even on locked ports without MAB,
> > > port
> > > association on ingress is still needed in future as I have a dynamic
> > > ATU
> > > patch set coming, that uses age out violation and hardware
> > > refreshing to let
> > > the hardware keep the dynamic entries as long as the authorized
> > > station is
> > > sending, but will age the entry out if the station keeps silent for
> > > the
> > > ageing time. But that patch set is dependent on this patch set, and
> > > I don't
> > > think I can send it before this is accepted...
> > 
> > Can you explain how you envision user space to work once everything is
> > merged? I want to make sure we have the full picture before more stuff
> > is merged. From what you describe, I expect the following:
> > 
> > 1. Create topology, assuming two unauthorized ports:
> > 
> > # ip link add name br0 type bridge no_linklocal_learn 1 (*)
> > # ip link set dev swp1 master br0
> > # ip link set dev swp2 master br0
> > # bridge link set dev swp1 learning on locked on
> > # bridge link set dev swp2 learning on locked on
> 
> The final decision on this rests with you I would say.

If the requirement for this feature (with or without MAB) is to work
with dynamic entries (which is not what is currently implemented in the
selftests), then learning needs to be enabled for the sole reason of
refreshing the dynamic entries added by user space. That is, updating
'fdb->updated' with current jiffies value.

So, is this the requirement? I checked the hostapd fork you posted some
time ago and I get the impression that the answer is yes [1], but I want
to verify I'm not missing something.

[1] https://github.com/westermo/hostapd/commit/95dc96f9e89131b2319f5eae8ae7ac99868b7cd0#diff-338b6fad34b4bdb015d7d96930974bd96796b754257473b6c91527789656d6edR11


> Actually I forgot to remove the port association in the driver in this
> version.
> 
> > # ip link set dev swp1 up
> > # ip link set dev swp2 up
> > # ip link set dev br0 up
> > 
> > 2. Assuming h1 behind swp1 was authorized using 802.1X:
> > 
> > # bridge fdb replace $H1_MAC dev swp1 master dynamic
> 
> With the new MAB flag 'replace' is not needed when MAB is not enabled.

Yes, but replace works in both cases.

> 
> > 
> > 3. Assuming 802.1X authentication failed for h2 behind swp2, enable MAB:
> > 
> > # bridge link set dev swp2 mab on
> > 
> > 4. Assuming $H2_MAC is in our allow list:
> > 
> > # bridge fdb replace $H2_MAC dev swp2 master dynamic
> > 
> > Learning is on in order to refresh the dynamic entries that user space
> > installed.
> 
> Yes, port association is needed for those reasons. :-)

Given that the current tests use "static" entries that cannot age, is
there a reason to have "learning on"?

> 
> > 
> > (*) Need to add support for this option in iproute2. Already exposed
> > over netlink (see 'IFLA_BR_MULTI_BOOLOPT').
> 
> Should I do that in this patch set?

No, I'm saying that this option is already exposed over netlink, but
missing iproute2 support. No kernel changes needed.
Hans Schultz Aug. 29, 2022, 12:04 p.m. UTC | #7
On 2022-08-29 13:32, Ido Schimmel wrote:
>> The final decision on this rests with you I would say.
> 
> If the requirement for this feature (with or without MAB) is to work
> with dynamic entries (which is not what is currently implemented in the
> selftests), then learning needs to be enabled for the sole reason of
> refreshing the dynamic entries added by user space. That is, updating
> 'fdb->updated' with current jiffies value.
> 
> So, is this the requirement? I checked the hostapd fork you posted some
> time ago and I get the impression that the answer is yes [1], but I 
> want
> to verify I'm not missing something.
> 
> [1] 
> https://github.com/westermo/hostapd/commit/95dc96f9e89131b2319f5eae8ae7ac99868b7cd0#diff-338b6fad34b4bdb015d7d96930974bd96796b754257473b6c91527789656d6edR11
> 
> 

I cannot say that it is a requirement with respect to the bridge 
implementation, but it is with the driver implementation. But you are 
right that it is to be used with dynamic entries.

>> > # ip link set dev swp1 up
>> > # ip link set dev swp2 up
>> > # ip link set dev br0 up
>> >
>> > 2. Assuming h1 behind swp1 was authorized using 802.1X:
>> >
>> > # bridge fdb replace $H1_MAC dev swp1 master dynamic
>> 
>> With the new MAB flag 'replace' is not needed when MAB is not enabled.
> 
> Yes, but replace works in both cases.
> 

Yes, of course.

>> 
>> >
>> > 3. Assuming 802.1X authentication failed for h2 behind swp2, enable MAB:
>> >
>> > # bridge link set dev swp2 mab on
>> >
>> > 4. Assuming $H2_MAC is in our allow list:
>> >
>> > # bridge fdb replace $H2_MAC dev swp2 master dynamic
>> >
>> > Learning is on in order to refresh the dynamic entries that user space
>> > installed.
>> 
>> Yes, port association is needed for those reasons. :-)
> 
> Given that the current tests use "static" entries that cannot age, is
> there a reason to have "learning on"?
> 

Port association is needed for MAB to work at all on mv88e6xxx, but for 
802.1X port association is only needed for dynamic ATU entries.

>> 
>> >
>> > (*) Need to add support for this option in iproute2. Already exposed
>> > over netlink (see 'IFLA_BR_MULTI_BOOLOPT').
>> 
>> Should I do that in this patch set?
> 
> No, I'm saying that this option is already exposed over netlink, but
> missing iproute2 support. No kernel changes needed.

Oh yes, I meant in the iproute2 accompanying patch set to this one?
Ido Schimmel Aug. 29, 2022, 2:37 p.m. UTC | #8
On Mon, Aug 29, 2022 at 02:04:42PM +0200, netdev@kapio-technology.com wrote:
> On 2022-08-29 13:32, Ido Schimmel wrote:
> > > The final decision on this rests with you I would say.
> > 
> > If the requirement for this feature (with or without MAB) is to work
> > with dynamic entries (which is not what is currently implemented in the
> > selftests), then learning needs to be enabled for the sole reason of
> > refreshing the dynamic entries added by user space. That is, updating
> > 'fdb->updated' with current jiffies value.
> > 
> > So, is this the requirement? I checked the hostapd fork you posted some
> > time ago and I get the impression that the answer is yes [1], but I want
> > to verify I'm not missing something.
> > 
> > [1] https://github.com/westermo/hostapd/commit/95dc96f9e89131b2319f5eae8ae7ac99868b7cd0#diff-338b6fad34b4bdb015d7d96930974bd96796b754257473b6c91527789656d6edR11
> > 
> > 
> 
> I cannot say that it is a requirement with respect to the bridge
> implementation, but it is with the driver implementation. But you are right
> that it is to be used with dynamic entries.

OK, so it's a requirement for both since we need both data paths to act
the same.

[...]

> Port association is needed for MAB to work at all on mv88e6xxx, but for
> 802.1X port association is only needed for dynamic ATU entries.

Ageing of dynamic entries in the bridge requires learning to be on as
well, but in these test cases you are only using static entries and
there is no reason to enable learning in the bridge for that. I prefer
not to leak this mv88e6xxx implementation detail to user space and
instead have the driver enable port association based on whether
"learning" or "mab" is on.

[...]

> Oh yes, I meant in the iproute2 accompanying patch set to this one?

You can send it as a standalone patch to iproute2-next:
https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git

Subject prefix should be "[PATCH iproute2-next]". See this commit for
reference:
https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=d2eecb9d1d4823a04431debd990824a5d610bfcf
Hans Schultz Aug. 29, 2022, 3:08 p.m. UTC | #9
On 2022-08-29 16:37, Ido Schimmel wrote:
> On Mon, Aug 29, 2022 at 02:04:42PM +0200, netdev@kapio-technology.com 
> wrote:
>> On 2022-08-29 13:32, Ido Schimmel wrote:
>> Port association is needed for MAB to work at all on mv88e6xxx, but 
>> for
>> 802.1X port association is only needed for dynamic ATU entries.
> 
> Ageing of dynamic entries in the bridge requires learning to be on as
> well, but in these test cases you are only using static entries and
> there is no reason to enable learning in the bridge for that. I prefer
> not to leak this mv88e6xxx implementation detail to user space and
> instead have the driver enable port association based on whether
> "learning" or "mab" is on.
> 

Then it makes most sense to have the mv88e6xxx driver enable port 
association when then port is locked, as it does now.
Ido Schimmel Aug. 29, 2022, 4:03 p.m. UTC | #10
On Mon, Aug 29, 2022 at 05:08:23PM +0200, netdev@kapio-technology.com wrote:
> On 2022-08-29 16:37, Ido Schimmel wrote:
> > On Mon, Aug 29, 2022 at 02:04:42PM +0200, netdev@kapio-technology.com
> > wrote:
> > > On 2022-08-29 13:32, Ido Schimmel wrote:
> > > Port association is needed for MAB to work at all on mv88e6xxx, but
> > > for
> > > 802.1X port association is only needed for dynamic ATU entries.
> > 
> > Ageing of dynamic entries in the bridge requires learning to be on as
> > well, but in these test cases you are only using static entries and
> > there is no reason to enable learning in the bridge for that. I prefer
> > not to leak this mv88e6xxx implementation detail to user space and
> > instead have the driver enable port association based on whether
> > "learning" or "mab" is on.
> > 
> 
> Then it makes most sense to have the mv88e6xxx driver enable port
> association when then port is locked, as it does now.

As you wish, but like you wrote "802.1X port association is only needed
for dynamic ATU entries" and in this case user space needs to enable
learning (for refresh only) so you can really key off learning on
"learning || mab". User space can decide to lock the port and work with
static entries and then learning is not required.
Hans Schultz Aug. 29, 2022, 4:07 p.m. UTC | #11
On 2022-08-27 20:21, Ido Schimmel wrote:
> On Fri, Aug 26, 2022 at 01:45:38PM +0200, Hans Schultz wrote:

>> +	$MZ $h2 -q -t udp -a $mac -b rand
>> +	bridge fdb show dev $swp2 | grep -q "$mac vlan 1 master br0"
>> +	check_err $? "Locked port station move: Entry not found on unlocked 
>> port"
> 
> Looks like this is going to fail with offloaded data path as according
> to fdb_print_flags() in iproute2 both the "extern_learn" and "offload"
> flags will be printed before "master".
> 

The output shows like:
74:e1:e1:2c:4f:18 dev eth8 vlan 1 master br0 extern_learn offload sticky 
locked blackhole

"sticky" will of course become "permanent", but I can still make it more 
resilient by piping grep.

I suppose that I will keep the "sticky_no_roaming" test even though it 
is not really needed here anymore?
Hans Schultz Aug. 29, 2022, 4:13 p.m. UTC | #12
On 2022-08-29 18:03, Ido Schimmel wrote:
> On Mon, Aug 29, 2022 at 05:08:23PM +0200, netdev@kapio-technology.com 
> wrote:
>> On 2022-08-29 16:37, Ido Schimmel wrote:
>> > On Mon, Aug 29, 2022 at 02:04:42PM +0200, netdev@kapio-technology.com
>> > wrote:
>> > > On 2022-08-29 13:32, Ido Schimmel wrote:
>> > > Port association is needed for MAB to work at all on mv88e6xxx, but
>> > > for
>> > > 802.1X port association is only needed for dynamic ATU entries.
>> >
>> > Ageing of dynamic entries in the bridge requires learning to be on as
>> > well, but in these test cases you are only using static entries and
>> > there is no reason to enable learning in the bridge for that. I prefer
>> > not to leak this mv88e6xxx implementation detail to user space and
>> > instead have the driver enable port association based on whether
>> > "learning" or "mab" is on.
>> >
>> 
>> Then it makes most sense to have the mv88e6xxx driver enable port
>> association when then port is locked, as it does now.
> 
> As you wish, but like you wrote "802.1X port association is only needed
> for dynamic ATU entries" and in this case user space needs to enable
> learning (for refresh only) so you can really key off learning on
> "learning || mab". User space can decide to lock the port and work with
> static entries and then learning is not required.

I will of course remove all "learning on" in the selftests, which is 
what I think you are referring to. In the previous I am referring to the 
code in the driver itself which I understand shall turn on port 
association with locked ports, e.g. no need for "learning on" when using 
the feature in general outside selftests...
Ido Schimmel Sept. 3, 2022, 2:47 p.m. UTC | #13
On Mon, Aug 29, 2022 at 06:13:14PM +0200, netdev@kapio-technology.com wrote:
> On 2022-08-29 18:03, Ido Schimmel wrote:
> > On Mon, Aug 29, 2022 at 05:08:23PM +0200, netdev@kapio-technology.com
> > wrote:
> > > On 2022-08-29 16:37, Ido Schimmel wrote:
> > > > On Mon, Aug 29, 2022 at 02:04:42PM +0200, netdev@kapio-technology.com
> > > > wrote:
> > > > > On 2022-08-29 13:32, Ido Schimmel wrote:
> > > > > Port association is needed for MAB to work at all on mv88e6xxx, but
> > > > > for
> > > > > 802.1X port association is only needed for dynamic ATU entries.
> > > >
> > > > Ageing of dynamic entries in the bridge requires learning to be on as
> > > > well, but in these test cases you are only using static entries and
> > > > there is no reason to enable learning in the bridge for that. I prefer
> > > > not to leak this mv88e6xxx implementation detail to user space and
> > > > instead have the driver enable port association based on whether
> > > > "learning" or "mab" is on.
> > > >
> > > 
> > > Then it makes most sense to have the mv88e6xxx driver enable port
> > > association when then port is locked, as it does now.
> > 
> > As you wish, but like you wrote "802.1X port association is only needed
> > for dynamic ATU entries" and in this case user space needs to enable
> > learning (for refresh only) so you can really key off learning on
> > "learning || mab". User space can decide to lock the port and work with
> > static entries and then learning is not required.
> 
> I will of course remove all "learning on" in the selftests, which is what I
> think you are referring to. In the previous I am referring to the code in
> the driver itself which I understand shall turn on port association with
> locked ports, e.g. no need for "learning on" when using the feature in
> general outside selftests...

"learning on" is needed when dynamic FDB entries are used to authorize
hosts. Without learning being enabled, the bridge driver (or the
underlying hardware) will not refresh the entries during forwarding and
they will age out, resulting in packet loss until the hosts are
re-authorized.

Given the current test cases only use static entries, there is no need
to enable learning on locked ports. This will change when test cases are
added with dynamic entries.

Regarding mv88e6xxx, my understanding is that you also need learning
enabled for MAB (I assume for the violation interrupts). Therefore, for
mv88e6xxx, learning can be enabled if learning is on or MAB is on.
Enabling it based on whether the port is locked or not seems inaccurate.
Ido Schimmel Sept. 3, 2022, 2:49 p.m. UTC | #14
On Mon, Aug 29, 2022 at 06:07:59PM +0200, netdev@kapio-technology.com wrote:
> On 2022-08-27 20:21, Ido Schimmel wrote:
> > On Fri, Aug 26, 2022 at 01:45:38PM +0200, Hans Schultz wrote:
> 
> > > +	$MZ $h2 -q -t udp -a $mac -b rand
> > > +	bridge fdb show dev $swp2 | grep -q "$mac vlan 1 master br0"
> > > +	check_err $? "Locked port station move: Entry not found on
> > > unlocked port"
> > 
> > Looks like this is going to fail with offloaded data path as according
> > to fdb_print_flags() in iproute2 both the "extern_learn" and "offload"
> > flags will be printed before "master".
> > 
> 
> The output shows like:
> 74:e1:e1:2c:4f:18 dev eth8 vlan 1 master br0 extern_learn offload sticky
> locked blackhole
> 
> "sticky" will of course become "permanent", but I can still make it more
> resilient by piping grep.

OK.

> 
> I suppose that I will keep the "sticky_no_roaming" test even though it is
> not really needed here anymore?

You can send it separately if you want.
Hans Schultz Sept. 7, 2022, 9:10 p.m. UTC | #15
On 2022-09-03 16:47, Ido Schimmel wrote:
> On Mon, Aug 29, 2022 at 06:13:14PM +0200, netdev@kapio-technology.com 
> wrote:
>> On 2022-08-29 18:03, Ido Schimmel wrote:
>> > On Mon, Aug 29, 2022 at 05:08:23PM +0200, netdev@kapio-technology.com
>> > wrote:
>> > > On 2022-08-29 16:37, Ido Schimmel wrote:
>> > > > On Mon, Aug 29, 2022 at 02:04:42PM +0200, netdev@kapio-technology.com
>> > > > wrote:
>> > > > > On 2022-08-29 13:32, Ido Schimmel wrote:
>> > > > > Port association is needed for MAB to work at all on mv88e6xxx, but
>> > > > > for
>> > > > > 802.1X port association is only needed for dynamic ATU entries.
>> > > >
>> > > > Ageing of dynamic entries in the bridge requires learning to be on as
>> > > > well, but in these test cases you are only using static entries and
>> > > > there is no reason to enable learning in the bridge for that. I prefer
>> > > > not to leak this mv88e6xxx implementation detail to user space and
>> > > > instead have the driver enable port association based on whether
>> > > > "learning" or "mab" is on.
>> > > >
>> > >
>> > > Then it makes most sense to have the mv88e6xxx driver enable port
>> > > association when then port is locked, as it does now.
>> >
>> > As you wish, but like you wrote "802.1X port association is only needed
>> > for dynamic ATU entries" and in this case user space needs to enable
>> > learning (for refresh only) so you can really key off learning on
>> > "learning || mab". User space can decide to lock the port and work with
>> > static entries and then learning is not required.
>> 
>> I will of course remove all "learning on" in the selftests, which is 
>> what I
>> think you are referring to. In the previous I am referring to the code 
>> in
>> the driver itself which I understand shall turn on port association 
>> with
>> locked ports, e.g. no need for "learning on" when using the feature in
>> general outside selftests...
> 
> "learning on" is needed when dynamic FDB entries are used to authorize
> hosts. Without learning being enabled, the bridge driver (or the
> underlying hardware) will not refresh the entries during forwarding and
> they will age out, resulting in packet loss until the hosts are
> re-authorized.
> 
> Given the current test cases only use static entries, there is no need
> to enable learning on locked ports. This will change when test cases 
> are
> added with dynamic entries.
> 
> Regarding mv88e6xxx, my understanding is that you also need learning
> enabled for MAB (I assume for the violation interrupts). Therefore, for
> mv88e6xxx, learning can be enabled if learning is on or MAB is on.
> Enabling it based on whether the port is locked or not seems 
> inaccurate.

Given that 'learning on' is needed for hardware refreshing of ATU 
entries (mv88e6xxx), and that will in the future be needed in general, I 
think it is best to enable it when a port is locked. Also the matter is 
that the locked feature needs to modify the register that contains the 
PAV. So I see it as natural that it is done there, as it will eventually 
have to be done there.
That the selftests do not need it besides when activating MAB, I think, 
is a special case.

I am at the blackhole driver implementation now, as I suppose that the 
iproute2 command should work with the mv88e6xxx driver when adding 
blackhole entries (with a added selftest)?
I decided to add the blackhole feature as new ops for drivers with 
functions blackhole_fdb_add() and blackhole_fdb_del(). Do you agree with 
that approach?
Ido Schimmel Sept. 8, 2022, 7:59 a.m. UTC | #16
On Wed, Sep 07, 2022 at 11:10:07PM +0200, netdev@kapio-technology.com wrote:
> I am at the blackhole driver implementation now, as I suppose that the
> iproute2 command should work with the mv88e6xxx driver when adding blackhole
> entries (with a added selftest)?
> I decided to add the blackhole feature as new ops for drivers with functions
> blackhole_fdb_add() and blackhole_fdb_del(). Do you agree with that
> approach?

I assume you are talking about extending 'dsa_switch_ops'? If so, it's
up to the DSA maintainers to decide.
Hans Schultz Sept. 8, 2022, 11:14 a.m. UTC | #17
On 2022-09-08 09:59, Ido Schimmel wrote:
> On Wed, Sep 07, 2022 at 11:10:07PM +0200, netdev@kapio-technology.com 
> wrote:
>> I am at the blackhole driver implementation now, as I suppose that the
>> iproute2 command should work with the mv88e6xxx driver when adding 
>> blackhole
>> entries (with a added selftest)?
>> I decided to add the blackhole feature as new ops for drivers with 
>> functions
>> blackhole_fdb_add() and blackhole_fdb_del(). Do you agree with that
>> approach?
> 
> I assume you are talking about extending 'dsa_switch_ops'?

Yes, that is the idea.

> If so, it's up to the DSA maintainers to decide.
Vladimir Oltean Sept. 8, 2022, 11:20 a.m. UTC | #18
On Thu, Sep 08, 2022 at 01:14:59PM +0200, netdev@kapio-technology.com wrote:
> On 2022-09-08 09:59, Ido Schimmel wrote:
> > On Wed, Sep 07, 2022 at 11:10:07PM +0200, netdev@kapio-technology.com wrote:
> > > I am at the blackhole driver implementation now, as I suppose that the
> > > iproute2 command should work with the mv88e6xxx driver when adding blackhole
> > > entries (with a added selftest)?
> > > I decided to add the blackhole feature as new ops for drivers with functions
> > > blackhole_fdb_add() and blackhole_fdb_del(). Do you agree with that approach?
> > 
> > I assume you are talking about extending 'dsa_switch_ops'?
> 
> Yes, that is the idea.
> 
> > If so, it's up to the DSA maintainers to decide.

What will be the usefulness of adding a blackhole FDB entry from user space?
Hans Schultz Sept. 9, 2022, 1:11 p.m. UTC | #19
On 2022-09-08 13:20, Vladimir Oltean wrote:
> On Thu, Sep 08, 2022 at 01:14:59PM +0200, netdev@kapio-technology.com 
> wrote:
>> On 2022-09-08 09:59, Ido Schimmel wrote:
>> > On Wed, Sep 07, 2022 at 11:10:07PM +0200, netdev@kapio-technology.com wrote:
>> > > I am at the blackhole driver implementation now, as I suppose that the
>> > > iproute2 command should work with the mv88e6xxx driver when adding blackhole
>> > > entries (with a added selftest)?
>> > > I decided to add the blackhole feature as new ops for drivers with functions
>> > > blackhole_fdb_add() and blackhole_fdb_del(). Do you agree with that approach?
>> >
>> > I assume you are talking about extending 'dsa_switch_ops'?
>> 
>> Yes, that is the idea.
>> 
>> > If so, it's up to the DSA maintainers to decide.
> 
> What will be the usefulness of adding a blackhole FDB entry from user 
> space?

With the software bridge it could be used to signal a untrusted host in 
connection with a locked port entry attempt. I don't see so much use 
other that test purposes with the driver though.
Vladimir Oltean Sept. 11, 2022, 12:13 a.m. UTC | #20
On Fri, Sep 09, 2022 at 03:11:56PM +0200, netdev@kapio-technology.com wrote:
> > > > On Wed, Sep 07, 2022 at 11:10:07PM +0200, netdev@kapio-technology.com wrote:
> > > > > I am at the blackhole driver implementation now, as I suppose that the
> > > > > iproute2 command should work with the mv88e6xxx driver when adding blackhole
> > > > > entries (with a added selftest)?
> > > > > I decided to add the blackhole feature as new ops for drivers with functions
> > > > > blackhole_fdb_add() and blackhole_fdb_del(). Do you agree with that approach?
> > > >
> > > > I assume you are talking about extending 'dsa_switch_ops'?
> > > 
> > > Yes, that is the idea.
> > > 
> > > > If so, it's up to the DSA maintainers to decide.
> > 
> > What will be the usefulness of adding a blackhole FDB entry from user
> > space?
> 
> With the software bridge it could be used to signal a untrusted host in
> connection with a locked port entry attempt. I don't see so much use other
> that test purposes with the driver though.

Not a huge selling point, to be honest. Can't the blackhole flag remain
settable only in the device -> bridge direction, with user space just
reading it?
Hans Schultz Sept. 11, 2022, 9:23 a.m. UTC | #21
On 2022-09-11 02:13, Vladimir Oltean wrote:
> On Fri, Sep 09, 2022 at 03:11:56PM +0200, netdev@kapio-technology.com 
> wrote:
>> > > > On Wed, Sep 07, 2022 at 11:10:07PM +0200, netdev@kapio-technology.com wrote:
>> > > > > I am at the blackhole driver implementation now, as I suppose that the
>> > > > > iproute2 command should work with the mv88e6xxx driver when adding blackhole
>> > > > > entries (with a added selftest)?
>> > > > > I decided to add the blackhole feature as new ops for drivers with functions
>> > > > > blackhole_fdb_add() and blackhole_fdb_del(). Do you agree with that approach?
>> > > >
>> > > > I assume you are talking about extending 'dsa_switch_ops'?
>> > >
>> > > Yes, that is the idea.
>> > >
>> > > > If so, it's up to the DSA maintainers to decide.
>> >
>> > What will be the usefulness of adding a blackhole FDB entry from user
>> > space?
>> 
>> With the software bridge it could be used to signal a untrusted host 
>> in
>> connection with a locked port entry attempt. I don't see so much use 
>> other
>> that test purposes with the driver though.
> 
> Not a huge selling point, to be honest. Can't the blackhole flag remain
> settable only in the device -> bridge direction, with user space just
> reading it?

That is possible, but it would of course not make sense to have 
selftests of the feature as that would not work unless there is a driver 
with this capability (now just mv88e6xxx).
Ido Schimmel Sept. 12, 2022, 9:08 a.m. UTC | #22
On Sun, Sep 11, 2022 at 11:23:55AM +0200, netdev@kapio-technology.com wrote:
> On 2022-09-11 02:13, Vladimir Oltean wrote:
> > On Fri, Sep 09, 2022 at 03:11:56PM +0200, netdev@kapio-technology.com
> > wrote:
> > > > > > On Wed, Sep 07, 2022 at 11:10:07PM +0200, netdev@kapio-technology.com wrote:
> > > > > > > I am at the blackhole driver implementation now, as I suppose that the
> > > > > > > iproute2 command should work with the mv88e6xxx driver when adding blackhole
> > > > > > > entries (with a added selftest)?
> > > > > > > I decided to add the blackhole feature as new ops for drivers with functions
> > > > > > > blackhole_fdb_add() and blackhole_fdb_del(). Do you agree with that approach?
> > > > > >
> > > > > > I assume you are talking about extending 'dsa_switch_ops'?
> > > > >
> > > > > Yes, that is the idea.
> > > > >
> > > > > > If so, it's up to the DSA maintainers to decide.
> > > >
> > > > What will be the usefulness of adding a blackhole FDB entry from user
> > > > space?
> > > 
> > > With the software bridge it could be used to signal a untrusted host
> > > in
> > > connection with a locked port entry attempt. I don't see so much use
> > > other
> > > that test purposes with the driver though.
> > 
> > Not a huge selling point, to be honest. Can't the blackhole flag remain
> > settable only in the device -> bridge direction, with user space just
> > reading it?
> 
> That is possible, but it would of course not make sense to have selftests of
> the feature as that would not work unless there is a driver with this
> capability (now just mv88e6xxx).

The new "blackhole" flag requires changes in the bridge driver and
without allowing user space to add such entries, the only way to test
these changes is with mv88e6xxx which many of us do not have...
Hans Schultz Sept. 20, 2022, 9:29 p.m. UTC | #23
On 2022-09-12 11:08, Ido Schimmel wrote:
> On Sun, Sep 11, 2022 at 11:23:55AM +0200, netdev@kapio-technology.com 
> wrote:
>> On 2022-09-11 02:13, Vladimir Oltean wrote:
>> > On Fri, Sep 09, 2022 at 03:11:56PM +0200, netdev@kapio-technology.com
>> > wrote:
>> > > > > > On Wed, Sep 07, 2022 at 11:10:07PM +0200, netdev@kapio-technology.com wrote:
>> > > > > > > I am at the blackhole driver implementation now, as I suppose that the
>> > > > > > > iproute2 command should work with the mv88e6xxx driver when adding blackhole
>> > > > > > > entries (with a added selftest)?
>> > > > > > > I decided to add the blackhole feature as new ops for drivers with functions
>> > > > > > > blackhole_fdb_add() and blackhole_fdb_del(). Do you agree with that approach?
>> > > > > >
>> > > > > > I assume you are talking about extending 'dsa_switch_ops'?
>> > > > >
>> > > > > Yes, that is the idea.
>> > > > >
>> > > > > > If so, it's up to the DSA maintainers to decide.
>> > > >
>> > > > What will be the usefulness of adding a blackhole FDB entry from user
>> > > > space?
>> > >
>> > > With the software bridge it could be used to signal a untrusted host
>> > > in
>> > > connection with a locked port entry attempt. I don't see so much use
>> > > other
>> > > that test purposes with the driver though.
>> >
>> > Not a huge selling point, to be honest. Can't the blackhole flag remain
>> > settable only in the device -> bridge direction, with user space just
>> > reading it?
>> 
>> That is possible, but it would of course not make sense to have 
>> selftests of
>> the feature as that would not work unless there is a driver with this
>> capability (now just mv88e6xxx).
> 
> The new "blackhole" flag requires changes in the bridge driver and
> without allowing user space to add such entries, the only way to test
> these changes is with mv88e6xxx which many of us do not have...

I am now building from new system (comp), and the kernel selftests are 
not being installed correctly, so I haven't been able to run the 
selftests yet.

I have made a blackhole selftest, which looks like this:

test_blackhole_fdb()
{
         RET=0

         check_blackhole_fdb_support || return 0

         tcpdump_start $h2
         $MZ $h1 -q -t udp -a $h1 -b $h2
         tcpdump_stop
         tcpdump_show | grep -q udp
         check_err $? "test_blackhole_fdb: No packet seen on initial"
         tcpdump_cleanup

         bridge fdb add `mac_get $h2` dev br0 blackhole
         bridge fdb show dev br0 | grep -q "blackhole"
         check_err $? "test_blackhole_fdb: No blackhole FDB entry found"

         tcpdump_start $h2
         $MZ $h1 -q -t udp -a $h1 -b $h2
         tcpdump_stop
         tcpdump_show | grep -q udp
         check_fail $? "test_blackhole_fdb: packet seen with blackhole 
fdb entry"
         tcpdump_cleanup

         bridge fdb del `mac_get $h2` dev br0 blackhole
         bridge fdb show dev br0 | grep -q "blackhole"
         check_fail $? "test_blackhole_fdb: Blackhole FDB entry not 
deleted"

         tcpdump_start $h2
         $MZ $h1 -q -t udp -a $h1 -b $h2
         tcpdump_stop
         tcpdump_show | grep -q udp
         check_err $? "test_blackhole_fdb: No packet seen after removing 
blackhole FDB entry"
         tcpdump_cleanup

         log_test "Blackhole FDB entry test"
}

the setup is simple and is the same as in bridge_sticky_fdb.sh.

Does the test look sound or is there obvious mistakes?
Ido Schimmel Sept. 21, 2022, 7:15 a.m. UTC | #24
On Tue, Sep 20, 2022 at 11:29:12PM +0200, netdev@kapio-technology.com wrote:
> I have made a blackhole selftest, which looks like this:
> 
> test_blackhole_fdb()
> {
>         RET=0
> 
>         check_blackhole_fdb_support || return 0
> 
>         tcpdump_start $h2
>         $MZ $h1 -q -t udp -a $h1 -b $h2

I don't think you can give an interface name to '-a' and '-b'?

>         tcpdump_stop
>         tcpdump_show | grep -q udp
>         check_err $? "test_blackhole_fdb: No packet seen on initial"
>         tcpdump_cleanup
> 
>         bridge fdb add `mac_get $h2` dev br0 blackhole
>         bridge fdb show dev br0 | grep -q "blackhole"

Make this grep more specific so that we are sure it is the entry user
space installed. Something like this:

bridge fdb get `mac_get $h2` br br0 | grep -q blackhole

>         check_err $? "test_blackhole_fdb: No blackhole FDB entry found"
> 
>         tcpdump_start $h2
>         $MZ $h1 -q -t udp -a $h1 -b $h2
>         tcpdump_stop
>         tcpdump_show | grep -q udp
>         check_fail $? "test_blackhole_fdb: packet seen with blackhole fdb
> entry"
>         tcpdump_cleanup

The tcpdump filter is not specific enough. It can catch other UDP
packets (e.g., multicast) being received by $h2. Anyway, to be sure the
feature works as expected we need to make sure that the packets are not
even egressing $swp2. Checking that they are not received by $h2 is not
enough. See this (untested) suggestion [1] that uses a tc filter on the
egress of $swp2.

> 
>         bridge fdb del `mac_get $h2` dev br0 blackhole
>         bridge fdb show dev br0 | grep -q "blackhole"
>         check_fail $? "test_blackhole_fdb: Blackhole FDB entry not deleted"
> 
>         tcpdump_start $h2
>         $MZ $h1 -q -t udp -a $h1 -b $h2
>         tcpdump_stop
>         tcpdump_show | grep -q udp
>         check_err $? "test_blackhole_fdb: No packet seen after removing
> blackhole FDB entry"
>         tcpdump_cleanup
> 
>         log_test "Blackhole FDB entry test"
> }
> 
> the setup is simple and is the same as in bridge_sticky_fdb.sh.
> 
> Does the test look sound or is there obvious mistakes?

[1]
blackhole_fdb()
{
	RET=0

	tc filter add dev $swp2 egress protocol ip pref 1 handle 1 flower \
		dst_ip 192.0.2.2 ip_proto udp dst_port 12345 action pass

	$MZ $h1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
		-a own -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q

	tc_check_packets "dev $swp2 egress" 1 1
	check_err $? "Packet not seen on egress before adding blackhole entry"

	bridge fdb add `mac_get $h2` dev br0 blackhole
	bridge fdb get `mac_get $h2` br br0 | grep -q blackhole
	check_err $? "Blackhole entry not found"

	$MZ $h1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
		-a own -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q

	tc_check_packets "dev $swp2 egress" 1 1
	check_err $? "Packet seen on egress after adding blackhole entry"

	# Check blackhole entries can be replaced.
	bridge fdb replace `mac_get $h2` dev $swp2 master static
	bridge fdb get `mac_get $h2` br br0 | grep -q blackhole
	check_fail $? "Blackhole entry found after replacement"

	$MZ $h1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
		-a own -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q

	tc_check_packets "dev $swp2 egress" 1 2
	check_err $? "Packet not seen on egress after replacing blackhole entry"

	bridge fdb del `mac_get $h2` dev $swp2 master static
	tc filter del dev $swp2 egress protocol ip pref 1 handle 1 flower

	log_test "Blackhole FDB entry"
}
Hans Schultz Sept. 21, 2022, 7:53 p.m. UTC | #25
On 2022-09-12 11:08, Ido Schimmel wrote:
> 
> The new "blackhole" flag requires changes in the bridge driver and
> without allowing user space to add such entries, the only way to test
> these changes is with mv88e6xxx which many of us do not have...

There seems to be a little inconvenience when adding/deleting blackhole 
entries, and that is since all slaves are the listeners to 
SWITCHDEV_FDB_ADD(DEL)_TO_DEVICE events and blackhole entries are not to 
any slave devices, the ops will be called for every slave device as 
there is no way to distinguish. This said, the add and del operations 
work.
Hans Schultz Sept. 22, 2022, 8:35 p.m. UTC | #26
On 2022-09-21 09:15, Ido Schimmel wrote:
> On Tue, Sep 20, 2022 at 11:29:12PM +0200, netdev@kapio-technology.com 
> wrote:
>> I have made a blackhole selftest, which looks like this:
>> 
>> test_blackhole_fdb()
>> {
>>         RET=0
>> 
>>         check_blackhole_fdb_support || return 0
>> 
>>         tcpdump_start $h2
>>         $MZ $h1 -q -t udp -a $h1 -b $h2
> 
> I don't think you can give an interface name to '-a' and '-b'?
> 
>>         tcpdump_stop
>>         tcpdump_show | grep -q udp
>>         check_err $? "test_blackhole_fdb: No packet seen on initial"
>>         tcpdump_cleanup
>> 
>>         bridge fdb add `mac_get $h2` dev br0 blackhole
>>         bridge fdb show dev br0 | grep -q "blackhole"
> 
> Make this grep more specific so that we are sure it is the entry user
> space installed. Something like this:
> 
> bridge fdb get `mac_get $h2` br br0 | grep -q blackhole
> 
>>         check_err $? "test_blackhole_fdb: No blackhole FDB entry 
>> found"
>> 
>>         tcpdump_start $h2
>>         $MZ $h1 -q -t udp -a $h1 -b $h2
>>         tcpdump_stop
>>         tcpdump_show | grep -q udp
>>         check_fail $? "test_blackhole_fdb: packet seen with blackhole 
>> fdb
>> entry"
>>         tcpdump_cleanup
> 
> The tcpdump filter is not specific enough. It can catch other UDP
> packets (e.g., multicast) being received by $h2. Anyway, to be sure the
> feature works as expected we need to make sure that the packets are not
> even egressing $swp2. Checking that they are not received by $h2 is not
> enough. See this (untested) suggestion [1] that uses a tc filter on the
> egress of $swp2.
> 
>> 
>>         bridge fdb del `mac_get $h2` dev br0 blackhole
>>         bridge fdb show dev br0 | grep -q "blackhole"
>>         check_fail $? "test_blackhole_fdb: Blackhole FDB entry not 
>> deleted"
>> 
>>         tcpdump_start $h2
>>         $MZ $h1 -q -t udp -a $h1 -b $h2
>>         tcpdump_stop
>>         tcpdump_show | grep -q udp
>>         check_err $? "test_blackhole_fdb: No packet seen after 
>> removing
>> blackhole FDB entry"
>>         tcpdump_cleanup
>> 
>>         log_test "Blackhole FDB entry test"
>> }
>> 
>> the setup is simple and is the same as in bridge_sticky_fdb.sh.
>> 
>> Does the test look sound or is there obvious mistakes?
> 
> [1]
> blackhole_fdb()
> {
> 	RET=0
> 
> 	tc filter add dev $swp2 egress protocol ip pref 1 handle 1 flower \
> 		dst_ip 192.0.2.2 ip_proto udp dst_port 12345 action pass
> 
> 	$MZ $h1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
> 		-a own -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
> 
> 	tc_check_packets "dev $swp2 egress" 1 1
> 	check_err $? "Packet not seen on egress before adding blackhole entry"
> 
> 	bridge fdb add `mac_get $h2` dev br0 blackhole
> 	bridge fdb get `mac_get $h2` br br0 | grep -q blackhole
> 	check_err $? "Blackhole entry not found"
> 
> 	$MZ $h1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
> 		-a own -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
> 
> 	tc_check_packets "dev $swp2 egress" 1 1
> 	check_err $? "Packet seen on egress after adding blackhole entry"
> 
> 	# Check blackhole entries can be replaced.
> 	bridge fdb replace `mac_get $h2` dev $swp2 master static
> 	bridge fdb get `mac_get $h2` br br0 | grep -q blackhole
> 	check_fail $? "Blackhole entry found after replacement"
> 
> 	$MZ $h1 -c 1 -p 128 -t udp "sp=54321,dp=12345" \
> 		-a own -b `mac_get $h2` -A 192.0.2.1 -B 192.0.2.2 -q
> 
> 	tc_check_packets "dev $swp2 egress" 1 2
> 	check_err $? "Packet not seen on egress after replacing blackhole 
> entry"
> 
> 	bridge fdb del `mac_get $h2` dev $swp2 master static
> 	tc filter del dev $swp2 egress protocol ip pref 1 handle 1 flower
> 
> 	log_test "Blackhole FDB entry"
> }

Thx, looks good.
I have tried to run the test as far as I can manually, but I don't seem 
to have 'busywait' in the system, which tc_check_packets() depends on, 
and I couldn't find any 'busywait' in Buildroot.
Hans Schultz Sept. 23, 2022, 11:34 a.m. UTC | #27
On 2022-09-21 09:15, Ido Schimmel wrote:

> 	# Check blackhole entries can be replaced.
> 	bridge fdb replace `mac_get $h2` dev $swp2 master static
> 	bridge fdb get `mac_get $h2` br br0 | grep -q blackhole
> 	check_fail $? "Blackhole entry found after replacement"

There seems to be a problem with replacing blackhole fdb entries as 
fdb_find_rcu() does not find the associated fdb entry (addr, vid) and I 
don't know why that is the case?
Hans Schultz Sept. 23, 2022, 12:01 p.m. UTC | #28
On 2022-09-21 09:15, Ido Schimmel wrote:

> 	# Check blackhole entries can be replaced.
> 	bridge fdb replace `mac_get $h2` dev $swp2 master static
> 	bridge fdb get `mac_get $h2` br br0 | grep -q blackhole
> 	check_fail $? "Blackhole entry found after replacement"
> 

I am quite in doubt if the driver will be able to overwrite a blackhole 
entry added by userspace as the replace action must be to delete and 
then add the replacement afaics, but a NEWNEIGH event using 
port_fdb_add() will not succeed with that using the ops I use now. 
Otherwise it has be all with port_fb_add() with new lists keeping the 
userspace added blackhole fdb entries.
Hans Schultz Sept. 23, 2022, 12:21 p.m. UTC | #29
On 2022-09-23 13:34, netdev@kapio-technology.com wrote:
> On 2022-09-21 09:15, Ido Schimmel wrote:
> 
>> 	# Check blackhole entries can be replaced.
>> 	bridge fdb replace `mac_get $h2` dev $swp2 master static
>> 	bridge fdb get `mac_get $h2` br br0 | grep -q blackhole
>> 	check_fail $? "Blackhole entry found after replacement"
> 
> There seems to be a problem with replacing blackhole fdb entries as
> fdb_find_rcu() does not find the associated fdb entry (addr, vid) and
> I don't know why that is the case?

I realize that the reason why fdb_find_rcu() does not find the entry is 
surely that it is stored in the bridge device fdb and not 'master' fdb.
Hans Schultz Sept. 27, 2022, 8:33 a.m. UTC | #30
On 2022-09-21 09:15, Ido Schimmel wrote:
> 	bridge fdb add `mac_get $h2` dev br0 blackhole

To make this work, I think we need to change the concept, so that 
blackhole FDB entries are added to ports connected to the bridge, thus
      bridge fdb add MAC dev $swpX master blackhole

This makes sense as the driver adds them based on the port where the 
SMAC is seen, even though the effect of the blackhole FDB entry is 
switch wide.

Adding them to the bridge (e.g. f.ex. br0) will not work in the SW 
bridge as the entries then are not found. We could deny this possibility 
or just document the use?


For offloaded I can change the add, so that it does a delete (even if 
none are present) and a add, thus facilitating the replace.

How does this sound?
Petr Machata Sept. 27, 2022, 3:19 p.m. UTC | #31
netdev@kapio-technology.com writes:

> Thx, looks good.
> I have tried to run the test as far as I can manually, but I don't seem to have 'busywait' in the
> system, which tc_check_packets() depends on, and I couldn't find any 'busywait' in Buildroot.

It's a helper defined in tools/testing/selftests/net/forwarding/lib.sh
Ido Schimmel Sept. 28, 2022, 6:59 a.m. UTC | #32
Sorry for the delay, was away.

On Tue, Sep 27, 2022 at 10:33:10AM +0200, netdev@kapio-technology.com wrote:
> On 2022-09-21 09:15, Ido Schimmel wrote:
> > 	bridge fdb add `mac_get $h2` dev br0 blackhole
> 
> To make this work, I think we need to change the concept, so that blackhole
> FDB entries are added to ports connected to the bridge, thus
>      bridge fdb add MAC dev $swpX master blackhole
> 
> This makes sense as the driver adds them based on the port where the SMAC is
> seen, even though the effect of the blackhole FDB entry is switch wide.

Asking user space to associate a blackhole entry with a bridge port does
not make sense to me because unlike regular entries, blackhole entries
do not forward packets out of this port. Blackhole routes and nexthops
are not associated with a device either.

> Adding them to the bridge (e.g. f.ex. br0) will not work in the SW bridge as
> the entries then are not found.

Why not found? This works:

 # bridge fdb add 00:11:22:33:44:55 dev br0 self local
 $ bridge fdb get 00:11:22:33:44:55 br br0
 00:11:22:33:44:55 dev br0 master br0 permanent

With blackhole support I expect:

 # bridge fdb add 00:11:22:33:44:55 dev br0 self local blackhole
 $ bridge fdb get 00:11:22:33:44:55 br br0
 00:11:22:33:44:55 dev br0 master br0 permanent blackhole
Hans Schultz Sept. 28, 2022, 7:29 a.m. UTC | #33
On 2022-09-28 08:59, Ido Schimmel wrote:
> Sorry for the delay, was away.

Good to have you back. :-)

> 
> On Tue, Sep 27, 2022 at 10:33:10AM +0200, netdev@kapio-technology.com 
> wrote:
>> On 2022-09-21 09:15, Ido Schimmel wrote:
>> > 	bridge fdb add `mac_get $h2` dev br0 blackhole
>> 
>> To make this work, I think we need to change the concept, so that 
>> blackhole
>> FDB entries are added to ports connected to the bridge, thus
>>      bridge fdb add MAC dev $swpX master blackhole
>> 
>> This makes sense as the driver adds them based on the port where the 
>> SMAC is
>> seen, even though the effect of the blackhole FDB entry is switch 
>> wide.
> 
> Asking user space to associate a blackhole entry with a bridge port 
> does
> not make sense to me because unlike regular entries, blackhole entries
> do not forward packets out of this port. Blackhole routes and nexthops
> are not associated with a device either.
> 
>> Adding them to the bridge (e.g. f.ex. br0) will not work in the SW 
>> bridge as
>> the entries then are not found.
> 
> Why not found? This works:
> 
>  # bridge fdb add 00:11:22:33:44:55 dev br0 self local
>  $ bridge fdb get 00:11:22:33:44:55 br br0
>  00:11:22:33:44:55 dev br0 master br0 permanent
> 
> With blackhole support I expect:
> 
>  # bridge fdb add 00:11:22:33:44:55 dev br0 self local blackhole
>  $ bridge fdb get 00:11:22:33:44:55 br br0
>  00:11:22:33:44:55 dev br0 master br0 permanent blackhole

In my previous replies, I have notified that fdb_find_rcu() does not 
find the entry added with br0, and thus fdb_add_entry() that does the 
replace does not replace but adds a new entry. I have been thinking that 
it is because when added with br0 as dev it is added to dev br0's fdb, 
which is not the same as 'dev <Dev> master' fdb...

I think bridge fdb get works in a different way, as I know the get 
functionality gets all fdb entries from all devices and filters them (if 
I am not mistaken)...
Hans Schultz Sept. 28, 2022, 7:47 a.m. UTC | #34
On 2022-09-28 08:59, Ido Schimmel wrote:

> Why not found? This works:
> 
>  # bridge fdb add 00:11:22:33:44:55 dev br0 self local
>  $ bridge fdb get 00:11:22:33:44:55 br br0

With:
  # bridge fdb replace 00.11.22.33.44.55 dev $swpX static

fdb_find_rcu() will not find the entry added with 'dev br0' above, and 
will thus add a new entry afaik.
Ido Schimmel Sept. 28, 2022, 8:46 a.m. UTC | #35
On Wed, Sep 28, 2022 at 09:47:42AM +0200, netdev@kapio-technology.com wrote:
> On 2022-09-28 08:59, Ido Schimmel wrote:
> 
> > Why not found? This works:
> > 
> >  # bridge fdb add 00:11:22:33:44:55 dev br0 self local
> >  $ bridge fdb get 00:11:22:33:44:55 br br0
> 
> With:
>  # bridge fdb replace 00.11.22.33.44.55 dev $swpX static
> 
> fdb_find_rcu() will not find the entry added with 'dev br0' above, and will
> thus add a new entry afaik.

It needs "master" keyword:

 $ bridge fdb get 00:11:22:33:44:55 br br0
 Error: Fdb entry not found.
 # bridge fdb add 00:11:22:33:44:55 dev br0 self local
 $ bridge fdb get 00:11:22:33:44:55 br br0
 00:11:22:33:44:55 dev br0 master br0 permanent
 # bridge fdb replace 00:11:22:33:44:55 dev dummy10 master static
 $ bridge fdb get 00:11:22:33:44:55 br br0
 00:11:22:33:44:55 dev dummy10 master br0 static

"master" means manipulate the FDB of the master device. Therefore, the
replace command manipulates the FDB of br0.

"self" (which is the default [1]) means manipulate the FDB of the device
itself. In case of br0 it means manipulate the FDB of the bridge device.
For physical devices it usually translates to manipulating the unicast
address filter list.

[1] https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/tree/bridge/fdb.c#n511
Hans Schultz Sept. 28, 2022, 10:16 a.m. UTC | #36
On 2022-09-28 10:46, Ido Schimmel wrote:
> On Wed, Sep 28, 2022 at 09:47:42AM +0200, netdev@kapio-technology.com 
> wrote:

> It needs "master" keyword:
> 

I must have forgotten it when I tested at some point...

It seems to be working, only that I am at a loss to why port_fdb_add() 
is called for each port after going through the DSA layer. I understand 
that all slave devices are listening on events, but I think the ops 
should only be called when the port matches?
Also for some reason the blackhole (zero-DPV) ATU entry is not added on 
my device in case of no vlan (vid = 0), but there is also some phy 
problems that are unrelated to this patch set.
Hans Schultz Sept. 28, 2022, 10:19 a.m. UTC | #37
On 2022-09-28 10:46, Ido Schimmel wrote:
> On Wed, Sep 28, 2022 at 09:47:42AM +0200, netdev@kapio-technology.com 
> wrote:
>> On 2022-09-28 08:59, Ido Schimmel wrote:
>> 

BTW, I have added FDB flags in the DSA layer as a u16, so that now 
port_fdb_add() is as:

         int     (*port_fdb_add)(struct dsa_switch *ds, int port,
                                 const unsigned char *addr, u16 vid,
                                 u16 fdb_flags, struct dsa_db db);
Hans Schultz Sept. 29, 2022, 10:26 p.m. UTC | #38
On 2022-09-28 10:46, Ido Schimmel wrote:

> "master" means manipulate the FDB of the master device. Therefore, the
> replace command manipulates the FDB of br0.
> 
> "self" (which is the default [1]) means manipulate the FDB of the 
> device
> itself. In case of br0 it means manipulate the FDB of the bridge 
> device.
> For physical devices it usually translates to manipulating the unicast
> address filter list.

Hi Ido, can you check the selftests of the v6 I have sent out using the 
iproute2-next I have also sent?
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
index 5b02b6b60ce7..b763b3b9fdf0 100755
--- a/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_locked_port.sh
@@ -1,7 +1,15 @@ 
 #!/bin/bash
 # SPDX-License-Identifier: GPL-2.0
 
-ALL_TESTS="locked_port_ipv4 locked_port_ipv6 locked_port_vlan"
+ALL_TESTS="
+	locked_port_ipv4
+	locked_port_ipv6
+	locked_port_vlan
+	locked_port_mab
+	locked_port_station_move
+	locked_port_mab_station_move
+"
+
 NUM_NETIFS=4
 CHECK_TC="no"
 source lib.sh
@@ -166,6 +174,103 @@  locked_port_ipv6()
 	log_test "Locked port ipv6"
 }
 
+locked_port_mab()
+{
+	RET=0
+	check_locked_port_support || return 0
+
+	ping_do $h1 192.0.2.2
+	check_err $? "MAB: Ping did not work before locking port"
+
+	bridge link set dev $swp1 locked on
+	bridge link set dev $swp1 learning on
+	if ! bridge link set dev $swp1 mab on 2>/dev/null; then
+		echo "SKIP: iproute2 too old; MacAuth feature not supported."
+		return $ksft_skip
+	fi
+
+	ping_do $h1 192.0.2.2
+	check_fail $? "MAB: Ping worked on locked port without FDB entry"
+
+	bridge fdb show | grep `mac_get $h1` | grep -q "locked"
+	check_err $? "MAB: No locked fdb entry after ping on locked port"
+
+	bridge fdb replace `mac_get $h1` dev $swp1 master static
+
+	ping_do $h1 192.0.2.2
+	check_err $? "MAB: Ping did not work with fdb entry without locked flag"
+
+	bridge fdb del `mac_get $h1` dev $swp1 master
+	bridge link set dev $swp1 learning off
+	bridge link set dev $swp1 locked off
+
+	log_test "Locked port MAB"
+}
+
+# No roaming allowed to a simple locked port
+locked_port_station_move()
+{
+	local mac=a0:b0:c0:c0:b0:a0
+
+	RET=0
+	check_locked_port_support || return 0
+
+	bridge link set dev $swp1 locked on
+	bridge link set dev $swp1 learning on
+
+	$MZ $h1 -q -t udp -a $mac -b rand
+	bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0"
+	check_fail $? "Locked port station move: FDB entry on first injection"
+
+	$MZ $h2 -q -t udp -a $mac -b rand
+	bridge fdb show dev $swp2 | grep -q "$mac vlan 1 master br0"
+	check_err $? "Locked port station move: Entry not found on unlocked port"
+
+	$MZ $h1 -q -t udp -a $mac -b rand
+	bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0"
+	check_fail $? "Locked port station move: entry roamed to locked port"
+
+	log_test "Locked port station move"
+}
+
+# Roaming to and from a MAB enabled port should work if sticky flag is not set
+locked_port_mab_station_move()
+{
+	local mac=10:20:30:30:20:10
+
+	RET=0
+	check_locked_port_support || return 0
+
+	bridge link set dev $swp1 locked on
+	bridge link set dev $swp1 learning on
+	if ! bridge link set dev $swp1 mab on 2>/dev/null; then
+		echo "SKIP: iproute2 too old; MacAuth feature not supported."
+		return $ksft_skip
+	fi
+
+	$MZ $h1 -q -t udp -a $mac -b rand
+	if bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0" | grep -q sticky; then
+		echo "SKIP: Roaming not possible with sticky flag, run sticky flag roaming test"
+		return $ksft_skip
+	fi
+
+	bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0 locked"
+	check_err $? "MAB station move: no locked entry on first injection"
+
+	$MZ $h2 -q -t udp -a $mac -b rand
+	bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0 locked"
+	check_fail $? "MAB station move: locked entry did not move"
+
+	bridge fdb show dev $swp2 | grep -q "$mac vlan 1 master br0"
+	check_err $? "MAB station move: roamed entry not found"
+
+	$MZ $h1 -q -t udp -a $mac -b rand
+	bridge fdb show dev $swp1 | grep -q "$mac vlan 1 master br0 locked"
+	check_err $? "MAB station move: entry did not roam back to locked port"
+
+	log_test "Locked port MAB station move"
+}
+
 trap cleanup EXIT
 
 setup_prepare
diff --git a/tools/testing/selftests/net/forwarding/bridge_sticky_fdb.sh b/tools/testing/selftests/net/forwarding/bridge_sticky_fdb.sh
index 1f8ef0eff862..bca77bc3fe09 100755
--- a/tools/testing/selftests/net/forwarding/bridge_sticky_fdb.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_sticky_fdb.sh
@@ -1,7 +1,7 @@ 
 #!/bin/bash
 # SPDX-License-Identifier: GPL-2.0
 
-ALL_TESTS="sticky"
+ALL_TESTS="sticky sticky_no_roaming"
 NUM_NETIFS=4
 TEST_MAC=de:ad:be:ef:13:37
 source lib.sh
@@ -59,6 +59,25 @@  sticky()
 	log_test "Sticky fdb entry"
 }
 
+# No roaming allowed with the sticky flag set
+sticky_no_roaming()
+{
+	local mac=a8:b4:c2:c2:b4:a8
+
+	RET=0
+
+	bridge link set dev $swp2 learning on
+	bridge fdb add $mac dev $swp1 master static sticky
+	bridge fdb show dev $swp1 | grep "$mac master br0" | grep -q sticky
+	check_err $? "Sticky no roaming: No sticky FDB entry found after adding"
+
+	$MZ $h2 -q -t udp -c 10 -d 100msec -a $mac -b rand
+	bridge fdb show dev $swp2 | grep "$mac master br0" | grep -q sticky
+	check_fail $? "Sticky no roaming: Sticky entry roamed"
+
+	log_test "Sticky no roaming"
+}
+
 trap cleanup EXIT
 
 setup_prepare