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