linux-kernel - Re: [net-next v2 08/11] net: bridge: mcast: split router port del+notify for mcast router split

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: <640d2ba9-9e56-a36c-3734-44d72d8a6512@nvidia.com>
Date:   Tue, 11 May 2021 12:19:59 +0300
From:   Nikolay Aleksandrov <nikolay@...dia.com>
To:     Linus Lüssing <linus.luessing@...3.blue>,
        netdev@...r.kernel.org
Cc:     Roopa Prabhu <roopa@...dia.com>, Jakub Kicinski <kuba@...nel.org>,
        "David S . Miller" <davem@...emloft.net>,
        bridge@...ts.linux-foundation.org, b.a.t.m.a.n@...ts.open-mesh.org,
        linux-kernel@...r.kernel.org
Subject: Re: [net-next v2 08/11] net: bridge: mcast: split router port
 del+notify for mcast router split

On 09/05/2021 22:45, Linus Lüssing wrote:
> In preparation for the upcoming split of multicast router state into
> their IPv4 and IPv6 variants split router port deletion and notification
> into two functions. When we disable a port for instance later we want to
> only send one notification to switchdev and netlink for compatibility
> and want to avoid sending one for IPv4 and one for IPv6. For that the
> split is needed.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@...3.blue>
> ---
>  net/bridge/br_multicast.c | 40 ++++++++++++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 839d21b..39854d5 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -60,7 +60,8 @@ static void br_ip4_multicast_leave_group(struct net_bridge *br,
>  					 const unsigned char *src);
>  static void br_multicast_port_group_rexmit(struct timer_list *t);
>  
> -static void __del_port_router(struct net_bridge_port *p);
> +static void
> +br_multicast_rport_del_notify(struct net_bridge_port *p, bool deleted);
>  #if IS_ENABLED(CONFIG_IPV6)
>  static void br_ip6_multicast_leave_group(struct net_bridge *br,
>  					 struct net_bridge_port *port,
> @@ -1354,11 +1355,26 @@ static int br_ip6_multicast_add_group(struct net_bridge *br,
>  }
>  #endif
>  
> +static bool br_multicast_rport_del(struct hlist_node *rlist)
> +{
> +	if (hlist_unhashed(rlist))
> +		return false;
> +
> +	hlist_del_init_rcu(rlist);
> +	return true;
> +}
> +
> +static inline bool br_ip4_multicast_rport_del(struct net_bridge_port *p)
> +{
> +	return br_multicast_rport_del(&p->ip4_rlist);
> +}
> +

Same comment about inline in .c files, either drop the inline or move it to br_private.h
For functions that are not critical for performance(fast-path) I'd just drop the inline
in the .c files and leave them there.

>  static void br_multicast_router_expired(struct net_bridge_port *port,
>  					struct timer_list *t,
>  					struct hlist_node *rlist)
>  {
>  	struct net_bridge *br = port->br;
> +	bool del;
>  
>  	spin_lock(&br->multicast_lock);
>  	if (port->multicast_router == MDB_RTR_TYPE_DISABLED ||
> @@ -1366,7 +1382,8 @@ static void br_multicast_router_expired(struct net_bridge_port *port,
>  	    timer_pending(t))
>  		goto out;
>  
> -	__del_port_router(port);
> +	del = br_multicast_rport_del(rlist);
> +	br_multicast_rport_del_notify(port, del);
>  out:
>  	spin_unlock(&br->multicast_lock);
>  }
> @@ -1706,19 +1723,20 @@ void br_multicast_disable_port(struct net_bridge_port *port)
>  	struct net_bridge *br = port->br;
>  	struct net_bridge_port_group *pg;
>  	struct hlist_node *n;
> +	bool del = false;
>  
>  	spin_lock(&br->multicast_lock);
>  	hlist_for_each_entry_safe(pg, n, &port->mglist, mglist)
>  		if (!(pg->flags & MDB_PG_FLAGS_PERMANENT))
>  			br_multicast_find_del_pg(br, pg);
>  
> -	__del_port_router(port);
> -
> +	del |= br_ip4_multicast_rport_del(port);
>  	del_timer(&port->ip4_mc_router_timer);
>  	del_timer(&port->ip4_own_query.timer);
>  #if IS_ENABLED(CONFIG_IPV6)
>  	del_timer(&port->ip6_own_query.timer);
>  #endif
> +	br_multicast_rport_del_notify(port, del);
>  	spin_unlock(&br->multicast_lock);
>  }
>  
> @@ -3508,11 +3526,12 @@ int br_multicast_set_router(struct net_bridge *br, unsigned long val)
>  	return err;
>  }
>  
> -static void __del_port_router(struct net_bridge_port *p)
> +static void
> +br_multicast_rport_del_notify(struct net_bridge_port *p, bool deleted)
>  {
> -	if (hlist_unhashed(&p->ip4_rlist))
> +	if (!deleted)
>  		return;
> -	hlist_del_init_rcu(&p->ip4_rlist);
> +
>  	br_rtr_notify(p->br->dev, p, RTM_DELMDB);
>  	br_port_mc_router_state_change(p, false);
>  
> @@ -3526,6 +3545,7 @@ int br_multicast_set_port_router(struct net_bridge_port *p, unsigned long val)
>  	struct net_bridge *br = p->br;
>  	unsigned long now = jiffies;
>  	int err = -EINVAL;
> +	bool del = false;
>  
>  	spin_lock(&br->multicast_lock);
>  	if (p->multicast_router == val) {
> @@ -3539,12 +3559,14 @@ int br_multicast_set_port_router(struct net_bridge_port *p, unsigned long val)
>  	switch (val) {
>  	case MDB_RTR_TYPE_DISABLED:
>  		p->multicast_router = MDB_RTR_TYPE_DISABLED;
> -		__del_port_router(p);
> +		del |= br_ip4_multicast_rport_del(p);
>  		del_timer(&p->ip4_mc_router_timer);
> +		br_multicast_rport_del_notify(p, del);
>  		break;
>  	case MDB_RTR_TYPE_TEMP_QUERY:
>  		p->multicast_router = MDB_RTR_TYPE_TEMP_QUERY;
> -		__del_port_router(p);
> +		del |= br_ip4_multicast_rport_del(p);
> +		br_multicast_rport_del_notify(p, del);
>  		break;
>  	case MDB_RTR_TYPE_PERM:
>  		p->multicast_router = MDB_RTR_TYPE_PERM;
>