[Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Hans Schultz]

This patch is related to the patch set
"Add support for locked bridge ports (for 802.1X)"
Link: https://lore.kernel.org/netdev/20220223101650.1212814-1-schultz.hans+netdev@gmail.com/

This patch makes the locked port feature work with learning turned on,
which is enabled with the command:

bridge link set dev DEV learning on

Without this patch, link local traffic (01:80:c2) like EAPOL packets will
create a fdb entry when ingressing on a locked port with learning turned
on, thus unintentionally opening up the port for traffic for the said MAC.

Some switchcore features like Mac-Auth and refreshing of FDB entries,
require learning enables on some switchcores, f.ex. the mv88e6xxx family.
Other features may apply too.

Since many switchcores trap or mirror various multicast packets to the
CPU, link local traffic will unintentionally unlock the port for the
SA mac in question unless prevented by this patch.

Signed-off-by: Hans Schultz <hans@kapio-technology.com>
---
 net/bridge/br_input.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 68b3e850bcb9..a3ce0a151817 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -215,6 +215,7 @@ static void __br_handle_local_finish(struct sk_buff *skb)
 	if ((p->flags & BR_LEARNING) &&
 	    nbp_state_should_learn(p) &&
 	    !br_opt_get(p->br, BROPT_NO_LL_LEARN) &&
+	    !(p->flags & BR_PORT_LOCKED) &&
 	    br_should_learn(p, skb, &vid))
 		br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid, 0);
 }
-- 
2.30.2

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Nikolay Aleksandrov]

On 30/06/2022 14:16, Hans Schultz wrote:
> This patch is related to the patch set
> "Add support for locked bridge ports (for 802.1X)"
> Link: https://lore.kernel.org/netdev/20220223101650.1212814-1-schultz.hans+netdev@gmail.com/
> 
> This patch makes the locked port feature work with learning turned on,
> which is enabled with the command:
> 
> bridge link set dev DEV learning on
> 
> Without this patch, link local traffic (01:80:c2) like EAPOL packets will
> create a fdb entry when ingressing on a locked port with learning turned
> on, thus unintentionally opening up the port for traffic for the said MAC.
> 
> Some switchcore features like Mac-Auth and refreshing of FDB entries,
> require learning enables on some switchcores, f.ex. the mv88e6xxx family.
> Other features may apply too.
> 
> Since many switchcores trap or mirror various multicast packets to the
> CPU, link local traffic will unintentionally unlock the port for the
> SA mac in question unless prevented by this patch.
> 
> Signed-off-by: Hans Schultz <hans@kapio-technology.com>
> ---
>  net/bridge/br_input.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 68b3e850bcb9..a3ce0a151817 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -215,6 +215,7 @@ static void __br_handle_local_finish(struct sk_buff *skb)
>  	if ((p->flags & BR_LEARNING) &&
>  	    nbp_state_should_learn(p) &&
>  	    !br_opt_get(p->br, BROPT_NO_LL_LEARN) &&
> +	    !(p->flags & BR_PORT_LOCKED) &&
>  	    br_should_learn(p, skb, &vid))
>  		br_fdb_update(p->br, p, eth_hdr(skb)->h_source, vid, 0);
>  }

LGTM, thanks!
Acked-by: Nikolay Aleksandrov <razor@blackwall.org>

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Ido Schimmel]

On Thu, Jun 30, 2022 at 01:16:34PM +0200, Hans Schultz wrote:
> This patch is related to the patch set
> "Add support for locked bridge ports (for 802.1X)"
> Link: https://lore.kernel.org/netdev/20220223101650.1212814-1-schultz.hans+netdev@gmail.com/
> 
> This patch makes the locked port feature work with learning turned on,
> which is enabled with the command:
> 
> bridge link set dev DEV learning on
> 
> Without this patch, link local traffic (01:80:c2) like EAPOL packets will
> create a fdb entry when ingressing on a locked port with learning turned
> on, thus unintentionally opening up the port for traffic for the said MAC.
> 
> Some switchcore features like Mac-Auth and refreshing of FDB entries,
> require learning enables on some switchcores, f.ex. the mv88e6xxx family.
> Other features may apply too.
> 
> Since many switchcores trap or mirror various multicast packets to the
> CPU, link local traffic will unintentionally unlock the port for the
> SA mac in question unless prevented by this patch.

Why not just teach hostapd to do:

echo 1 > /sys/class/net/br0/bridge/no_linklocal_learn

?

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Hans Schultz]

On Thu, Jun 30, 2022 at 14:37, Ido Schimmel <idosch@nvidia.com> wrote:
> On Thu, Jun 30, 2022 at 01:16:34PM +0200, Hans Schultz wrote:
>
> Why not just teach hostapd to do:
>
> echo 1 > /sys/class/net/br0/bridge/no_linklocal_learn
>
> ?

