netdev - Re: [PATCH 1/7] phy: fix of_mdio_find

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150921193206.GU21084@n2100.arm.linux.org.uk>
Date:	Mon, 21 Sep 2015 20:32:07 +0100
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	David Miller <davem@...emloft.net>
Cc:	f.fainelli@...il.com, devicetree@...r.kernel.org,
	frowand.list@...il.com, grant.likely@...aro.org,
	isubramanian@....com, kchudgar@....com,
	linux-arm-kernel@...ts.infradead.org,
	linuxppc-dev@...ts.ozlabs.org, leoli@...escale.com,
	michal.simek@...inx.com, netdev@...r.kernel.org, rric@...nel.org,
	robh+dt@...nel.org, soren.brinkmann@...inx.com,
	sgoutham@...ium.com, thomas.petazzoni@...e-electrons.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/7] phy: fix of_mdio_find_bus() device refcount leak

On Mon, Sep 21, 2015 at 12:01:59PM -0700, David Miller wrote:
> From: Russell King <rmk+kernel@....linux.org.uk>
> Date: Fri, 18 Sep 2015 10:54:55 +0100
> 
> > Update the comment, and arrange for the only user of this function
> > to drop this refcount when disposing of a reference to it.
> 
> mdio_mux is not the only user of of_mdio_find_bus(), DSA uses it as
> well.
> 
> So if anything this commit message is inaccurate.

Yes, I missed that as it wasn't under drivers/net.  It doesn't change
the validity of this patch, the existing code is wrong and I'm not
introducing anything that makes the code any more wrong than it is.

I'll fix the commit message, and I'll fix the DSA code too but in a
separate patch.  Thanks for pointing it out.

> I also wonder about this refcounting scheme.

It's the standard driver model refcounting rules that we've lived with
for about a decade, ever since the driver model was introduced by
Patrick Mochel.

> If you are going to drop the inner device reference, then we take the
> mdio bus returned from of_mdio_find_bus() what holds onto it and keeps
> it from disappearing on us?
> 
> Don't we have to hold onto some reference count of some kind here?

In the case of the mdio mux code, I'm dropping the reference when
either (a) we've encountered an error during initialisation and we're
cleaning up, or (b) when the mdio mux code is being torn down after
the mdiomux bus has been unregistered and freed.  In both cases, we're
done with the mdio bus that was returned from of_mdio_find_bus().

In case (a), the devres code will release the kmalloc'd memory when
mdio_mux_gpio_probe() or mdio_mux_mmioreg_probe() propagates the error
out of their probe() function.

I'm not sure why you think anything is wrong here - maybe it's the odd
code structure to the success path at the bottom of mdio_mux_init()?

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html