| Message ID | 1393427905-6811-7-git-send-email-vyasevic@redhat.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Feb 26, 2014 at 10:18:24AM -0500, Vlad Yasevich wrote: > If the user configures vlan interfaces on top of the bridge and the bridge > doesn't have vlan filtering enabled, we have to place all the ports in > promsic mode so that we can correctly receive tagged frames. > When vlan filtering is enabled, the vlan configuration will be provided > via filtering interface. > When the vlan filtering is toggled, we also have mange promiscuity. have to manage? > > Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> I wonder if it matters that we scan all ports on vlan add/del now. If yes, we could optimize some cases by using a counter of promisc ports. > --- > net/bridge/br_device.c | 14 ++++++++++++++ > net/bridge/br_if.c | 17 +++++++++++++---- > net/bridge/br_private.h | 9 +++++++++ > net/bridge/br_vlan.c | 1 + > 4 files changed, 37 insertions(+), 4 deletions(-) > > diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c > index 0af9d6c..967abb3 100644 > --- a/net/bridge/br_device.c > +++ b/net/bridge/br_device.c > @@ -297,6 +297,18 @@ void br_netpoll_disable(struct net_bridge_port *p) > > #endif > > +static int br_dev_rx_add_vid(struct net_device *br_dev, __be16 proto, u16 vid) > +{ > + br_manage_promisc(netdev_priv(br_dev)); > + return 0; > +} > + > +static int br_dev_rx_kill_vid(struct net_device *br_dev, __be16 proto, u16 vid) > +{ > + br_manage_promisc(netdev_priv(br_dev)); > + return 0; > +} > + > static int br_add_slave(struct net_device *dev, struct net_device *slave_dev) > > { > @@ -328,6 +340,8 @@ static const struct net_device_ops br_netdev_ops = { > .ndo_change_rx_flags = br_dev_change_rx_flags, > .ndo_change_mtu = br_change_mtu, > .ndo_do_ioctl = br_dev_ioctl, > + .ndo_vlan_rx_add_vid = br_dev_rx_add_vid, > + .ndo_vlan_rx_kill_vid = br_dev_rx_kill_vid, > #ifdef CONFIG_NET_POLL_CONTROLLER > .ndo_netpoll_setup = br_netpoll_setup, > .ndo_netpoll_cleanup = br_netpoll_cleanup, > diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c > index 7e92bd0..55e4e28 100644 > --- a/net/bridge/br_if.c > +++ b/net/bridge/br_if.c > @@ -497,12 +497,21 @@ static void br_port_clear_promisc(struct net_bridge_port *p) > void br_manage_promisc(struct net_bridge *br) > { > struct net_bridge_port *p; > + int set_all = false; > + > + if (br->dev->flags & IFF_PROMISC) > + set_all = true; > + > + /* If vlan filtering is disabled and there are any VLANs > + * configured on top of the bridge, set promisc on all > + * ports. > + */ > + if (!br_vlan_enabled(br) && vlan_uses_dev(br->dev)) > + set_all = true; > > list_for_each_entry(p, &br->port_list, list) { > - if (br->dev->flags & IFF_PROMISC) { > - /* PROMISC flag has been turned on for the bridge > - * itself. Turn on promisc on all ports. > - */ > + if (set_all) { > + /* Set all the ports to promisc mode. */ > br_port_set_promisc(p); > > } else { > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index 4042f86..87dcc09 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -641,6 +641,10 @@ static inline u16 br_get_pvid(const struct net_port_vlans *v) > return v->pvid ?: VLAN_N_VID; > } > > +static inline int br_vlan_enabled(struct net_bridge *br) > +{ > + return br->vlan_enabled; > +} > #else > static inline bool br_allowed_ingress(struct net_bridge *br, > struct net_port_vlans *v, > @@ -721,6 +725,11 @@ static inline u16 br_get_pvid(const struct net_port_vlans *v) > { > return VLAN_N_VID; /* Returns invalid vid */ > } > + > +static inline int br_vlan_enabled(struct net_bridge *br); > +{ > + return 0; > +} > #endif > > /* br_netfilter.c */ > diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c > index 8249ca7..eddc2f6 100644 > --- a/net/bridge/br_vlan.c > +++ b/net/bridge/br_vlan.c > @@ -321,6 +321,7 @@ int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val) > goto unlock; > > br->vlan_enabled = val; > + br_manage_promisc(br); > > unlock: > rtnl_unlock(); > -- > 1.8.5.3 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/26/2014 11:00 AM, Michael S. Tsirkin wrote: > On Wed, Feb 26, 2014 at 10:18:24AM -0500, Vlad Yasevich wrote: >> If the user configures vlan interfaces on top of the bridge and the bridge >> doesn't have vlan filtering enabled, we have to place all the ports in >> promsic mode so that we can correctly receive tagged frames. >> When vlan filtering is enabled, the vlan configuration will be provided >> via filtering interface. >> When the vlan filtering is toggled, we also have mange promiscuity. > > have to manage? > >> >> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> > > > I wonder if it matters that we scan all ports > on vlan add/del now. > > If yes, we could optimize some cases by using > a counter of promisc ports. Or a bridge flag. I'll see if can add that to save repeated list walks without doing any useful work. Thanks -vlad > > >> --- >> net/bridge/br_device.c | 14 ++++++++++++++ >> net/bridge/br_if.c | 17 +++++++++++++---- >> net/bridge/br_private.h | 9 +++++++++ >> net/bridge/br_vlan.c | 1 + >> 4 files changed, 37 insertions(+), 4 deletions(-) >> >> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c >> index 0af9d6c..967abb3 100644 >> --- a/net/bridge/br_device.c >> +++ b/net/bridge/br_device.c >> @@ -297,6 +297,18 @@ void br_netpoll_disable(struct net_bridge_port *p) >> >> #endif >> >> +static int br_dev_rx_add_vid(struct net_device *br_dev, __be16 proto, u16 vid) >> +{ >> + br_manage_promisc(netdev_priv(br_dev)); >> + return 0; >> +} >> + >> +static int br_dev_rx_kill_vid(struct net_device *br_dev, __be16 proto, u16 vid) >> +{ >> + br_manage_promisc(netdev_priv(br_dev)); >> + return 0; >> +} >> + >> static int br_add_slave(struct net_device *dev, struct net_device *slave_dev) >> >> { >> @@ -328,6 +340,8 @@ static const struct net_device_ops br_netdev_ops = { >> .ndo_change_rx_flags = br_dev_change_rx_flags, >> .ndo_change_mtu = br_change_mtu, >> .ndo_do_ioctl = br_dev_ioctl, >> + .ndo_vlan_rx_add_vid = br_dev_rx_add_vid, >> + .ndo_vlan_rx_kill_vid = br_dev_rx_kill_vid, >> #ifdef CONFIG_NET_POLL_CONTROLLER >> .ndo_netpoll_setup = br_netpoll_setup, >> .ndo_netpoll_cleanup = br_netpoll_cleanup, >> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c >> index 7e92bd0..55e4e28 100644 >> --- a/net/bridge/br_if.c >> +++ b/net/bridge/br_if.c >> @@ -497,12 +497,21 @@ static void br_port_clear_promisc(struct net_bridge_port *p) >> void br_manage_promisc(struct net_bridge *br) >> { >> struct net_bridge_port *p; >> + int set_all = false; >> + >> + if (br->dev->flags & IFF_PROMISC) >> + set_all = true; >> + >> + /* If vlan filtering is disabled and there are any VLANs >> + * configured on top of the bridge, set promisc on all >> + * ports. >> + */ >> + if (!br_vlan_enabled(br) && vlan_uses_dev(br->dev)) >> + set_all = true; >> >> list_for_each_entry(p, &br->port_list, list) { >> - if (br->dev->flags & IFF_PROMISC) { >> - /* PROMISC flag has been turned on for the bridge >> - * itself. Turn on promisc on all ports. >> - */ >> + if (set_all) { >> + /* Set all the ports to promisc mode. */ >> br_port_set_promisc(p); >> >> } else { >> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h >> index 4042f86..87dcc09 100644 >> --- a/net/bridge/br_private.h >> +++ b/net/bridge/br_private.h >> @@ -641,6 +641,10 @@ static inline u16 br_get_pvid(const struct net_port_vlans *v) >> return v->pvid ?: VLAN_N_VID; >> } >> >> +static inline int br_vlan_enabled(struct net_bridge *br) >> +{ >> + return br->vlan_enabled; >> +} >> #else >> static inline bool br_allowed_ingress(struct net_bridge *br, >> struct net_port_vlans *v, >> @@ -721,6 +725,11 @@ static inline u16 br_get_pvid(const struct net_port_vlans *v) >> { >> return VLAN_N_VID; /* Returns invalid vid */ >> } >> + >> +static inline int br_vlan_enabled(struct net_bridge *br); >> +{ >> + return 0; >> +} >> #endif >> >> /* br_netfilter.c */ >> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c >> index 8249ca7..eddc2f6 100644 >> --- a/net/bridge/br_vlan.c >> +++ b/net/bridge/br_vlan.c >> @@ -321,6 +321,7 @@ int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val) >> goto unlock; >> >> br->vlan_enabled = val; >> + br_manage_promisc(br); >> >> unlock: >> rtnl_unlock(); >> -- >> 1.8.5.3 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 26, 2014 at 11:05:40AM -0500, Vlad Yasevich wrote: > On 02/26/2014 11:00 AM, Michael S. Tsirkin wrote: > > On Wed, Feb 26, 2014 at 10:18:24AM -0500, Vlad Yasevich wrote: > >> If the user configures vlan interfaces on top of the bridge and the bridge > >> doesn't have vlan filtering enabled, we have to place all the ports in > >> promsic mode so that we can correctly receive tagged frames. > >> When vlan filtering is enabled, the vlan configuration will be provided > >> via filtering interface. > >> When the vlan filtering is toggled, we also have mange promiscuity. > > > > have to manage? > > > >> > >> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> > > > > > > I wonder if it matters that we scan all ports > > on vlan add/del now. > > > > If yes, we could optimize some cases by using > > a counter of promisc ports. > > Or a bridge flag. I'll see if can add that to save > repeated list walks without doing any useful work. > > Thanks > -vlad Need to check if there are port list walks there anyway. If yes then it doesn't matter much I think. > > > > > >> --- > >> net/bridge/br_device.c | 14 ++++++++++++++ > >> net/bridge/br_if.c | 17 +++++++++++++---- > >> net/bridge/br_private.h | 9 +++++++++ > >> net/bridge/br_vlan.c | 1 + > >> 4 files changed, 37 insertions(+), 4 deletions(-) > >> > >> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c > >> index 0af9d6c..967abb3 100644 > >> --- a/net/bridge/br_device.c > >> +++ b/net/bridge/br_device.c > >> @@ -297,6 +297,18 @@ void br_netpoll_disable(struct net_bridge_port *p) > >> > >> #endif > >> > >> +static int br_dev_rx_add_vid(struct net_device *br_dev, __be16 proto, u16 vid) > >> +{ > >> + br_manage_promisc(netdev_priv(br_dev)); > >> + return 0; > >> +} > >> + > >> +static int br_dev_rx_kill_vid(struct net_device *br_dev, __be16 proto, u16 vid) > >> +{ > >> + br_manage_promisc(netdev_priv(br_dev)); > >> + return 0; > >> +} > >> + > >> static int br_add_slave(struct net_device *dev, struct net_device *slave_dev) > >> > >> { > >> @@ -328,6 +340,8 @@ static const struct net_device_ops br_netdev_ops = { > >> .ndo_change_rx_flags = br_dev_change_rx_flags, > >> .ndo_change_mtu = br_change_mtu, > >> .ndo_do_ioctl = br_dev_ioctl, > >> + .ndo_vlan_rx_add_vid = br_dev_rx_add_vid, > >> + .ndo_vlan_rx_kill_vid = br_dev_rx_kill_vid, > >> #ifdef CONFIG_NET_POLL_CONTROLLER > >> .ndo_netpoll_setup = br_netpoll_setup, > >> .ndo_netpoll_cleanup = br_netpoll_cleanup, > >> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c > >> index 7e92bd0..55e4e28 100644 > >> --- a/net/bridge/br_if.c > >> +++ b/net/bridge/br_if.c > >> @@ -497,12 +497,21 @@ static void br_port_clear_promisc(struct net_bridge_port *p) > >> void br_manage_promisc(struct net_bridge *br) > >> { > >> struct net_bridge_port *p; > >> + int set_all = false; > >> + > >> + if (br->dev->flags & IFF_PROMISC) > >> + set_all = true; > >> + > >> + /* If vlan filtering is disabled and there are any VLANs > >> + * configured on top of the bridge, set promisc on all > >> + * ports. > >> + */ > >> + if (!br_vlan_enabled(br) && vlan_uses_dev(br->dev)) > >> + set_all = true; > >> > >> list_for_each_entry(p, &br->port_list, list) { > >> - if (br->dev->flags & IFF_PROMISC) { > >> - /* PROMISC flag has been turned on for the bridge > >> - * itself. Turn on promisc on all ports. > >> - */ > >> + if (set_all) { > >> + /* Set all the ports to promisc mode. */ > >> br_port_set_promisc(p); > >> > >> } else { > >> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > >> index 4042f86..87dcc09 100644 > >> --- a/net/bridge/br_private.h > >> +++ b/net/bridge/br_private.h > >> @@ -641,6 +641,10 @@ static inline u16 br_get_pvid(const struct net_port_vlans *v) > >> return v->pvid ?: VLAN_N_VID; > >> } > >> > >> +static inline int br_vlan_enabled(struct net_bridge *br) > >> +{ > >> + return br->vlan_enabled; > >> +} > >> #else > >> static inline bool br_allowed_ingress(struct net_bridge *br, > >> struct net_port_vlans *v, > >> @@ -721,6 +725,11 @@ static inline u16 br_get_pvid(const struct net_port_vlans *v) > >> { > >> return VLAN_N_VID; /* Returns invalid vid */ > >> } > >> + > >> +static inline int br_vlan_enabled(struct net_bridge *br); > >> +{ > >> + return 0; > >> +} > >> #endif > >> > >> /* br_netfilter.c */ > >> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c > >> index 8249ca7..eddc2f6 100644 > >> --- a/net/bridge/br_vlan.c > >> +++ b/net/bridge/br_vlan.c > >> @@ -321,6 +321,7 @@ int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val) > >> goto unlock; > >> > >> br->vlan_enabled = val; > >> + br_manage_promisc(br); > >> > >> unlock: > >> rtnl_unlock(); > >> -- > >> 1.8.5.3 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
(2014/02/27 0:18), Vlad Yasevich wrote: > If the user configures vlan interfaces on top of the bridge and the bridge > doesn't have vlan filtering enabled, we have to place all the ports in > promsic mode so that we can correctly receive tagged frames. > When vlan filtering is enabled, the vlan configuration will be provided > via filtering interface. > When the vlan filtering is toggled, we also have mange promiscuity. If we disable vlan_filtering and no vlan interface is configured on the bridge, we cannot forward any tagged traffic? If we want to forward frames from one port to another port (not from/to bridge device), we have to add vlan interface or set promisc mode, right? Sorry if I misunderstood it. I'm not sure.. Thanks, Toshiaki Makita > > Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> > --- > net/bridge/br_device.c | 14 ++++++++++++++ > net/bridge/br_if.c | 17 +++++++++++++---- > net/bridge/br_private.h | 9 +++++++++ > net/bridge/br_vlan.c | 1 + > 4 files changed, 37 insertions(+), 4 deletions(-) > > diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c > index 0af9d6c..967abb3 100644 > --- a/net/bridge/br_device.c > +++ b/net/bridge/br_device.c > @@ -297,6 +297,18 @@ void br_netpoll_disable(struct net_bridge_port *p) > > #endif > > +static int br_dev_rx_add_vid(struct net_device *br_dev, __be16 proto, u16 vid) > +{ > + br_manage_promisc(netdev_priv(br_dev)); > + return 0; > +} > + > +static int br_dev_rx_kill_vid(struct net_device *br_dev, __be16 proto, u16 vid) > +{ > + br_manage_promisc(netdev_priv(br_dev)); > + return 0; > +} > + > static int br_add_slave(struct net_device *dev, struct net_device *slave_dev) > > { > @@ -328,6 +340,8 @@ static const struct net_device_ops br_netdev_ops = { > .ndo_change_rx_flags = br_dev_change_rx_flags, > .ndo_change_mtu = br_change_mtu, > .ndo_do_ioctl = br_dev_ioctl, > + .ndo_vlan_rx_add_vid = br_dev_rx_add_vid, > + .ndo_vlan_rx_kill_vid = br_dev_rx_kill_vid, > #ifdef CONFIG_NET_POLL_CONTROLLER > .ndo_netpoll_setup = br_netpoll_setup, > .ndo_netpoll_cleanup = br_netpoll_cleanup, > diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c > index 7e92bd0..55e4e28 100644 > --- a/net/bridge/br_if.c > +++ b/net/bridge/br_if.c > @@ -497,12 +497,21 @@ static void br_port_clear_promisc(struct net_bridge_port *p) > void br_manage_promisc(struct net_bridge *br) > { > struct net_bridge_port *p; > + int set_all = false; > + > + if (br->dev->flags & IFF_PROMISC) > + set_all = true; > + > + /* If vlan filtering is disabled and there are any VLANs > + * configured on top of the bridge, set promisc on all > + * ports. > + */ > + if (!br_vlan_enabled(br) && vlan_uses_dev(br->dev)) > + set_all = true; > > list_for_each_entry(p, &br->port_list, list) { > - if (br->dev->flags & IFF_PROMISC) { > - /* PROMISC flag has been turned on for the bridge > - * itself. Turn on promisc on all ports. > - */ > + if (set_all) { > + /* Set all the ports to promisc mode. */ > br_port_set_promisc(p); > > } else { > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index 4042f86..87dcc09 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -641,6 +641,10 @@ static inline u16 br_get_pvid(const struct net_port_vlans *v) > return v->pvid ?: VLAN_N_VID; > } > > +static inline int br_vlan_enabled(struct net_bridge *br) > +{ > + return br->vlan_enabled; > +} > #else > static inline bool br_allowed_ingress(struct net_bridge *br, > struct net_port_vlans *v, > @@ -721,6 +725,11 @@ static inline u16 br_get_pvid(const struct net_port_vlans *v) > { > return VLAN_N_VID; /* Returns invalid vid */ > } > + > +static inline int br_vlan_enabled(struct net_bridge *br); > +{ > + return 0; > +} > #endif > > /* br_netfilter.c */ > diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c > index 8249ca7..eddc2f6 100644 > --- a/net/bridge/br_vlan.c > +++ b/net/bridge/br_vlan.c > @@ -321,6 +321,7 @@ int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val) > goto unlock; > > br->vlan_enabled = val; > + br_manage_promisc(br); > > unlock: > rtnl_unlock(); > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/27/2014 07:06 AM, Toshiaki Makita wrote: > (2014/02/27 0:18), Vlad Yasevich wrote: >> If the user configures vlan interfaces on top of the bridge and the bridge >> doesn't have vlan filtering enabled, we have to place all the ports in >> promsic mode so that we can correctly receive tagged frames. >> When vlan filtering is enabled, the vlan configuration will be provided >> via filtering interface. >> When the vlan filtering is toggled, we also have mange promiscuity. > > If we disable vlan_filtering and no vlan interface is configured on the > bridge, we cannot forward any tagged traffic? We can't receive tagged traffic, so we turn promisc on. > If we want to forward frames from one port to another port (not from/to > bridge device), we have to add vlan interface or set promisc mode, right? > Hm.. Good point. This isn't enough to address the scenario that Patch7 tries to solve. I'll need to think about that. This is partially why I split functionality in Patch7 out. It made things more difficult. -vlad > Sorry if I misunderstood it. I'm not sure.. > > Thanks, > Toshiaki Makita > >> >> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> >> --- >> net/bridge/br_device.c | 14 ++++++++++++++ >> net/bridge/br_if.c | 17 +++++++++++++---- >> net/bridge/br_private.h | 9 +++++++++ >> net/bridge/br_vlan.c | 1 + >> 4 files changed, 37 insertions(+), 4 deletions(-) >> >> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c >> index 0af9d6c..967abb3 100644 >> --- a/net/bridge/br_device.c >> +++ b/net/bridge/br_device.c >> @@ -297,6 +297,18 @@ void br_netpoll_disable(struct net_bridge_port *p) >> >> #endif >> >> +static int br_dev_rx_add_vid(struct net_device *br_dev, __be16 proto, u16 vid) >> +{ >> + br_manage_promisc(netdev_priv(br_dev)); >> + return 0; >> +} >> + >> +static int br_dev_rx_kill_vid(struct net_device *br_dev, __be16 proto, u16 vid) >> +{ >> + br_manage_promisc(netdev_priv(br_dev)); >> + return 0; >> +} >> + >> static int br_add_slave(struct net_device *dev, struct net_device *slave_dev) >> >> { >> @@ -328,6 +340,8 @@ static const struct net_device_ops br_netdev_ops = { >> .ndo_change_rx_flags = br_dev_change_rx_flags, >> .ndo_change_mtu = br_change_mtu, >> .ndo_do_ioctl = br_dev_ioctl, >> + .ndo_vlan_rx_add_vid = br_dev_rx_add_vid, >> + .ndo_vlan_rx_kill_vid = br_dev_rx_kill_vid, >> #ifdef CONFIG_NET_POLL_CONTROLLER >> .ndo_netpoll_setup = br_netpoll_setup, >> .ndo_netpoll_cleanup = br_netpoll_cleanup, >> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c >> index 7e92bd0..55e4e28 100644 >> --- a/net/bridge/br_if.c >> +++ b/net/bridge/br_if.c >> @@ -497,12 +497,21 @@ static void br_port_clear_promisc(struct net_bridge_port *p) >> void br_manage_promisc(struct net_bridge *br) >> { >> struct net_bridge_port *p; >> + int set_all = false; >> + >> + if (br->dev->flags & IFF_PROMISC) >> + set_all = true; >> + >> + /* If vlan filtering is disabled and there are any VLANs >> + * configured on top of the bridge, set promisc on all >> + * ports. >> + */ >> + if (!br_vlan_enabled(br) && vlan_uses_dev(br->dev)) >> + set_all = true; >> >> list_for_each_entry(p, &br->port_list, list) { >> - if (br->dev->flags & IFF_PROMISC) { >> - /* PROMISC flag has been turned on for the bridge >> - * itself. Turn on promisc on all ports. >> - */ >> + if (set_all) { >> + /* Set all the ports to promisc mode. */ >> br_port_set_promisc(p); >> >> } else { >> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h >> index 4042f86..87dcc09 100644 >> --- a/net/bridge/br_private.h >> +++ b/net/bridge/br_private.h >> @@ -641,6 +641,10 @@ static inline u16 br_get_pvid(const struct net_port_vlans *v) >> return v->pvid ?: VLAN_N_VID; >> } >> >> +static inline int br_vlan_enabled(struct net_bridge *br) >> +{ >> + return br->vlan_enabled; >> +} >> #else >> static inline bool br_allowed_ingress(struct net_bridge *br, >> struct net_port_vlans *v, >> @@ -721,6 +725,11 @@ static inline u16 br_get_pvid(const struct net_port_vlans *v) >> { >> return VLAN_N_VID; /* Returns invalid vid */ >> } >> + >> +static inline int br_vlan_enabled(struct net_bridge *br); >> +{ >> + return 0; >> +} >> #endif >> >> /* br_netfilter.c */ >> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c >> index 8249ca7..eddc2f6 100644 >> --- a/net/bridge/br_vlan.c >> +++ b/net/bridge/br_vlan.c >> @@ -321,6 +321,7 @@ int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val) >> goto unlock; >> >> br->vlan_enabled = val; >> + br_manage_promisc(br); >> >> unlock: >> rtnl_unlock(); >> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/27/2014 08:17 AM, Vlad Yasevich wrote: > On 02/27/2014 07:06 AM, Toshiaki Makita wrote: >> (2014/02/27 0:18), Vlad Yasevich wrote: >>> If the user configures vlan interfaces on top of the bridge and the bridge >>> doesn't have vlan filtering enabled, we have to place all the ports in >>> promsic mode so that we can correctly receive tagged frames. >>> When vlan filtering is enabled, the vlan configuration will be provided >>> via filtering interface. >>> When the vlan filtering is toggled, we also have mange promiscuity. >> >> If we disable vlan_filtering and no vlan interface is configured on the >> bridge, we cannot forward any tagged traffic? > > We can't receive tagged traffic, so we turn promisc on. > >> If we want to forward frames from one port to another port (not from/to >> bridge device), we have to add vlan interface or set promisc mode, right? >> > > Hm.. Good point. This isn't enough to address the scenario that Patch7 > tries to solve. I'll need to think about that. This is partially why > I split functionality in Patch7 out. It made things more difficult. > I now understood what you were referring to above a bit better. This patch solves just part of the problem. The other part is what happens when someone behind the bridge is using vlan tagging without the bridge being aware of it and expects the bridge to forward such traffic. So, if we ever want to disable promiscuous mode on the bridge ports, we either need to depend on lan filtering being configured in the bridge or have the ability to disable vlan filtering in the driver. Neither is really a good thing. I'll need to think about this. -vlad > -vlad > >> Sorry if I misunderstood it. I'm not sure.. >> >> Thanks, >> Toshiaki Makita >> >>> >>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> >>> --- >>> net/bridge/br_device.c | 14 ++++++++++++++ >>> net/bridge/br_if.c | 17 +++++++++++++---- >>> net/bridge/br_private.h | 9 +++++++++ >>> net/bridge/br_vlan.c | 1 + >>> 4 files changed, 37 insertions(+), 4 deletions(-) >>> >>> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c >>> index 0af9d6c..967abb3 100644 >>> --- a/net/bridge/br_device.c >>> +++ b/net/bridge/br_device.c >>> @@ -297,6 +297,18 @@ void br_netpoll_disable(struct net_bridge_port *p) >>> >>> #endif >>> >>> +static int br_dev_rx_add_vid(struct net_device *br_dev, __be16 proto, u16 vid) >>> +{ >>> + br_manage_promisc(netdev_priv(br_dev)); >>> + return 0; >>> +} >>> + >>> +static int br_dev_rx_kill_vid(struct net_device *br_dev, __be16 proto, u16 vid) >>> +{ >>> + br_manage_promisc(netdev_priv(br_dev)); >>> + return 0; >>> +} >>> + >>> static int br_add_slave(struct net_device *dev, struct net_device *slave_dev) >>> >>> { >>> @@ -328,6 +340,8 @@ static const struct net_device_ops br_netdev_ops = { >>> .ndo_change_rx_flags = br_dev_change_rx_flags, >>> .ndo_change_mtu = br_change_mtu, >>> .ndo_do_ioctl = br_dev_ioctl, >>> + .ndo_vlan_rx_add_vid = br_dev_rx_add_vid, >>> + .ndo_vlan_rx_kill_vid = br_dev_rx_kill_vid, >>> #ifdef CONFIG_NET_POLL_CONTROLLER >>> .ndo_netpoll_setup = br_netpoll_setup, >>> .ndo_netpoll_cleanup = br_netpoll_cleanup, >>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c >>> index 7e92bd0..55e4e28 100644 >>> --- a/net/bridge/br_if.c >>> +++ b/net/bridge/br_if.c >>> @@ -497,12 +497,21 @@ static void br_port_clear_promisc(struct net_bridge_port *p) >>> void br_manage_promisc(struct net_bridge *br) >>> { >>> struct net_bridge_port *p; >>> + int set_all = false; >>> + >>> + if (br->dev->flags & IFF_PROMISC) >>> + set_all = true; >>> + >>> + /* If vlan filtering is disabled and there are any VLANs >>> + * configured on top of the bridge, set promisc on all >>> + * ports. >>> + */ >>> + if (!br_vlan_enabled(br) && vlan_uses_dev(br->dev)) >>> + set_all = true; >>> >>> list_for_each_entry(p, &br->port_list, list) { >>> - if (br->dev->flags & IFF_PROMISC) { >>> - /* PROMISC flag has been turned on for the bridge >>> - * itself. Turn on promisc on all ports. >>> - */ >>> + if (set_all) { >>> + /* Set all the ports to promisc mode. */ >>> br_port_set_promisc(p); >>> >>> } else { >>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h >>> index 4042f86..87dcc09 100644 >>> --- a/net/bridge/br_private.h >>> +++ b/net/bridge/br_private.h >>> @@ -641,6 +641,10 @@ static inline u16 br_get_pvid(const struct net_port_vlans *v) >>> return v->pvid ?: VLAN_N_VID; >>> } >>> >>> +static inline int br_vlan_enabled(struct net_bridge *br) >>> +{ >>> + return br->vlan_enabled; >>> +} >>> #else >>> static inline bool br_allowed_ingress(struct net_bridge *br, >>> struct net_port_vlans *v, >>> @@ -721,6 +725,11 @@ static inline u16 br_get_pvid(const struct net_port_vlans *v) >>> { >>> return VLAN_N_VID; /* Returns invalid vid */ >>> } >>> + >>> +static inline int br_vlan_enabled(struct net_bridge *br); >>> +{ >>> + return 0; >>> +} >>> #endif >>> >>> /* br_netfilter.c */ >>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c >>> index 8249ca7..eddc2f6 100644 >>> --- a/net/bridge/br_vlan.c >>> +++ b/net/bridge/br_vlan.c >>> @@ -321,6 +321,7 @@ int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val) >>> goto unlock; >>> >>> br->vlan_enabled = val; >>> + br_manage_promisc(br); >>> >>> unlock: >>> rtnl_unlock(); >>> > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-02-28 at 14:34 -0500, Vlad Yasevich wrote: > On 02/27/2014 08:17 AM, Vlad Yasevich wrote: > > On 02/27/2014 07:06 AM, Toshiaki Makita wrote: > >> (2014/02/27 0:18), Vlad Yasevich wrote: > >>> If the user configures vlan interfaces on top of the bridge and the bridge > >>> doesn't have vlan filtering enabled, we have to place all the ports in > >>> promsic mode so that we can correctly receive tagged frames. > >>> When vlan filtering is enabled, the vlan configuration will be provided > >>> via filtering interface. > >>> When the vlan filtering is toggled, we also have mange promiscuity. > >> > >> If we disable vlan_filtering and no vlan interface is configured on the > >> bridge, we cannot forward any tagged traffic? > > > > We can't receive tagged traffic, so we turn promisc on. > > > >> If we want to forward frames from one port to another port (not from/to > >> bridge device), we have to add vlan interface or set promisc mode, right? > >> > > > > Hm.. Good point. This isn't enough to address the scenario that Patch7 > > tries to solve. I'll need to think about that. This is partially why > > I split functionality in Patch7 out. It made things more difficult. > > > > I now understood what you were referring to above a bit better. > This patch solves just part of the problem. The other part is what > happens when someone behind the bridge is using vlan tagging without > the bridge being aware of it and expects the bridge to forward such traffic. > So, if we ever want to disable promiscuous mode on the bridge ports, we > either need to depend on lan filtering being configured in the bridge > or have the ability to disable vlan filtering in the driver. > > Neither is really a good thing. I'll need to think about this. Yes, that is what I was worried about. As a bridge has no way to know which vid will be used in incoming frame's vlan tag, we maybe have to call vlan_vid_add() for all vids when we disable promiscuous on a port? If we had an API to simply disable vlan filtering of a NIC, it could be better... Thanks, Toshiaki Makita -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/01/2014 09:57 AM, Toshiaki Makita wrote: > On Fri, 2014-02-28 at 14:34 -0500, Vlad Yasevich wrote: >> On 02/27/2014 08:17 AM, Vlad Yasevich wrote: >>> On 02/27/2014 07:06 AM, Toshiaki Makita wrote: >>>> (2014/02/27 0:18), Vlad Yasevich wrote: >>>>> If the user configures vlan interfaces on top of the bridge and the bridge >>>>> doesn't have vlan filtering enabled, we have to place all the ports in >>>>> promsic mode so that we can correctly receive tagged frames. >>>>> When vlan filtering is enabled, the vlan configuration will be provided >>>>> via filtering interface. >>>>> When the vlan filtering is toggled, we also have mange promiscuity. >>>> >>>> If we disable vlan_filtering and no vlan interface is configured on the >>>> bridge, we cannot forward any tagged traffic? >>> >>> We can't receive tagged traffic, so we turn promisc on. >>> >>>> If we want to forward frames from one port to another port (not from/to >>>> bridge device), we have to add vlan interface or set promisc mode, right? >>>> >>> >>> Hm.. Good point. This isn't enough to address the scenario that Patch7 >>> tries to solve. I'll need to think about that. This is partially why >>> I split functionality in Patch7 out. It made things more difficult. >>> >> >> I now understood what you were referring to above a bit better. >> This patch solves just part of the problem. The other part is what >> happens when someone behind the bridge is using vlan tagging without >> the bridge being aware of it and expects the bridge to forward such traffic. >> So, if we ever want to disable promiscuous mode on the bridge ports, we >> either need to depend on lan filtering being configured in the bridge >> or have the ability to disable vlan filtering in the driver. >> >> Neither is really a good thing. I'll need to think about this. > > Yes, that is what I was worried about. > As a bridge has no way to know which vid will be used in incoming > frame's vlan tag, we maybe have to call vlan_vid_add() for all vids when > we disable promiscuous on a port? If we had an API to simply disable > vlan filtering of a NIC, it could be better... That's what I am looking at now. Some nics appear to handle this better then others. -vlad > > Thanks, > Toshiaki Makita > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c index 0af9d6c..967abb3 100644 --- a/net/bridge/br_device.c +++ b/net/bridge/br_device.c @@ -297,6 +297,18 @@ void br_netpoll_disable(struct net_bridge_port *p) #endif +static int br_dev_rx_add_vid(struct net_device *br_dev, __be16 proto, u16 vid) +{ + br_manage_promisc(netdev_priv(br_dev)); + return 0; +} + +static int br_dev_rx_kill_vid(struct net_device *br_dev, __be16 proto, u16 vid) +{ + br_manage_promisc(netdev_priv(br_dev)); + return 0; +} + static int br_add_slave(struct net_device *dev, struct net_device *slave_dev) { @@ -328,6 +340,8 @@ static const struct net_device_ops br_netdev_ops = { .ndo_change_rx_flags = br_dev_change_rx_flags, .ndo_change_mtu = br_change_mtu, .ndo_do_ioctl = br_dev_ioctl, + .ndo_vlan_rx_add_vid = br_dev_rx_add_vid, + .ndo_vlan_rx_kill_vid = br_dev_rx_kill_vid, #ifdef CONFIG_NET_POLL_CONTROLLER .ndo_netpoll_setup = br_netpoll_setup, .ndo_netpoll_cleanup = br_netpoll_cleanup, diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c index 7e92bd0..55e4e28 100644 --- a/net/bridge/br_if.c +++ b/net/bridge/br_if.c @@ -497,12 +497,21 @@ static void br_port_clear_promisc(struct net_bridge_port *p) void br_manage_promisc(struct net_bridge *br) { struct net_bridge_port *p; + int set_all = false; + + if (br->dev->flags & IFF_PROMISC) + set_all = true; + + /* If vlan filtering is disabled and there are any VLANs + * configured on top of the bridge, set promisc on all + * ports. + */ + if (!br_vlan_enabled(br) && vlan_uses_dev(br->dev)) + set_all = true; list_for_each_entry(p, &br->port_list, list) { - if (br->dev->flags & IFF_PROMISC) { - /* PROMISC flag has been turned on for the bridge - * itself. Turn on promisc on all ports. - */ + if (set_all) { + /* Set all the ports to promisc mode. */ br_port_set_promisc(p); } else { diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 4042f86..87dcc09 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -641,6 +641,10 @@ static inline u16 br_get_pvid(const struct net_port_vlans *v) return v->pvid ?: VLAN_N_VID; } +static inline int br_vlan_enabled(struct net_bridge *br) +{ + return br->vlan_enabled; +} #else static inline bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v, @@ -721,6 +725,11 @@ static inline u16 br_get_pvid(const struct net_port_vlans *v) { return VLAN_N_VID; /* Returns invalid vid */ } + +static inline int br_vlan_enabled(struct net_bridge *br); +{ + return 0; +} #endif /* br_netfilter.c */ diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c index 8249ca7..eddc2f6 100644 --- a/net/bridge/br_vlan.c +++ b/net/bridge/br_vlan.c @@ -321,6 +321,7 @@ int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val) goto unlock; br->vlan_enabled = val; + br_manage_promisc(br); unlock: rtnl_unlock();
If the user configures vlan interfaces on top of the bridge and the bridge doesn't have vlan filtering enabled, we have to place all the ports in promsic mode so that we can correctly receive tagged frames. When vlan filtering is enabled, the vlan configuration will be provided via filtering interface. When the vlan filtering is toggled, we also have mange promiscuity. Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> --- net/bridge/br_device.c | 14 ++++++++++++++ net/bridge/br_if.c | 17 +++++++++++++---- net/bridge/br_private.h | 9 +++++++++ net/bridge/br_vlan.c | 1 + 4 files changed, 37 insertions(+), 4 deletions(-)