Ahh yes, link local vs mc. If that covers all (as it seems to) that
should be sufficient.

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Hans S]

One question though... wouldn't it be an issue that the mentioned
option setting is bridge wide, while the patch applies a per-port
effect?

On Thu, Jun 30, 2022 at 1:38 PM Ido Schimmel <idosch@nvidia.com> wrote:
>
> On Thu, Jun 30, 2022 at 01:16:34PM +0200, Hans Schultz wrote:
> > This patch is related to the patch set
> > "Add support for locked bridge ports (for 802.1X)"
> > Link: https://lore.kernel.org/netdev/20220223101650.1212814-1-schultz.hans+netdev@gmail.com/
> >
> > This patch makes the locked port feature work with learning turned on,
> > which is enabled with the command:
> >
> > bridge link set dev DEV learning on
> >
> > Without this patch, link local traffic (01:80:c2) like EAPOL packets will
> > create a fdb entry when ingressing on a locked port with learning turned
> > on, thus unintentionally opening up the port for traffic for the said MAC.
> >
> > Some switchcore features like Mac-Auth and refreshing of FDB entries,
> > require learning enables on some switchcores, f.ex. the mv88e6xxx family.
> > Other features may apply too.
> >
> > Since many switchcores trap or mirror various multicast packets to the
> > CPU, link local traffic will unintentionally unlock the port for the
> > SA mac in question unless prevented by this patch.
>
> Why not just teach hostapd to do:
>
> echo 1 > /sys/class/net/br0/bridge/no_linklocal_learn
>
> ?

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Ido Schimmel]

On Fri, Jul 01, 2022 at 09:47:24AM +0200, Hans S wrote:
> One question though... wouldn't it be an issue that the mentioned
> option setting is bridge wide, while the patch applies a per-port
> effect?

Why would it be an issue? To me, the bigger issue is changing the
semantics of "locked" in 5.20 compared to previous kernels.

What is even the use case for enabling learning when the port is locked?
In current kernels, only SAs from link local traffic will be learned,
but with this patch, nothing will be learned. So why enable learning in
the first place? As an administrator, I mark a port as "locked" so that
only traffic with SAs that I configured will be allowed. Enabling
learning when the port is locked seems to defeat the purpose?

It would be helpful to explain why mv88e6xxx needs to have learning
enabled in the first place. IIUC, the software bridge can function
correctly with learning disabled. It might be better to solve this in
mv88e6xxx so that user space would not need to enable learning on the SW
bridge and then work around issues caused by it such as learning from
link local traffic.

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Vladimir Oltean]

On Fri, Jul 01, 2022 at 04:51:44PM +0300, Ido Schimmel wrote:
> On Fri, Jul 01, 2022 at 09:47:24AM +0200, Hans S wrote:
> > One question though... wouldn't it be an issue that the mentioned
> > option setting is bridge wide, while the patch applies a per-port
> > effect?
> 
> Why would it be an issue? To me, the bigger issue is changing the
> semantics of "locked" in 5.20 compared to previous kernels.
> 
> What is even the use case for enabling learning when the port is locked?
> In current kernels, only SAs from link local traffic will be learned,
> but with this patch, nothing will be learned. So why enable learning in
> the first place? As an administrator, I mark a port as "locked" so that
> only traffic with SAs that I configured will be allowed. Enabling
> learning when the port is locked seems to defeat the purpose?

I think if learning on a locked port doesn't make sense, the bridge
should just reject that configuration.

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Ido Schimmel]

On Fri, Jul 01, 2022 at 06:27:00PM +0300, Vladimir Oltean wrote:
> On Fri, Jul 01, 2022 at 04:51:44PM +0300, Ido Schimmel wrote:
> > On Fri, Jul 01, 2022 at 09:47:24AM +0200, Hans S wrote:
> > > One question though... wouldn't it be an issue that the mentioned
> > > option setting is bridge wide, while the patch applies a per-port
> > > effect?
> > 
> > Why would it be an issue? To me, the bigger issue is changing the
> > semantics of "locked" in 5.20 compared to previous kernels.
> > 
> > What is even the use case for enabling learning when the port is locked?
> > In current kernels, only SAs from link local traffic will be learned,
> > but with this patch, nothing will be learned. So why enable learning in
> > the first place? As an administrator, I mark a port as "locked" so that
> > only traffic with SAs that I configured will be allowed. Enabling
> > learning when the port is locked seems to defeat the purpose?
> 
> I think if learning on a locked port doesn't make sense, the bridge
> should just reject that configuration.

I tend to agree... Let's wait for Hans to explain why learning needs to
be enabled on mv88e6xxx and see how we handle it in mv88e6xxx and the
bridge.

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Hans S]

There is several issues when learning is turned off with the mv88e6xxx driver:

