When error'ing out from phy_device_create(), the previously kzalloc'd "dev"
pointer gets overwritten with an error pointer, without freeing it
beforehand, thus leaking the allocated phy_device. Add the missing kfree
back.
Fixes: d02cbc461361 ("net: phy: fix memory leak in device-create error path")
Signed-off-by: Maxime Chevallier <[email protected]>
---
drivers/net/phy/phy_device.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3611ea64875e..2b4d04e3d479 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -711,6 +711,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
if (ret) {
put_device(&mdiodev->dev);
+ kfree(dev);
dev = ERR_PTR(ret);
}
--
2.43.2
On Fri, Feb 23, 2024 at 05:01:54PM +0100, Maxime Chevallier wrote:
> When error'ing out from phy_device_create(), the previously kzalloc'd "dev"
> pointer gets overwritten with an error pointer, without freeing it
> beforehand, thus leaking the allocated phy_device. Add the missing kfree
> back.
>
> Fixes: d02cbc461361 ("net: phy: fix memory leak in device-create error path")
No, it doesn't fix anything.
Sadly, this is the second patch that I've received recently which shows
a complete lack of understanding of the driver model, so I suspect
someone has documented something as a task, and that documentation is
either incomplete, or basically wrong.
In this case:
/* We allocate the device, and initialize the default values */
dev = kzalloc(sizeof(*dev), GFP_KERNEL);
if (!dev)
return ERR_PTR(-ENOMEM);
mdiodev = &dev->mdio;
..
device_initialize(&mdiodev->dev);
This sets the reference count on dev->mdio.dev to '1', and means that
at _this_ point, "dev" becomes a refcounted object. device_initialize()
is documented thusly:
/**
* device_initialize - init device structure.
* @dev: device.
*
* This prepares the device for use by other layers by initializing
* its fields.
..
* NOTE: Use put_device() to give up your reference instead of freeing
* @dev directly once you have called this function.
*/
Now, the error path does this:
if (ret) {
put_device(&mdiodev->dev);
dev = ERR_PTR(ret);
}
which is (a) compliant with the device_initialize() documentation, and
(b) will drop the reference count of '1' down to '0' resulting in the
release function being called - and it is the responsibility of the
release function to free the memory.
Adding a kfree() in this path will lead to a double-kfree() of the
allocated memory, and that is _incorrect_.
So, given that this is the second such instance of someone wanting to
incorrectly kfree() a structure after a call to device_initialize(),
can I please ask everyone who reads this message, and who receives a
patch like this to _please_ not assume that it is correct, and check
it _very_ _carefully_.
Can I also ask those who propose to send out such patches _also_ do
the due dilligence and check this before creating noise.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Fri, 23 Feb 2024 17:01:54 +0100
Maxime Chevallier <[email protected]> wrote:
> When error'ing out from phy_device_create(), the previously kzalloc'd "dev"
> pointer gets overwritten with an error pointer, without freeing it
> beforehand, thus leaking the allocated phy_device. Add the missing kfree
> back.
Disregard , I immediatly realised that this was freed in
phy_device_release in our case. Sorry about the noise.
Maxime
On Fri, Feb 23, 2024 at 05:06:07PM +0100, Maxime Chevallier wrote:
> On Fri, 23 Feb 2024 17:01:54 +0100
> Maxime Chevallier <[email protected]> wrote:
>
> > When error'ing out from phy_device_create(), the previously kzalloc'd "dev"
> > pointer gets overwritten with an error pointer, without freeing it
> > beforehand, thus leaking the allocated phy_device. Add the missing kfree
> > back.
>
> Disregard , I immediatly realised that this was freed in
> phy_device_release in our case. Sorry about the noise.
Sorry your emails came in in reverse order.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!