netdev - Re: [PATCH net-next v2 01/12] 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]

Date:   Wed, 30 Jan 2019 07:36:59 +0000
From:   Ido Schimmel <idosch@...lanox.com>
To:     Florian Fainelli <f.fainelli@...il.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 v2 01/12] net: bridge: multicast: Propagate
 br_mc_disabled_update() return

On Tue, Jan 29, 2019 at 04:55:37PM -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 | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 3aeff0895669..aff5e003d34f 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -813,20 +813,22 @@ 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,
>  		.id = SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED,
> -		.flags = SWITCHDEV_F_DEFER,
> +		.flags = SWITCHDEV_F_DEFER | SWITCHDEV_F_SKIP_EOPNOTSUPP,

Actually, since the operation is deferred I don't think the return value
from the driver is ever checked. Can you test it?

I think it would be good to convert the attributes to use the switchdev
notifier like commit d17d9f5e5143 ("switchdev: Replace port obj add/del
SDO with a notification") did for objects. Then you can have your
listener veto the operation in the same context it is happening.

>  		.u.mc_disabled = !value,
>  	};
>  
> -	switchdev_port_attr_set(dev, &attr);
> +	return switchdev_port_attr_set(dev, &attr);
>  }
>  
>  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 = 0;
>  
>  	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;
> @@ -1957,7 +1966,7 @@ int br_multicast_toggle(struct net_bridge *br, unsigned long val)
>  unlock:
>  	spin_unlock_bh(&br->multicast_lock);
>  
> -	return 0;
> +	return err;
>  }
>  
>  bool br_multicast_enabled(const struct net_device *dev)
> -- 
> 2.17.1
>