Mac-Auth requires learning turned on, otherwise there will be no miss
violation interrupts afair.
Refreshing of ATU entries does not work with learning turn off, as the
PAV is set to zero when learning is turned off.
This then further eliminates the use of the HoldAt1 feature and
age-out interrupts.

With dynamic ATU entries (an upcoming patch set), an authorized unit
gets a dynamic ATU entry, and if it goes quiet for 5 minutes, it's
entry will age out and thus get removed.
That also solves the port relocation issue as if a device relocates to
another port it will be able to get access again after 5 minutes.

On Fri, Jul 1, 2022 at 3:51 PM Ido Schimmel <idosch@nvidia.com> wrote:
>
> On Fri, Jul 01, 2022 at 09:47:24AM +0200, Hans S wrote:
> > One question though... wouldn't it be an issue that the mentioned
> > option setting is bridge wide, while the patch applies a per-port
> > effect?
>
> Why would it be an issue? To me, the bigger issue is changing the
> semantics of "locked" in 5.20 compared to previous kernels.
>
> What is even the use case for enabling learning when the port is locked?
> In current kernels, only SAs from link local traffic will be learned,
> but with this patch, nothing will be learned. So why enable learning in
> the first place? As an administrator, I mark a port as "locked" so that
> only traffic with SAs that I configured will be allowed. Enabling
> learning when the port is locked seems to defeat the purpose?
>
> It would be helpful to explain why mv88e6xxx needs to have learning
> enabled in the first place. IIUC, the software bridge can function
> correctly with learning disabled. It might be better to solve this in
> mv88e6xxx so that user space would not need to enable learning on the SW
> bridge and then work around issues caused by it such as learning from
> link local traffic.

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Ido Schimmel]

On Fri, Jul 01, 2022 at 06:07:10PM +0200, Hans S wrote:
> There is several issues when learning is turned off with the mv88e6xxx driver:

Please don't top-post...

> 
> Mac-Auth requires learning turned on, otherwise there will be no miss
> violation interrupts afair.
> Refreshing of ATU entries does not work with learning turn off, as the
> PAV is set to zero when learning is turned off.
> This then further eliminates the use of the HoldAt1 feature and
> age-out interrupts.
> 
> With dynamic ATU entries (an upcoming patch set), an authorized unit
> gets a dynamic ATU entry, and if it goes quiet for 5 minutes, it's
> entry will age out and thus get removed.
> That also solves the port relocation issue as if a device relocates to
> another port it will be able to get access again after 5 minutes.

You assume I'm familiar with mv88e6xxx, when in fact I'm not. Here is
what I think you are saying:

1. When a port is locked and a packet is received with a SA that is not
in the FDB, it will only generate a miss violation if learning is
enabled. In which case, you will notify the bridge driver about this
entry as externally learned and locked entry.

2. When a port is locked and a packet is received with a SA that matches
a different port, it will be dropped regardless if learning is enabled
or not.

3. From the above I conclude that the HW will not auto-populate its FDB
when a port is locked.

4. FDB entries that point to a port that does not have learning enabled
are not subject to ageing (why?).

Assuming the above is correct, in order for mv88e6xxx to work correctly,
it needs to enable learning on all locked ports, but it should happen
regardless of the bridge driver learning configuration let alone impose
any limitations on it. In fact, hostapd must disable learning for all
locked ports.

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Hans S]

On Fri, Jul 1, 2022 at 7:00 PM Ido Schimmel <idosch@nvidia.com> wrote:
>
> On Fri, Jul 01, 2022 at 06:07:10PM +0200, Hans S wrote:
> > There is several issues when learning is turned off with the mv88e6xxx driver:
>
> Please don't top-post...

Sorry, I am using gmails own web interface for a short while now as my
other options are not supported anymore by Google (not secure apps)

>
> >
> > Mac-Auth requires learning turned on, otherwise there will be no miss
> > violation interrupts afair.
> > Refreshing of ATU entries does not work with learning turn off, as the
> > PAV is set to zero when learning is turned off.
> > This then further eliminates the use of the HoldAt1 feature and
> > age-out interrupts.
> >
> > With dynamic ATU entries (an upcoming patch set), an authorized unit
> > gets a dynamic ATU entry, and if it goes quiet for 5 minutes, it's
> > entry will age out and thus get removed.
> > That also solves the port relocation issue as if a device relocates to
> > another port it will be able to get access again after 5 minutes.
>
> You assume I'm familiar with mv88e6xxx, when in fact I'm not. Here is
> what I think you are saying:
>
> 1. When a port is locked and a packet is received with a SA that is not
> in the FDB, it will only generate a miss violation if learning is
> enabled. In which case, you will notify the bridge driver about this
> entry as externally learned and locked entry.

Right.

> 2. When a port is locked and a packet is received with a SA that matches
> a different port, it will be dropped regardless if learning is enabled
> or not.

I would think so.

