netdev - Re: [PATCH] netpoll: Fix extra refcount release in netpoll

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: <20160324.145745.451518711282450835.davem@davemloft.net>
Date:	Thu, 24 Mar 2016 14:57:45 -0400 (EDT)
From:	David Miller <davem@...emloft.net>
To:	bhelgaas@...gle.com
Cc:	nikolay@...ulusnetworks.com, netdev@...r.kernel.org,
	nhorman@...hat.com, aduyck@...antis.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] netpoll: Fix extra refcount release in
 netpoll_cleanup()

From: Bjorn Helgaas <bhelgaas@...gle.com>
Date: Thu, 24 Mar 2016 11:13:34 -0500

> netpoll_setup() does a dev_hold() on np->dev, the netpoll device.  If it
> fails, it correctly does a dev_put() but leaves np->dev set.  If we call
> netpoll_cleanup() after the failure, np->dev is still set so we do another
> dev_put(), which decrements the refcount an extra time.
> 
> It's questionable to call netpoll_cleanup() after netpoll_setup() fails,
> but it can be difficult to find the problem, and we can easily avoid it in
> this case.  The extra decrements can lead to hangs like this:
> 
>   unregister_netdevice: waiting for bond0 to become free. Usage count = -3
> 
> In __netpoll_setup(), don't set np->dev until we know we're going to
> succeed.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@...gle.com>

The reason this bug exists is because the thing doing the reference
counting is separated from the thing that assigns the device pointer.

That's how this was allowed to happen.

If you instead do the np->dev = dev; assignment where the get is
performed, and do a np->dev = NULL; where the error path puts
the reference, everything is obvious and this error is unlikely to
be reintroduced.

So could you please implement your fix like that?

Thanks.