| Message ID | 20240223160155.861528-1-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: phy: phy_device: free the phy_device on the phy_device_create error path | expand |
On Fri, 23 Feb 2024 17:01:54 +0100 Maxime Chevallier <maxime.chevallier@bootlin.com> 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: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.
On Fri, Feb 23, 2024 at 05:06:07PM +0100, Maxime Chevallier wrote: > On Fri, 23 Feb 2024 17:01:54 +0100 > Maxime Chevallier <maxime.chevallier@bootlin.com> 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.
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); }
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 <maxime.chevallier@bootlin.com> --- drivers/net/phy/phy_device.c | 1 + 1 file changed, 1 insertion(+)