> 3. From the above I conclude that the HW will not auto-populate its FDB
> when a port is locked.

Right, and it should not as the locked port feature is basically CPU
controlled learning.
(yes it is an irony to have CPU controlled learning and learning
turned on, but that is just how it is with the mv88e6xxx series :-) )

> 4. FDB entries that point to a port that does not have learning enabled
> are not subject to ageing (why?).

Sorry if I said so. Dynamic ATU entries will age I am sure, but they
will not refresh unless there is a match between the ingress port and
the Port Association Vector (PAV).
But an age out violation will not occur, and the HoldAt1 (entries age
from 7 -> 0) feature will not work either as it is related to the
refresh mechanism.

>
> Assuming the above is correct, in order for mv88e6xxx to work correctly,
> it needs to enable learning on all locked ports, but it should happen
> regardless of the bridge driver learning configuration let alone impose
> any limitations on it. In fact, hostapd must disable learning for all
> locked ports.

To have hardware induced refreshing I would say learning should be on
also for 802.1X (hostapd). This relies of course on user added dynamic
ATU entries, which is what my follow-up patch set is about. Besides it
is perfectly feasible to have both 802.1X and Mac-Auth on the same
port.

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Ido Schimmel]

On Fri, Jul 01, 2022 at 09:17:27PM +0200, Hans S wrote:
> On Fri, Jul 1, 2022 at 7:00 PM Ido Schimmel <idosch@nvidia.com> wrote:
> >
> > On Fri, Jul 01, 2022 at 06:07:10PM +0200, Hans S wrote:
> > > There is several issues when learning is turned off with the mv88e6xxx driver:
> >
> > Please don't top-post...
> 
> Sorry, I am using gmails own web interface for a short while now as my
> other options are not supported anymore by Google (not secure apps)
> 
> >
> > >
> > > Mac-Auth requires learning turned on, otherwise there will be no miss
> > > violation interrupts afair.
> > > Refreshing of ATU entries does not work with learning turn off, as the
> > > PAV is set to zero when learning is turned off.
> > > This then further eliminates the use of the HoldAt1 feature and
> > > age-out interrupts.
> > >
> > > With dynamic ATU entries (an upcoming patch set), an authorized unit
> > > gets a dynamic ATU entry, and if it goes quiet for 5 minutes, it's
> > > entry will age out and thus get removed.
> > > That also solves the port relocation issue as if a device relocates to
> > > another port it will be able to get access again after 5 minutes.
> >
> > You assume I'm familiar with mv88e6xxx, when in fact I'm not. Here is
> > what I think you are saying:
> >
> > 1. When a port is locked and a packet is received with a SA that is not
> > in the FDB, it will only generate a miss violation if learning is
> > enabled. In which case, you will notify the bridge driver about this
> > entry as externally learned and locked entry.
> 
> Right.
> 
> > 2. When a port is locked and a packet is received with a SA that matches
> > a different port, it will be dropped regardless if learning is enabled
> > or not.
> 
> I would think so.
> 
> > 3. From the above I conclude that the HW will not auto-populate its FDB
> > when a port is locked.
> 
> Right, and it should not as the locked port feature is basically CPU
> controlled learning.
> (yes it is an irony to have CPU controlled learning and learning
> turned on, but that is just how it is with the mv88e6xxx series :-) )
> 
> > 4. FDB entries that point to a port that does not have learning enabled
> > are not subject to ageing (why?).
> 
> Sorry if I said so. Dynamic ATU entries will age I am sure, but they
> will not refresh unless there is a match between the ingress port and
> the Port Association Vector (PAV).
> But an age out violation will not occur, and the HoldAt1 (entries age
> from 7 -> 0) feature will not work either as it is related to the
> refresh mechanism.
> 
> >
> > Assuming the above is correct, in order for mv88e6xxx to work correctly,
> > it needs to enable learning on all locked ports, but it should happen
> > regardless of the bridge driver learning configuration let alone impose
> > any limitations on it. In fact, hostapd must disable learning for all
> > locked ports.
> 
> To have hardware induced refreshing I would say learning should be on
> also for 802.1X (hostapd). This relies of course on user added dynamic
> ATU entries, which is what my follow-up patch set is about. Besides it
> is perfectly feasible to have both 802.1X and Mac-Auth on the same
> port.

IIUC, with mv88e6xxx, when the port is locked and learning is disabled:

1. You do not get miss violation interrupts. Meaning, you can't report
'locked' entries to the bridge driver.

2. You do not get aged-out interrupts. Meaning, you can't tell the
bridge driver to remove aged-out entries.

My point is that this should happen regardless if learning is enabled on
the bridge driver or not. Just make sure it is always enabled in
mv88e6xxx when the port is locked. Learning in the bridge driver itself
can be off, thereby eliminating the need to disable learning from
link-local packets.

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Hans S]

