netdev - Re: [RFC net-next 2/2] net: bridge: multicast: update multicast contex when vlan state gets changed

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] [day] [month] [year] [list]

Message-ID: <4522c622-3e77-4191-8af6-f0ee8cd9061e@blackwall.org>
Date: Wed, 27 Nov 2024 17:26:39 +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 2/2] net: bridge: multicast: update multicast
 contex when vlan state gets changed

On 26/11/2024 23:34, Yong Wang wrote:
> Add br_vlan_set_state_finish() helper function to be executed right after
> br_vlan_set_state() when vlan state gets changed, similar to port state,
> vlan state could impact multicast behaviors as well such as igmp query.
> When bridge is running with userspace STP, vlan state can be manipulated by
> "bridge vlan" commands. Updating the corresponding multicast context
> will ensure the port query timer to continue when vlan state gets changed
> to those "allowed" states like "forwarding" etc.
> 
> Signed-off-by: Yong Wang <yongwang@...dia.com>
> Reviewed-by: Andy Roulin <aroulin@...dia.com>
> ---
>  net/bridge/br_mst.c          |  5 +++--
>  net/bridge/br_multicast.c    | 18 ++++++++++++++++++
>  net/bridge/br_private.h      | 11 +++++++++++
>  net/bridge/br_vlan_options.c |  2 ++
>  4 files changed, 34 insertions(+), 2 deletions(-)
> 

A few comments below,

> diff --git a/net/bridge/br_mst.c b/net/bridge/br_mst.c
> index 1820f09ff59c..b77c31a24257 100644
> --- a/net/bridge/br_mst.c
> +++ b/net/bridge/br_mst.c
> @@ -80,10 +80,11 @@ static void br_mst_vlan_set_state(struct net_bridge_vlan_group *vg,
>  	if (br_vlan_get_state(v) == state)
>  		return;
>  
> -	br_vlan_set_state(v, state);
> -
>  	if (v->vid == vg->pvid)
>  		br_vlan_set_pvid_state(vg, state);
> +
> +	br_vlan_set_state(v, state);
> +	br_vlan_set_state_finish(v, state);

This state_finish function is called after every instance of br_vlan_set_state(),
just add that call to br_vlan_set_state.

>  }
>  
>  int br_mst_set_state(struct net_bridge_port *p, u16 msti, u8 state,
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 8b23b0dc6129..3a3b63c97c92 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -4270,6 +4270,24 @@ static void __br_multicast_stop(struct net_bridge_mcast *brmctx)
>  #endif
>  }
>  
> +void br_multicast_update_vlan_mcast_ctx(struct net_bridge_vlan *v, u8 state)
> +{
> +	struct net_bridge *br;
> +
> +	if (!br_vlan_should_use(v))
> +		return;
> +
> +	if (br_vlan_is_master(v))
> +		return;
> +
> +	br = v->port->br;
> +
> +	if (br_vlan_state_allowed(state, true) &&
> +	    (v->priv_flags & BR_VLFLAG_MCAST_ENABLED) &&

checking this flag without mcast lock is racy.

> +	    br_opt_get(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED))

this should be the first check

> +		br_multicast_enable_port_ctx(&v->port_mcast_ctx);

What about disable? What if the state is != LEARNING/FORWARDING ?

> +}
> +
>  void br_multicast_toggle_one_vlan(struct net_bridge_vlan *vlan, bool on)
>  {
>  	struct net_bridge *br;
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 9853cfbb9d14..9c72070956e3 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -1052,6 +1052,7 @@ void br_multicast_port_ctx_init(struct net_bridge_port *port,
>  				struct net_bridge_vlan *vlan,
>  				struct net_bridge_mcast_port *pmctx);
>  void br_multicast_port_ctx_deinit(struct net_bridge_mcast_port *pmctx);
> +void br_multicast_update_vlan_mcast_ctx(struct net_bridge_vlan *v, u8 state);
>  void br_multicast_toggle_one_vlan(struct net_bridge_vlan *vlan, bool on);
>  int br_multicast_toggle_vlan_snooping(struct net_bridge *br, bool on,
>  				      struct netlink_ext_ack *extack);
> @@ -1502,6 +1503,10 @@ static inline void br_multicast_port_ctx_deinit(struct net_bridge_mcast_port *pm
>  {
>  }
>  
> +static inline void br_multicast_update_vlan_mcast_ctx(struct net_bridge_vlan *v, u8 state)
> +{
> +}
> +
>  static inline void br_multicast_toggle_one_vlan(struct net_bridge_vlan *vlan,
>  						bool on)
>  {
> @@ -1853,6 +1858,12 @@ bool br_vlan_global_opts_can_enter_range(const struct net_bridge_vlan *v_curr,
>  bool br_vlan_global_opts_fill(struct sk_buff *skb, u16 vid, u16 vid_range,
>  			      const struct net_bridge_vlan *v_opts);
>  
> +/* helper function to be called right after br_vlan_set_state() when vlan state gets changed */
> +static inline void br_vlan_set_state_finish(struct net_bridge_vlan *v, u8 state)
> +{

A one line helper that directly calls another function is not helping anything.
Please just call that function directly and remove the helper.

> +	br_multicast_update_vlan_mcast_ctx(v, state);
> +}
> +
>  /* vlan state manipulation helpers using *_ONCE to annotate lock-free access */
>  static inline u8 br_vlan_get_state(const struct net_bridge_vlan *v)
>  {
> diff --git a/net/bridge/br_vlan_options.c b/net/bridge/br_vlan_options.c
> index 8fa89b04ee94..bad187c4f16d 100644
> --- a/net/bridge/br_vlan_options.c
> +++ b/net/bridge/br_vlan_options.c
> @@ -123,6 +123,8 @@ static int br_vlan_modify_state(struct net_bridge_vlan_group *vg,
>  		br_vlan_set_pvid_state(vg, state);
>  
>  	br_vlan_set_state(v, state);
> +	br_vlan_set_state_finish(v, state);
> +
>  	*changed = true;
>  
>  	return 0;