net: vlan: fix bridge binding behavior and add selftests

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

Quoted 28 lines · Sevinj Aghayeva

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

Quoted 8 lines

 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

Quoted 1 line

Hi,
NETDEV_CHANGE event is already propagated when the vlan changes 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.

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

On Wed, Aug 10, 2022 at 4:54 AM Nikolay Aleksandrov <[email protected] <mailto:[email protected]>> 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.

Quoted 43 lines · Sevinj Aghayeva

On Wed, Aug 10, 2022 at 4:54 AM Nikolay Aleksandrov <[email protected] <mailto:[email protected]>> 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 >

Quoted 5 lines

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.

Quoted 45 lines · Nikolay Aleksandrov

Quoted 43 lines · Sevinj Aghayeva

On Wed, Aug 10, 2022 at 4:54 AM Nikolay Aleksandrov <[email protected] <mailto:[email protected]>> 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 >

Quoted 8 lines

Quoted 5 lines

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

Quoted 84 lines · Sevinj Aghayeva

Quoted 45 lines · Nikolay Aleksandrov

Quoted 43 lines · Sevinj Aghayeva

On Wed, Aug 10, 2022 at 4:54 AM Nikolay Aleksandrov <[email protected] <mailto:[email protected]>> 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 >

Quoted 8 lines

Quoted 5 lines

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.

Quoted 10 lines

              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.

Quoted 28 lines · Sevinj Aghayeva

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

Quoted 9 lines

 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, 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.

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

Quoted 52 lines · Nikolay Aleksandrov

Quoted 28 lines · Sevinj Aghayeva

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

Quoted 9 lines

 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, 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.

Quoted 9 lines

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.

Hi Nik,

Can you please clarify the following?

1) should the new NETDEV_ notifier be about the vlan device and not the bridge? That is, should I handle it in br_device_event?

2) is it still okay to export call_netdevice_notifiers_info or should i write a new function for this?

The answers to the above wasn't clear to me, but I came up with the following patch anyway, so perhaps you can also comment on it. I'm pasting it inline; this is against 5.19.

include/linux/netdevice.h +0 -0

 extern rwlock_t dev_base_lock; /* Device list lock */

net/8021q/vlan.h +1 -1

index 5eaf38875554..71947cdcfaaa 100644
@@ -130,7 +130,7 @@ void vlan_dev_set_ingress_priority(const struct
 net_device *dev,
 int vlan_dev_set_egress_priority(const struct net_device *dev,
  u32 skb_prio, u16 vlan_prio);
 void vlan_dev_free_egress_priority(const struct net_device *dev);
-int vlan_dev_change_flags(const struct net_device *dev, u32 flag, u32 mask);
+int vlan_dev_change_flags(struct net_device *dev, u32 flag, u32 mask);
 void vlan_dev_get_realdev_name(const struct net_device *dev, char *result,
        size_t size);

net/8021q/vlan_dev.c +0 -0

index 839f2020b015..68da3901dfb0 100644

net/8021q/vlan_dev.c +18 -3

 /* Flags are defined in the vlan_flags enum in
  * include/uapi/linux/if_vlan.h file.
  */
-int vlan_dev_change_flags(const struct net_device *dev, u32 flags, u32 mask)
+int vlan_dev_change_flags(struct net_device *dev, u32 flags, u32 mask)
 {
+ struct netdev_notifier_changeupperflags_info info;
  struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
  u32 old_flags = vlan->flags;

@@ -223,19 +230,33 @@ int vlan_dev_change_flags(const struct
 net_device *dev, u32 flags, u32 mask)
 
  vlan->flags = (old_flags & ~mask) | (flags & mask);
 
- if (netif_running(dev) && (vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) {
+ if (!netif_running(dev))
+ return 0;
+
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) {
  if (vlan->flags & VLAN_FLAG_GVRP)
  vlan_gvrp_request_join(dev);
  else
  vlan_gvrp_request_leave(dev);
  }
 
- if (netif_running(dev) && (vlan->flags ^ old_flags) & VLAN_FLAG_MVRP) {
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_MVRP) {
  if (vlan->flags & VLAN_FLAG_MVRP)
  vlan_mvrp_request_join(dev);
  else
  vlan_mvrp_request_leave(dev);
  }
+
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_BRIDGE_BINDING &&
+    netif_is_bridge(vlan->real_dev)) {
+ info.info.dev = vlan->real_dev;
+ info.upper_dev = dev;
+ info.vlan_bridge_binding =
+    !!(vlan->flags & VLAN_FLAG_BRIDGE_BINDING);
+ call_netdevice_notifiers_info(NETDEV_CHANGEUPPERFLAGS,
+    &info.info);
+ }
+
  return 0;
 }

net/bridge/br_vlan.c +1 -0

index 0f5e75ccac79..cbcb0877d4a4 100644
@@ -1718,6 +1718,7 @@ static void nbp_vlan_set_vlan_dev_state(struct
 net_bridge_port *p, u16 vid)
 /* Must be protected by RTNL. */
 int br_vlan_bridge_event(struct net_device *dev, unsigned long event,
 void *ptr)
 {
+ struct netdev_notifier_changeupperflags_info *flags_info;
  struct netdev_notifier_changeupper_info *info;

  struct net_bridge *br = netdev_priv(dev);
  int vlcmd = 0, ret = 0;

net/bridge/br_vlan.c +5 -1

@@ -1739,7 +1740,11 @@ int br_vlan_bridge_event(struct net_device
 *dev, unsigned long event, void *ptr)
  info = ptr;
  br_vlan_upper_change(dev, info->upper_dev, info->linking);
  break;
-
+ case NETDEV_CHANGEUPPERFLAGS:
+ flags_info = ptr;
+ br_vlan_upper_change(dev, flags_info->upper_dev,
+    flags_info->vlan_bridge_binding);
+ break;
  case NETDEV_CHANGE:
  case NETDEV_UP:

if (!br_opt_get(br, BROPT_VLAN_BRIDGE_BINDING))

net/core/dev.c +3 -5

index 30a1603a7225..bc8640d77d83 100644
@@ -160,8 +160,6 @@ struct list_head ptype_base[PTYPE_HASH_SIZE] __read_mostly;
 struct list_head ptype_all __read_mostly; /* Taps */
 
 static int netif_rx_internal(struct sk_buff *skb);
-static int call_netdevice_notifiers_info(unsigned long val,
- struct netdev_notifier_info *info);
 static int call_netdevice_notifiers_extack(unsigned long val,
    struct net_device *dev,
    struct netlink_ext_ack *extack);

@@ -1624,7 +1622,7 @@ const char *netdev_cmd_to_name(enum netdev_cmd cmd)
  N(POST_INIT) N(RELEASE) N(NOTIFY_PEERS) N(JOIN) N(CHANGEUPPER)
  N(RESEND_IGMP) N(PRECHANGEMTU) N(CHANGEINFODATA) N(BONDING_INFO)
  N(PRECHANGEUPPER) N(CHANGELOWERSTATE) N(UDP_TUNNEL_PUSH_INFO)
- N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN)
+ N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN) N(CHANGEUPPERFLAGS)
  N(CVLAN_FILTER_PUSH_INFO) N(CVLAN_FILTER_DROP_INFO)
  N(SVLAN_FILTER_PUSH_INFO) N(SVLAN_FILTER_DROP_INFO)
  N(PRE_CHANGEADDR) N(OFFLOAD_XSTATS_ENABLE) N(OFFLOAD_XSTATS_DISABLE)