>
> IIUC, with mv88e6xxx, when the port is locked and learning is disabled:
>
> 1. You do not get miss violation interrupts. Meaning, you can't report
> 'locked' entries to the bridge driver.
>
> 2. You do not get aged-out interrupts. Meaning, you can't tell the
> bridge driver to remove aged-out entries.
>
> My point is that this should happen regardless if learning is enabled on
> the bridge driver or not. Just make sure it is always enabled in
> mv88e6xxx when the port is locked. Learning in the bridge driver itself
> can be off, thereby eliminating the need to disable learning from
> link-local packets.

So you suggest that we enable learning in the driver when locking the
port and document that learning should be turned off from user space
before locking the port?

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Ido Schimmel]

On Mon, Jul 04, 2022 at 09:54:31AM +0200, Hans S wrote:
> >
> > IIUC, with mv88e6xxx, when the port is locked and learning is disabled:
> >
> > 1. You do not get miss violation interrupts. Meaning, you can't report
> > 'locked' entries to the bridge driver.
> >
> > 2. You do not get aged-out interrupts. Meaning, you can't tell the
> > bridge driver to remove aged-out entries.
> >
> > My point is that this should happen regardless if learning is enabled on
> > the bridge driver or not. Just make sure it is always enabled in
> > mv88e6xxx when the port is locked. Learning in the bridge driver itself
> > can be off, thereby eliminating the need to disable learning from
> > link-local packets.
> 
> So you suggest that we enable learning in the driver when locking the
> port and document that learning should be turned off from user space
> before locking the port?

Yes. Ideally, the bridge driver would reject configurations where
learning is enabled and the port is locked, but it might be too late for
that. It would be good to add a note in the man page that learning
should be disabled when the port is locked to avoid "unlocking" the port
by accident.

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Hans S]

On Mon, Jul 4, 2022 at 1:00 PM Ido Schimmel <idosch@nvidia.com> wrote:
>
> On Mon, Jul 04, 2022 at 09:54:31AM +0200, Hans S wrote:
> > >
> > > IIUC, with mv88e6xxx, when the port is locked and learning is disabled:
> > >
> > > 1. You do not get miss violation interrupts. Meaning, you can't report
> > > 'locked' entries to the bridge driver.
> > >
> > > 2. You do not get aged-out interrupts. Meaning, you can't tell the
> > > bridge driver to remove aged-out entries.
> > >
> > > My point is that this should happen regardless if learning is enabled on
> > > the bridge driver or not. Just make sure it is always enabled in
> > > mv88e6xxx when the port is locked. Learning in the bridge driver itself
> > > can be off, thereby eliminating the need to disable learning from
> > > link-local packets.
> >
> > So you suggest that we enable learning in the driver when locking the
> > port and document that learning should be turned off from user space
> > before locking the port?
>
> Yes. Ideally, the bridge driver would reject configurations where
> learning is enabled and the port is locked, but it might be too late for
> that. It would be good to add a note in the man page that learning
> should be disabled when the port is locked to avoid "unlocking" the port
> by accident.

Well you cannot unlock the port by either enabling or disabling
learning after the port is locked, but Mac-Auth and refreshing might
not work. I clarify just so that no-one gets confused.

I can do so that the driver returns -EINVAL if learning is on when
locking the port, but that would of course only be for mv88e6xxx...

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Ido Schimmel]

On Mon, Jul 04, 2022 at 04:36:12PM +0200, Hans S wrote:
> On Mon, Jul 4, 2022 at 1:00 PM Ido Schimmel <idosch@nvidia.com> wrote:
> >
> > On Mon, Jul 04, 2022 at 09:54:31AM +0200, Hans S wrote:
> > > >
> > > > IIUC, with mv88e6xxx, when the port is locked and learning is disabled:
> > > >
> > > > 1. You do not get miss violation interrupts. Meaning, you can't report
> > > > 'locked' entries to the bridge driver.
> > > >
> > > > 2. You do not get aged-out interrupts. Meaning, you can't tell the
> > > > bridge driver to remove aged-out entries.
> > > >
> > > > My point is that this should happen regardless if learning is enabled on
> > > > the bridge driver or not. Just make sure it is always enabled in
> > > > mv88e6xxx when the port is locked. Learning in the bridge driver itself
> > > > can be off, thereby eliminating the need to disable learning from
> > > > link-local packets.
> > >
> > > So you suggest that we enable learning in the driver when locking the
> > > port and document that learning should be turned off from user space
> > > before locking the port?
> >
> > Yes. Ideally, the bridge driver would reject configurations where
> > learning is enabled and the port is locked, but it might be too late for
> > that. It would be good to add a note in the man page that learning
> > should be disabled when the port is locked to avoid "unlocking" the port
> > by accident.
> 
> Well you cannot unlock the port by either enabling or disabling
> learning after the port is locked, but Mac-Auth and refreshing might
> not work. I clarify just so that no-one gets confused.

