[PATCH RFC] net: bridge: handle ports in locked mode for ll learning

[PATCH RFC] net: bridge: handle ports in locked mode for ll learning

[Jonas Gorski]

When support for locked ports was added with commit a21d9a670d81 ("net:
bridge: Add support for bridge port in locked mode"), learning is
inhibited when the port is locked in br_handle_frame_finish().

It was later extended in commit a35ec8e38cdd ("bridge: Add MAC
Authentication Bypass (MAB) support") where optionally learning is done
with locked entries.

Unfortunately both missed that learning may also happen on frames to
link local addresses (01:80:c2:00:00:0X) in br_handle_frame(), which
will call __br_handle_local_finish(), which may update the fdb unless
(ll) learning is disabled as well.

This can be easily observed by e.g. EAPOL frames to 01:80:c2:00:00:03 on
a port causing the source mac to be learned, which is then forwarded
normally, essentially bypassing any authentication.

Fix this by moving the BR_PORT_LOCKED handling into its own function,
and call it from both places.

Fixes: a21d9a670d81 ("net: bridge: Add support for bridge port in locked mode")
Fixes: a35ec8e38cdd ("bridge: Add MAC Authentication Bypass (MAB) support")
Signed-off-by: Jonas Gorski <jonas.gorski@bisdn.de>
---
Sent as RFC since I'm not 100% sure this is the right way to fix.

 net/bridge/br_input.c | 78 ++++++++++++++++++++++++-------------------
 1 file changed, 44 insertions(+), 34 deletions(-)

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index ceaa5a89b947..f269a9f1b871 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -72,6 +72,46 @@ static int br_pass_frame_up(struct sk_buff *skb, bool promisc)
 		       br_netif_receive_skb);
 }
 
+static int br_fdb_locked_learn(struct net_bridge_port *p, struct sk_buff *skb,
+			       u16 vid, bool mark)
+{
+	struct net_bridge *br = p->br;
+
+	if (p->flags & BR_PORT_LOCKED) {
+		struct net_bridge_fdb_entry *fdb_src =
+			br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);
+		if (!fdb_src) {
+			/* FDB miss. Create locked FDB entry if MAB is enabled
+			 * and drop the packet.
+			 */
+			if (p->flags & BR_PORT_MAB)
+				br_fdb_update(br, p, eth_hdr(skb)->h_source,
+					      vid, BIT(BR_FDB_LOCKED));
+			return NET_RX_DROP;
+		} else if (READ_ONCE(fdb_src->dst) != p ||
+			   test_bit(BR_FDB_LOCAL, &fdb_src->flags)) {
+			/* FDB mismatch. Drop the packet without roaming. */
+			return NET_RX_DROP;
+		} else if (test_bit(BR_FDB_LOCKED, &fdb_src->flags)) {
+			/* FDB match, but entry is locked. Refresh it and drop
+			 * the packet.
+			 */
+			br_fdb_update(br, p, eth_hdr(skb)->h_source, vid,
+				      BIT(BR_FDB_LOCKED));
+			return NET_RX_DROP;
+		}
+	}
+
+	if (mark)
+		nbp_switchdev_frame_mark(p, skb);
+
+	/* insert into forwarding database after filtering to avoid spoofing */
+	if (p->flags & BR_LEARNING)
+		br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, 0);
+
+	return NET_RX_SUCCESS;
+}
+
 /* note: already called with rcu_read_lock */
 int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb)
 {
@@ -108,37 +148,8 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 				&state, &vlan))
 		goto out;
 
-	if (p->flags & BR_PORT_LOCKED) {
-		struct net_bridge_fdb_entry *fdb_src =
-			br_fdb_find_rcu(br, eth_hdr(skb)->h_source, vid);
-
-		if (!fdb_src) {
-			/* FDB miss. Create locked FDB entry if MAB is enabled
-			 * and drop the packet.
-			 */
-			if (p->flags & BR_PORT_MAB)
-				br_fdb_update(br, p, eth_hdr(skb)->h_source,
-					      vid, BIT(BR_FDB_LOCKED));
-			goto drop;
-		} else if (READ_ONCE(fdb_src->dst) != p ||
-			   test_bit(BR_FDB_LOCAL, &fdb_src->flags)) {
-			/* FDB mismatch. Drop the packet without roaming. */
-			goto drop;
-		} else if (test_bit(BR_FDB_LOCKED, &fdb_src->flags)) {
-			/* FDB match, but entry is locked. Refresh it and drop
-			 * the packet.
-			 */
-			br_fdb_update(br, p, eth_hdr(skb)->h_source, vid,
-				      BIT(BR_FDB_LOCKED));
-			goto drop;
-		}
-	}
-
-	nbp_switchdev_frame_mark(p, skb);
-
-	/* insert into forwarding database after filtering to avoid spoofing */
-	if (p->flags & BR_LEARNING)
-		br_fdb_update(br, p, eth_hdr(skb)->h_source, vid, 0);
+	if (br_fdb_locked_learn(p, skb, vid, true) == NET_RX_DROP)
+		goto drop;
 
 	promisc = !!(br->dev->flags & IFF_PROMISC);
 	local_rcv = promisc;
