Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register
From: Dan Carpenter <hidden>
Date: 2021-09-28 08:56:41
Also in:
lkml
On Sun, Sep 26, 2021 at 12:53:13PM +0800, Yanfei Xu wrote:
Once device_register() failed, we should call put_device() to decrement reference count for cleanup. Or it will cause memory leak.
[ snipped stack trace ]
quoted hunk ↗ jump to hunk
Reported-by: syzbot+398e7dc692ddbbb4cfec@syzkaller.appspotmail.com Signed-off-by: Yanfei Xu <redacted> --- drivers/net/phy/mdio_bus.c | 1 + 1 file changed, 1 insertion(+)diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c index 53f034fc2ef7..6f4b4e5df639 100644 --- a/drivers/net/phy/mdio_bus.c +++ b/drivers/net/phy/mdio_bus.c@@ -537,6 +537,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner) err = device_register(&bus->dev); if (err) { pr_err("mii_bus %s failed to register\n", bus->id); + put_device(&bus->dev);
No this isn't right. The dev.class is &mdio_bus_class. It's set a few lines earlier. bus->dev.class = &mdio_bus_class; The release function is mdiobus_release(). It will free bus and lead to use after frees in the callers. Look at greth_mdio_init(). There are a lot of callers which will crash now. This patch was a clear layering violation. If you didn't allocate "bus" then you should not free it. Keeping that rule in mind can help prevent future bugs. Also he other error handling paths are careful not to call put_device() so why would this one be special? It should have stood out that this one error path is the only place that doesn't use a goto to clean up. I don't have a solution. I have commented before that I hate kobjects for this reason because they lead to unfixable memory leaks during probe. But this leak will only happen with fault injection and so it doesn't affect real life. And even if it did, a leak it preferable to a crash. Unfortunately, this patch has already been applied so please can you send a patch to revert it? regards, dan carpenter