Sevinj Aghayeva
2022-Aug-10 14:42 UTC
[Bridge] [PATCH RFC net-next 0/3] net: vlan: fix bridge binding behavior and add selftests
On Wed, Aug 10, 2022 at 4:54 AM Nikolay Aleksandrov <razor at blackwall.org> wrote:> On 10/08/2022 06:11, 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: core: export call_netdevice_notifiers_info > > net: 8021q: fix bridge binding behavior for vlan interfaces > > selftests: net: tests for bridge binding behavior > > > > include/linux/netdevice.h | 2 + > > net/8021q/vlan.h | 2 +- > > net/8021q/vlan_dev.c | 25 ++- > > net/core/dev.c | 7 +- > > tools/testing/selftests/net/Makefile | 1 + > > .../selftests/net/bridge_vlan_binding_test.sh | 143 ++++++++++++++++++ > > 6 files changed, 172 insertions(+), 8 deletions(-) > > create mode 100755 > tools/testing/selftests/net/bridge_vlan_binding_test.sh > > > > Hi, > NETDEV_CHANGE event is already propagated when the vlan changes flags,I'm not sure if NETDEV_CHANGE is actually propagated when the vlan changes flags. The two functions in the bridge module that handle NETDEV_CHANGE are br_vlan_port_event and br_vlan_bridge_event. I've installed probes for both, and when I'm changing flags using "sudo ip link set vlan10 type vlan bridge_binding on", I don't see any of those functions getting called, although I do see vlan_dev_change_flags getting called. I think there may be a bug in core/dev.c:__dev_notify_flags.> > NETDEV_CHANGEUPPER is used when the devices' relationship changes not > their flags. > The only problem you have to figure out is that the flag has changed. The > fix itself > must be done within the bridge, not 8021q. You can figure it out based on > current bridge > loose binding state and the vlan's changed state, again in the bridge's > NETDEV_CHANGE > handler. Unfortunately the proper fix is much more involved and will need > new > infra, you'll have to track the loose binding vlans in the bridge. To do > that you should > add logic that reflects the current vlans' loose binding state *only* for > vlans that also > exist in the bridge, the rest which are upper should be carrier off if > they have the loose > binding flag set. > > Alternatively you can add a new NETDEV_ notifier (using something similar > to struct netdev_notifier_pre_changeaddr_info) > and add link type-specific space (e.g. union of link type-specific > structs) in the struct which will contain > what changed for 8021q and will be properly interpreted by the bridge. The > downside is that we'll generate > 2 notifications when changing the loose binding flag, but on the bright > side won't have to track anything > in the bridge, just handle the new notifier type. This might be the > easiest path, the fix is still in > the bridge though, the 8021q module just needs to fill in the new struct > and emit the notification on > any loose binding changes, the bridge must decide if it should process it > (i.e. based on upper/lower > relationship). Such notifier can be also re-used by other link types to > propagate link-type specific > changes. >I'll discuss this option with my mentors. Thanks.> > Both of these avoid any direct dependencies between the bridge and 8021q. > Any other suggestions that > are simpler, avoid direct dependencies and solve the issue in a generic > way would be appreciated. > > Just be careful about introducing too much unnecessary processing because > we > can have lots of vlan devices in a system. > > Cheers, > Nik >-- Sevinj.Aghayeva -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.linuxfoundation.org/pipermail/bridge/attachments/20220810/3ec2e174/attachment.html>
Nikolay Aleksandrov
2022-Aug-10 14:50 UTC
[Bridge] [PATCH RFC net-next 0/3] net: vlan: fix bridge binding behavior and add selftests
On 10/08/2022 17:42, Sevinj Aghayeva wrote:> > > On Wed, Aug 10, 2022 at 4:54 AM Nikolay Aleksandrov <razor at blackwall.org <mailto:razor at blackwall.org>> wrote: > > On 10/08/2022 06:11, 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: core: export call_netdevice_notifiers_info > >? ?net: 8021q: fix bridge binding behavior for vlan interfaces > >? ?selftests: net: tests for bridge binding behavior > > > >? include/linux/netdevice.h? ? ? ? ? ? ? ? ? ? ?|? ?2 + > >? net/8021q/vlan.h? ? ? ? ? ? ? ? ? ? ? ? ? ? ? |? ?2 +- > >? net/8021q/vlan_dev.c? ? ? ? ? ? ? ? ? ? ? ? ? |? 25 ++- > >? net/core/dev.c? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? |? ?7 +- > >? tools/testing/selftests/net/Makefile? ? ? ? ? |? ?1 + > >? .../selftests/net/bridge_vlan_binding_test.sh | 143 ++++++++++++++++++ > >? 6 files changed, 172 insertions(+), 8 deletions(-) > >? create mode 100755 tools/testing/selftests/net/bridge_vlan_binding_test.sh > > > > Hi, > NETDEV_CHANGE event is already propagated when the vlan changes flags, > > > I'm not sure if NETDEV_CHANGE is actually propagated when the vlan changes flags. The two functions in the bridge module that handle NETDEV_CHANGE are br_vlan_port_event? and br_vlan_bridge_event. I've installed probes for both, and when I'm changing flags using "sudo ip link set vlan10 type vlan bridge_binding on", I don't see any of those functions getting called, although I do see vlan_dev_change_flags getting called. I think there may be a bug in core/dev.c:__dev_notify_flags.are both vlan and bridge interfaces up? what exactly are you probing for? I can see the NETDEV_CHANGE event go through when changing the loose binding.