Re: [PATCH RFC net-next 0/3] net: vlan: fix bridge binding behavior and add selftests

[Sevinj Aghayeva]

On Wed, Aug 10, 2022 at 11:10 AM Nikolay Aleksandrov
<razor@xxxxxxxxxxxxx> wrote:
>
> On 10/08/2022 18:00, Sevinj Aghayeva wrote:
> > On Wed, Aug 10, 2022 at 10:50 AM Nikolay Aleksandrov
> > <razor@xxxxxxxxxxxxx> wrote:
> >>
> >> On 10/08/2022 17:42, Sevinj Aghayeva wrote:
> >>>
> >>>
> >>> On Wed, Aug 10, 2022 at 4:54 AM Nikolay Aleksandrov <razor@xxxxxxxxxxxxx <mailto:razor@xxxxxxxxxxxxx>> 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 first run the attached pre.sh script that sets up the environment
> > and creates a vlan interface with bridge binding off. I then start
> > recording with perf, and here's the list of probes:
> >
> > $ sudo ./k/linux/tools/perf/perf probe -l
> >   probe:br_vlan_bridge_event (on br_vlan_bridge_event in bridge with event dev)
> >   probe:br_vlan_port_event (on br_vlan_port_event in bridge with event)
> >   probe:br_vlan_set_vlan_dev_state (on br_vlan_set_vlan_dev_state in
> > bridge with br vlan_dev)
> >   probe:register_vlan_dev (on register_vlan_dev in 8021q with dev)
> >   probe:vlan_changelink (on vlan_changelink in 8021q with dev)
> >   probe:vlan_dev_change_flags (on vlan_dev_change_flags in 8021q with dev)
> >   probe:vlan_dev_fix_features (on vlan_dev_fix_features in 8021q with dev)
> >   probe:vlan_dev_init  (on vlan_dev_init in 8021q with dev)
> >   probe:vlan_dev_ioctl (on vlan_dev_ioctl in 8021q with dev)
> >   probe:vlan_dev_open  (on vlan_dev_open in 8021q with dev)
> >   probe:vlan_dev_stop  (on vlan_dev_stop in 8021q with dev)
> >   probe:vlan_dev_uninit (on vlan_dev_uninit in 8021q with dev)
> >   probe:vlan_newlink   (on vlan_newlink in 8021q with dev)
> >
> > I then run the following command to turn the bridge binding flag on:
> >
> > $ sudo ip link set vlan10 type vlan bridge_binding on
> >
> > Then I stop the recording and print out the events, and I see this. I
> > don't see br_vlan_port_event or br_vlan_bridge_event getting called.
> >
> >               ip  5933 [003]  2204.722470:
> > probe:vlan_changelink: (ffffffffc1042b50) dev="vlan10"
> >               ip  5933 [003]  2204.722476:
> > probe:vlan_dev_change_flags: (ffffffffc1042600) dev="vlan10"
> >
> > Am I doing something wrong?
> >
> > Thanks
> >
> >
>
> You can't expect to see br_vlan_bridge_event() called because the notification
> target device is vlan10 and not the bridge. See br_device_event():
> ...
>         if (netif_is_bridge_master(dev)) {
>                 err = br_vlan_bridge_event(dev, event, ptr);
>                 if (err)
>                         return notifier_from_errno(err);
> ...
>
>
> Try probing for br_device_event(), you'll see it gets called every time you change the flag.
>
You're right, I did see br_device_event() getting called. Thanks!

-- 

Sevinj.Aghayeva