I was referring to the fact that if learning is enabled, a host can
populate the FDB with whatever MAC it wants by crafting a link-local
packet with this MAC as SA. Subsequent packets with this MAC as SA will
pass the locking check in the bridge driver.

> I can do so that the driver returns -EINVAL if learning is on when
> locking the port, but that would of course only be for mv88e6xxx...

Working around this issue in the mv88e6xxx driver is the correct thing
to do, IMO. We avoid leaking this implementation detail (i.e., forcing
learning to be enabled) to user space, which in turn helps us avoid
working around issues created by it (this patch, for example).

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Vladimir Oltean]

Hi Hans,

On Fri, Jul 01, 2022 at 06:07:10PM +0200, Hans S wrote:
> On Fri, Jul 1, 2022 at 3:51 PM Ido Schimmel <idosch@nvidia.com> wrote:
> >
> > On Fri, Jul 01, 2022 at 09:47:24AM +0200, Hans S wrote:
> > > One question though... wouldn't it be an issue that the mentioned
> > > option setting is bridge wide, while the patch applies a per-port
> > > effect?
> >
> > Why would it be an issue? To me, the bigger issue is changing the
> > semantics of "locked" in 5.20 compared to previous kernels.
> >
> > What is even the use case for enabling learning when the port is locked?
> > In current kernels, only SAs from link local traffic will be learned,
> > but with this patch, nothing will be learned. So why enable learning in
> > the first place? As an administrator, I mark a port as "locked" so that
> > only traffic with SAs that I configured will be allowed. Enabling
> > learning when the port is locked seems to defeat the purpose?
> >
> > It would be helpful to explain why mv88e6xxx needs to have learning
> > enabled in the first place. IIUC, the software bridge can function
> > correctly with learning disabled. It might be better to solve this in
> > mv88e6xxx so that user space would not need to enable learning on the SW
> > bridge and then work around issues caused by it such as learning from
> > link local traffic.
> 
> There is several issues when learning is turned off with the mv88e6xxx driver:
> 
> Mac-Auth requires learning turned on, otherwise there will be no miss
> violation interrupts afair.
> Refreshing of ATU entries does not work with learning turn off, as the
> PAV is set to zero when learning is turned off.
> This then further eliminates the use of the HoldAt1 feature and
> age-out interrupts.
> 
> With dynamic ATU entries (an upcoming patch set), an authorized unit
> gets a dynamic ATU entry, and if it goes quiet for 5 minutes, it's
> entry will age out and thus get removed.
> That also solves the port relocation issue as if a device relocates to
> another port it will be able to get access again after 5 minutes.

I think the discussion derailed at this exact point, when you responded
that "Mac-Auth requires learning turned on".

What precise feature do you describe when you say "Mac-Auth"? Do you
mean 802.1X MAC-based authentication in general (where data plane
packets on a locked port are dropped unless their MAC SA is in the FDB,
and populating the FDB is *entirely* up to user space, there aren't any
locked FDB entries on locked ports), or MAC authentication *bypass*
(where the kernel auto-adds locked FDB entries on locked ports)?

I *think* it's just the bypass that requires learning in mv88e6xxx.
But the bypass (the feature where the kernel auto-adds locked FDB
entries on locked ports) doesn't exist in net-next.

Here, what happens is that a locked port learns the MAC SA from the
traffic it didn't drop, i.e. link-local. In other words, the bridge
behaves as expected and instructed: +locked +learning will cause just
that. It's the administrator's fault for not disabling learning.
It's also the mv88e6xxx driver's fault for not validating the "locked" +
"learning" brport flag *combination* until it properly supports "+locked
+learning" (the feature you are currently working on).

I'm still confused why we don't just say that "+locked -learning" means
plain 802.1X, "+locked +learning" means MAB where we learn locked FDB entries.

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Vladimir Oltean]

On Sun, Jul 17, 2022 at 04:46:10PM +0300, Vladimir Oltean wrote:
> Here, what happens is that a locked port learns the MAC SA from the
> traffic it didn't drop, i.e. link-local. In other words, the bridge
> behaves as expected and instructed: +locked +learning will cause just
> that. It's the administrator's fault for not disabling learning.
> It's also the mv88e6xxx driver's fault for not validating the "locked" +
> "learning" brport flag *combination* until it properly supports "+locked
> +learning" (the feature you are currently working on).
> 
> I'm still confused why we don't just say that "+locked -learning" means
> plain 802.1X, "+locked +learning" means MAB where we learn locked FDB entries.

Or is it the problem that a "+locked +learning" bridge port will learn
MAC SA from link-local traffic, but it will create FDB entries without
the locked flag while doing so? The mv88e6xxx driver should react to the
'locked' flag from both directions (ADD_TO_DEVICE too, not just ADD_TO_BRIDGE).

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Hans S]

