netdev - Re: [PATCH net-next 4/4] bridge: update vlan dev state when port added to or deleted from vlan

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 3 Apr 2019 18:44:31 +0100
From:   Mike Manning <mmanning@...tta.att-mail.com>
To:     Nikolay Aleksandrov <nikolay@...ulusnetworks.com>,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next 4/4] bridge: update vlan dev state when port
 added to or deleted from vlan

On 02/04/2019 21:10, Nikolay Aleksandrov wrote:
> On 02/04/2019 18:35, Mike Manning wrote:
>> If vlan bridge binding is enabled, then the link state of a vlan device
>> that is an upper device of the bridge should track the state of bridge
>> ports that are members of that vlan. So if a bridge port becomes or
>> stops being a member of a vlan, then update the link state of the
>> vlan device if necessary.
>>
>> Signed-off-by: Mike Manning <mmanning@...tta.att-mail.com>
>> ---
>>  net/bridge/br_vlan.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>> index 642373231386..7c11607cf1f4 100644
>> --- a/net/bridge/br_vlan.c
>> +++ b/net/bridge/br_vlan.c
>> @@ -7,6 +7,9 @@
>>  #include "br_private.h"
>>  #include "br_private_tunnel.h"
>>  
>> +static void nbp_vlan_set_vlan_dev_state(struct net_bridge_port *p,
>> +					struct net_bridge *br, u16 vid);
>> +
>>  static inline int br_vlan_cmp(struct rhashtable_compare_arg *arg,
>>  			      const void *ptr)
>>  {
>> @@ -294,6 +297,9 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags,
>>  
>>  	__vlan_add_list(v);
>>  	__vlan_add_flags(v, flags);
>> +
>> +	if (p)
>> +		nbp_vlan_set_vlan_dev_state(p, br, v->vid);
> since you need this as a last action after vlan_add and acts only on ports
> why not move it to nbp_vlan_add() ?
The call is in __vlan_add() rather than nbp_vlan_add() only for reasons
of consistency with the delete case, please see below.
>
>>  out:
>>  	return err;
>>  
>> @@ -358,6 +364,8 @@ static int __vlan_del(struct net_bridge_vlan *v)
>>  		rhashtable_remove_fast(&vg->vlan_hash, &v->vnode,
>>  				       br_vlan_rht_params);
>>  		__vlan_del_list(v);
>> +		if (p)
>> +			nbp_vlan_set_vlan_dev_state(p, p->br, v->vid);
> p is guaranteed to be set here, but also there's an if (p) earlier in the function.
> this can probably also move to nbp_vlan_delete if the other set_state moves to 
> nbp_vlan_add
>
Agreed re p being set here, I was just safeguarding against future
changes where this might not be true, but I will get rid of this.

The call is made here so as to be after the removal of the node from the
rhashtable.

The check is also needed as a result of calling nbp_vlan_flush(), hence
making the call here rather than in nbp_vlan_delete().

>>  		call_rcu(&v->rcu, nbp_vlan_rcu_free);
>>  	}
>>  
>> @@ -1357,6 +1365,21 @@ static void br_vlan_set_vlan_dev_state(struct net_bridge *br,
>>  	}
>>  }
>>  
>> +static void nbp_vlan_set_vlan_dev_state(struct net_bridge_port *p,
>> +					struct net_bridge *br, u16 vid)
>> +{
>> +	struct net_device *vlan_dev;
>> +
>> +	if (!br->vlan_bridge_binding)
>> +		return;
>> +
>> +	vlan_dev = br_vlan_get_upper_bind_vlan_dev(br->dev, vid);
>> +	if (vlan_dev) {
>> +		br_vlan_set_vlan_dev_state(br, vlan_dev);
>> +		dev_put(vlan_dev);
> I think this is running under rtnl.
Agreed, I will get rid of this and the other instance of this use.