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 09:20:55 +0100 From: Paolo Abeni <pabeni@...hat.com> To: Niels Dossche <dossche.niels@...il.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 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> 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. Side note: AFAICS after this patch, there is still the suspicious tempaddr_list usage in addrconf_ifdown to be handled. Cheers, Paolo
Powered by blists - more mailing lists