On Sun, Jul 17, 2022 at 4:03 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Sun, Jul 17, 2022 at 04:46:10PM +0300, Vladimir Oltean wrote:
> > Here, what happens is that a locked port learns the MAC SA from the
> > traffic it didn't drop, i.e. link-local. In other words, the bridge
> > behaves as expected and instructed: +locked +learning will cause just
> > that. It's the administrator's fault for not disabling learning.
> > It's also the mv88e6xxx driver's fault for not validating the "locked" +
> > "learning" brport flag *combination* until it properly supports "+locked
> > +learning" (the feature you are currently working on).
> >
> > I'm still confused why we don't just say that "+locked -learning" means
> > plain 802.1X, "+locked +learning" means MAB where we learn locked FDB entries.
>
> Or is it the problem that a "+locked +learning" bridge port will learn
> MAC SA from link-local traffic, but it will create FDB entries without
> the locked flag while doing so? The mv88e6xxx driver should react to the
> 'locked' flag from both directions (ADD_TO_DEVICE too, not just ADD_TO_BRIDGE).

Yes, it creates an FDB entry in the bridge without the locked flag
set, and sends an ADD_TO_DEVICE notice with it.
And furthermore link-local packets include of course EAPOL packets, so
that's why +learning is a problem.

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Vladimir Oltean]

On Sun, Jul 17, 2022 at 06:22:57PM +0200, Hans S wrote:
> On Sun, Jul 17, 2022 at 4:03 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > On Sun, Jul 17, 2022 at 04:46:10PM +0300, Vladimir Oltean wrote:
> > > Here, what happens is that a locked port learns the MAC SA from the
> > > traffic it didn't drop, i.e. link-local. In other words, the bridge
> > > behaves as expected and instructed: +locked +learning will cause just
> > > that. It's the administrator's fault for not disabling learning.
> > > It's also the mv88e6xxx driver's fault for not validating the "locked" +
> > > "learning" brport flag *combination* until it properly supports "+locked
> > > +learning" (the feature you are currently working on).
> > >
> > > I'm still confused why we don't just say that "+locked -learning" means
> > > plain 802.1X, "+locked +learning" means MAB where we learn locked FDB entries.
> >
> > Or is it the problem that a "+locked +learning" bridge port will learn
> > MAC SA from link-local traffic, but it will create FDB entries without
> > the locked flag while doing so? The mv88e6xxx driver should react to the
> > 'locked' flag from both directions (ADD_TO_DEVICE too, not just ADD_TO_BRIDGE).
> 
> Yes, it creates an FDB entry in the bridge without the locked flag
> set, and sends an ADD_TO_DEVICE notice with it.
> And furthermore link-local packets include of course EAPOL packets, so
> that's why +learning is a problem.

So if we fix that, and make the dynamically learned FDB entry be locked
because the port is locked (and offload them correctly in mv88e6xxx),
what would be the problem, exactly? The +learning is what would allow
these locked FDB entries to be created, and would allow the MAB to work.
User space may still decide to not authorize this address, and it will
remain locked.

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Hans S]

On Sun, Jul 17, 2022 at 8:38 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Sun, Jul 17, 2022 at 06:22:57PM +0200, Hans S wrote:
> > On Sun, Jul 17, 2022 at 4:03 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > Yes, it creates an FDB entry in the bridge without the locked flag
> > set, and sends an ADD_TO_DEVICE notice with it.
> > And furthermore link-local packets include of course EAPOL packets, so
> > that's why +learning is a problem.
>
> So if we fix that, and make the dynamically learned FDB entry be locked
> because the port is locked (and offload them correctly in mv88e6xxx),
> what would be the problem, exactly? The +learning is what would allow
> these locked FDB entries to be created, and would allow the MAB to work.
> User space may still decide to not authorize this address, and it will
> remain locked.

The alternative is to have -learning and let the driver only enable
the PAV to admit the interrupts, which is what this implementation
does.
The plus side of this is that having EAPOL packets triggering locked
entries from the bridge side is not really so nice IMHO. In a
situation with 802.1X and MAB on the same port, there will then not be
any triggering of MAB when initiating the 802.1X session, which I
think is the best option. It then also lessens the confusion between
hostapd and the daemon that handles MAB sessions.

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Vladimir Oltean]

