netdev - Re: [PATCH net-next v3 05/15] net: bridge: mcast: factor out port group del

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: <89eb1120-5776-081e-52ce-1ef92f41ecbe@cumulusnetworks.com>
Date:   Mon, 7 Sep 2020 00:29:10 +0300
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, roopa@...dia.com,
        bridge@...ts.linux-foundation.org, davem@...emloft.net
Subject: Re: [PATCH net-next v3 05/15] net: bridge: mcast: factor out port
 group del

On 9/6/20 11:56 PM, Jakub Kicinski wrote:
> On Sat,  5 Sep 2020 11:24:00 +0300 Nikolay Aleksandrov wrote:
>> @@ -843,24 +843,11 @@ static int __br_mdb_del(struct net_bridge *br, struct br_mdb_entry *entry)
>>   		if (!p->port || p->port->dev->ifindex != entry->ifindex)
>>   			continue;
>>   
>> -		if (!hlist_empty(&p->src_list)) {
>> -			err = -EINVAL;
>> -			goto unlock;
>> -		}
>> -
>>   		if (p->port->state == BR_STATE_DISABLED)
>>   			goto unlock;
>>   
>> -		__mdb_entry_fill_flags(entry, p->flags);
> 
> Just from staring at the code it's unclear why the list_empty() check
> and this __mdb_entry_fill_flags() are removed as well.
> 

The hlist_empty check is added by patch 01 temporarily for correctness.
Otherwise I'd have to edit all open-coded pg del places and add src delete
code and then remove it here.

__mdb_entry_fill_flags() are called by __mdb_fill_info() which is the only
function used to fill an mdb both for dumping and notifications after patch 08.

>> -		rcu_assign_pointer(*pp, p->next);
>> -		hlist_del_init(&p->mglist);
>> -		del_timer(&p->timer);
>> -		kfree_rcu(p, rcu);
>> +		br_multicast_del_pg(mp, p, pp);
>>   		err = 0;
>> -
>> -		if (!mp->ports && !mp->host_joined &&
>> -		    netif_running(br->dev))
>> -			mod_timer(&mp->timer, jiffies);
>>   		break;
> 
> 
>> +void br_multicast_del_pg(struct net_bridge_mdb_entry *mp,
>> +			 struct net_bridge_port_group *pg,
>> +			 struct net_bridge_port_group __rcu **pp)
>> +{
>> +	struct net_bridge *br = pg->port->br;
>> +	struct net_bridge_group_src *ent;
>> +	struct hlist_node *tmp;
>> +
>> +	rcu_assign_pointer(*pp, pg->next);
>> +	hlist_del_init(&pg->mglist);
>> +	del_timer(&pg->timer);
>> +	hlist_for_each_entry_safe(ent, tmp, &pg->src_list, node)
>> +		br_multicast_del_group_src(ent);
>> +	br_mdb_notify(br->dev, pg->port, &pg->addr, RTM_DELMDB, pg->flags);
>> +	kfree_rcu(pg, rcu);
>> +
>> +	if (!mp->ports && !mp->host_joined && netif_running(br->dev))
>> +		mod_timer(&mp->timer, jiffies);
>> +}
> 
>> @@ -1641,16 +1647,7 @@ br_multicast_leave_group(struct net_bridge *br,
>>   			if (p->flags & MDB_PG_FLAGS_PERMANENT)
>>   				break;
>>   
>> -			rcu_assign_pointer(*pp, p->next);
>> -			hlist_del_init(&p->mglist);
>> -			del_timer(&p->timer);
>> -			kfree_rcu(p, rcu);
>> -			br_mdb_notify(br->dev, port, group, RTM_DELMDB,
>> -				      p->flags | MDB_PG_FLAGS_FAST_LEAVE);
> 
> And here we'll loose MDB_PG_FLAGS_FAST_LEAVE potentially?
> 

Good catch, we will lose the fast leave indeed.
This is a 1 line change to set the flag before notifying. Would you prefer
me to send a v4 or a follow up for it?

Thanks,
  Nik

>> -			if (!mp->ports && !mp->host_joined &&
>> -			    netif_running(br->dev))
>> -				mod_timer(&mp->timer, jiffies);
>> +			br_multicast_del_pg(mp, p, pp);