netdev - Re: [PATCH net] ipv6: Handle race in addrconf_dad

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <8a53dcff-729f-aaea-b136-ff997cf43e08@gmail.com>
Date:   Mon, 30 Sep 2019 19:40:37 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     David Ahern <dsahern@...il.com>,
        Eric Dumazet <eric.dumazet@...il.com>,
        David Ahern <dsahern@...nel.org>, davem@...emloft.net,
        jakub.kicinski@...ronome.com
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net] ipv6: Handle race in addrconf_dad_work



On 9/30/19 7:23 PM, David Ahern wrote:
> On 9/30/19 8:01 PM, Eric Dumazet wrote:
>>

>>
>> Do we need to keep the test on IF_READY done later in this function ?
>>
>> If IF_READY can disappear only under RTNL, we might clean this.
>>
>> (unless addrconf_dad_work() releases rtnl and reacquires it)
> 
> Unless I am missing something none of the functions called by dad_work
> release the rtnl, but your comment did have me second guessing the locking.
> 
> The interesting cases for changing the idev flag are addrconf_notify
> (NETDEV_UP and NETDEV_CHANGE) and addrconf_ifdown (reset the flag). The
> former does not have the idev lock - only rtnl. The latter has both.
> Checking the flag is inconsistent with respect to locks.
> 
> As for your suggestion, the 'dead' flag is set only under rtnl in
> addrconf_ifdown and it means the device is getting removed (or IPv6 is
> disabled). Based on that I think the existing:
> 
> 	if (idev->dead || !(idev->if_flags & IF_READY))
> 		goto out;
> 
> can be moved to right after the rtnl_lock in addrconf_dad_work in place
> of the above change, so the end result is:
> 
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 6a576ff92c39..dd3be06d5a06 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -4032,6 +4032,12 @@ static void addrconf_dad_work(struct work_struct *w)
> 
>         rtnl_lock();
> 
> +       /* check if device was taken down before this delayed work
> +        * function could be canceled
> +        */
> +       if (idev->dead || !(idev->if_flags & IF_READY))
> +               goto out;
> +
>         spin_lock_bh(&ifp->lock);
>         if (ifp->state == INET6_IFADDR_STATE_PREDAD) {
>                 action = DAD_BEGIN;
> @@ -4077,11 +4083,6 @@ static void addrconf_dad_work(struct work_struct *w)
>                 goto out;
> 
>         write_lock_bh(&idev->lock);
> -       if (idev->dead || !(idev->if_flags & IF_READY)) {
> -               write_unlock_bh(&idev->lock);
> -               goto out;
> -       }
> -
>         spin_lock(&ifp->lock);
>         if (ifp->state == INET6_IFADDR_STATE_DEAD) {
>                 spin_unlock(&ifp->lock);
> 
> 
> agree?
> 

SGTM, thanks !