Sevinj Aghayeva
2022-Jul-30 16:46 UTC
[Bridge] [PATCH net-next 0/3] net: vlan: fix bridge binding behavior and add selftests
On Sat, Jul 30, 2022 at 12:22 PM Nikolay Aleksandrov <razor at blackwall.org> wrote:> On 7/30/22 19:03, Sevinj Aghayeva wrote: > > When bridge binding is enabled for a vlan interface, it is expected > > that the link state of the vlan interface will track the subset of the > > ports that are also members of the corresponding vlan, rather than > > that of all ports. > > > > Currently, this feature works as expected when a vlan interface is > > created with bridge binding enabled: > > > > ip link add link br name vlan10 type vlan id 10 protocol 802.1q \ > > bridge_binding on > > > > However, the feature does not work when a vlan interface is created > > with bridge binding disabled, and then enabled later: > > > > ip link add link br name vlan10 type vlan id 10 protocol 802.1q \ > > bridge_binding off > > ip link set vlan10 type vlan bridge_binding on > > > > After these two commands, the link state of the vlan interface > > continues to track that of all ports, which is inconsistent and > > confusing to users. This series fixes this bug and introduces two > > tests for the valid behavior. > > > > Sevinj Aghayeva (3): > > net: bridge: export br_vlan_upper_change > > net: 8021q: fix bridge binding behavior for vlan interfaces > > selftests: net: tests for bridge binding behavior > > > > include/linux/if_bridge.h | 9 ++ > > net/8021q/vlan.h | 2 +- > > net/8021q/vlan_dev.c | 21 ++- > > net/bridge/br_vlan.c | 7 +- > > tools/testing/selftests/net/Makefile | 1 + > > .../selftests/net/bridge_vlan_binding_test.sh | 143 ++++++++++++++++++ > > 6 files changed, 176 insertions(+), 7 deletions(-) > > create mode 100755 > tools/testing/selftests/net/bridge_vlan_binding_test.sh > > > > Hmm.. I don't like this and don't think this bridge function should be > exported at all. > > Calling bridge state changing functions from 8021q module is not the > proper way to solve this. The problem is that the bridge doesn't know > that the state has changed, so you can process NETDEV_CHANGE events and > check for the bridge vlan which got its state changed and react based on > it. I haven't checked in detail, but I think it should be doable. So all > the logic is kept inside the bridge. >Hi Nik, Can please elaborate on where I should process NETDEV_CHANGE events? I'm doing this as part of outreachy project and this is my first kernel task, so I don't know the bridging code that well. Thanks!> > Cheers, > Nik >-- Sevinj.Aghayeva -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.linuxfoundation.org/pipermail/bridge/attachments/20220730/03440b34/attachment.html>
Sevinj Aghayeva
2022-Jul-30 16:48 UTC
[Bridge] [PATCH net-next 0/3] net: vlan: fix bridge binding behavior and add selftests
(Resending this because the first email was rejected due to being in HTML.) On Sat, Jul 30, 2022 at 12:46 PM Sevinj Aghayeva <sevinj.aghayeva at gmail.com> wrote:> > > > On Sat, Jul 30, 2022 at 12:22 PM Nikolay Aleksandrov <razor at blackwall.org> wrote: >> >> On 7/30/22 19:03, Sevinj Aghayeva wrote: >> > When bridge binding is enabled for a vlan interface, it is expected >> > that the link state of the vlan interface will track the subset of the >> > ports that are also members of the corresponding vlan, rather than >> > that of all ports. >> > >> > Currently, this feature works as expected when a vlan interface is >> > created with bridge binding enabled: >> > >> > ip link add link br name vlan10 type vlan id 10 protocol 802.1q \ >> > bridge_binding on >> > >> > However, the feature does not work when a vlan interface is created >> > with bridge binding disabled, and then enabled later: >> > >> > ip link add link br name vlan10 type vlan id 10 protocol 802.1q \ >> > bridge_binding off >> > ip link set vlan10 type vlan bridge_binding on >> > >> > After these two commands, the link state of the vlan interface >> > continues to track that of all ports, which is inconsistent and >> > confusing to users. This series fixes this bug and introduces two >> > tests for the valid behavior. >> > >> > Sevinj Aghayeva (3): >> > net: bridge: export br_vlan_upper_change >> > net: 8021q: fix bridge binding behavior for vlan interfaces >> > selftests: net: tests for bridge binding behavior >> > >> > include/linux/if_bridge.h | 9 ++ >> > net/8021q/vlan.h | 2 +- >> > net/8021q/vlan_dev.c | 21 ++- >> > net/bridge/br_vlan.c | 7 +- >> > tools/testing/selftests/net/Makefile | 1 + >> > .../selftests/net/bridge_vlan_binding_test.sh | 143 ++++++++++++++++++ >> > 6 files changed, 176 insertions(+), 7 deletions(-) >> > create mode 100755 tools/testing/selftests/net/bridge_vlan_binding_test.sh >> > >> >> Hmm.. I don't like this and don't think this bridge function should be >> exported at all. >> >> Calling bridge state changing functions from 8021q module is not the >> proper way to solve this. The problem is that the bridge doesn't know >> that the state has changed, so you can process NETDEV_CHANGE events and >> check for the bridge vlan which got its state changed and react based on >> it. I haven't checked in detail, but I think it should be doable. So all >> the logic is kept inside the bridge. > > > Hi Nik, > > Can please elaborate on where I should process NETDEV_CHANGE events? I'm doing this as part of outreachy project and this is my first kernel task, so I don't know the bridging code that well. > > Thanks! > >> >> >> Cheers, >> Nik > > > > -- > > Sevinj.Aghayeva-- Sevinj.Aghayeva
Roopa Prabhu
2022-Jul-31 01:54 UTC
[Bridge] [PATCH net-next 0/3] net: vlan: fix bridge binding behavior and add selftests
On 7/30/22 09:48, Sevinj Aghayeva wrote:> (Resending this because the first email was rejected due to being in HTML.) > > > On Sat, Jul 30, 2022 at 12:46 PM Sevinj Aghayeva > <sevinj.aghayeva at gmail.com> wrote: >> >> >> On Sat, Jul 30, 2022 at 12:22 PM Nikolay Aleksandrov <razor at blackwall.org> wrote: >>> On 7/30/22 19:03, Sevinj Aghayeva wrote: >>>> When bridge binding is enabled for a vlan interface, it is expected >>>> that the link state of the vlan interface will track the subset of the >>>> ports that are also members of the corresponding vlan, rather than >>>> that of all ports. >>>> >>>> Currently, this feature works as expected when a vlan interface is >>>> created with bridge binding enabled: >>>> >>>> ip link add link br name vlan10 type vlan id 10 protocol 802.1q \ >>>> bridge_binding on >>>> >>>> However, the feature does not work when a vlan interface is created >>>> with bridge binding disabled, and then enabled later: >>>> >>>> ip link add link br name vlan10 type vlan id 10 protocol 802.1q \ >>>> bridge_binding off >>>> ip link set vlan10 type vlan bridge_binding on >>>> >>>> After these two commands, the link state of the vlan interface >>>> continues to track that of all ports, which is inconsistent and >>>> confusing to users. This series fixes this bug and introduces two >>>> tests for the valid behavior. >>>> >>>> Sevinj Aghayeva (3): >>>> net: bridge: export br_vlan_upper_change >>>> net: 8021q: fix bridge binding behavior for vlan interfaces >>>> selftests: net: tests for bridge binding behavior >>>> >>>> include/linux/if_bridge.h | 9 ++ >>>> net/8021q/vlan.h | 2 +- >>>> net/8021q/vlan_dev.c | 21 ++- >>>> net/bridge/br_vlan.c | 7 +- >>>> tools/testing/selftests/net/Makefile | 1 + >>>> .../selftests/net/bridge_vlan_binding_test.sh | 143 ++++++++++++++++++ >>>> 6 files changed, 176 insertions(+), 7 deletions(-) >>>> create mode 100755 tools/testing/selftests/net/bridge_vlan_binding_test.sh >>>> >>> Hmm.. I don't like this and don't think this bridge function should be >>> exported at all. >>> >>> Calling bridge state changing functions from 8021q module is not the >>> proper way to solve this. The problem is that the bridge doesn't know >>> that the state has changed, so you can process NETDEV_CHANGE events and >>> check for the bridge vlan which got its state changed and react based on >>> it. I haven't checked in detail, but I think it should be doable. So all >>> the logic is kept inside the bridge. >> >> Hi Nik, >> >> Can please elaborate on where I should process NETDEV_CHANGE events? I'm doing this as part of outreachy project and this is my first kernel task, so I don't know the bridging code that well. >> >> Thanks!good point Nikolay. Sevinj, see br_vlan_bridge_event and __vlan_device_event? for how both drivers react to netdev change events. I have not looked at it in detail yet, but lets explore and discuss if we can make use of events to achieve same results.