| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d6658dba-efac-c162-38db-bda14a8d867d@vyatta.att-mail.com>
Date: Thu, 18 Apr 2019 15:42:19 +0100
From: Mike Manning <mmanning@...tta.att-mail.com>
To: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>,
netdev@...r.kernel.org, roopa@...ulusnetworks.com
Subject: Re: [PATCH net-next v2 3/5] bridge: support binding vlan dev link
state to vlan member bridge ports
On 18/04/2019 12:28, Nikolay Aleksandrov wrote:
> On 17/04/2019 21:16, Mike Manning wrote:
>> In the case of vlan filtering on bridges, the bridge may also have the
>> corresponding vlan devices as upper devices. A vlan bridge binding mode
>> is added to allow the link state of the vlan device to track only the
>> state of the subset of bridge ports that are also members of the vlan,
>> rather than that of all bridge ports. This mode is set with a vlan flag
>> rather than a bridge sysfs so that the 8021q module is aware that it
>> should not set the link state for the vlan device.
>>
>> If bridge vlan is configured, the bridge device event handling results
>> in the link state for an upper device being set, if it is a vlan device
>> with the vlan bridge binding mode enabled. This also sets a
>> vlan_bridge_binding flag so that subsequent UP/DOWN/CHANGE events for
>> the ports in that bridge result in a link state update of the vlan
>> device if required.
>>
>> The link state of the vlan device is up if there is at least one bridge
>> port that is a vlan member that is admin & oper up, otherwise its oper
>> state is IF_OPER_LOWERLAYERDOWN.
>>
>> Signed-off-by: Mike Manning <mmanning@...tta.att-mail.com>
>> ---
>> net/bridge/br.c | 17 ++++--
>> net/bridge/br_private.h | 14 +++++
>> net/bridge/br_vlan.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 178 insertions(+), 4 deletions(-)
>>
> Hi Mike,
> One minor comment below.
>
>> diff --git a/net/bridge/br.c b/net/bridge/br.c
>> index a5174e5001d8..a9bb5cd962c6 100644
>> --- a/net/bridge/br.c
>> +++ b/net/bridge/br.c
>> @@ -40,10 +40,15 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
>> bool changed_addr;
>> int err;
>>
>> - /* register of bridge completed, add sysfs entries */
>> - if ((dev->priv_flags & IFF_EBRIDGE) && event == NETDEV_REGISTER) {
>> - br_sysfs_addbr(dev);
>> - return NOTIFY_DONE;
>> + if (dev->priv_flags & IFF_EBRIDGE) {
>> + if (event == NETDEV_REGISTER) {
>> + /* register of bridge completed, add sysfs entries */
>> + br_sysfs_addbr(dev);
>> + return NOTIFY_DONE;
>> + }
>> +#ifdef CONFIG_BRIDGE_VLAN_FILTERING
>> + br_vlan_bridge_event(dev, event, ptr);
>> +#endif
> Why the ifdef here ? You have this function defined for both cases, one when
> configured with vlans and a noop for the no-vlan case.
Hi Nikolay, thank you very much for the review, I will annotate v3
appropriately. You are quite right, there is no need for these ugly
inline #ifdef, as I followed your example of providing stubs for the
no-vlan case.
>> }
>>
>> /* not a port of a bridge */
>> @@ -126,6 +131,10 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v
>> break;
>> }
>>
>> +#ifdef CONFIG_BRIDGE_VLAN_FILTERING
>> + br_vlan_port_event(p, event);
>> +#endif
>> +
> Same question here.
>
>> /* Events that may cause spanning tree to refresh */
>> if (!notified && (event == NETDEV_CHANGEADDR || event == NETDEV_UP ||
>> event == NETDEV_CHANGE || event == NETDEV_DOWN))
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 4bea2f11da9b..334a8c496b50 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -321,6 +321,7 @@ enum net_bridge_opts {
>> BROPT_MTU_SET_BY_USER,
>> BROPT_VLAN_STATS_PER_PORT,
>> BROPT_NO_LL_LEARN,
>> + BROPT_VLAN_BRIDGE_BINDING,
>> };
>>
>> struct net_bridge {
>> @@ -895,6 +896,9 @@ int nbp_vlan_init(struct net_bridge_port *port, struct netlink_ext_ack *extack);
>> int nbp_get_num_vlan_infos(struct net_bridge_port *p, u32 filter_mask);
>> void br_vlan_get_stats(const struct net_bridge_vlan *v,
>> struct br_vlan_stats *stats);
>> +void br_vlan_port_event(struct net_bridge_port *p, unsigned long event);
>> +void br_vlan_bridge_event(struct net_device *dev, unsigned long event,
>> + void *ptr);
>>
>> static inline struct net_bridge_vlan_group *br_vlan_group(
>> const struct net_bridge *br)
>> @@ -1078,6 +1082,16 @@ static inline void br_vlan_get_stats(const struct net_bridge_vlan *v,
>> struct br_vlan_stats *stats)
>> {
>> }
>> +
>> +static inline void br_vlan_port_event(struct net_bridge_port *p,
>> + unsigned long event)
>> +{
>> +}
>> +
>> +static inline void br_vlan_bridge_event(struct net_device *dev,
>> + unsigned long event, void *ptr)
>> +{
>> +}
>> #endif
>>
>> struct nf_br_ops {
>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>> index 0a02822b5667..b903689a8fc5 100644
>> --- a/net/bridge/br_vlan.c
>> +++ b/net/bridge/br_vlan.c
>> @@ -1264,3 +1264,154 @@ int br_vlan_get_info(const struct net_device *dev, u16 vid,
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(br_vlan_get_info);
>> +
>> +static int br_vlan_is_bind_vlan_dev(const struct net_device *dev)
>> +{
>> + return is_vlan_dev(dev) &&
>> + !!(vlan_dev_priv(dev)->flags & VLAN_FLAG_BRIDGE_BINDING);
>> +}
>> +
>> +static int br_vlan_is_bind_vlan_dev_fn(struct net_device *dev,
>> + __always_unused void *data)
>> +{
>> + return br_vlan_is_bind_vlan_dev(dev);
>> +}
>> +
>> +static bool br_vlan_has_upper_bind_vlan_dev(struct net_device *dev)
>> +{
>> + int found;
>> +
>> + rcu_read_lock();
>> + found = netdev_walk_all_upper_dev_rcu(dev, br_vlan_is_bind_vlan_dev_fn,
>> + NULL);
>> + rcu_read_unlock();
>> +
>> + return !!found;
>> +}
>> +
>> +struct br_vlan_bind_walk_data {
>> + u16 vid;
>> + struct net_device *result;
>> +};
>> +
>> +static int br_vlan_match_bind_vlan_dev_fn(struct net_device *dev,
>> + void *data_in)
>> +{
>> + struct br_vlan_bind_walk_data *data = data_in;
>> + int found = 0;
>> +
>> + if (br_vlan_is_bind_vlan_dev(dev) &&
>> + vlan_dev_priv(dev)->vlan_id == data->vid) {
>> + data->result = dev;
>> + found = 1;
>> + }
>> +
>> + return found;
>> +}
>> +
>> +static struct net_device *
>> +br_vlan_get_upper_bind_vlan_dev(struct net_device *dev, u16 vid)
>> +{
>> + struct br_vlan_bind_walk_data data = {
>> + .vid = vid,
>> + };
>> +
>> + rcu_read_lock();
>> + netdev_walk_all_upper_dev_rcu(dev, br_vlan_match_bind_vlan_dev_fn,
>> + &data);
>> + rcu_read_unlock();
>> +
>> + return data.result;
>> +}
>> +
>> +static bool br_vlan_is_dev_up(const struct net_device *dev)
>> +{
>> + return !!(dev->flags & IFF_UP) && netif_oper_up(dev);
>> +}
>> +
>> +static void br_vlan_set_vlan_dev_state(const struct net_bridge *br,
>> + struct net_device *vlan_dev)
>> +{
>> + u16 vid = vlan_dev_priv(vlan_dev)->vlan_id;
>> + struct net_bridge_vlan_group *vg;
>> + struct net_bridge_port *p;
>> + bool has_carrier = false;
>> +
>> + list_for_each_entry(p, &br->port_list, list) {
>> + vg = nbp_vlan_group(p);
>> + if (br_vlan_find(vg, vid) && br_vlan_is_dev_up(p->dev)) {
>> + has_carrier = true;
>> + break;
>> + }
>> + }
>> +
>> + if (has_carrier)
>> + netif_carrier_on(vlan_dev);
>> + else
>> + netif_carrier_off(vlan_dev);
>> +}
>> +
>> +static void br_vlan_set_all_vlan_dev_state(struct net_bridge_port *p)
>> +{
>> + struct net_bridge_vlan_group *vg = nbp_vlan_group(p);
>> + struct net_bridge_vlan *vlan;
>> + struct net_device *vlan_dev;
>> +
>> + list_for_each_entry(vlan, &vg->vlan_list, vlist) {
>> + vlan_dev = br_vlan_get_upper_bind_vlan_dev(p->br->dev,
>> + vlan->vid);
>> + if (vlan_dev) {
>> + if (br_vlan_is_dev_up(p->dev))
>> + netif_carrier_on(vlan_dev);
>> + else
>> + br_vlan_set_vlan_dev_state(p->br, vlan_dev);
>> + }
>> + }
>> +}
>> +
>> +static void br_vlan_upper_change(struct net_device *dev,
>> + struct net_device *upper_dev,
>> + bool linking)
>> +{
>> + struct net_bridge *br = netdev_priv(dev);
>> +
>> + if (!br_vlan_is_bind_vlan_dev(upper_dev))
>> + return;
>> +
>> + if (linking) {
>> + br_vlan_set_vlan_dev_state(br, upper_dev);
>> + br_opt_toggle(br, BROPT_VLAN_BRIDGE_BINDING, true);
>> + } else {
>> + br_opt_toggle(br, BROPT_VLAN_BRIDGE_BINDING,
>> + br_vlan_has_upper_bind_vlan_dev(dev));
>> + }
>> +}
>> +
>> +/* Must be protected by RTNL. */
>> +void br_vlan_bridge_event(struct net_device *dev, unsigned long event,
>> + void *ptr)
>> +{
>> + struct netdev_notifier_changeupper_info *info;
>> +
>> + switch (event) {
>> + case NETDEV_CHANGEUPPER:
>> + info = ptr;
>> + br_vlan_upper_change(dev, info->upper_dev, info->linking);
>> + break;
>> + }
>> +}
>> +
>> +/* Must be protected by RTNL. */
>> +void br_vlan_port_event(struct net_bridge_port *p, unsigned long event)
>> +{
>> + if (!br_opt_get(p->br, BROPT_VLAN_BRIDGE_BINDING))
>> + return;
>> +
>> + switch (event) {
>> + case NETDEV_CHANGE:
>> + case NETDEV_DOWN:
>> + case NETDEV_UP:
>> + br_vlan_set_all_vlan_dev_state(p);
>> + break;
>> + }
>> +}
>>
Powered by blists - more mailing lists