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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 23 Mar 2022 13:56:02 +0100 From: Niels Dossche <dossche.niels@...il.com> To: Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org Cc: "David S. Miller" <davem@...emloft.net>, Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>, David Ahern <dsahern@...nel.org>, Jakub Kicinski <kuba@...nel.org> Subject: Re: [PATCH] ipv6: fix locking issues with loops over idev->addr_list On 23/03/2022 09:20, Paolo Abeni wrote: > On Tue, 2022-03-22 at 22:34 +0100, Niels Dossche wrote: >> idev->addr_list needs to be protected by idev->lock. However, it is not >> always possible to do so while iterating and performing actions on >> inet6_ifaddr instances. For example, multiple functions (like >> addrconf_{join,leave}_anycast) eventually call down to other functions >> that acquire the idev->lock. The current code temporarily unlocked the >> idev->lock during the loops, which can cause race conditions. Moving the >> locks up is also not an appropriate solution as the ordering of lock >> acquisition will be inconsistent with for example mc_lock. >> >> This solution adds an additional field to inet6_ifaddr that is used >> to temporarily add the instances to a temporary list while holding >> idev->lock. The temporary list can then be traversed without holding >> idev->lock. This change was done in two places. In addrconf_ifdown, the >> list_for_each_entry_safe variant of the list loop is also no longer >> necessary as there is no deletion within that specific loop. >> >> The remaining loop in addrconf_ifdown that unlocks idev->lock in its >> loop body is of no issue. This is because that loop always gets the >> first entry and performs the delete and condition check under the >> idev->lock. >> >> Suggested-by: Paolo Abeni <pabeni@...hat.com> >> Signed-off-by: Niels Dossche <dossche.niels@...il.com> >> --- >> >> This was previously discussed in the mailing thread of >> [PATCH v2] ipv6: acquire write lock for addr_list in dev_forward_change >> >> include/net/if_inet6.h | 7 +++++++ >> net/ipv6/addrconf.c | 29 +++++++++++++++++++++++------ >> 2 files changed, 30 insertions(+), 6 deletions(-) >> >> diff --git a/include/net/if_inet6.h b/include/net/if_inet6.h >> index f026cf08a8e8..a17f29f75e9a 100644 >> --- a/include/net/if_inet6.h >> +++ b/include/net/if_inet6.h >> @@ -64,6 +64,13 @@ struct inet6_ifaddr { >> >> struct hlist_node addr_lst; >> struct list_head if_list; >> + /* >> + * Used to safely traverse idev->addr_list in process context >> + * if the idev->lock needed to protect idev->addr_list cannot be held. >> + * In that case, add the items to this list temporarily and iterate >> + * without holding idev->lock. See addrconf_ifdown and dev_forward_change. >> + */ >> + struct list_head if_list_aux; >> >> struct list_head tmp_list; >> struct inet6_ifaddr *ifpub; >> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c >> index f908e2fd30b2..72790d1934c7 100644 >> --- a/net/ipv6/addrconf.c >> +++ b/net/ipv6/addrconf.c >> @@ -800,6 +800,7 @@ static void dev_forward_change(struct inet6_dev *idev) >> { >> struct net_device *dev; >> struct inet6_ifaddr *ifa; >> + LIST_HEAD(tmp_addr_list); >> >> if (!idev) >> return; >> @@ -818,14 +819,23 @@ static void dev_forward_change(struct inet6_dev *idev) >> } >> } >> >> + read_lock_bh(&idev->lock); >> list_for_each_entry(ifa, &idev->addr_list, if_list) { >> if (ifa->flags&IFA_F_TENTATIVE) >> continue; >> + list_add_tail(&ifa->if_list_aux, &tmp_addr_list); >> + } >> + read_unlock_bh(&idev->lock); >> + >> + while (!list_empty(&tmp_addr_list)) { >> + ifa = list_first_entry(&tmp_addr_list, struct inet6_ifaddr, if_list_aux); >> + list_del(&ifa->if_list_aux); >> if (idev->cnf.forwarding) >> addrconf_join_anycast(ifa); >> else >> addrconf_leave_anycast(ifa); >> } >> + >> inet6_netconf_notify_devconf(dev_net(dev), RTM_NEWNETCONF, >> NETCONFA_FORWARDING, >> dev->ifindex, &idev->cnf); >> @@ -3730,10 +3740,11 @@ static int addrconf_ifdown(struct net_device *dev, bool unregister) >> unsigned long event = unregister ? NETDEV_UNREGISTER : NETDEV_DOWN; >> struct net *net = dev_net(dev); >> struct inet6_dev *idev; >> - struct inet6_ifaddr *ifa, *tmp; >> + struct inet6_ifaddr *ifa; >> bool keep_addr = false; >> bool was_ready; >> int state, i; >> + LIST_HEAD(tmp_addr_list); > > Very minot nit: I guess it's better to try to enforce the reverse x-mas > tree order for newly added variables - that is: this declaration should > me moved up, just after 'ifa'. > >> ASSERT_RTNL(); >> >> @@ -3822,16 +3833,23 @@ static int addrconf_ifdown(struct net_device *dev, bool unregister) >> write_lock_bh(&idev->lock); >> } >> >> - list_for_each_entry_safe(ifa, tmp, &idev->addr_list, if_list) { >> + list_for_each_entry(ifa, &idev->addr_list, if_list) { >> + list_add_tail(&ifa->if_list_aux, &tmp_addr_list); >> + } > > > Other minor nit: the braces are not required here. > > Otherwise LGTM: > > Acked-by: Paolo Abeni <pabeni@...hat.com> > I will apply the suggestions to the patch, thanks for the review. > However this looks like net-next material, and we are in the merge > window right now. I think you should re-post in (slighly less) than 2w. > Please add the target tree into the subj. > Okay, I'll send it for net-next when the merge window is closed. > Side note: AFAICS after this patch, there is still the suspicious > tempaddr_list usage in addrconf_ifdown to be handled. > > Cheers, > > Paolo > I have taken a look at that loop. I'm not sure that it is really problematic since the iteration and deletion is at least atomic under idev->lock. But I can write a patch for that as well. Cheers Niels
Powered by blists - more mailing lists