netdev - Re: [PATCH net-next v2] Avoid traversing addrconf hash on ifdown

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: <daef8c89-a27b-492f-935e-60fd55718841@linux.dev>
Date: Sun, 10 Nov 2024 22:31:17 +0000
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Gilad Naaman <gnaaman@...venets.com>
Cc: davem@...emloft.net, dsahern@...nel.org, edumazet@...gle.com,
 horms@...nel.org, kuba@...nel.org, kuniyu@...zon.com,
 netdev@...r.kernel.org, pabeni@...hat.com
Subject: Re: [PATCH net-next v2] Avoid traversing addrconf hash on ifdown

On 10/11/2024 06:53, Gilad Naaman wrote:
>>> -		spin_unlock_bh(&net->ipv6.addrconf_hash_lock);
>>> +	list_for_each_entry(ifa, &idev->addr_list, if_list) {
>>> +		addrconf_del_dad_work(ifa);
>>> +
>>> +		/* combined flag + permanent flag decide if
>>> +		 * address is retained on a down event
>>> +		 */
>>> +		if (!keep_addr ||
>>> +		    !(ifa->flags & IFA_F_PERMANENT) ||
>>> +		    addr_is_local(&ifa->addr))
>>> +			hlist_del_init_rcu(&ifa->addr_lst);
>>>    	}
>>>    
>>> +	spin_unlock(&net->ipv6.addrconf_hash_lock);
>>> +	read_unlock_bh(&idev->lock);
>>
>> Why is this read lock needed here? spinlock addrconf_hash_lock will
>> block any RCU grace period to happen, so we can safely traverse
>> idev->addr_list with list_for_each_entry_rcu()...
> 
> Oh, sorry, I didn't realize the hash lock encompasses this one;
> although it seems obvious in retrospect.
> 
>>> +
>>>    	write_lock_bh(&idev->lock);
>>
>> if we are trying to protect idev->addr_list against addition, then we
>> have to extend write_lock scope. Otherwise it may happen that another
>> thread will grab write lock between read_unlock and write_lock.
>>
>> Am I missing something?
> 
> I wanted to ensure that access to `idev->addr_list` is performed under lock,
> the same way it is done immediately afterwards;
> No particular reason not to extend the existing lock, I just didn't think
> about it.
> 
> For what it's worth, the original code didn't have this protection either,
> since the another thread could have grabbed the lock between
> `spin_unlock_bh(&net->ipv6.addrconf_hash_lock);` of the last loop iteration,
> and the `write_lock`.
> 
> Should I extend the write_lock upwards, or just leave it off?

Well, you are doing write manipulation with the list, which is protected
by read-write lock. I would expect this lock to be held in write mode. 
And you have to protect hash map at the same time. So yes, write_lock
and spin_lock altogether, I believe.

> 
> Thank you for your time,
> Gilad