| 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: <0cf800e8bb28116fce7466cacbabde395abfac4f.camel@redhat.com>
Date: Fri, 18 Mar 2022 17:50:54 +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 16:45 +0100, Niels Dossche wrote:
> On 18/03/2022 16:42, Paolo Abeni wrote:
> > On Fri, 2022-03-18 at 13:48 +0100, Paolo Abeni wrote:
> > > 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...
> >
> > For the records, I mean something alongside the following -
> > completely
> > not tested:
> > ---
> > diff --git a/include/net/if_inet6.h b/include/net/if_inet6.h
> > index 4cfdef6ca4f6..2df3c98b9e55 100644
> > --- a/include/net/if_inet6.h
> > +++ b/include/net/if_inet6.h
> > @@ -64,6 +64,7 @@ struct inet6_ifaddr {
> >
> > struct hlist_node addr_lst;
> > struct list_head if_list;
> > + 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 b22504176588..27d1081b693e 100644
> > --- a/net/ipv6/addrconf.c
> > +++ b/net/ipv6/addrconf.c
> > @@ -797,6 +797,7 @@ static void dev_forward_change(struct inet6_dev
> > *idev)
> > {
> > struct net_device *dev;
> > struct inet6_ifaddr *ifa;
> > + LIST_HEAD(tmp);
> >
> > if (!idev)
> > return;
> > @@ -815,9 +816,17 @@ static void dev_forward_change(struct
> > inet6_dev *idev)
> > }
> > }
> >
> > + rcu_read_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);
> > + }
> > + rcu_read_unlock();
> > +
> > + while (!list_empty(&tmp)) {
> > + ifa = list_first_entry(&tmp, struct inet6_ifaddr,
> > if_list_aux);
> > + list_del(&ifa->if_list_aux);
> > if (idev->cnf.forwarding)
> > addrconf_join_anycast(ifa);
> > else
> >
>
> I see, nice small change.
> Only thing I notice is that list_for_each_entry_rcu should be used
> instead of list_for_each_entry inside the rcu lock, right?
Yes you are right. Or you can replace rcu_read_lock() with
write_lock_bh(&idev->lock).
Probably we need some commend nearby if_list_aux definition alike "used
to safely traverse the idev address list in process context, see
dev_forward_change"
Cheers,
Paolo
Powered by blists - more mailing lists