Linux Ethernet Bridging - Oct 2017 - [Bridge] [PATCH net-next v6 0/2] bridge: make setlink/dellink notifications more accurate

On 27/10/17 11:24, Toshiaki Makita wrote:
> On 2017/10/26 22:41, Nikolay Aleksandrov wrote:
>> Before this patch there was no way to tell if the vlan add operation
>> actually changed anything, thus we would always generate a notification
>> on adds. Let's make the notifications more precise and generate
them
>> only if anything changed, so use the new bool parameter to signal that
the
>> vlan was updated. We cannot return an error because there are valid use
>> cases that will be broken (e.g. overlapping range add) and also we
can't
>> risk masking errors due to calls into drivers for vlan add which can
>> potentially return anything.
>>
>> Signed-off-by: Nikolay Aleksandrov <nikolay at
cumulusnetworks.com>
> 
> Thanks for the explanation in the previous mail.
> 
>> ---
> ...
>> @@ -576,8 +592,11 @@ int br_vlan_add(struct net_bridge *br, u16 vid,
u16 flags)
>>  			refcount_inc(&vlan->refcnt);
>>  			vlan->flags |= BRIDGE_VLAN_INFO_BRENTRY;
>>  			vg->num_vlans++;
>> +			*changed = true;
>>  		}
>> -		__vlan_add_flags(vlan, flags);
>> +		if (__vlan_add_flags(vlan, flags))
>> +			*changed = true;
>> +
>>  		return 0;
>>  	}
>>  
>> @@ -601,6 +620,7 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16
flags)
>>  		free_percpu(vlan->stats);
>>  		kfree(vlan);
> 
> We should not set changed=true in this failure case?
> 
Right, I don't know what I was thinking, clearly I missed the fact that we
directly
clean up and return "ret" with an error. Very good catch, thanks! That
should
go in the else branch, will send a v6 later with that fixed up.

Thanks,
 Nik
>>  	}
>> +	*changed = true;
>>  
>>  	return ret;
>>  }
> ...
>> @@ -1050,6 +1083,7 @@ int nbp_vlan_add(struct net_bridge_port *port,
u16 vid, u16 flags)
>>  	ret = __vlan_add(vlan, flags);
>>  	if (ret)
>>  		kfree(vlan);
> 
> Likewise.
> 
>> +	*changed = true;
>>  
>>  	return ret;
>>  }
>>
>

Hi,
Before this set the bridge would generate a notification on vlan add or del
even if they didn't actually do any changes, which confuses listeners and
is generally not preferred. We could also lose notifications on actual
changes if one adds a range of vlans and there's an error in the middle.
The problem with just breaking and returning an error is that we could
break existing user-space scripts which rely on the vlan delete to clear
all existing entries in the specified range and ignore the non-existing
errors (typically used to clear the current vlan config).
So in order to make the notifications more accurate while keeping backwards
compatibility we add a boolean that tracks if anything actually changed
during the config calls.

The vlan add is more difficult to fix because it always returns 0 even if
nothing changed, but we cannot use a specific error because the drivers
can return anything and we may mask it, also we'd need to update all places
that directly return the add result, thus to signal that a vlan was created
or updated and in order not to break overlapping vlan range add we pass
down the new boolean that tracks changes to the add functions to check
if anything was actually updated.

v6: moved "changed" in else branch in br|nbp_vlan_add, thanks to
    Toshiaki Makita and retested everything again
v5: fix br_vlan_add return (v1 leftover) spotted by Toshiaki Makita
v4: set changed always to false in the non-vlan config case and retested
v3: rebased to latest net-next and fixed non-vlan config functions reported
    by kbuild test bot
v2: pass changed down to vlan add instead of masking errors

Thanks,
 Nik

Nikolay Aleksandrov (2):
  bridge: netlink: make setlink/dellink notifications more accurate
  bridge: vlan: signal if anything changed on vlan add

 net/bridge/br_netlink.c        | 51 ++++++++++++++++-----------
 net/bridge/br_netlink_tunnel.c | 14 +++++---
 net/bridge/br_private.h        | 14 +++++---
 net/bridge/br_private_tunnel.h |  3 +-
 net/bridge/br_vlan.c           | 78 ++++++++++++++++++++++++++++++------------
 5 files changed, 109 insertions(+), 51 deletions(-)

-- 
2.1.4