@@ -234,11 +245,10 @@ static void __br_handle_local_finish(struct sk_buff *skb)
 	u16 vid = 0;
 
 	/* check if vlan is allowed, to avoid spoofing */
-	if ((p->flags & BR_LEARNING) &&
-	    nbp_state_should_learn(p) &&
+	if (nbp_state_should_learn(p) &&
 	    !br_opt_get(p->br, BROPT_NO_LL_LEARN) &&
 	    br_should_learn(p, skb, &vid))
-		br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid, 0);
+		br_fdb_locked_learn(p, skb, vid, false);
 }
 
 /* note: already called with rcu_read_lock */
-- 
2.47.1


-- 
BISDN GmbH
Körnerstraße 7-10
10785 Berlin
Germany


Phone: 
+49-30-6108-1-6100


Managing Directors: 
Dr.-Ing. Hagen Woesner, Andreas 
Köpsel


Commercial register: 
Amtsgericht Berlin-Charlottenburg HRB 141569 
B
VAT ID No: DE283257294

Re: [PATCH RFC] net: bridge: handle ports in locked mode for ll learning

[Vladimir Oltean]

On Tue, Dec 10, 2024 at 03:06:53PM +0100, Jonas Gorski wrote:
> 
> When support for locked ports was added with commit a21d9a670d81 ("net:
> bridge: Add support for bridge port in locked mode"), learning is
> inhibited when the port is locked in br_handle_frame_finish().
> 
> It was later extended in commit a35ec8e38cdd ("bridge: Add MAC
> Authentication Bypass (MAB) support") where optionally learning is done
> with locked entries.
> 
> Unfortunately both missed that learning may also happen on frames to
> link local addresses (01:80:c2:00:00:0X) in br_handle_frame(), which
> will call __br_handle_local_finish(), which may update the fdb unless
> (ll) learning is disabled as well.
> 
> This can be easily observed by e.g. EAPOL frames to 01:80:c2:00:00:03 on
> a port causing the source mac to be learned, which is then forwarded
> normally, essentially bypassing any authentication.
> 
> Fix this by moving the BR_PORT_LOCKED handling into its own function,
> and call it from both places.
> 
> Fixes: a21d9a670d81 ("net: bridge: Add support for bridge port in locked mode")
> Fixes: a35ec8e38cdd ("bridge: Add MAC Authentication Bypass (MAB) support")
> Signed-off-by: Jonas Gorski <jonas.gorski@bisdn.de>
> ---
> Sent as RFC since I'm not 100% sure this is the right way to fix.

It was decided that this is expected behavior.
https://man7.org/linux/man-pages/man8/bridge.8.html
       locked on or locked off
              Controls whether a port is locked or not. When locked,
              non-link-local frames received through the port are
              dropped unless an FDB entry with the MAC source address
              points to the port. The common use case is IEEE 802.1X
              where hosts can authenticate themselves by exchanging
              EAPOL frames with an authenticator. After authentication
              is complete, the user space control plane can install a
              matching FDB entry to allow traffic from the host to be
              forwarded by the bridge. When learning is enabled on a
                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
              locked port, the no_linklocal_learn bridge option needs to
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
              be on to prevent the bridge from learning from received
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
              EAPOL frames. By default this flag is off.
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Re: [PATCH RFC] net: bridge: handle ports in locked mode for ll learning

[Jonas Gorski]

Am Di., 10. Dez. 2024 um 15:34 Uhr schrieb Vladimir Oltean
<vladimir.oltean@nxp.com>:
>
> On Tue, Dec 10, 2024 at 03:06:53PM +0100, Jonas Gorski wrote:
> >
> > When support for locked ports was added with commit a21d9a670d81 ("net:
> > bridge: Add support for bridge port in locked mode"), learning is
> > inhibited when the port is locked in br_handle_frame_finish().
> >
> > It was later extended in commit a35ec8e38cdd ("bridge: Add MAC
> > Authentication Bypass (MAB) support") where optionally learning is done
> > with locked entries.
> >
> > Unfortunately both missed that learning may also happen on frames to
> > link local addresses (01:80:c2:00:00:0X) in br_handle_frame(), which
> > will call __br_handle_local_finish(), which may update the fdb unless
> > (ll) learning is disabled as well.
> >
> > This can be easily observed by e.g. EAPOL frames to 01:80:c2:00:00:03 on
> > a port causing the source mac to be learned, which is then forwarded
> > normally, essentially bypassing any authentication.
> >
> > Fix this by moving the BR_PORT_LOCKED handling into its own function,
> > and call it from both places.
> >
> > Fixes: a21d9a670d81 ("net: bridge: Add support for bridge port in locked mode")
> > Fixes: a35ec8e38cdd ("bridge: Add MAC Authentication Bypass (MAB) support")
> > Signed-off-by: Jonas Gorski <jonas.gorski@bisdn.de>
> > ---
> > Sent as RFC since I'm not 100% sure this is the right way to fix.
>
> It was decided that this is expected behavior.
> https://man7.org/linux/man-pages/man8/bridge.8.html
>        locked on or locked off
>               Controls whether a port is locked or not. When locked,
>               non-link-local frames received through the port are
>               dropped unless an FDB entry with the MAC source address
>               points to the port. The common use case is IEEE 802.1X
>               where hosts can authenticate themselves by exchanging
>               EAPOL frames with an authenticator. After authentication
>               is complete, the user space control plane can install a
>               matching FDB entry to allow traffic from the host to be
>               forwarded by the bridge. When learning is enabled on a
>                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>               locked port, the no_linklocal_learn bridge option needs to
>               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>               be on to prevent the bridge from learning from received
>               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>               EAPOL frames. By default this flag is off.
>               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Huh, indeed. Unexpected decision, didn't think that this was
intentional. I wonder what the use case for that is.

Ah well, then disregard my patch.

Best Regards,
Jonas

-- 
BISDN GmbH
Körnerstraße 7-10
10785 Berlin
Germany


Phone: 
+49-30-6108-1-6100


Managing Directors: 
Dr.-Ing. Hagen Woesner, Andreas 
Köpsel


Commercial register: 
Amtsgericht Berlin-Charlottenburg HRB 141569 
B
VAT ID No: DE283257294

Re: [PATCH RFC] net: bridge: handle ports in locked mode for ll learning

[Vladimir Oltean]

On Tue, Dec 10, 2024 at 03:47:11PM +0100, Jonas Gorski wrote:
> Huh, indeed. Unexpected decision, didn't think that this was
> intentional. I wonder what the use case for that is.
> 
> Ah well, then disregard my patch.

The discussion was here, I don't remember the details:
https://lore.kernel.org/all/20220630111634.610320-1-hans@kapio-technology.com/

Re: [PATCH RFC] net: bridge: handle ports in locked mode for ll learning

[Jonas Gorski]

Am Di., 10. Dez. 2024 um 15:55 Uhr schrieb Vladimir Oltean
<vladimir.oltean@nxp.com>:
>
> On Tue, Dec 10, 2024 at 03:47:11PM +0100, Jonas Gorski wrote:
> > Huh, indeed. Unexpected decision, didn't think that this was
> > intentional. I wonder what the use case for that is.
> >
> > Ah well, then disregard my patch.
>
> The discussion was here, I don't remember the details:
> https://lore.kernel.org/all/20220630111634.610320-1-hans@kapio-technology.com/

Thanks for the pointer. Reading the discussion, it seems this was
before the explicit BR_PORT_MAB option and locked learning support, so
there was some ambiguity around whether learning on locked ports is
desired or not, and this was needed(?) for the out-of-tree(?) MAB
implementation.

But now that we do have an explicit flag for MAB, maybe this should be
revisited? Especially since with BR_PORT_MAB enabled, entries are
supposed to be learned as locked. But link local learned entries are
still learned unlocked. So no_linklocal_learn still needs to be
enabled for +locked, +learning, +mab.

AFACT at least Hans thought that this should be done when there an
explicit MAB opt in in
https://lore.kernel.org/all/CAKUejP6wCaOKiafvbxYqQs0-RibC0FMKtvkiG=R2Ts0Xfa3-tg@mail.gmail.com/
and https://lore.kernel.org/all/CAKUejP6xR81p1QeSCnDP_3uh9owafdYr1pifeCzekzUvU3_dPw@mail.gmail.com/.

Best Regards,
Jonas

-- 
BISDN GmbH
Körnerstraße 7-10
10785 Berlin
Germany


Phone: 
+49-30-6108-1-6100


Managing Directors: 
Dr.-Ing. Hagen Woesner, Andreas 
Köpsel


Commercial register: 
Amtsgericht Berlin-Charlottenburg HRB 141569 
B
VAT ID No: DE283257294

Re: [PATCH RFC] net: bridge: handle ports in locked mode for ll learning

[Ido Schimmel]

On Tue, Dec 10, 2024 at 04:28:54PM +0100, Jonas Gorski wrote:
> Thanks for the pointer. Reading the discussion, it seems this was
> before the explicit BR_PORT_MAB option and locked learning support, so
> there was some ambiguity around whether learning on locked ports is
> desired or not, and this was needed(?) for the out-of-tree(?) MAB
> implementation.

There is a use case for learning on a locked port even without MAB. If
user space is granting access via dynamic FDB entires, then you need
learning enabled to refresh these entries.

> But now that we do have an explicit flag for MAB, maybe this should be
> revisited? Especially since with BR_PORT_MAB enabled, entries are
> supposed to be learned as locked. But link local learned entries are
> still learned unlocked. So no_linklocal_learn still needs to be
> enabled for +locked, +learning, +mab.

I mentioned this in the man page and added "no_linklocal_learn" to
iproute2, but looks like it is not enough. You can try reposting the
original patch (skip learning from link-local frames on a locked port)
with a Fixes tag and see how it goes. I think it is unfortunate to
change the behavior when there is already a dedicated knob for what you
want to achieve, but I suspect the change will not introduce regressions
so maybe people will find it acceptable.

Re: [PATCH RFC] net: bridge: handle ports in locked mode for ll learning

[Jonas Gorski]

Am Mi., 11. Dez. 2024 um 09:42 Uhr schrieb Ido Schimmel <idosch@nvidia.com>:
>
> On Tue, Dec 10, 2024 at 04:28:54PM +0100, Jonas Gorski wrote:
> > Thanks for the pointer. Reading the discussion, it seems this was
> > before the explicit BR_PORT_MAB option and locked learning support, so
> > there was some ambiguity around whether learning on locked ports is
> > desired or not, and this was needed(?) for the out-of-tree(?) MAB
> > implementation.
>
> There is a use case for learning on a locked port even without MAB. If
> user space is granting access via dynamic FDB entires, then you need
> learning enabled to refresh these entries.

AFAICT this would still work with my patch, as long learning is
enabled for the port. The difference would be that new dynamic entries
won't be created anymore from link local learning, so userspace would
now have to add them themselves. But any existing dynamic entries will
be refreshed via the normal input paths.

Though I see that this would break offloading these, since USER
dynamic entries are ignored in br_switchdev_fdb_notify() since
927cdea5d209 ("net: bridge: switchdev: don't notify FDB entries with
"master dynamic""). Side note, br_switchdev_fdb_replay() seems to
still pass them on. Do I miss something or shouldn't replay also need
to ignore/skip them?

> > But now that we do have an explicit flag for MAB, maybe this should be
> > revisited? Especially since with BR_PORT_MAB enabled, entries are
> > supposed to be learned as locked. But link local learned entries are
> > still learned unlocked. So no_linklocal_learn still needs to be
> > enabled for +locked, +learning, +mab.
>
> I mentioned this in the man page and added "no_linklocal_learn" to
> iproute2, but looks like it is not enough. You can try reposting the
> original patch (skip learning from link-local frames on a locked port)
> with a Fixes tag and see how it goes. I think it is unfortunate to
> change the behavior when there is already a dedicated knob for what you
> want to achieve, but I suspect the change will not introduce regressions
> so maybe people will find it acceptable.

Absolutely not your fault; my reference was the original cover letters
for BR_PORT_LOCKED and BR_PORT_MAB and reading br_input.c where the
flags are handled (not even looking at if_link.h's doc comments). And
there the constraint/side effect isn't mentioned anywhere, so I
assumed it was unintentional. And I never looked at any man pages,
just used bridge link help to find out what the arguments are to
(un)set those port flags. So I looked everywhere except where this
constraint is pointed out.

Anyway, I understand your concern about already having a knob to avoid
the issue, my concern here is that the knob isn't quite obvious, and
that you do need an additional knob to have a "secure" default. So
IMHO it's easy to miss as an inexperienced user. Though at least in
the !MAB case, disabling learning on the port is also enough to avoid
that (and keeps learning via link local enabled for unlocked ports).

At least in the case of having enabled BR_PORT_MAB, I would consider
it a bug that the entries learned via link local traffic aren't marked
as BR_FDB_LOCKED. If you agree, I can send in a reduced patch for
that, so that the entries are initially locked regardless the source
of learning.

Best Regards,
Jonas

-- 
BISDN GmbH
Körnerstraße 7-10
10785 Berlin
Germany


Phone: 
+49-30-6108-1-6100


Managing Directors: 
Dr.-Ing. Hagen Woesner, Andreas 
Köpsel


Commercial register: 
Amtsgericht Berlin-Charlottenburg HRB 141569 
B
VAT ID No: DE283257294

Re: [PATCH RFC] net: bridge: handle ports in locked mode for ll learning

[Ido Schimmel]

On Wed, Dec 11, 2024 at 11:32:38AM +0100, Jonas Gorski wrote:
> Am Mi., 11. Dez. 2024 um 09:42 Uhr schrieb Ido Schimmel <idosch@nvidia.com>:
> >
> > On Tue, Dec 10, 2024 at 04:28:54PM +0100, Jonas Gorski wrote:
> > > Thanks for the pointer. Reading the discussion, it seems this was
> > > before the explicit BR_PORT_MAB option and locked learning support, so
> > > there was some ambiguity around whether learning on locked ports is
> > > desired or not, and this was needed(?) for the out-of-tree(?) MAB
> > > implementation.
> >
> > There is a use case for learning on a locked port even without MAB. If
> > user space is granting access via dynamic FDB entires, then you need
> > learning enabled to refresh these entries.
> 
> AFAICT this would still work with my patch, as long learning is
> enabled for the port. The difference would be that new dynamic entries
> won't be created anymore from link local learning, so userspace would
> now have to add them themselves. But any existing dynamic entries will
> be refreshed via the normal input paths.
> 
> Though I see that this would break offloading these, since USER
> dynamic entries are ignored in br_switchdev_fdb_notify() since
> 927cdea5d209 ("net: bridge: switchdev: don't notify FDB entries with
> "master dynamic""). Side note, br_switchdev_fdb_replay() seems to
> still pass them on. Do I miss something or shouldn't replay also need
> to ignore/skip them?
> 
> > > But now that we do have an explicit flag for MAB, maybe this should be
> > > revisited? Especially since with BR_PORT_MAB enabled, entries are
> > > supposed to be learned as locked. But link local learned entries are
> > > still learned unlocked. So no_linklocal_learn still needs to be
> > > enabled for +locked, +learning, +mab.
> >
> > I mentioned this in the man page and added "no_linklocal_learn" to
> > iproute2, but looks like it is not enough. You can try reposting the
> > original patch (skip learning from link-local frames on a locked port)
> > with a Fixes tag and see how it goes. I think it is unfortunate to
> > change the behavior when there is already a dedicated knob for what you
> > want to achieve, but I suspect the change will not introduce regressions
> > so maybe people will find it acceptable.
> 
> Absolutely not your fault; my reference was the original cover letters
> for BR_PORT_LOCKED and BR_PORT_MAB and reading br_input.c where the
> flags are handled (not even looking at if_link.h's doc comments). And
> there the constraint/side effect isn't mentioned anywhere, so I
> assumed it was unintentional. And I never looked at any man pages,
> just used bridge link help to find out what the arguments are to
> (un)set those port flags. So I looked everywhere except where this
> constraint is pointed out.
> 
> Anyway, I understand your concern about already having a knob to avoid
> the issue, my concern here is that the knob isn't quite obvious, and
> that you do need an additional knob to have a "secure" default. So
> IMHO it's easy to miss as an inexperienced user. Though at least in
> the !MAB case, disabling learning on the port is also enough to avoid
> that (and keeps learning via link local enabled for unlocked ports).
> 
> At least in the case of having enabled BR_PORT_MAB, I would consider
> it a bug that the entries learned via link local traffic aren't marked
> as BR_FDB_LOCKED. If you agree, I can send in a reduced patch for
> that, so that the entries are initially locked regardless the source
> of learning.

I will give a bit of background so that my answer will make more sense.

AFAICT, there are three different ways to deploy 802.1X / MAB:

1. 802.1X with static FDB entries. In this case learning can be
disabled.

2. 802.1X with dynamic FDB entries. In this case learning needs to be
enabled so that entries will be refreshed by incoming traffic.

3. MAB. In this case learning needs to be enabled so that user space
will be notified about hosts that are trying to communicate through the
bridge.

When the original patch was posted I was not aware of the last two use
cases that require learning to be enabled.

In any scenario where you have +learning +locked (regardless of +/-mab)
you need to have +no_linklocal_learn for things to work correctly, so
the potential for regressions from the original patch seems low to me.

The original patch also provides a more comprehensive solution to the
problem than marking entries learned from link local traffic with
BR_FDB_LOCKED. It applies regardless of +/-mab (i.e., it covers both
cases 2 and 3 and not only 3). That is why I prefer the original patch
over the proposed approach.

Re: [PATCH RFC] net: bridge: handle ports in locked mode for ll learning

[Jonas Gorski]

On 11.12.24 15:50, Ido Schimmel wrote:
> On Wed, Dec 11, 2024 at 11:32:38AM +0100, Jonas Gorski wrote:
>> Am Mi., 11. Dez. 2024 um 09:42 Uhr schrieb Ido Schimmel <idosch@nvidia.com>:
>>>
>>> On Tue, Dec 10, 2024 at 04:28:54PM +0100, Jonas Gorski wrote:
>>>> Thanks for the pointer. Reading the discussion, it seems this was
>>>> before the explicit BR_PORT_MAB option and locked learning support, so
>>>> there was some ambiguity around whether learning on locked ports is
>>>> desired or not, and this was needed(?) for the out-of-tree(?) MAB
>>>> implementation.
>>>
>>> There is a use case for learning on a locked port even without MAB. If
>>> user space is granting access via dynamic FDB entires, then you need
>>> learning enabled to refresh these entries.
>>
>> AFAICT this would still work with my patch, as long learning is
>> enabled for the port. The difference would be that new dynamic entries
>> won't be created anymore from link local learning, so userspace would
>> now have to add them themselves. But any existing dynamic entries will
>> be refreshed via the normal input paths.
>>
>> Though I see that this would break offloading these, since USER
>> dynamic entries are ignored in br_switchdev_fdb_notify() since
>> 927cdea5d209 ("net: bridge: switchdev: don't notify FDB entries with
>> "master dynamic""). Side note, br_switchdev_fdb_replay() seems to
>> still pass them on. Do I miss something or shouldn't replay also need
>> to ignore/skip them?
>>
>>>> But now that we do have an explicit flag for MAB, maybe this should be
>>>> revisited? Especially since with BR_PORT_MAB enabled, entries are
>>>> supposed to be learned as locked. But link local learned entries are
>>>> still learned unlocked. So no_linklocal_learn still needs to be
>>>> enabled for +locked, +learning, +mab.
>>>
>>> I mentioned this in the man page and added "no_linklocal_learn" to
>>> iproute2, but looks like it is not enough. You can try reposting the
>>> original patch (skip learning from link-local frames on a locked port)
>>> with a Fixes tag and see how it goes. I think it is unfortunate to
>>> change the behavior when there is already a dedicated knob for what you
>>> want to achieve, but I suspect the change will not introduce regressions
>>> so maybe people will find it acceptable.
>>
>> Absolutely not your fault; my reference was the original cover letters
>> for BR_PORT_LOCKED and BR_PORT_MAB and reading br_input.c where the
>> flags are handled (not even looking at if_link.h's doc comments). And
>> there the constraint/side effect isn't mentioned anywhere, so I
>> assumed it was unintentional. And I never looked at any man pages,
>> just used bridge link help to find out what the arguments are to
>> (un)set those port flags. So I looked everywhere except where this
>> constraint is pointed out.
>>
>> Anyway, I understand your concern about already having a knob to avoid
>> the issue, my concern here is that the knob isn't quite obvious, and
>> that you do need an additional knob to have a "secure" default. So
>> IMHO it's easy to miss as an inexperienced user. Though at least in
>> the !MAB case, disabling learning on the port is also enough to avoid
>> that (and keeps learning via link local enabled for unlocked ports).
>>
>> At least in the case of having enabled BR_PORT_MAB, I would consider
>> it a bug that the entries learned via link local traffic aren't marked
>> as BR_FDB_LOCKED. If you agree, I can send in a reduced patch for
>> that, so that the entries are initially locked regardless the source
>> of learning.
> 
> I will give a bit of background so that my answer will make more sense.
> 
> AFAICT, there are three different ways to deploy 802.1X / MAB:
> 
> 1. 802.1X with static FDB entries. In this case learning can be
> disabled.
> 
> 2. 802.1X with dynamic FDB entries. In this case learning needs to be
> enabled so that entries will be refreshed by incoming traffic.
> 
> 3. MAB. In this case learning needs to be enabled so that user space
> will be notified about hosts that are trying to communicate through the
> bridge.
> 
> When the original patch was posted I was not aware of the last two use
> cases that require learning to be enabled.
> 
> In any scenario where you have +learning +locked (regardless of +/-mab)
> you need to have +no_linklocal_learn for things to work correctly, so
> the potential for regressions from the original patch seems low to me.
> 
> The original patch also provides a more comprehensive solution to the
> problem than marking entries learned from link local traffic with
> BR_FDB_LOCKED. It applies regardless of +/-mab (i.e., it covers both
> cases 2 and 3 and not only 3). That is why I prefer the original patch
> over the proposed approach.

The original patch (just disabling LL learning if port is locked) has
the same issue as mine, it will indirectly break switchdev offloading
for case 2 when not using MAB (the kernel feature).

Once we disable creating dynamic entries in the kernel, userspace needs
to create them, and userspace dynamic entries have the user bit set,
which makes them get ignored by switchdev.

Ofc enabling MAB and then unlocking the locked entries hosts that
successfully authenticated should still work for 2, as long as the host
sent something other than link local traffic to create a (locked)
dynamic entry AFAIU.

FWIW, my proposed change/fix would be:

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index ceaa5a89b947..41b69ea300bf 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -238,7 +238,8 @@ static void __br_handle_local_finish(struct sk_buff *skb)
 	    nbp_state_should_learn(p) &&
 	    !br_opt_get(p->br, BROPT_NO_LL_LEARN) &&
 	    br_should_learn(p, skb, &vid))
-		br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid, 0);
+		br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid,
+			      p->flags & BR_PORT_MAB ? BIT(BR_FDB_LOCKED) : 0);
 }
 
 /* note: already called with rcu_read_lock */

which just makes sure that when MAB is enabled, link local learned
entries are also locked. This relies on br_fdb_update() ignoring most
flags for existing entries, not sure if this is a good idea though.


Best Regards,
Jonas

-- 
BISDN GmbH
Körnerstraße 7-10
10785 Berlin
Germany


Phone: 
+49-30-6108-1-6100


Managing Directors: 
Dr.-Ing. Hagen Woesner, Andreas 
Köpsel


Commercial register: 
Amtsgericht Berlin-Charlottenburg HRB 141569 
B
VAT ID No: DE283257294

Re: [PATCH RFC] net: bridge: handle ports in locked mode for ll learning

[Ido Schimmel]

On Thu, Dec 12, 2024 at 10:50:10AM +0100, Jonas Gorski wrote:
> The original patch (just disabling LL learning if port is locked) has
> the same issue as mine, it will indirectly break switchdev offloading
> for case 2 when not using MAB (the kernel feature).
> 
> Once we disable creating dynamic entries in the kernel, userspace needs
> to create them,

But the whole point of the "locked" feature is to defer the installation
of FDB entries to user space so that the control plane will be able to
decide which hosts can communicate through the bridge. Having the kernel
auto-populate the FDB based on incoming packets defeats this purpose,
which is why the man page mentions the "no_linklocal_learn" option and
why I think there is a very low risk of regressions from the original
patch.

> and userspace dynamic entries have the user bit set, which makes them
> get ignored by switchdev.

The second use case never worked correctly in the offload case. It is
not a regression.

> 
> Ofc enabling MAB and then unlocking the locked entries hosts that
> successfully authenticated should still work for 2, as long as the host
> sent something other than link local traffic to create a (locked)
> dynamic entry AFAIU.
> 
> FWIW, my proposed change/fix would be:
> 
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index ceaa5a89b947..41b69ea300bf 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -238,7 +238,8 @@ static void __br_handle_local_finish(struct sk_buff *skb)
>  	    nbp_state_should_learn(p) &&
>  	    !br_opt_get(p->br, BROPT_NO_LL_LEARN) &&
>  	    br_should_learn(p, skb, &vid))
> -		br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid, 0);
> +		br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid,
> +			      p->flags & BR_PORT_MAB ? BIT(BR_FDB_LOCKED) : 0);

IIUC, this will potentially roam FDB entries to unauthorized ports,
unlike the implementation in br_handle_frame_finish(). I documented it
in commit a35ec8e38cdd ("bridge: Add MAC Authentication Bypass (MAB)
support") in "1. Roaming".

>  }
>  
>  /* note: already called with rcu_read_lock */
> 
> which just makes sure that when MAB is enabled, link local learned
> entries are also locked. This relies on br_fdb_update() ignoring most
> flags for existing entries, not sure if this is a good idea though.
> 
> 
> Best Regards,
> Jonas
> 
> -- 
> BISDN GmbH
> Körnerstraße 7-10
> 10785 Berlin
> Germany
> 
> 
> Phone: 
> +49-30-6108-1-6100
> 
> 
> Managing Directors: 
> Dr.-Ing. Hagen Woesner, Andreas 
> Köpsel
> 
> 
> Commercial register: 
> Amtsgericht Berlin-Charlottenburg HRB 141569 
> B
> VAT ID No: DE283257294
> 

Re: [PATCH RFC] net: bridge: handle ports in locked mode for ll learning

[Jonas Gorski]

On 12.12.24 13:25, Ido Schimmel wrote:
> On Thu, Dec 12, 2024 at 10:50:10AM +0100, Jonas Gorski wrote:
>> The original patch (just disabling LL learning if port is locked) has
>> the same issue as mine, it will indirectly break switchdev offloading
>> for case 2 when not using MAB (the kernel feature).
>>
>> Once we disable creating dynamic entries in the kernel, userspace needs
>> to create them,
> 
> But the whole point of the "locked" feature is to defer the installation
> of FDB entries to user space so that the control plane will be able to
> decide which hosts can communicate through the bridge. Having the kernel
> auto-populate the FDB based on incoming packets defeats this purpose,
> which is why the man page mentions the "no_linklocal_learn" option and
> why I think there is a very low risk of regressions from the original
> patch.
> 
>> and userspace dynamic entries have the user bit set, which makes them
>> get ignored by switchdev.
> 
> The second use case never worked correctly in the offload case. It is
> not a regression.

Ah, I just noticed I incorrectly assumed that unlocking an entry just 
means removing the locked flag, but any change from user space also 
implicitly adds the user bit. So they would never get offloaded.

So there currently is no way to get offloaded dynamic entries with 
locked ports, regardless of MAB or not. And implementing that is 
definitely not a oneliner.

>> FWIW, my proposed change/fix would be:
>>
>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>> index ceaa5a89b947..41b69ea300bf 100644
>> --- a/net/bridge/br_input.c
>> +++ b/net/bridge/br_input.c
>> @@ -238,7 +238,8 @@ static void __br_handle_local_finish(struct sk_buff *skb)
>>   	    nbp_state_should_learn(p) &&
>>   	    !br_opt_get(p->br, BROPT_NO_LL_LEARN) &&
>>   	    br_should_learn(p, skb, &vid))
>> -		br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid, 0);
>> +		br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid,
>> +			      p->flags & BR_PORT_MAB ? BIT(BR_FDB_LOCKED) : 0);
> 
> IIUC, this will potentially roam FDB entries to unauthorized ports,
> unlike the implementation in br_handle_frame_finish(). I documented it
> in commit a35ec8e38cdd ("bridge: Add MAC Authentication Bypass (MAB)
> support") in "1. Roaming".
Good point, missed that br_fdb_update() will automatically clear the 
locked flag on roaming regardless of flags including it.

So disregard that idea as well, and we would need to do what I 
originally proposed to allow locked learning via link local traffic.

But since there is no difference between kernel entries that userspace 
unlocked and dynamic entries userspace added as I now found out, it also 
isn't needed at all. Userspace can just add dynamic entries if needed.

So in conclusion, I agree with the original patch. Shall I resend it? 
Should I extend the commit message?

Best Regards,
Jonas

-- 
BISDN GmbH
Körnerstraße 7-10
10785 Berlin
Germany


Phone: 
+49-30-6108-1-6100


Managing Directors: 
Dr.-Ing. Hagen Woesner, Andreas 
Köpsel


Commercial register: 
Amtsgericht Berlin-Charlottenburg HRB 141569 
B
VAT ID No: DE283257294

Re: [PATCH RFC] net: bridge: handle ports in locked mode for ll learning

[Ido Schimmel]

On Fri, Dec 13, 2024 at 12:25:20PM +0100, Jonas Gorski wrote:
> So in conclusion, I agree with the original patch. Shall I resend it? Should
> I extend the commit message?

Yes. I would use something like:

"
There are legitimate use cases for enabling learning on a locked bridge
port such as MAC Authentication Bypass (MAB) or when user space
authorizes hosts using dynamic FDB entries.

Currently, by default, the bridge will autonomously populate its FDB
with addresses learned from link-local frames. This is true even when a
port is locked which defeats the purpose of the "locked" bridge port
option. The behavior can be controlled by the "no_linklocal_learn"
bridge option, but it is easy to miss which leads to insecure
configurations.

Fix this by skipping learning from link-local frames when a port is
locked.

Fixes: a21d9a670d81 ("net: bridge: Add support for bridge port in locked mode")
"