netdev - Re: [RFC net-next 1/2] net: bridge: multicast: re-implement port multicast enable/disable functions

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]

Message-ID: <e1db249d-6b34-4bd6-beb8-e8754a773b71@blackwall.org>
Date: Wed, 27 Nov 2024 17:18:31 +0200
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Yong Wang <yongwang@...dia.com>, roopa@...dia.com, davem@...emloft.net,
 netdev@...r.kernel.org
Cc: aroulin@...dia.com, idosch@...dia.com, ndhar@...dia.com
Subject: Re: [RFC net-next 1/2] net: bridge: multicast: re-implement port
 multicast enable/disable functions

On 26/11/2024 23:34, Yong Wang wrote:
> Re-implement br_multicast_enable_port() / br_multicast_disable_port() to
> support per vlan multicast context enabling/disabling for bridge ports.
> The port state could be changed by STP, that impacts multicast behaviors
> like igmp query. The corresponding context should be used for per port
> context or per vlan context accordingly.
> 
> Signed-off-by: Yong Wang <yongwang@...dia.com>
> Reviewed-by: Andy Roulin <aroulin@...dia.com>
> ---
>  net/bridge/br_multicast.c | 75 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 67 insertions(+), 8 deletions(-)
> 

Hi,
A few comments below

> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index b2ae0d2434d2..8b23b0dc6129 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -2105,15 +2105,45 @@ static void __br_multicast_enable_port_ctx(struct net_bridge_mcast_port *pmctx)
>  	}
>  }
>  
> -void br_multicast_enable_port(struct net_bridge_port *port)
> +static void br_multicast_enable_port_ctx(struct net_bridge_mcast_port *pmctx)
>  {
> -	struct net_bridge *br = port->br;
> +	struct net_bridge *br = pmctx->port->br;
>  
>  	spin_lock_bh(&br->multicast_lock);
> -	__br_multicast_enable_port_ctx(&port->multicast_ctx);
> +	__br_multicast_enable_port_ctx(pmctx);
>  	spin_unlock_bh(&br->multicast_lock);
>  }
>  
> +void br_multicast_enable_port(struct net_bridge_port *port)
> +{
> +	struct net_bridge *br = port->br;
> +
> +	if (br_opt_get(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED)) {
> +		struct net_bridge_vlan_group *vg;
> +		struct net_bridge_vlan *vlan;
> +
> +		rcu_read_lock();
> +		vg = nbp_vlan_group_rcu(port);
> +		if (!vg) {
> +			rcu_read_unlock();
> +			return;
> +		}
> +
> +		/* iterate each vlan of the port, enable port_mcast_ctx per vlan
> +		 * when vlan is in allowed states.
> +		 */
> +		list_for_each_entry_rcu(vlan, &vg->vlan_list, vlist) {
> +			if ((vlan->priv_flags & BR_VLFLAG_MCAST_ENABLED) &&

This is racy, the flag is changed only under multicast_lock and should be used
only with the lock held. I'd suggest moving this check in br_multicast_enable_port_ctx()
after taking the lock where you should check if the context is a vlan one or not.

> +			    br_vlan_state_allowed(br_vlan_get_state(vlan), true))
> +				br_multicast_enable_port_ctx(&vlan->port_mcast_ctx);
> +		}
> +		rcu_read_unlock();
> +	} else {
> +		/* use the port's multicast context when vlan snooping is disabled */
> +		br_multicast_enable_port_ctx(&port->multicast_ctx);
> +	}
> +}
> +
>  static void __br_multicast_disable_port_ctx(struct net_bridge_mcast_port *pmctx)
>  {
>  	struct net_bridge_port_group *pg;
> @@ -2137,11 +2167,40 @@ static void __br_multicast_disable_port_ctx(struct net_bridge_mcast_port *pmctx)
>  	br_multicast_rport_del_notify(pmctx, del);
>  }
>  
> +static void br_multicast_disable_port_ctx(struct net_bridge_mcast_port *pmctx)
> +{
> +	struct net_bridge *br = pmctx->port->br;
> +
> +	spin_lock_bh(&br->multicast_lock);
> +	__br_multicast_disable_port_ctx(pmctx);
> +	spin_unlock_bh(&br->multicast_lock);
> +}
> +
>  void br_multicast_disable_port(struct net_bridge_port *port)
>  {
> -	spin_lock_bh(&port->br->multicast_lock);
> -	__br_multicast_disable_port_ctx(&port->multicast_ctx);
> -	spin_unlock_bh(&port->br->multicast_lock);
> +	struct net_bridge *br = port->br;
> +
> +	if (br_opt_get(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED)) {

These blocks in enable and disable are almost identical, maybe make
a single helper called _toggle (similar to vlan mcast snooping toggle)
with a on/off bool argument and call the appropriate function based on it.

> +		struct net_bridge_vlan_group *vg;
> +		struct net_bridge_vlan *vlan;
> +
> +		rcu_read_lock();
> +		vg = nbp_vlan_group_rcu(port);
> +		if (!vg) {
> +			rcu_read_unlock();
> +			return;
> +		}
> +
> +		/* iterate each vlan of the port, disable port_mcast_ctx per vlan */
> +		list_for_each_entry(vlan, &vg->vlan_list, vlist) {
> +			if (vlan->priv_flags & BR_VLFLAG_MCAST_ENABLED)

Same comment about the flag check being racy.

> +				br_multicast_disable_port_ctx(&vlan->port_mcast_ctx);
> +		}
> +		rcu_read_unlock();
> +	} else {
> +		/* use the port's multicast context when vlan snooping is disabled */
> +		br_multicast_disable_port_ctx(&port->multicast_ctx);
> +	}
>  }
>  
>  static int __grp_src_delete_marked(struct net_bridge_port_group *pg)
> @@ -4304,9 +4363,9 @@ int br_multicast_toggle_vlan_snooping(struct net_bridge *br, bool on,
>  		__br_multicast_open(&br->multicast_ctx);
>  	list_for_each_entry(p, &br->port_list, list) {
>  		if (on)
> -			br_multicast_disable_port(p);
> +			br_multicast_disable_port_ctx(&p->multicast_ctx);
>  		else
> -			br_multicast_enable_port(p);
> +			br_multicast_enable_port_ctx(&p->multicast_ctx);
>  	}
>  
>  	list_for_each_entry(vlan, &vg->vlan_list, vlist)