Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Mon, 4 Apr 2022 14:47:58 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Niels Dossche <dossche.niels@gmail.com>
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        "David S. Miller" <davem@davemloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
        David Ahern <dsahern@kernel.org>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>
Subject: Re: [PATCH net-next v2] ipv6: fix locking issues with loops over
 idev->addr_list
Message-ID: <Ykro/s5+kd+po26e@lunn.ch>
References: <20220403231523.45843-1-dossche.niels@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220403231523.45843-1-dossche.niels@gmail.com>
Precedence: bulk

On Mon, Apr 04, 2022 at 01:15:24AM +0200, 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.

Hi Niels

What sort of issues could the race result in?

I've been chasing a netdev reference leak, when using the GNS3
simulator. Shutting down the system can result in one interface having
a netdev reference count of 5, and it never gets destroyed. Using the
tracker code Eric recently added, i found one of the leaks is idev,
its reference count does not go to 0, and hence the reference it holds
on the netdev is never released.

I will test this patch out, see if it helps, but i'm just wondering if
you think the issue i'm seeing is theoretically possible because of
this race? If it is, we might want this applied to stable, not just
net-next.

Thanks
	Andrew