| 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
| ||
|
Date: Fri, 18 Mar 2022 10:13:52 +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 v2] ipv6: acquire write lock for addr_list in
dev_forward_change
On Thu, 2022-03-17 at 16:56 +0100, Niels Dossche wrote:
> No path towards dev_forward_change (common ancestor of paths is in
> addrconf_fixup_forwarding) acquires idev->lock for idev->addr_list.
> We need to hold the lock during the whole loop in dev_forward_change.
> __ipv6_dev_ac_{inc,dec} both acquire the write lock on idev->lock in
> their function body. Since addrconf_{join,leave}_anycast call to
> __ipv6_dev_ac_inc and __ipv6_dev_ac_dec respectively, we need to move
> the responsibility of locking upwards.
>
> This patch moves the locking up. For __ipv6_dev_ac_dec, there is one
> place where the caller can directly acquire the idev->lock, that is in
> ipv6_dev_ac_dec. The other caller is addrconf_leave_anycast, which now
> needs to be called under idev->lock, and thus it becomes the
> responsibility of the callers of addrconf_leave_anycast to hold that
> lock. For __ipv6_dev_ac_inc, there are also 2 callers, one is
> ipv6_sock_ac_join, which can acquire the lock during the call to
> __ipv6_dev_ac_inc. The other caller is addrconf_join_anycast, which now
> needs to be called under idev->lock, and thus it becomes the
> responsibility of the callers of addrconf_join_anycast to hold that
> lock.
>
> Signed-off-by: Niels Dossche <dossche.niels@...il.com>
> ---
>
> Changes in v2:
> - Move the locking upwards
>
> net/ipv6/addrconf.c | 21 ++++++++++++++++-----
> net/ipv6/anycast.c | 37 ++++++++++++++++---------------------
> 2 files changed, 32 insertions(+), 26 deletions(-)
>
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index f908e2fd30b2..69e9f81e2045 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -818,6 +818,7 @@ static void dev_forward_change(struct inet6_dev *idev)
> }
> }
>
> + write_lock_bh(&idev->lock);
> list_for_each_entry(ifa, &idev->addr_list, if_list) {
> if (ifa->flags&IFA_F_TENTATIVE)
> continue;
> @@ -826,6 +827,7 @@ static void dev_forward_change(struct inet6_dev *idev)
> else
> addrconf_leave_anycast(ifa);
> }
> + write_unlock_bh(&idev->lock);
> inet6_netconf_notify_devconf(dev_net(dev), RTM_NEWNETCONF,
> NETCONFA_FORWARDING,
> dev->ifindex, &idev->cnf);
> @@ -2191,7 +2193,7 @@ void addrconf_leave_solict(struct inet6_dev *idev, const struct in6_addr *addr)
> __ipv6_dev_mc_dec(idev, &maddr);
> }
>
> -/* caller must hold RTNL */
> +/* caller must hold RTNL and write lock idev->lock */
> static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
> {
> struct in6_addr addr;
> @@ -2204,7 +2206,7 @@ static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
> __ipv6_dev_ac_inc(ifp->idev, &addr);
> }
>
> -/* caller must hold RTNL */
> +/* caller must hold RTNL and write lock idev->lock */
> static void addrconf_leave_anycast(struct inet6_ifaddr *ifp)
> {
> struct in6_addr addr;
> @@ -3857,8 +3859,11 @@ static int addrconf_ifdown(struct net_device *dev, bool unregister)
> __ipv6_ifa_notify(RTM_DELADDR, ifa);
> inet6addr_notifier_call_chain(NETDEV_DOWN, ifa);
> } else {
> - if (idev->cnf.forwarding)
> + if (idev->cnf.forwarding) {
> + write_lock_bh(&idev->lock);
> addrconf_leave_anycast(ifa);
> + write_unlock_bh(&idev->lock);
> + }
> addrconf_leave_solict(ifa->idev, &ifa->addr);
> }
>
> @@ -6136,16 +6141,22 @@ static void __ipv6_ifa_notify(int event, struct inet6_ifaddr *ifp)
> &ifp->addr, ifp->idev->dev->name);
> }
>
> - if (ifp->idev->cnf.forwarding)
> + if (ifp->idev->cnf.forwarding) {
> + write_lock_bh(&ifp->idev->lock);
> addrconf_join_anycast(ifp);
> + write_unlock_bh(&ifp->idev->lock);
> + }
> if (!ipv6_addr_any(&ifp->peer_addr))
> addrconf_prefix_route(&ifp->peer_addr, 128,
> ifp->rt_priority, ifp->idev->dev,
> 0, 0, GFP_ATOMIC);
> break;
> case RTM_DELADDR:
> - if (ifp->idev->cnf.forwarding)
> + if (ifp->idev->cnf.forwarding) {
> + write_lock_bh(&ifp->idev->lock);
> addrconf_leave_anycast(ifp);
> + write_unlock_bh(&ifp->idev->lock);
> + }
> addrconf_leave_solict(ifp->idev, &ifp->addr);
> if (!ipv6_addr_any(&ifp->peer_addr)) {
> struct fib6_info *rt;
> diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
> index dacdea7fcb62..f3017ed6f005 100644
> --- a/net/ipv6/anycast.c
> +++ b/net/ipv6/anycast.c
> @@ -136,7 +136,9 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
> goto error;
> }
>
> + write_lock_bh(&idev->lock);
> err = __ipv6_dev_ac_inc(idev, addr);
> + write_unlock_bh(&idev->lock);
I feat this is problematic, due this call chain:
__ipv6_dev_ac_inc() -> addrconf_join_solict() -> ipv6_dev_mc_inc ->
__ipv6_dev_mc_inc -> mutex_lock(&idev->mc_lock);
The latter call requires process context.
One alternarive (likely very hackish way) to solve this could be:
- adding another list entry into struct inet6_dev, rtnl protected.
- traverse addr_list under idev->lock and add each entry with
forwarding on to into a tmp list (e.g. tmp_join) using the field above;
add the entries with forwarding off into another tmp list (e.g.
tmp_leave), still using the same field.
- traverse the tmp_join list under rtnl lock only and do the
addrconf_join_anycast
- traverse the tmp_leave list under rtnl lock only and do
addrconf_leave_anycast
Side note: I'm wondering if we should complete the RCU conversion (e.g.
moving tempaddr_list to RCU and changing idev->lock). The current mixed
schema looks a bit confusing to me.
Cheers,
Paolo
Powered by blists - more mailing lists