thr3ads.net - Linux Ethernet Bridging - [Bridge] [PATCH RFC net-next 0/3] net: vlan: fix bridge binding behavior and add selftests [Aug 2022]

If this information is useful, please help other people find it:
Share via:

Sevinj Aghayeva

2022-Aug-22 23:18 UTC

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

On Mon, Aug 22, 2022 at 4:01 AM Nikolay Aleksandrov <razor at
blackwall.org>
wrote:
> On 20/08/2022 14:33, Sevinj Aghayeva wrote:
> > 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:
> [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 = 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);
> > +
>
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.linuxfoundation.org/pipermail/bridge/attachments/20220822/b246b402/attachment.html>

Sevinj Aghayeva

2022-Aug-29 20:22 UTC

head link

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

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.

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 2be4dd7e90a9..ad0eddf1386f 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -37,6 +37,10 @@ struct vlan_hdr {
  __be16 h_vlan_encapsulated_proto;
 };

+struct vlan_change_details {
+ bool bridge_binding;
+};
+
 /**
  * struct vlan_ethhdr - vlan ethernet header (ethhdr + vlan_hdr)
  * @h_dest: destination ethernet address
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2563d30736e9..ba4746f05d7f 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,
@@ -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 */
diff --git a/include/linux/notifier_info.h b/include/linux/notifier_info.h
new file mode 100644
index 000000000000..c103240588dc
--- /dev/null
+++ b/include/linux/notifier_info.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_NOTIFIER_INFO_H_
+#define _LINUX_NOTIFIER_INFO_H_
+
+#include <linux/netdevice.h>
+#include <linux/if_vlan.h>
+
+struct netdev_notifier_change_details_info {
+ struct netdev_notifier_info info; /* must be first */
+ union {
+ struct vlan_change_details vlan;
+ } details;
+};
+
+#endif /* !(_LINUX_NOTIFIER_INFO_H_) */
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..d737e259e295 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -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;

@@ -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 = dev;
+ info.details.vlan.bridge_binding +    !!(vlan->flags &
VLAN_FLAG_BRIDGE_BINDING);
+ call_netdevice_notifiers_info(NETDEV_CHANGE_DETAILS, &info.info);
+ }
+
  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..fe00efa46ce8 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -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)
  }
 }

+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;
+ if (netif_is_bridge_master(vlan_dev_priv(dev)->real_dev)) {
+ br_dev = vlan_dev_priv(dev)->real_dev;
+ br_vlan_upper_change(br_dev, dev,
+    info->details.vlan.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..2eed064e9529 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -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);
-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,
@@ -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().
  */

-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);
@@ -1944,6 +1944,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

On Mon, Aug 22, 2022 at 7:18 PM Sevinj Aghayeva <sevinj.aghayeva at
gmail.com>
wrote:
>
>
> On Mon, Aug 22, 2022 at 4:01 AM Nikolay Aleksandrov <razor at
blackwall.org>
> wrote:
>
>> On 20/08/2022 14:33, Sevinj Aghayeva wrote:
>> > 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:
>> [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 = 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);
>> > +
>>
>> 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
>

-- 

Sevinj.Aghayeva
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.linuxfoundation.org/pipermail/bridge/attachments/20220829/bde8691a/attachment-0001.html>

Linux Ethernet Bridging - Aug 2022 - [Bridge] [PATCH RFC net-next 0/3] net: vlan: fix bridge binding behavior and add selftests

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

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