| 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
| ||
|
Message-ID: <a24b13ea10ca898bb003300084039b459d553f6d.camel@redhat.com>
Date: Fri, 18 Mar 2022 13:48:25 +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 Fri, 2022-03-18 at 10:13 +0100, Paolo Abeni wrote:
> 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.
Typo above: the new field should be added to 'struct inet6_ifaddr'.
> - 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.
Again confusing text above, sorry. As the forwarding flag is per
device, all the addr entries will land into the same tmp list.
It's probably better if I sketch up some code...
/P
Powered by blists - more mailing lists