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

[Gilad Naaman <gnaaman@drivenets.com>]

> 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.
> 

Note that within the changed lines, the list itself is only iterated-on,
not manipulated.
The changes are to the `addr_lst` list, which is the hashtable, not the
list this lock protects.

I'll send v3 with the write-lock extended.
Thank you!