Nikolay Aleksandrov
2022-Aug-18 12:00 UTC
[Bridge] [PATCH RFC net-next 0/3] net: vlan: fix bridge binding behavior and add selftests
On 18/08/2022 14:50, Sevinj Aghayeva wrote:> On Sun, Aug 14, 2022 at 3:38 AM Nikolay Aleksandrov <razor at blackwall.org> wrote: >> >> On 12/08/2022 18:30, Sevinj Aghayeva wrote: >>> On Wed, Aug 10, 2022 at 4:54 AM Nikolay Aleksandrov <razor at blackwall.org> wrote: >>>> >>>> On 10/08/2022 06:11, Sevinj Aghayeva wrote: >>>>> When bridge binding is enabled for a vlan interface, it is expected >>>>> that the link state of the vlan interface will track the subset of the >>>>> ports that are also members of the corresponding vlan, rather than >>>>> that of all ports. >>>>> >>>>> Currently, this feature works as expected when a vlan interface is >>>>> created with bridge binding enabled: >>>>> >>>>> ip link add link br name vlan10 type vlan id 10 protocol 802.1q \ >>>>> bridge_binding on >>>>> >>>>> However, the feature does not work when a vlan interface is created >>>>> with bridge binding disabled, and then enabled later: >>>>> >>>>> ip link add link br name vlan10 type vlan id 10 protocol 802.1q \ >>>>> bridge_binding off >>>>> ip link set vlan10 type vlan bridge_binding on >>>>> >>>>> After these two commands, the link state of the vlan interface >>>>> continues to track that of all ports, which is inconsistent and >>>>> confusing to users. This series fixes this bug and introduces two >>>>> tests for the valid behavior. >>>>> >>>>> Sevinj Aghayeva (3): >>>>> net: core: export call_netdevice_notifiers_info >>>>> net: 8021q: fix bridge binding behavior for vlan interfaces >>>>> selftests: net: tests for bridge binding behavior >>>>> >>>>> include/linux/netdevice.h | 2 + >>>>> net/8021q/vlan.h | 2 +- >>>>> net/8021q/vlan_dev.c | 25 ++- >>>>> net/core/dev.c | 7 +- >>>>> tools/testing/selftests/net/Makefile | 1 + >>>>> .../selftests/net/bridge_vlan_binding_test.sh | 143 ++++++++++++++++++ >>>>> 6 files changed, 172 insertions(+), 8 deletions(-) >>>>> create mode 100755 tools/testing/selftests/net/bridge_vlan_binding_test.sh >>>>> >>>> >>>> Hi, >>>> NETDEV_CHANGE event is already propagated when the vlan changes flags, >>>> 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. >> >> Hi, >> >>> >>> 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 >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
Sevinj Aghayeva
2022-Aug-20 11:33 UTC
[Bridge] [PATCH RFC net-next 0/3] net: vlan: fix bridge binding behavior and add selftests
On Thu, Aug 18, 2022 at 8:00 AM Nikolay Aleksandrov <razor at blackwall.org> wrote:> > On 18/08/2022 14:50, Sevinj Aghayeva wrote: > > On Sun, Aug 14, 2022 at 3:38 AM Nikolay Aleksandrov <razor at blackwall.org> wrote: > >> > >> On 12/08/2022 18:30, Sevinj Aghayeva wrote: > >>> On Wed, Aug 10, 2022 at 4:54 AM Nikolay Aleksandrov <razor at blackwall.org> wrote: > >>>> > >>>> On 10/08/2022 06:11, Sevinj Aghayeva wrote: > >>>>> When bridge binding is enabled for a vlan interface, it is expected > >>>>> that the link state of the vlan interface will track the subset of the > >>>>> ports that are also members of the corresponding vlan, rather than > >>>>> that of all ports. > >>>>> > >>>>> Currently, this feature works as expected when a vlan interface is > >>>>> created with bridge binding enabled: > >>>>> > >>>>> ip link add link br name vlan10 type vlan id 10 protocol 802.1q \ > >>>>> bridge_binding on > >>>>> > >>>>> However, the feature does not work when a vlan interface is created > >>>>> with bridge binding disabled, and then enabled later: > >>>>> > >>>>> ip link add link br name vlan10 type vlan id 10 protocol 802.1q \ > >>>>> bridge_binding off > >>>>> ip link set vlan10 type vlan bridge_binding on > >>>>> > >>>>> After these two commands, the link state of the vlan interface > >>>>> continues to track that of all ports, which is inconsistent and > >>>>> confusing to users. This series fixes this bug and introduces two > >>>>> tests for the valid behavior. > >>>>> > >>>>> Sevinj Aghayeva (3): > >>>>> net: core: export call_netdevice_notifiers_info > >>>>> net: 8021q: fix bridge binding behavior for vlan interfaces > >>>>> selftests: net: tests for bridge binding behavior > >>>>> > >>>>> include/linux/netdevice.h | 2 + > >>>>> net/8021q/vlan.h | 2 +- > >>>>> net/8021q/vlan_dev.c | 25 ++- > >>>>> net/core/dev.c | 7 +- > >>>>> tools/testing/selftests/net/Makefile | 1 + > >>>>> .../selftests/net/bridge_vlan_binding_test.sh | 143 ++++++++++++++++++ > >>>>> 6 files changed, 172 insertions(+), 8 deletions(-) > >>>>> create mode 100755 tools/testing/selftests/net/bridge_vlan_binding_test.sh > >>>>> > >>>> > >>>> Hi, > >>>> NETDEV_CHANGE event is already propagated when the vlan changes flags, > >>>> 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. > >> > >> Hi, > >> > >>> > >>> 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 > > > > 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! 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; + } 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); 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) { 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); + } + 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; + 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 = dev, + .info.extack = extack, + .details.bridge_binding = bridge_binding, + }; + int rc; + + rc = call_netdevice_notifiers_info(NETDEV_CHANGE_DETAILS, &info.info); + return notifier_to_errno(rc); +} +EXPORT_SYMBOL(dev_change_details_notify); + /** * dev_set_mac_address - Change Media Access Control Address * @dev: device> > Thanks, > Nik > >-- Sevinj.Aghayeva