netdev - Re: [PATCH net-next 01/14] net: bridge: multicast: Propagate br_mc_disabled

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <89a9019b-2adc-1806-c893-297e603a73c9@gmail.com>
Date:   Thu, 17 Jan 2019 11:27:18 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Ido Schimmel <idosch@...lanox.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Jiri Pirko <jiri@...lanox.com>,
        "ilias.apalodimas@...aro.org" <ilias.apalodimas@...aro.org>,
        "ivan.khoronzhuk@...aro.org" <ivan.khoronzhuk@...aro.org>,
        "roopa@...ulusnetworks.com" <roopa@...ulusnetworks.com>,
        "nikolay@...ulusnetworks.com" <nikolay@...ulusnetworks.com>
Subject: Re: [PATCH net-next 01/14] net: bridge: multicast: Propagate
 br_mc_disabled_update() return

On 1/17/19 5:47 AM, Ido Schimmel wrote:
> On Wed, Jan 16, 2019 at 12:00:49PM -0800, Florian Fainelli wrote:
>> Some Ethernet switches might not be able to support disabling multicast
>> flooding globally when e.g: several bridges span the same physical
>> device, propagate the return value of br_mc_disabled_update() such that
>> this propagates correctly to user-space.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
>> ---
>>  net/bridge/br_multicast.c | 19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
>> index 3aeff0895669..09fc92541873 100644
>> --- a/net/bridge/br_multicast.c
>> +++ b/net/bridge/br_multicast.c
>> @@ -813,7 +813,7 @@ static void br_ip6_multicast_port_query_expired(struct timer_list *t)
>>  }
>>  #endif
>>  
>> -static void br_mc_disabled_update(struct net_device *dev, bool value)
>> +static int br_mc_disabled_update(struct net_device *dev, bool value)
>>  {
>>  	struct switchdev_attr attr = {
>>  		.orig_dev = dev,
>> @@ -822,11 +822,13 @@ static void br_mc_disabled_update(struct net_device *dev, bool value)
>>  		.u.mc_disabled = !value,
>>  	};
>>  
>> -	switchdev_port_attr_set(dev, &attr);
>> +	return switchdev_port_attr_set(dev, &attr);
> 
> Did you test this for veth? If switchdev ops are not defined, this
> returns -EOPNOTSUPP. See __br_vlan_filter_toggle() for reference

I did not, good catch, I will add a check on -EOPNOTSUPP and silence
that error code:

if (err && err != -ENOTSUPP)
	return err;

> 
>>  }
>>  
>>  int br_multicast_add_port(struct net_bridge_port *port)
>>  {
>> +	int ret;
>> +
>>  	port->multicast_router = MDB_RTR_TYPE_TEMP_QUERY;
>>  
>>  	timer_setup(&port->multicast_router_timer,
>> @@ -837,8 +839,11 @@ int br_multicast_add_port(struct net_bridge_port *port)
>>  	timer_setup(&port->ip6_own_query.timer,
>>  		    br_ip6_multicast_port_query_expired, 0);
>>  #endif
>> -	br_mc_disabled_update(port->dev,
>> -			      br_opt_get(port->br, BROPT_MULTICAST_ENABLED));
>> +	ret = br_mc_disabled_update(port->dev,
>> +				    br_opt_get(port->br,
>> +					       BROPT_MULTICAST_ENABLED));
>> +	if (ret)
>> +		return ret;
>>  
>>  	port->mcast_stats = netdev_alloc_pcpu_stats(struct bridge_mcast_stats);
>>  	if (!port->mcast_stats)
>> @@ -1937,12 +1942,16 @@ static void br_multicast_start_querier(struct net_bridge *br,
>>  int br_multicast_toggle(struct net_bridge *br, unsigned long val)
>>  {
>>  	struct net_bridge_port *port;
>> +	int err;
>>  
>>  	spin_lock_bh(&br->multicast_lock);
>>  	if (!!br_opt_get(br, BROPT_MULTICAST_ENABLED) == !!val)
>>  		goto unlock;
>>  
>> -	br_mc_disabled_update(br->dev, val);
>> +	err = br_mc_disabled_update(br->dev, val);
>> +	if (err)
>> +		goto unlock;
>> +
>>  	br_opt_toggle(br, BROPT_MULTICAST_ENABLED, !!val);
>>  	if (!br_opt_get(br, BROPT_MULTICAST_ENABLED))
>>  		goto unlock;
> 
> You never return the error

Huh, indeed, thanks!
-- 
Florian