@@ -1927,8 +1925,8 @@ static void
 move_netdevice_notifiers_dev_net(struct net_device *dev,
  * are as for raw_notifier_call_chain().
  */
 
-static int call_netdevice_notifiers_info(unsigned long val,
- struct netdev_notifier_info *info)
+int call_netdevice_notifiers_info(unsigned long val,
+  struct netdev_notifier_info *info)
 {
  struct net *net = dev_net(info->dev);

int ret;

net/core/dev.c +1 -0

@@ -1944,6 +1942,7 @@ static int
 call_netdevice_notifiers_info(unsigned long val,
  return ret;
  return raw_notifier_call_chain(&netdev_chain, val, info);
 }
+EXPORT_SYMBOL(call_netdevice_notifiers_info);
 
 /**

* call_netdevice_notifiers_info_robust - call per-netns notifier blocks

Quoted 9 lines

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

Quoted 53 lines · Sevinj Aghayeva

Quoted 52 lines · Nikolay Aleksandrov

Quoted 28 lines · Sevinj Aghayeva

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

Quoted 9 lines

 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, 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.

Quoted 9 lines

Quoted 9 lines

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.

Hi,

Quoted 1 line

Quoted 6 lines

Hi Nik,

Can you please clarify the following?

1) should the new NETDEV_ notifier be about the vlan device and not the bridge? That is, should I handle it in br_device_event?

Yes, it should be about the vlan device (i.e. the target device that changes its state).

Quoted 4 lines

2) is it still okay to export call_netdevice_notifiers_info or should i write a new function for this?

If you need it, export it. But if you do it similar to netdev_notifier_pre_changeaddr_info then you don't have to, more below.

Quoted 4 lines

The answers to the above wasn't clear to me, but I came up with the following patch anyway, so perhaps you can also comment on it. I'm pasting it inline; this is against 5.19.

A few comments inline below,

include/linux/netdevice.h enum netdev_cmd { · line 2765

Quoted 11 lines

Thanks!

include/linux/netdevice.h +1 -0

index 2563d30736e9..c63205eb1f72 100644
@@ -2762,6 +2762,7 @@ enum netdev_cmd {
  NETDEV_UNREGISTER,
  NETDEV_CHANGEMTU, /* notify after mtu change happened */
  NETDEV_CHANGEADDR, /* notify after the address change */
+ NETDEV_CHANGEUPPERFLAGS,

Please don't use CHANGEUPPER, that is about a device changing its upper device. Also make it more generic, NETDEV_CHANGEFLAGS is too specific. For example today we have NETDEV_CHANGEINFODATA which TBH sounds good, but is tied to bonding in a few places, e.g.:

        case NETDEV_CHANGEINFODATA:
                rtnl_event_type = IFLA_EVENT_BONDING_OPTIONS;

which is very unfortunate. We really need a generic notifier that can pass link-type specific information alongside the device. As I mentioned please see how netdev_notifier_pre_changeaddr_info is handled, we need something generic that extends netdev_notifier_info and the various link types can add their own structures in a union which is to be interpreted based on the link type. For example if the new notifier is called NETDEV_CHANGE_DETAILS then in the bridge we'll check if the target device is a vlan and interpret the structure's union as the vlan change information. It'd be nice to get more feedback about this from others as well.

Also note that this notifier is for internal use for the time being so it's not necessary to export these notifications to user-space yet.

I would've opted for extending NETDEV_CHANGE itself, but that would be quite the adventure. :)

include/linux/netdevice.h struct netdev_notifier_changelowerstate_info { · line 2843

Quoted 10 lines

include/linux/netdevice.h +3 -0

  NETDEV_PRE_CHANGEADDR, /* notify before the address change */
  NETDEV_GOING_DOWN,
  NETDEV_CHANGENAME,

@@ -2837,6 +2838,12 @@ struct netdev_notifier_changelowerstate_info {
  void *lower_state_info; /* is lower dev state */
 };
 
+struct netdev_notifier_changeupperflags_info {
+ struct netdev_notifier_info info; /* must be first */
+ struct net_device *upper_dev;

just dev, not upper
we should be able to use this construct for any link type and actually we don't need the device here, we already have it in info.dev

include/linux/netdevice.h struct netdev_notifier_changelowerstate_info { · line 2844

Quoted 1 line

include/linux/netdevice.h +1 -0

+ bool vlan_bridge_binding;

add this into a vlan-specific structure that should be in a union here so other link types can add their own later

include/linux/netdevice.h netdev_notifier_info_to_extack(const struct · line 2910

Quoted 12 lines

include/linux/netdevice.h +4 -0

+};
+
 struct netdev_notifier_pre_changeaddr_info {
  struct netdev_notifier_info info; /* must be first */
  const unsigned char *dev_addr;

@@ -2898,6 +2905,8 @@ netdev_notifier_info_to_extack(const struct
 netdev_notifier_info *info)
 }
 
 int call_netdevice_notifiers(unsigned long val, struct net_device *dev);
+int call_netdevice_notifiers_info(unsigned long val,
+  struct netdev_notifier_info *info);

No need for this if you handle notifications similar to dev_pre_changeaddr_notify() with netdev_notifier_pre_changeaddr_info

Quoted 22 lines

include/linux/netdevice.h +0 -0

 extern rwlock_t dev_base_lock; /* Device list lock */

net/8021q/vlan.h +1 -1

index 5eaf38875554..71947cdcfaaa 100644
@@ -130,7 +130,7 @@ void vlan_dev_set_ingress_priority(const struct
 net_device *dev,
 int vlan_dev_set_egress_priority(const struct net_device *dev,
  u32 skb_prio, u16 vlan_prio);
 void vlan_dev_free_egress_priority(const struct net_device *dev);
-int vlan_dev_change_flags(const struct net_device *dev, u32 flag, u32 mask);
+int vlan_dev_change_flags(struct net_device *dev, u32 flag, u32 mask);
 void vlan_dev_get_realdev_name(const struct net_device *dev, char *result,
        size_t size);

net/8021q/vlan_dev.c +0 -0

index 839f2020b015..68da3901dfb0 100644

net/8021q/vlan_dev.c int vlan_dev_set_egress_priority(const struct · line 212

Quoted 5 lines

net/8021q/vlan_dev.c +1 -0

@@ -208,11 +208,18 @@ int vlan_dev_set_egress_priority(const struct
 net_device *dev,
  return 0;
 }
 
+static inline bool netif_is_bridge(const struct net_device *dev)

no inline in .c files, let the compiler decide

net/8021q/vlan_dev.c int vlan_dev_set_egress_priority(const struct · line 217

Quoted 5 lines

net/8021q/vlan_dev.c +5 -0

+{
+ return dev->rtnl_link_ops &&
+    !strcmp(dev->rtnl_link_ops->kind, "bridge");
+}
+

there is already netif_is_bridge_master()

Quoted 132 lines

net/8021q/vlan_dev.c +18 -3

 /* Flags are defined in the vlan_flags enum in
  * include/uapi/linux/if_vlan.h file.
  */
-int vlan_dev_change_flags(const struct net_device *dev, u32 flags, u32 mask)
+int vlan_dev_change_flags(struct net_device *dev, u32 flags, u32 mask)
 {
+ struct netdev_notifier_changeupperflags_info info;
  struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
  u32 old_flags = vlan->flags;

@@ -223,19 +230,33 @@ int vlan_dev_change_flags(const struct
 net_device *dev, u32 flags, u32 mask)
 
  vlan->flags = (old_flags & ~mask) | (flags & mask);
 
- if (netif_running(dev) && (vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) {
+ if (!netif_running(dev))
+ return 0;
+
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) {
  if (vlan->flags & VLAN_FLAG_GVRP)
  vlan_gvrp_request_join(dev);
  else
  vlan_gvrp_request_leave(dev);
  }
 
- if (netif_running(dev) && (vlan->flags ^ old_flags) & VLAN_FLAG_MVRP) {
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_MVRP) {
  if (vlan->flags & VLAN_FLAG_MVRP)
  vlan_mvrp_request_join(dev);
  else
  vlan_mvrp_request_leave(dev);
  }
+
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_BRIDGE_BINDING &&
+    netif_is_bridge(vlan->real_dev)) {
+ info.info.dev = vlan->real_dev;
+ info.upper_dev = dev;
+ info.vlan_bridge_binding =
+    !!(vlan->flags & VLAN_FLAG_BRIDGE_BINDING);
+ call_netdevice_notifiers_info(NETDEV_CHANGEUPPERFLAGS,
+    &info.info);
+ }
+
  return 0;
 }

net/bridge/br_vlan.c +1 -0

index 0f5e75ccac79..cbcb0877d4a4 100644
@@ -1718,6 +1718,7 @@ static void nbp_vlan_set_vlan_dev_state(struct
 net_bridge_port *p, u16 vid)
 /* Must be protected by RTNL. */
 int br_vlan_bridge_event(struct net_device *dev, unsigned long event,
 void *ptr)
 {
+ struct netdev_notifier_changeupperflags_info *flags_info;
  struct netdev_notifier_changeupper_info *info;

  struct net_bridge *br = netdev_priv(dev);
  int vlcmd = 0, ret = 0;

net/bridge/br_vlan.c +5 -1

@@ -1739,7 +1740,11 @@ int br_vlan_bridge_event(struct net_device
 *dev, unsigned long event, void *ptr)
  info = ptr;
  br_vlan_upper_change(dev, info->upper_dev, info->linking);
  break;
-
+ case NETDEV_CHANGEUPPERFLAGS:
+ flags_info = ptr;
+ br_vlan_upper_change(dev, flags_info->upper_dev,
+    flags_info->vlan_bridge_binding);
+ break;
  case NETDEV_CHANGE:
  case NETDEV_UP:

if (!br_opt_get(br, BROPT_VLAN_BRIDGE_BINDING))

net/core/dev.c +3 -5

index 30a1603a7225..bc8640d77d83 100644
@@ -160,8 +160,6 @@ struct list_head ptype_base[PTYPE_HASH_SIZE] __read_mostly;
 struct list_head ptype_all __read_mostly; /* Taps */
 
 static int netif_rx_internal(struct sk_buff *skb);
-static int call_netdevice_notifiers_info(unsigned long val,
- struct netdev_notifier_info *info);
 static int call_netdevice_notifiers_extack(unsigned long val,
    struct net_device *dev,
    struct netlink_ext_ack *extack);

@@ -1624,7 +1622,7 @@ const char *netdev_cmd_to_name(enum netdev_cmd cmd)
  N(POST_INIT) N(RELEASE) N(NOTIFY_PEERS) N(JOIN) N(CHANGEUPPER)
  N(RESEND_IGMP) N(PRECHANGEMTU) N(CHANGEINFODATA) N(BONDING_INFO)
  N(PRECHANGEUPPER) N(CHANGELOWERSTATE) N(UDP_TUNNEL_PUSH_INFO)
- N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN)
+ N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN) N(CHANGEUPPERFLAGS)
  N(CVLAN_FILTER_PUSH_INFO) N(CVLAN_FILTER_DROP_INFO)
  N(SVLAN_FILTER_PUSH_INFO) N(SVLAN_FILTER_DROP_INFO)
  N(PRE_CHANGEADDR) N(OFFLOAD_XSTATS_ENABLE) N(OFFLOAD_XSTATS_DISABLE)

@@ -1927,8 +1925,8 @@ static void
 move_netdevice_notifiers_dev_net(struct net_device *dev,
  * are as for raw_notifier_call_chain().
  */
 
-static int call_netdevice_notifiers_info(unsigned long val,
- struct netdev_notifier_info *info)
+int call_netdevice_notifiers_info(unsigned long val,
+  struct netdev_notifier_info *info)
 {
  struct net *net = dev_net(info->dev);

int ret;

net/core/dev.c +1 -0

@@ -1944,6 +1942,7 @@ static int
 call_netdevice_notifiers_info(unsigned long val,
  return ret;
  return raw_notifier_call_chain(&netdev_chain, val, info);
 }
+EXPORT_SYMBOL(call_netdevice_notifiers_info);
 
 /**

* call_netdevice_notifiers_info_robust - call per-netns notifier blocks

Quoted 9 lines

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

Quoted 68 lines · Nikolay Aleksandrov

Quoted 53 lines · Sevinj Aghayeva

Quoted 52 lines · Nikolay Aleksandrov

Quoted 28 lines · Sevinj Aghayeva

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

Quoted 9 lines

 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, 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.

Quoted 9 lines

Quoted 9 lines

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.

Hi,

Quoted 1 line

Quoted 8 lines

Quoted 6 lines

Hi Nik,

Can you please clarify the following?

1) should the new NETDEV_ notifier be about the vlan device and not the bridge? That is, should I handle it in br_device_event?

Yes, it should be about the vlan device (i.e. the target device that changes its state).

Hi Nik,

I implemented this and tried to handle NETDEV_CHANGE_DETAILS in br_device_event, but there's a check there that performs early return if the device is not a bridge port:

https://github.com/torvalds/linux/blob/master/net/bridge/br.c#L55-L57

Should I add a new function before that check, e.g. br_vlan_device_event, and handle vlan device events there, similar to br_vlan_bridge_event? Or do you have a better idea?

Thanks

Quoted 90 lines · Sevinj Aghayeva

Quoted 68 lines · Nikolay Aleksandrov

Quoted 53 lines · Sevinj Aghayeva

Quoted 52 lines · Nikolay Aleksandrov

Quoted 28 lines · Sevinj Aghayeva

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

Quoted 9 lines

 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, 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.

Quoted 9 lines

Quoted 9 lines

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.

Hi,

Quoted 1 line

Quoted 8 lines

Quoted 6 lines

Hi Nik,

Can you please clarify the following?

1) should the new NETDEV_ notifier be about the vlan device and not the bridge? That is, should I handle it in br_device_event?

Yes, it should be about the vlan device (i.e. the target device that changes its state).

Hi Nik,

I implemented this and tried to handle NETDEV_CHANGE_DETAILS in br_device_event, but there's a check there that performs early return if the device is not a bridge port:

https://github.com/torvalds/linux/blob/master/net/bridge/br.c#L55-L57

Should I add a new function before that check, e.g. br_vlan_device_event, and handle vlan device events there, similar to br_vlan_bridge_event? Or do you have a better idea?

Quoted 2 lines

Thanks

Hi,
Handling all vlan device-related changes in br_vlan_device_event() sounds good to me. Please add it to br_vlan.c.

Thanks,
Nik

Quoted 2 lines · Nikolay Aleksandrov

On 18/08/2022 14:50, Sevinj Aghayeva wrote:

Quoted 4 lines

Hi,
Handling all vlan device-related changes in br_vlan_device_event() sounds good to me. Please add it to br_vlan.c.

Hi Nik,

Can you please review this diff before I make it into a proper patchset? Thanks!

include/linux/netdevice.h +1 -0

index 2563d30736e9..0ce3da42325e 100644
@@ -2762,6 +2762,7 @@ enum netdev_cmd {
  NETDEV_UNREGISTER,
  NETDEV_CHANGEMTU, /* notify after mtu change happened */
  NETDEV_CHANGEADDR, /* notify after the address change */
+ NETDEV_CHANGE_DETAILS,
  NETDEV_PRE_CHANGEADDR, /* notify before the address change */
  NETDEV_GOING_DOWN,
  NETDEV_CHANGENAME,

include/linux/netdevice.h +0 -0

 int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
  struct netlink_ext_ack *extack);
 int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,

net/8021q/vlan.h +1 -1

index 5eaf38875554..71947cdcfaaa 100644
@@ -130,7 +130,7 @@ void vlan_dev_set_ingress_priority(const struct
 net_device *dev,
 int vlan_dev_set_egress_priority(const struct net_device *dev,
  u32 skb_prio, u16 vlan_prio);
 void vlan_dev_free_egress_priority(const struct net_device *dev);
-int vlan_dev_change_flags(const struct net_device *dev, u32 flag, u32 mask);
+int vlan_dev_change_flags(struct net_device *dev, u32 flag, u32 mask);
 void vlan_dev_get_realdev_name(const struct net_device *dev, char *result,
         size_t size);

net/8021q/vlan_dev.c +0 -0

index 839f2020b015..489baa8435de 100644

net/8021q/vlan_dev.c +0 -0

 {
  struct vlan_dev_priv *vlan = vlan_dev_priv(dev);

  u32 old_flags = vlan->flags;

net/8021q/vlan_dev.c +5 -2

@@ -223,19 +223,29 @@ int vlan_dev_change_flags(const struct
 net_device *dev, u32 flags, u32 mask)
 
  vlan->flags = (old_flags & ~mask) | (flags & mask);
 
- if (netif_running(dev) && (vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) {
+ if (!netif_running(dev))
+ return 0;
+
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) {
  if (vlan->flags & VLAN_FLAG_GVRP)
  vlan_gvrp_request_join(dev);
  else
  vlan_gvrp_request_leave(dev);
  }
 
- if (netif_running(dev) && (vlan->flags ^ old_flags) & VLAN_FLAG_MVRP) {
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_MVRP) {

net/8021q/vlan_dev.c +2 -0

+ }
+
  return 0;
 }

net/bridge/br.c +5 -0

index 96e91d69a9a8..62e939c6a3f0 100644
@@ -51,6 +51,11 @@ static int br_device_event(struct notifier_block
 *unused, unsigned long event, v
  }
  }
 
+ if (is_vlan_dev(dev)) {
+ br_vlan_device_event(dev, event, ptr);
+ return NOTIFY_DONE;
+ }
+
  /* not a port of a bridge */
  p = br_port_get_rtnl(dev);

if (!p)

net/bridge/br_private.h +7 -0

index 06e5f6faa431..a9a08e49c76c 100644
@@ -1470,6 +1470,8 @@ void br_vlan_get_stats(const struct net_bridge_vlan *v,
 void br_vlan_port_event(struct net_bridge_port *p, unsigned long event);
 int br_vlan_bridge_event(struct net_device *dev, unsigned long event,
  void *ptr);
+void br_vlan_device_event(struct net_device *dev, unsigned long event,
+   void *ptr);
 void br_vlan_rtnl_init(void);
 void br_vlan_rtnl_uninit(void);
 void br_vlan_notify(const struct net_bridge *br,

@@ -1701,6 +1703,11 @@ static inline int br_vlan_bridge_event(struct
 net_device *dev,
  return 0;
 }
 
+static void br_vlan_device_event(struct net_device *dev,
+ unsigned long event, void *ptr)
+{
+}
+
 static inline void br_vlan_rtnl_init(void)
 {

}

net/bridge/br_vlan.c +0 -0

index 0f5e75ccac79..70a9950df175 100644
@@ -1768,6 +1768,20 @@ void br_vlan_port_event(struct net_bridge_port
 *p, unsigned long event)
  }
 }
 

net/bridge/br_vlan.c +5 -0

+ br_vlan_upper_change(br_dev, dev, info->details.bridge_binding);
+ break;
+ }
+}
+
 static bool br_vlan_stats_fill(struct sk_buff *skb,

const struct net_bridge_vlan *v)
{

net/core/dev.c +12 -1

index 30a1603a7225..dcdbc625585d 100644
@@ -1624,7 +1624,7 @@ const char *netdev_cmd_to_name(enum netdev_cmd cmd)
  N(POST_INIT) N(RELEASE) N(NOTIFY_PEERS) N(JOIN) N(CHANGEUPPER)
  N(RESEND_IGMP) N(PRECHANGEMTU) N(CHANGEINFODATA) N(BONDING_INFO)
  N(PRECHANGEUPPER) N(CHANGELOWERSTATE) N(UDP_TUNNEL_PUSH_INFO)
- N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN)
+ N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN) N(CHANGE_DETAILS)
  N(CVLAN_FILTER_PUSH_INFO) N(CVLAN_FILTER_DROP_INFO)
  N(SVLAN_FILTER_PUSH_INFO) N(SVLAN_FILTER_DROP_INFO)
  N(PRE_CHANGEADDR) N(OFFLOAD_XSTATS_ENABLE) N(OFFLOAD_XSTATS_DISABLE)

@@ -8767,6 +8767,27 @@ int dev_pre_changeaddr_notify(struct net_device
 *dev, const char *addr,
 }
 EXPORT_SYMBOL(dev_pre_changeaddr_notify);
 
+/**
+ * dev_change_details_notify - Call NETDEV_PRE_CHANGE_DETAILS.
+ * @dev: device
+ * @bridge_binding: bridge binding setting
+ * @extack: netlink extended ack
+ */
+int dev_change_details_notify(struct net_device *dev, bool bridge_binding,
+       struct netlink_ext_ack *extack)
+{
+ struct netdev_notifier_change_details_info info = {
+ .info.dev = dev,

net/core/dev.c +0 -0

 /**
  * dev_set_mac_address - Change Media Access Control Address

* @dev: device

Quoted 5 lines

Thanks,
Nik

Quoted 1 line · Sevinj Aghayeva

On Thu, Aug 18, 2022 at 8:00 AM Nikolay Aleksandrov <[email protected]>

Quoted 2 lines · wrote:

Quoted 2 lines · Sevinj Aghayeva

[snip]

Quoted 3 lines

Quoted 3 lines

Hi,
Handling all vlan device-related changes in br_vlan_device_event()

sounds good to me.

Quoted 5 lines

Quoted 1 line

Please add it to br_vlan.c.

Hi Nik,

Can you please review this diff before I make it into a proper patchset?

Thanks!

Quoted 1 line

Hi,
A few comments inline below, but in general when you prepare the rfc commit please
explain the motivation in detail why this way was chosen and a new notification type
is needed (e.g. why not use NETDEV_CHANGEINFODATA or extend NETDEV_CHANGE). As I mentioned earlier it'd be nice to get feedback from others about adding this
new notification, so they should know the "why" in detail.

Quoted 17 lines

include/linux/netdevice.h +1 -0

index 2563d30736e9..0ce3da42325e 100644
@@ -2762,6 +2762,7 @@ enum netdev_cmd {
  NETDEV_UNREGISTER,
  NETDEV_CHANGEMTU, /* notify after mtu change happened */
  NETDEV_CHANGEADDR, /* notify after the address change */
+ NETDEV_CHANGE_DETAILS,
  NETDEV_PRE_CHANGEADDR, /* notify before the address change */
  NETDEV_GOING_DOWN,
  NETDEV_CHANGENAME,

@@ -2837,6 +2838,13 @@ struct netdev_notifier_changelowerstate_info {
  void *lower_state_info; /* is lower dev state */
 };
 

include/linux/netdevice.h struct netdev_notifier_changelowerstate_info { · line 2844

Quoted 4 lines

include/linux/netdevice.h +4 -0

+struct netdev_notifier_change_details_info {
+ struct netdev_notifier_info info; /* must be first */
+ union {
+ bool bridge_binding;

this should be in a vlan-specific structure, defined in if_vlan.h every other link type which wants to use the notification would define its own struct type

include/linux/netdevice.h int __dev_set_mtu(struct net_device *, int); · line 3847

Quoted 12 lines

include/linux/netdevice.h +4 -0

+ } details;
+};
+
 struct netdev_notifier_pre_changeaddr_info {
  struct netdev_notifier_info info; /* must be first */
  const unsigned char *dev_addr;

@@ -3836,6 +3844,8 @@ int __dev_set_mtu(struct net_device *, int);
 int dev_set_mtu(struct net_device *, int);
 int dev_pre_changeaddr_notify(struct net_device *dev, const char *addr,
        struct netlink_ext_ack *extack);
+int dev_change_details_notify(struct net_device *dev, bool

bridge_binding,

include/linux/netdevice.h int __dev_set_mtu(struct net_device *, int); · line 3848

Quoted 1 line

include/linux/netdevice.h +1 -0

+       struct netlink_ext_ack *extack);

this helper is not needed

include/linux/netdevice.h int __dev_set_mtu(struct net_device *, int);

Quoted 3 lines

include/linux/netdevice.h +0 -0

 int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
  struct netlink_ext_ack *extack);
 int dev_set_mac_address_user(struct net_device *dev, struct sockaddr

*sa,

net/8021q/vlan.h void vlan_dev_set_ingress_priority(const struct · line 134

Quoted 10 lines

net/8021q/vlan.h +0 -1

index 5eaf38875554..71947cdcfaaa 100644
@@ -130,7 +130,7 @@ void vlan_dev_set_ingress_priority(const struct
 net_device *dev,
 int vlan_dev_set_egress_priority(const struct net_device *dev,
  u32 skb_prio, u16 vlan_prio);
 void vlan_dev_free_egress_priority(const struct net_device *dev);
-int vlan_dev_change_flags(const struct net_device *dev, u32 flag, u32

mask);

net/8021q/vlan.h void vlan_dev_set_ingress_priority(const struct · line 135

Quoted 2 lines

net/8021q/vlan.h +1 -0

+int vlan_dev_change_flags(struct net_device *dev, u32 flag, u32 mask);
 void vlan_dev_get_realdev_name(const struct net_device *dev, char

*result,

net/8021q/vlan_dev.c int vlan_dev_set_egress_priority(const struct · line 215

Quoted 12 lines

net/8021q/vlan.h +0 -0

         size_t size);

net/8021q/vlan_dev.c +0 -1

index 839f2020b015..489baa8435de 100644
@@ -211,7 +211,7 @@ int vlan_dev_set_egress_priority(const struct
 net_device *dev,
 /* Flags are defined in the vlan_flags enum in
  * include/uapi/linux/if_vlan.h file.
  */
-int vlan_dev_change_flags(const struct net_device *dev, u32 flags, u32

mask)

net/8021q/vlan_dev.c int vlan_dev_set_egress_priority(const struct · line 215

Quoted 1 line

net/8021q/vlan_dev.c +1 -0

+int vlan_dev_change_flags(struct net_device *dev, u32 flags, u32 mask)

please don't remove the const, this function shouldn't change dev's struct

Quoted 20 lines

net/8021q/vlan_dev.c +0 -0

 {
  struct vlan_dev_priv *vlan = vlan_dev_priv(dev);

  u32 old_flags = vlan->flags;

net/8021q/vlan_dev.c +4 -1

@@ -223,19 +223,29 @@ int vlan_dev_change_flags(const struct
 net_device *dev, u32 flags, u32 mask)
 
  vlan->flags = (old_flags & ~mask) | (flags & mask);
 
- if (netif_running(dev) && (vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) {
+ if (!netif_running(dev))
+ return 0;
+
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) {
  if (vlan->flags & VLAN_FLAG_GVRP)
  vlan_gvrp_request_join(dev);
  else
  vlan_gvrp_request_leave(dev);
  }
 

net/8021q/vlan_dev.c int vlan_dev_change_flags(const struct · line 247

Quoted 12 lines

net/8021q/vlan_dev.c +6 -1

- if (netif_running(dev) && (vlan->flags ^ old_flags) & VLAN_FLAG_MVRP) {
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_MVRP) {
  if (vlan->flags & VLAN_FLAG_MVRP)
  vlan_mvrp_request_join(dev);
  else
  vlan_mvrp_request_leave(dev);
  }
+
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_BRIDGE_BINDING &&
+     netif_is_bridge_master(vlan->real_dev)) {
+ dev_change_details_notify(dev,
+     !!(vlan->flags & VLAN_FLAG_BRIDGE_BINDING), NULL);

this helper is not needed, just fill in the details here and send the notification

+ }

net/bridge/br_private.h void br_vlan_get_stats(const struct

Quoted 26 lines

net/8021q/vlan_dev.c +1 -0

+
  return 0;
 }

net/bridge/br.c +5 -0

index 96e91d69a9a8..62e939c6a3f0 100644
@@ -51,6 +51,11 @@ static int br_device_event(struct notifier_block
 *unused, unsigned long event, v
  }
  }
 
+ if (is_vlan_dev(dev)) {
+ br_vlan_device_event(dev, event, ptr);
+ return NOTIFY_DONE;
+ }
+
  /* not a port of a bridge */
  p = br_port_get_rtnl(dev);

if (!p)

net/bridge/br_private.h +0 -0

index 06e5f6faa431..a9a08e49c76c 100644
@@ -1470,6 +1470,8 @@ void br_vlan_get_stats(const struct

net_bridge_vlan *v,

Quoted 30 lines

net/bridge/br_private.h +7 -0

 void br_vlan_port_event(struct net_bridge_port *p, unsigned long event);
 int br_vlan_bridge_event(struct net_device *dev, unsigned long event,
  void *ptr);
+void br_vlan_device_event(struct net_device *dev, unsigned long event,
+   void *ptr);
 void br_vlan_rtnl_init(void);
 void br_vlan_rtnl_uninit(void);
 void br_vlan_notify(const struct net_bridge *br,

@@ -1701,6 +1703,11 @@ static inline int br_vlan_bridge_event(struct
 net_device *dev,
  return 0;
 }
 
+static void br_vlan_device_event(struct net_device *dev,
+ unsigned long event, void *ptr)
+{
+}
+
 static inline void br_vlan_rtnl_init(void)
 {

}

net/bridge/br_vlan.c +0 -0

index 0f5e75ccac79..70a9950df175 100644
@@ -1768,6 +1768,20 @@ void br_vlan_port_event(struct net_bridge_port
 *p, unsigned long event)
  }
 }
 

net/bridge/br_vlan.c void br_vlan_port_event(struct net_bridge_port · line 1781

Quoted 10 lines

net/bridge/br_vlan.c +9 -0

+void br_vlan_device_event(struct net_device *dev, unsigned long
 event, void *ptr)
+{
+ struct netdev_notifier_change_details_info *info;
+ struct net_device *br_dev;
+
+ switch (event) {
+ case NETDEV_CHANGE_DETAILS:
+ info = ptr;
+ br_dev = vlan_dev_priv(dev)->real_dev;

you're not guaranteed to have a bridge device as its real_dev, so you should
validate that the vlan's real dev is a bridge

Quoted 24 lines

net/bridge/br_vlan.c +5 -0

+ br_vlan_upper_change(br_dev, dev, info->details.bridge_binding);
+ break;
+ }
+}
+
 static bool br_vlan_stats_fill(struct sk_buff *skb,

const struct net_bridge_vlan *v)
{

net/core/dev.c +1 -1

index 30a1603a7225..dcdbc625585d 100644
@@ -1624,7 +1624,7 @@ const char *netdev_cmd_to_name(enum netdev_cmd cmd)
  N(POST_INIT) N(RELEASE) N(NOTIFY_PEERS) N(JOIN) N(CHANGEUPPER)
  N(RESEND_IGMP) N(PRECHANGEMTU) N(CHANGEINFODATA) N(BONDING_INFO)
  N(PRECHANGEUPPER) N(CHANGELOWERSTATE) N(UDP_TUNNEL_PUSH_INFO)
- N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN)
+ N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN) N(CHANGE_DETAILS)
  N(CVLAN_FILTER_PUSH_INFO) N(CVLAN_FILTER_DROP_INFO)
  N(SVLAN_FILTER_PUSH_INFO) N(SVLAN_FILTER_DROP_INFO)
  N(PRE_CHANGEADDR) N(OFFLOAD_XSTATS_ENABLE) N(OFFLOAD_XSTATS_DISABLE)

@@ -8767,6 +8767,27 @@ int dev_pre_changeaddr_notify(struct net_device
 *dev, const char *addr,

net/core/dev.c int dev_pre_changeaddr_notify(struct net_device · line 8777

Quoted 10 lines

net/core/dev.c +7 -0

 }
 EXPORT_SYMBOL(dev_pre_changeaddr_notify);
 
+/**
+ * dev_change_details_notify - Call NETDEV_PRE_CHANGE_DETAILS.
+ * @dev: device
+ * @bridge_binding: bridge binding setting
+ * @extack: netlink extended ack
+ */
+int dev_change_details_notify(struct net_device *dev, bool

bridge_binding,

Quoted 9 lines

net/core/dev.c +9 -0

+       struct netlink_ext_ack *extack)
+{
+ struct netdev_notifier_change_details_info info = {
+ .info.dev = dev,
+ .info.extack = extack,
+ .details.bridge_binding = bridge_binding,
+ };
+ int rc;
+

net/core/dev.c int dev_pre_changeaddr_notify(struct net_device · line 8791

Quoted 5 lines

net/core/dev.c +5 -0

+ rc = call_netdevice_notifiers_info(NETDEV_CHANGE_DETAILS, &info.info);
+ return notifier_to_errno(rc);
+}
+EXPORT_SYMBOL(dev_change_details_notify);
+

this helper is unnecessary, just fill in the struct at the caller site and send the notification directly

Quoted 11 lines

net/core/dev.c +0 -0

 /**
  * dev_set_mac_address - Change Media Access Control Address

* @dev: device

Quoted 5 lines

Thanks,
Nik

Cheers,
Nik

Quoted 18 lines · Nikolay Aleksandrov

Quoted 1 line · Sevinj Aghayeva

On Thu, Aug 18, 2022 at 8:00 AM Nikolay Aleksandrov <

Quoted 2 lines · [email protected]> wrote:

Quoted 2 lines · Sevinj Aghayeva

[snip]

Quoted 3 lines

Quoted 3 lines

Hi,
Handling all vlan device-related changes in br_vlan_device_event()

sounds good to me.

Quoted 5 lines

Quoted 1 line

Please add it to br_vlan.c.

Hi Nik,

Can you please review this diff before I make it into a proper

patchset? Thanks!

Quoted 1 line

Quoted 11 lines

Hi,
A few comments inline below, but in general when you prepare the rfc commit please
explain the motivation in detail why this way was chosen and a new notification type
is needed (e.g. why not use NETDEV_CHANGEINFODATA or extend NETDEV_CHANGE).
As I mentioned earlier it'd be nice to get feedback from others about adding this
new notification, so they should know the "why" in detail.

Sure, I will do that in the RFC patchset, but first I want to make sure I got the details right before making the RFC patchset. Thanks for the feedback. Please see inline.

Quoted 17 lines

Quoted 17 lines

include/linux/netdevice.h +1 -0

index 2563d30736e9..0ce3da42325e 100644
@@ -2762,6 +2762,7 @@ enum netdev_cmd {
  NETDEV_UNREGISTER,
  NETDEV_CHANGEMTU, /* notify after mtu change happened */
  NETDEV_CHANGEADDR, /* notify after the address change */
+ NETDEV_CHANGE_DETAILS,
  NETDEV_PRE_CHANGEADDR, /* notify before the address change */
  NETDEV_GOING_DOWN,
  NETDEV_CHANGENAME,

@@ -2837,6 +2838,13 @@ struct netdev_notifier_changelowerstate_info {
  void *lower_state_info; /* is lower dev state */
 };
 

Quoted 9 lines

include/linux/netdevice.h struct netdev_notifier_changelowerstate_info { · line 2844

Quoted 4 lines

include/linux/netdevice.h +4 -0

+struct netdev_notifier_change_details_info {
+ struct netdev_notifier_info info; /* must be first */
+ union {
+ bool bridge_binding;

this should be in a vlan-specific structure, defined in if_vlan.h every other link type which wants to use the notification would define its own struct type

Okay, will move it there.

Quoted 49 lines

include/linux/netdevice.h int __dev_set_mtu(struct net_device *, int); · line 3847

Quoted 12 lines

include/linux/netdevice.h +4 -0

+ } details;
+};
+
 struct netdev_notifier_pre_changeaddr_info {
  struct netdev_notifier_info info; /* must be first */
  const unsigned char *dev_addr;

@@ -3836,6 +3844,8 @@ int __dev_set_mtu(struct net_device *, int);
 int dev_set_mtu(struct net_device *, int);
 int dev_pre_changeaddr_notify(struct net_device *dev, const char *addr,
        struct netlink_ext_ack *extack);
+int dev_change_details_notify(struct net_device *dev, bool

bridge_binding,

include/linux/netdevice.h int __dev_set_mtu(struct net_device *, int); · line 3848

Quoted 1 line

include/linux/netdevice.h +1 -0

+       struct netlink_ext_ack *extack);

this helper is not needed

include/linux/netdevice.h int __dev_set_mtu(struct net_device *, int); · line 3851

Quoted 3 lines

include/linux/netdevice.h +0 -0

 int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
  struct netlink_ext_ack *extack);
 int dev_set_mac_address_user(struct net_device *dev, struct sockaddr

*sa,

net/8021q/vlan.h void vlan_dev_set_ingress_priority(const struct · line 134

Quoted 10 lines

net/8021q/vlan.h +0 -1

index 5eaf38875554..71947cdcfaaa 100644
@@ -130,7 +130,7 @@ void vlan_dev_set_ingress_priority(const struct
 net_device *dev,
 int vlan_dev_set_egress_priority(const struct net_device *dev,
  u32 skb_prio, u16 vlan_prio);
 void vlan_dev_free_egress_priority(const struct net_device *dev);
-int vlan_dev_change_flags(const struct net_device *dev, u32 flag, u32

mask);

net/8021q/vlan.h void vlan_dev_set_ingress_priority(const struct · line 135

Quoted 2 lines

net/8021q/vlan.h +1 -0

+int vlan_dev_change_flags(struct net_device *dev, u32 flag, u32 mask);
 void vlan_dev_get_realdev_name(const struct net_device *dev, char

*result,

net/8021q/vlan_dev.c int vlan_dev_set_egress_priority(const struct · line 215

Quoted 12 lines

net/8021q/vlan.h +0 -0

         size_t size);

net/8021q/vlan_dev.c +0 -1

index 839f2020b015..489baa8435de 100644
@@ -211,7 +211,7 @@ int vlan_dev_set_egress_priority(const struct
 net_device *dev,
 /* Flags are defined in the vlan_flags enum in
  * include/uapi/linux/if_vlan.h file.
  */
-int vlan_dev_change_flags(const struct net_device *dev, u32 flags, u32

mask)

Quoted 4 lines

net/8021q/vlan_dev.c int vlan_dev_set_egress_priority(const struct · line 215

Quoted 1 line

net/8021q/vlan_dev.c +1 -0

+int vlan_dev_change_flags(struct net_device *dev, u32 flags, u32 mask)

please don't remove the const, this function shouldn't change dev's struct

I tried not to remove const, but it seems impossible because call_netdevice_notifiers_info that we eventually call from vlan_dev_change_flags takes a non-const struct info that has a dev field being set from the const vlan_dev_change_flags.

Quoted 35 lines

net/8021q/vlan_dev.c int vlan_dev_change_flags(const struct · line 227

Quoted 10 lines

net/8021q/vlan_dev.c +0 -0

 {
  struct vlan_dev_priv *vlan = vlan_dev_priv(dev);

  u32 old_flags = vlan->flags;

net/8021q/vlan_dev.c +0 -1

@@ -223,19 +223,29 @@ int vlan_dev_change_flags(const struct
 net_device *dev, u32 flags, u32 mask)
 
  vlan->flags = (old_flags & ~mask) | (flags & mask);
 
- if (netif_running(dev) && (vlan->flags ^ old_flags) & VLAN_FLAG_GVRP)

{

net/8021q/vlan_dev.c int vlan_dev_change_flags(const struct · line 234

Quoted 11 lines

net/8021q/vlan_dev.c +4 -1

+ if (!netif_running(dev))
+ return 0;
+
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) {
  if (vlan->flags & VLAN_FLAG_GVRP)
  vlan_gvrp_request_join(dev);
  else
  vlan_gvrp_request_leave(dev);
  }
 
- if (netif_running(dev) && (vlan->flags ^ old_flags) & VLAN_FLAG_MVRP)

{

net/8021q/vlan_dev.c int vlan_dev_change_flags(const struct · line 247

Quoted 11 lines

net/8021q/vlan_dev.c +6 -0

+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_MVRP) {
  if (vlan->flags & VLAN_FLAG_MVRP)
  vlan_mvrp_request_join(dev);
  else
  vlan_mvrp_request_leave(dev);
  }
+
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_BRIDGE_BINDING &&
+     netif_is_bridge_master(vlan->real_dev)) {
+ dev_change_details_notify(dev,
+     !!(vlan->flags & VLAN_FLAG_BRIDGE_BINDING), NULL);

Quoted 4 lines

this helper is not needed, just fill in the details here and send the notification

Okay, in that case I will need to export call_netdevice_notifiers_info so that I can call it from here.

Quoted 59 lines

+ }

net/bridge/br_private.h void br_vlan_get_stats(const struct

Quoted 26 lines

net/8021q/vlan_dev.c +1 -0

+
  return 0;
 }

net/bridge/br.c +5 -0

index 96e91d69a9a8..62e939c6a3f0 100644
@@ -51,6 +51,11 @@ static int br_device_event(struct notifier_block
 *unused, unsigned long event, v
  }
  }
 
+ if (is_vlan_dev(dev)) {
+ br_vlan_device_event(dev, event, ptr);
+ return NOTIFY_DONE;
+ }
+
  /* not a port of a bridge */
  p = br_port_get_rtnl(dev);

if (!p)

net/bridge/br_private.h +0 -0

index 06e5f6faa431..a9a08e49c76c 100644
@@ -1470,6 +1470,8 @@ void br_vlan_get_stats(const struct

net_bridge_vlan *v,

net/bridge/br_private.h void br_vlan_get_stats(const struct · line 1470

Quoted 1 line

net/bridge/br_private.h +0 -0

 void br_vlan_port_event(struct net_bridge_port *p, unsigned long

event);

Quoted 29 lines

net/bridge/br_private.h +7 -0

 int br_vlan_bridge_event(struct net_device *dev, unsigned long event,
  void *ptr);
+void br_vlan_device_event(struct net_device *dev, unsigned long event,
+   void *ptr);
 void br_vlan_rtnl_init(void);
 void br_vlan_rtnl_uninit(void);
 void br_vlan_notify(const struct net_bridge *br,

@@ -1701,6 +1703,11 @@ static inline int br_vlan_bridge_event(struct
 net_device *dev,
  return 0;
 }
 
+static void br_vlan_device_event(struct net_device *dev,
+ unsigned long event, void *ptr)
+{
+}
+
 static inline void br_vlan_rtnl_init(void)
 {

}

net/bridge/br_vlan.c +0 -0

index 0f5e75ccac79..70a9950df175 100644
@@ -1768,6 +1768,20 @@ void br_vlan_port_event(struct net_bridge_port
 *p, unsigned long event)
  }
 }
 

Quoted 15 lines

net/bridge/br_vlan.c void br_vlan_port_event(struct net_bridge_port · line 1781

Quoted 10 lines

net/bridge/br_vlan.c +9 -0

+void br_vlan_device_event(struct net_device *dev, unsigned long
 event, void *ptr)
+{
+ struct netdev_notifier_change_details_info *info;
+ struct net_device *br_dev;
+
+ switch (event) {
+ case NETDEV_CHANGE_DETAILS:
+ info = ptr;
+ br_dev = vlan_dev_priv(dev)->real_dev;

you're not guaranteed to have a bridge device as its real_dev, so you should
validate that the vlan's real dev is a bridge

Okay, will do.

Quoted 47 lines

net/core/dev.c const char *netdev_cmd_to_name(enum netdev_cmd

Quoted 14 lines

net/bridge/br_vlan.c +5 -0

+ br_vlan_upper_change(br_dev, dev, info->details.bridge_binding);
+ break;
+ }
+}
+
 static bool br_vlan_stats_fill(struct sk_buff *skb,

const struct net_bridge_vlan *v)
{

net/core/dev.c +0 -0

index 30a1603a7225..dcdbc625585d 100644
@@ -1624,7 +1624,7 @@ const char *netdev_cmd_to_name(enum netdev_cmd

cmd)

Quoted 10 lines

net/core/dev.c +1 -1

  N(POST_INIT) N(RELEASE) N(NOTIFY_PEERS) N(JOIN) N(CHANGEUPPER)
  N(RESEND_IGMP) N(PRECHANGEMTU) N(CHANGEINFODATA) N(BONDING_INFO)
  N(PRECHANGEUPPER) N(CHANGELOWERSTATE) N(UDP_TUNNEL_PUSH_INFO)
- N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN)
+ N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN) N(CHANGE_DETAILS)
  N(CVLAN_FILTER_PUSH_INFO) N(CVLAN_FILTER_DROP_INFO)
  N(SVLAN_FILTER_PUSH_INFO) N(SVLAN_FILTER_DROP_INFO)
  N(PRE_CHANGEADDR) N(OFFLOAD_XSTATS_ENABLE) N(OFFLOAD_XSTATS_DISABLE)

@@ -8767,6 +8767,27 @@ int dev_pre_changeaddr_notify(struct net_device
 *dev, const char *addr,

net/core/dev.c int dev_pre_changeaddr_notify(struct net_device · line 8777

Quoted 10 lines

net/core/dev.c +7 -0

 }
 EXPORT_SYMBOL(dev_pre_changeaddr_notify);
 
+/**
+ * dev_change_details_notify - Call NETDEV_PRE_CHANGE_DETAILS.
+ * @dev: device
+ * @bridge_binding: bridge binding setting
+ * @extack: netlink extended ack
+ */
+int dev_change_details_notify(struct net_device *dev, bool

bridge_binding,

net/core/dev.c int dev_pre_changeaddr_notify(struct net_device · line 8787

Quoted 10 lines

net/core/dev.c +10 -0

+       struct netlink_ext_ack *extack)
+{
+ struct netdev_notifier_change_details_info info = {
+ .info.dev = dev,
+ .info.extack = extack,
+ .details.bridge_binding = bridge_binding,
+ };
+ int rc;
+
+ rc = call_netdevice_notifiers_info(NETDEV_CHANGE_DETAILS, &info.info

);

Quoted 8 lines

net/core/dev.c int dev_pre_changeaddr_notify(struct net_device · line 8791

Quoted 4 lines

net/core/dev.c +4 -0

+ return notifier_to_errno(rc);
+}
+EXPORT_SYMBOL(dev_change_details_notify);
+

this helper is unnecessary, just fill in the struct at the caller site and send the notification directly

Okay, will remove it.

Thanks for the review! I will send the updated patch soon.

Quoted 7 lines

Quoted 7 lines

net/core/dev.c +0 -0

 /**
  * dev_set_mac_address - Change Media Access Control Address

* @dev: device

Quoted 1 line

Quoted 10 lines

Quoted 4 lines

Quoted 4 lines

Thanks,
Nik

Cheers,
Nik

--

Sevinj.Aghayeva

Hi Nik,

Please see below.

I had to export call_netdev_notifiers_info. Also, I wanted to define the vlan-specific detail change struct in if_vlan.h and then include if_vlan.h from netdevice.h, where the netdev_notifier_change_details_info struct was defined, but I found out that I cannot include if_vlan.h in netdevice.h. After consulting with Andy I added a new file for the netdev_notifier_change_details_info. Again, I will send out a proper RFC patch once we sort out the details.

include/linux/if_vlan.h +0 -0

index 2be4dd7e90a9..ad0eddf1386f 100644
@@ -37,6 +37,10 @@ struct vlan_hdr {
   __be16 h_vlan_encapsulated_proto;
  };

include/linux/if_vlan.h +4 -0

+struct vlan_change_details {
+ bool bridge_binding;
+};
+

 /**
  * struct vlan_ethhdr - vlan ethernet header (ethhdr + vlan_hdr)
  * @h_dest: destination ethernet address

include/linux/netdevice.h +0 -0

index 2563d30736e9..ba4746f05d7f 100644
@@ -2762,6 +2762,7 @@ enum netdev_cmd {
   NETDEV_UNREGISTER,

  NETDEV_CHANGEMTU, /* notify after mtu change happened */ NETDEV_CHANGEADDR, /* notify after the address change */
+ NETDEV_CHANGE_DETAILS,
  NETDEV_PRE_CHANGEADDR, /* notify before the address change */ NETDEV_GOING_DOWN,
  NETDEV_CHANGENAME,

include/linux/netdevice.h +0 -0

@@ -2898,6 +2899,8 @@ netdev_notifier_info_to_extack(const struct netdev_notifier_info *info)
  }

 int call_netdevice_notifiers(unsigned long val, struct net_device *dev); +int call_netdevice_notifiers_info(unsigned long val,
+  struct netdev_notifier_info *info);


 extern rwlock_t dev_base_lock; /* Device list lock */

include/linux/notifier_info.h new file +0 -0

new file mode 100644
index 000000000000..c103240588dc

include/linux/notifier_info.h new file +3 -0

+};
+
+#endif /* !(_LINUX_NOTIFIER_INFO_H_) */

net/8021q/vlan.h +1 -1

index 5eaf38875554..71947cdcfaaa 100644
@@ -130,7 +130,7 @@ void vlan_dev_set_ingress_priority(const struct net_device *dev,
  int vlan_dev_set_egress_priority(const struct net_device *dev,
   u32 skb_prio, u16 vlan_prio);
  void vlan_dev_free_egress_priority(const struct net_device *dev);
-int vlan_dev_change_flags(const struct net_device *dev, u32 flag, u32 mask);
+int vlan_dev_change_flags(struct net_device *dev, u32 flag, u32 mask);
  void vlan_dev_get_realdev_name(const struct net_device *dev, char *result,
         size_t size);

net/8021q/vlan_dev.c +3 -1

index 839f2020b015..d737e259e295 100644
@@ -22,6 +22,7 @@
  #include <linux/skbuff.h>
  #include <linux/netdevice.h>
  #include <linux/net_tstamp.h>
+#include <linux/notifier_info.h>
  #include <linux/etherdevice.h>
  #include <linux/ethtool.h>
  #include <linux/phy.h>

@@ -211,8 +212,9 @@ int vlan_dev_set_egress_priority(const struct net_device *dev,
  /* Flags are defined in the vlan_flags enum in
   * include/uapi/linux/if_vlan.h file.
   */
-int vlan_dev_change_flags(const struct net_device *dev, u32 flags, u32 mask)
+int vlan_dev_change_flags(struct net_device *dev, u32 flags, u32 mask)
  {
+ struct netdev_notifier_change_details_info info;
   struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
   u32 old_flags = vlan->flags;

net/8021q/vlan_dev.c +0 -0

@@ -223,19 +225,31 @@ int vlan_dev_change_flags(const struct net_device *dev, u32 flags, u32 mask)

  vlan->flags = (old_flags & ~mask) | (flags & mask);

- if (netif_running(dev) && (vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) {
+ if (!netif_running(dev))
+ return 0;
+
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) {
  if (vlan->flags & VLAN_FLAG_GVRP) vlan_gvrp_request_join(dev);
  else
  vlan_gvrp_request_leave(dev);
  }

- if (netif_running(dev) && (vlan->flags ^ old_flags) & VLAN_FLAG_MVRP) {

+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_MVRP) { if (vlan->flags & VLAN_FLAG_MVRP) vlan_mvrp_request_join(dev);
  else
  vlan_mvrp_request_leave(dev);
  }
+
+ if ((vlan->flags ^ old_flags) & VLAN_FLAG_BRIDGE_BINDING &&

+    netif_is_bridge_master(vlan->real_dev)) {
+ info.info.dev <http://info.info.dev> = dev;
+ info.details.vlan.bridge_binding =
+    !!(vlan->flags & VLAN_FLAG_BRIDGE_BINDING);
+ call_netdevice_notifiers_info(NETDEV_CHANGE_DETAILS, &info.info <http://info.info>);

+ }
+
  return 0;
 }

net/bridge/br.c +0 -0

index 96e91d69a9a8..62e939c6a3f0 100644
@@ -51,6 +51,11 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
   }
   }

+ if (is_vlan_dev(dev)) {
+ br_vlan_device_event(dev, event, ptr);
+ return NOTIFY_DONE;
+ }
+
  /* not a port of a bridge */
  p = br_port_get_rtnl(dev);
  if (!p)

net/bridge/br_private.h +2 -0

index 06e5f6faa431..a9a08e49c76c 100644
@@ -1470,6 +1470,8 @@ void br_vlan_get_stats(const struct net_bridge_vlan *v,
  void br_vlan_port_event(struct net_bridge_port *p, unsigned long event);
  int br_vlan_bridge_event(struct net_device *dev, unsigned long event,
   void *ptr);
+void br_vlan_device_event(struct net_device *dev, unsigned long event,
+  void *ptr);
  void br_vlan_rtnl_init(void);
  void br_vlan_rtnl_uninit(void);
  void br_vlan_notify(const struct net_bridge *br,

@@ -1701,6 +1703,11 @@ static inline int br_vlan_bridge_event(struct net_device *dev,
   return 0;
  }

net/bridge/br_private.h +5 -0

+static void br_vlan_device_event(struct net_device *dev,
+ unsigned long event, void *ptr)
+{
+}
+

 static inline void br_vlan_rtnl_init(void)
 {
 }

net/bridge/br_vlan.c +1 -0

index 0f5e75ccac79..fe00efa46ce8 100644
@@ -1,6 +1,7 @@
  // SPDX-License-Identifier: GPL-2.0-only
  #include <linux/kernel.h>
  #include <linux/netdevice.h>
+#include <linux/notifier_info.h>
  #include <linux/rtnetlink.h>
  #include <linux/slab.h>
  #include <net/switchdev.h>

@@ -1768,6 +1769,23 @@ void br_vlan_port_event(struct net_bridge_port *p, unsigned long event)
   }
  }

+ br_dev = vlan_dev_priv(dev)->real_dev;
+ br_vlan_upper_change(br_dev, dev,
+    info->details.vlan.bridge_binding);
+ }
+ break;
+ }

net/bridge/br_vlan.c +2 -0

+}
+

 static bool br_vlan_stats_fill(struct sk_buff *skb, const struct net_bridge_vlan *v)
 {

net/core/dev.c +0 -0

index 30a1603a7225..2eed064e9529 100644
@@ -160,7 +160,7 @@ struct list_head ptype_base[PTYPE_HASH_SIZE] __read_mostly;

 struct list_head ptype_all __read_mostly; /* Taps */
 static int netif_rx_internal(struct sk_buff *skb);

net/core/dev.c +1 -1

-static int call_netdevice_notifiers_info(unsigned long val,
+int call_netdevice_notifiers_info(unsigned long val,

  struct netdev_notifier_info *info);

 static int call_netdevice_notifiers_extack(unsigned long val, struct net_device *dev,

net/core/dev.c +1 -1

@@ -1624,7 +1624,7 @@ const char *netdev_cmd_to_name(enum netdev_cmd cmd)
   N(POST_INIT) N(RELEASE) N(NOTIFY_PEERS) N(JOIN) N(CHANGEUPPER)
   N(RESEND_IGMP) N(PRECHANGEMTU) N(CHANGEINFODATA) N(BONDING_INFO)
   N(PRECHANGEUPPER) N(CHANGELOWERSTATE) N(UDP_TUNNEL_PUSH_INFO)
- N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN)
+ N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN) N(CHANGE_DETAILS)
   N(CVLAN_FILTER_PUSH_INFO) N(CVLAN_FILTER_DROP_INFO)
   N(SVLAN_FILTER_PUSH_INFO) N(SVLAN_FILTER_DROP_INFO)
   N(PRE_CHANGEADDR) N(OFFLOAD_XSTATS_ENABLE) N(OFFLOAD_XSTATS_DISABLE)

@@ -1927,7 +1927,7 @@ static void move_netdevice_notifiers_dev_net(struct net_device *dev,
   * are as for raw_notifier_call_chain().
   */

net/core/dev.c +1 -1

-static int call_netdevice_notifiers_info(unsigned long val,
+int call_netdevice_notifiers_info(unsigned long val,

  struct netdev_notifier_info *info)
 {
  struct net *net = dev_net(info->dev);

 /**
  * call_netdevice_notifiers_info_robust - call per-netns notifier blocks

On Mon, Aug 22, 2022 at 7:18 PM Sevinj Aghayeva <[email protected] <mailto:[email protected]>> wrote:

    On Mon, Aug 22, 2022 at 4:01 AM Nikolay Aleksandrov <[email protected] <mailto:[email protected]>> wrote:

        On 20/08/2022 14:33, Sevinj Aghayeva wrote:
        > On Thu, Aug 18, 2022 at 8:00 AM Nikolay Aleksandrov <[email protected] <mailto:[email protected]>> wrote:
        >>
        >> On 18/08/2022 14:50, Sevinj Aghayeva wrote:
        [snip]
        >>
        >> Hi,
        >> Handling all vlan device-related changes in br_vlan_device_event() sounds good to me.
        >> Please add it to br_vlan.c.
        >
        > Hi Nik,
        >
        > Can you please review this diff before I make it into a proper patchset? Thanks!
        >

        Hi,
        A few comments inline below, but in general when you prepare the rfc commit please
        explain the motivation in detail why this way was chosen and a new notification type
        is needed (e.g. why not use NETDEV_CHANGEINFODATA or extend NETDEV_CHANGE).
        As I mentioned earlier it'd be nice to get feedback from others about adding this
        new notification, so they should know the "why" in detail.


    Sure, I will do that in the RFC patchset, but first I want to make sure I got the details right before making the RFC patchset. Thanks for the feedback. Please see inline. 


        > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
        > index 2563d30736e9..0ce3da42325e 100644
        > --- a/include/linux/netdevice.h
        > +++ b/include/linux/netdevice.h
        > @@ -2762,6 +2762,7 @@ enum netdev_cmd {
        >   NETDEV_UNREGISTER,
        >   NETDEV_CHANGEMTU, /* notify after mtu change happened */
        >   NETDEV_CHANGEADDR, /* notify after the address change */
        > + NETDEV_CHANGE_DETAILS,
        >   NETDEV_PRE_CHANGEADDR, /* notify before the address change */
        >   NETDEV_GOING_DOWN,
        >   NETDEV_CHANGENAME,
        > @@ -2837,6 +2838,13 @@ struct netdev_notifier_changelowerstate_info {
        >   void *lower_state_info; /* is lower dev state */
        >  };
        >
        > +struct netdev_notifier_change_details_info {
        > + struct netdev_notifier_info info; /* must be first */
        > + union {
        > + bool bridge_binding;

        this should be in a vlan-specific structure, defined in if_vlan.h
        every other link type which wants to use the notification would define its
        own struct type


    Okay, will move it there.
     


        > + } details;
        > +};
        > +
        >  struct netdev_notifier_pre_changeaddr_info {
        >   struct netdev_notifier_info info; /* must be first */
        >   const unsigned char *dev_addr;
        > @@ -3836,6 +3844,8 @@ int __dev_set_mtu(struct net_device *, int);
        >  int dev_set_mtu(struct net_device *, int);
        >  int dev_pre_changeaddr_notify(struct net_device *dev, const char *addr,
        >         struct netlink_ext_ack *extack);
        > +int dev_change_details_notify(struct net_device *dev, bool bridge_binding,
        > +       struct netlink_ext_ack *extack);

        this helper is not needed 


        >  int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
        >   struct netlink_ext_ack *extack);
        >  int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
        > diff --git a/net/8021q/vlan.h b/net/8021q/vlan.h
        > index 5eaf38875554..71947cdcfaaa 100644
        > --- a/net/8021q/vlan.h
        > +++ b/net/8021q/vlan.h
        > @@ -130,7 +130,7 @@ void vlan_dev_set_ingress_priority(const struct
        > net_device *dev,
        >  int vlan_dev_set_egress_priority(const struct net_device *dev,
        >   u32 skb_prio, u16 vlan_prio);
        >  void vlan_dev_free_egress_priority(const struct net_device *dev);
        > -int vlan_dev_change_flags(const struct net_device *dev, u32 flag, u32 mask);
        > +int vlan_dev_change_flags(struct net_device *dev, u32 flag, u32 mask);
        >  void vlan_dev_get_realdev_name(const struct net_device *dev, char *result,
        >          size_t size);
        >
        > diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
        > index 839f2020b015..489baa8435de 100644
        > --- a/net/8021q/vlan_dev.c
        > +++ b/net/8021q/vlan_dev.c
        > @@ -211,7 +211,7 @@ int vlan_dev_set_egress_priority(const struct
        > net_device *dev,
        >  /* Flags are defined in the vlan_flags enum in
        >   * include/uapi/linux/if_vlan.h file.
        >   */
        > -int vlan_dev_change_flags(const struct net_device *dev, u32 flags, u32 mask)
        > +int vlan_dev_change_flags(struct net_device *dev, u32 flags, u32 mask)

        please don't remove the const, this function shouldn't change dev's struct


    I tried not to remove const, but it seems impossible because call_netdevice_notifiers_info that we eventually call from vlan_dev_change_flags takes a non-const struct info that has a dev field being set from the const vlan_dev_change_flags. 
     


        >  {
        >   struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
        >   u32 old_flags = vlan->flags;
        > @@ -223,19 +223,29 @@ int vlan_dev_change_flags(const struct
        > net_device *dev, u32 flags, u32 mask)
        >
        >   vlan->flags = (old_flags & ~mask) | (flags & mask);
        >
        > - if (netif_running(dev) && (vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) {
        > + if (!netif_running(dev))
        > + return 0;
        > +
        > + if ((vlan->flags ^ old_flags) & VLAN_FLAG_GVRP) {
        >   if (vlan->flags & VLAN_FLAG_GVRP)
        >   vlan_gvrp_request_join(dev);
        >   else
        >   vlan_gvrp_request_leave(dev);
        >   }
        >
        > - if (netif_running(dev) && (vlan->flags ^ old_flags) & VLAN_FLAG_MVRP) {
        > + if ((vlan->flags ^ old_flags) & VLAN_FLAG_MVRP) {
        >   if (vlan->flags & VLAN_FLAG_MVRP)
        >   vlan_mvrp_request_join(dev);
        >   else
        >   vlan_mvrp_request_leave(dev);
        >   }
        > +
        > + if ((vlan->flags ^ old_flags) & VLAN_FLAG_BRIDGE_BINDING &&
        > +     netif_is_bridge_master(vlan->real_dev)) {
        > + dev_change_details_notify(dev,
        > +     !!(vlan->flags & VLAN_FLAG_BRIDGE_BINDING), NULL);

        this helper is not needed, just fill in the details here and send the notification


    Okay, in that case I will need to export call_netdevice_notifiers_info so that I can call it from here.

        > + }
        > +
        >   return 0;
        >  }
        >
        > diff --git a/net/bridge/br.c b/net/bridge/br.c
        > index 96e91d69a9a8..62e939c6a3f0 100644
        > --- a/net/bridge/br.c
        > +++ b/net/bridge/br.c
        > @@ -51,6 +51,11 @@ static int br_device_event(struct notifier_block
        > *unused, unsigned long event, v
        >   }
        >   }
        >
        > + if (is_vlan_dev(dev)) {
        > + br_vlan_device_event(dev, event, ptr);
        > + return NOTIFY_DONE;
        > + }
        > +
        >   /* not a port of a bridge */
        >   p = br_port_get_rtnl(dev);
        >   if (!p)
        > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
        > index 06e5f6faa431..a9a08e49c76c 100644
        > --- a/net/bridge/br_private.h
        > +++ b/net/bridge/br_private.h
        > @@ -1470,6 +1470,8 @@ void br_vlan_get_stats(const struct net_bridge_vlan *v,
        >  void br_vlan_port_event(struct net_bridge_port *p, unsigned long event);
        >  int br_vlan_bridge_event(struct net_device *dev, unsigned long event,
        >   void *ptr);
        > +void br_vlan_device_event(struct net_device *dev, unsigned long event,
        > +   void *ptr);
        >  void br_vlan_rtnl_init(void);
        >  void br_vlan_rtnl_uninit(void);
        >  void br_vlan_notify(const struct net_bridge *br,
        > @@ -1701,6 +1703,11 @@ static inline int br_vlan_bridge_event(struct
        > net_device *dev,
        >   return 0;
        >  }
        >
        > +static void br_vlan_device_event(struct net_device *dev,
        > + unsigned long event, void *ptr)
        > +{
        > +}
        > +
        >  static inline void br_vlan_rtnl_init(void)
        >  {
        >  }
        > diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
        > index 0f5e75ccac79..70a9950df175 100644
        > --- a/net/bridge/br_vlan.c
        > +++ b/net/bridge/br_vlan.c
        > @@ -1768,6 +1768,20 @@ void br_vlan_port_event(struct net_bridge_port
        > *p, unsigned long event)
        >   }
        >  }
        >
        > +void br_vlan_device_event(struct net_device *dev, unsigned long
        > event, void *ptr)
        > +{
        > + struct netdev_notifier_change_details_info *info;
        > + struct net_device *br_dev;
        > +
        > + switch (event) {
        > + case NETDEV_CHANGE_DETAILS:
        > + info = ptr;
        > + br_dev = vlan_dev_priv(dev)->real_dev;

        you're not guaranteed to have a bridge device as its real_dev, so you should
        validate that the vlan's real dev is a bridge


    Okay, will do.
     


        > + br_vlan_upper_change(br_dev, dev, info->details.bridge_binding);
        > + break;
        > + }
        > +}
        > +
        >  static bool br_vlan_stats_fill(struct sk_buff *skb,
        >          const struct net_bridge_vlan *v)
        >  {
        > diff --git a/net/core/dev.c b/net/core/dev.c
        > index 30a1603a7225..dcdbc625585d 100644
        > --- a/net/core/dev.c
        > +++ b/net/core/dev.c
        > @@ -1624,7 +1624,7 @@ const char *netdev_cmd_to_name(enum netdev_cmd cmd)
        >   N(POST_INIT) N(RELEASE) N(NOTIFY_PEERS) N(JOIN) N(CHANGEUPPER)
        >   N(RESEND_IGMP) N(PRECHANGEMTU) N(CHANGEINFODATA) N(BONDING_INFO)
        >   N(PRECHANGEUPPER) N(CHANGELOWERSTATE) N(UDP_TUNNEL_PUSH_INFO)
        > - N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN)
        > + N(UDP_TUNNEL_DROP_INFO) N(CHANGE_TX_QUEUE_LEN) N(CHANGE_DETAILS)
        >   N(CVLAN_FILTER_PUSH_INFO) N(CVLAN_FILTER_DROP_INFO)
        >   N(SVLAN_FILTER_PUSH_INFO) N(SVLAN_FILTER_DROP_INFO)
        >   N(PRE_CHANGEADDR) N(OFFLOAD_XSTATS_ENABLE) N(OFFLOAD_XSTATS_DISABLE)
        > @@ -8767,6 +8767,27 @@ int dev_pre_changeaddr_notify(struct net_device
        > *dev, const char *addr,
        >  }
        >  EXPORT_SYMBOL(dev_pre_changeaddr_notify);
        >
        > +/**
        > + * dev_change_details_notify - Call NETDEV_PRE_CHANGE_DETAILS.
        > + * @dev: device
        > + * @bridge_binding: bridge binding setting
        > + * @extack: netlink extended ack
        > + */
        > +int dev_change_details_notify(struct net_device *dev, bool bridge_binding,
        > +       struct netlink_ext_ack *extack)
        > +{
        > + struct netdev_notifier_change_details_info info = {
        > + .info.dev <http://info.dev> = dev,
        > + .info.extack = extack,
        > + .details.bridge_binding = bridge_binding,
        > + };
        > + int rc;
        > +
        > + rc = call_netdevice_notifiers_info(NETDEV_CHANGE_DETAILS, &info.info <http://info.info>);
        > + return notifier_to_errno(rc);
        > +}
        > +EXPORT_SYMBOL(dev_change_details_notify);
        > +

        this helper is unnecessary, just fill in the struct at the caller site and
        send the notification directly


    Okay, will remove it.

    Thanks for the review! I will send the updated patch soon. 


        >  /**
        >   * dev_set_mac_address - Change Media Access Control Address
        >   * @dev: device
        >
        >
        >>
        >> Thanks,
        >>  Nik
        >>
        >>

        Cheers,
         Nik




    -- 

    Sevinj.Aghayeva

Signature · 1 line

Sevinj.Aghayeva