On Sun, Jul 17, 2022 at 09:20:57PM +0200, Hans S wrote:
> On Sun, Jul 17, 2022 at 8:38 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > On Sun, Jul 17, 2022 at 06:22:57PM +0200, Hans S wrote:
> > > On Sun, Jul 17, 2022 at 4:03 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > >
> > > Yes, it creates an FDB entry in the bridge without the locked flag
> > > set, and sends an ADD_TO_DEVICE notice with it.
> > > And furthermore link-local packets include of course EAPOL packets, so
> > > that's why +learning is a problem.
> >
> > So if we fix that, and make the dynamically learned FDB entry be locked
> > because the port is locked (and offload them correctly in mv88e6xxx),
> > what would be the problem, exactly? The +learning is what would allow
> > these locked FDB entries to be created, and would allow the MAB to work.
> > User space may still decide to not authorize this address, and it will
> > remain locked.
> 
> The alternative is to have -learning and let the driver only enable
> the PAV to admit the interrupts, which is what this implementation
> does.
> The plus side of this is that having EAPOL packets triggering locked
> entries from the bridge side is not really so nice IMHO. In a
> situation with 802.1X and MAB on the same port, there will then not be
> any triggering of MAB when initiating the 802.1X session, which I
> think is the best option. It then also lessens the confusion between
> hostapd and the daemon that handles MAB sessions.

Why is it "not really so nice" to "trigger MAB" (in fact only to learn a
locked entry on a locked port) when initiating the 802.1X session?
You can disable link-local learning via the bridge option if you're
really bothered by that. When you have MAB enabled on an 802.1X port,
I think it's reasonable to expect that there will be some locked entries
which user space won't ever unlock via MAB. If those entries happen to
be created as a side effect of the normal EAPOL authentication process,
I don't exactly see where is the functional problem. This shouldn't
block EAPOL from proceeding any further, because this protocol uses
link-local packets which are classified as control traffic, and that
isn't subject to FDB lookups but rather always trapped to CPU, so locked
or not, it should still be received.

I'm only pointing out the obvious here, we need an opt in for MAB, and
the implemented behavior I've seen here kind of points to mapping this
to "+learning +locked", where the learning process creates locked FDB entries.

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Hans S]

On Thu, Jul 21, 2022 at 1:45 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Why is it "not really so nice" to "trigger MAB" (in fact only to learn a
> locked entry on a locked port) when initiating the 802.1X session?

The consideration is mostly to limit (not eliminate) double
actrivation, e.g. activation of 802.1X and MAB at roughly the same
time, so that the daemons will have more to do coordinating which has
the session.

> You can disable link-local learning via the bridge option if you're

The issue here is that you can only disable it bridge wide and not per port.

> really bothered by that. When you have MAB enabled on an 802.1X port,
> I think it's reasonable to expect that there will be some locked entries
> which user space won't ever unlock via MAB. If those entries happen to
> be created as a side effect of the normal EAPOL authentication process,
> I don't exactly see where is the functional problem. This shouldn't
> block EAPOL from proceeding any further, because this protocol uses
> link-local packets which are classified as control traffic, and that
> isn't subject to FDB lookups but rather always trapped to CPU, so locked
> or not, it should still be received.
>
> I'm only pointing out the obvious here, we need an opt in for MAB, and
> the implemented behavior I've seen here kind of points to mapping this
> to "+learning +locked", where the learning process creates locked FDB entries.

If we need an opt in for MAB, you are right. Only then I think that we
need to solve the link-local learning issue so that it is disabled per
port?

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Hans S]

On Thu, Jul 21, 2022 at 1:45 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Sun, Jul 17, 2022 at 09:20:57PM +0200, Hans S wrote:
>
> I'm only pointing out the obvious here, we need an opt in for MAB, and
> the implemented behavior I've seen here kind of points to mapping this
> to "+learning +locked", where the learning process creates locked FDB entries.

I can go with the reasoning for the opt in for MAB, but disabling link
local learning system wide I don't think is a good idea, unless
someone can ensure me that it does not impact something else.
In general locked ports should never learn from link local, which is a
problem if they do, which suggests to me that this patch should
eventually be accepted as the best solution.

Re: [Bridge] [PATCH net-next v1 1/1] net: bridge: ensure that link-local traffic cannot unlock a locked port

[Hans S]

On Sun, Jul 24, 2022 at 10:09 AM Hans S <schultz.hans@gmail.com> wrote:
>
> On Thu, Jul 21, 2022 at 1:45 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > On Sun, Jul 17, 2022 at 09:20:57PM +0200, Hans S wrote:
> >
> > I'm only pointing out the obvious here, we need an opt in for MAB, and
> > the implemented behavior I've seen here kind of points to mapping this
> > to "+learning +locked", where the learning process creates locked FDB entries.
>
> I can go with the reasoning for the opt in for MAB, but disabling link
> local learning system wide I don't think is a good idea, unless
> someone can ensure me that it does not impact something else.
> In general locked ports should never learn from link local, which is a
> problem if they do, which suggests to me that this patch should
> eventually be accepted as the best solution.

Hi Vladimir,
sorry, I forget myself. We cannot use +learning as an opt in for MAB
with this driver, as there will be no HW refresh and other interrupts
like the age out violation will not occur either, which will be needed
further on.
If we really need an opt in for MAB, I think it will have to be a new flag.
Hans