| Message ID | E1ofOAB-00CzkG-UO@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0152dfee235e87660f52a117fc9f70dc55956bb4 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: mvpp2: fix mvpp2 debugfs leak | expand |
On Mon, Oct 03, 2022 at 05:19:27PM +0100, Russell King (Oracle) wrote: > When mvpp2 is unloaded, the driver specific debugfs directory is not > removed, which technically leads to a memory leak. However, this > directory is only created when the first device is probed, so the > hardware is present. Removing the module is only something a developer > would to when e.g. testing out changes, so the module would be > reloaded. So this memory leak is minor. > > The original attempt in commit fe2c9c61f668 ("net: mvpp2: debugfs: fix > memory leak when using debugfs_lookup()") that was labelled as a memory > leak fix was not, it fixed a refcount leak, but in doing so created a > problem when the module is reloaded - the directory already exists, but > mvpp2_root is NULL, so we lose all debugfs entries. This fix has been > reverted. > > This is the alternative fix, where we remove the offending directory > whenever the driver is unloaded. > > Fixes: 21da57a23125 ("net: mvpp2: add a debugfs interface for the Header Parser") > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Thanks for fixing this up properly. greg k-h
pon., 3 paź 2022 o 18:22 Greg Kroah-Hartman <gregkh@linuxfoundation.org> napisał(a): > > On Mon, Oct 03, 2022 at 05:19:27PM +0100, Russell King (Oracle) wrote: > > When mvpp2 is unloaded, the driver specific debugfs directory is not > > removed, which technically leads to a memory leak. However, this > > directory is only created when the first device is probed, so the > > hardware is present. Removing the module is only something a developer > > would to when e.g. testing out changes, so the module would be > > reloaded. So this memory leak is minor. > > > > The original attempt in commit fe2c9c61f668 ("net: mvpp2: debugfs: fix > > memory leak when using debugfs_lookup()") that was labelled as a memory > > leak fix was not, it fixed a refcount leak, but in doing so created a > > problem when the module is reloaded - the directory already exists, but > > mvpp2_root is NULL, so we lose all debugfs entries. This fix has been > > reverted. > > > > This is the alternative fix, where we remove the offending directory > > whenever the driver is unloaded. > > > > Fixes: 21da57a23125 ("net: mvpp2: add a debugfs interface for the Header Parser") > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Thanks for fixing this up properly. > I tested the patch and everything works as expected, thanks! Reviewed-by: Marcin Wojtas <mw@semihalf.com> Best regards, Marcin
Hello: This patch was applied to netdev/net.git (master) by Jakub Kicinski <kuba@kernel.org>: On Mon, 03 Oct 2022 17:19:27 +0100 you wrote: > When mvpp2 is unloaded, the driver specific debugfs directory is not > removed, which technically leads to a memory leak. However, this > directory is only created when the first device is probed, so the > hardware is present. Removing the module is only something a developer > would to when e.g. testing out changes, so the module would be > reloaded. So this memory leak is minor. > > [...] Here is the summary with links: - [net] net: mvpp2: fix mvpp2 debugfs leak https://git.kernel.org/netdev/net/c/0152dfee235e You are awesome, thank you!
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h index ad73a488fc5f..11e603686a27 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h @@ -1530,6 +1530,7 @@ u32 mvpp2_read(struct mvpp2 *priv, u32 offset); void mvpp2_dbgfs_init(struct mvpp2 *priv, const char *name); void mvpp2_dbgfs_cleanup(struct mvpp2 *priv); +void mvpp2_dbgfs_exit(void); void mvpp23_rx_fifo_fc_en(struct mvpp2 *priv, int port, bool en); diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c index 4a3baa7e0142..75e83ea2a926 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c @@ -691,6 +691,13 @@ static int mvpp2_dbgfs_port_init(struct dentry *parent, return 0; } +static struct dentry *mvpp2_root; + +void mvpp2_dbgfs_exit(void) +{ + debugfs_remove(mvpp2_root); +} + void mvpp2_dbgfs_cleanup(struct mvpp2 *priv) { debugfs_remove_recursive(priv->dbgfs_dir); @@ -700,10 +707,9 @@ void mvpp2_dbgfs_cleanup(struct mvpp2 *priv) void mvpp2_dbgfs_init(struct mvpp2 *priv, const char *name) { - struct dentry *mvpp2_dir, *mvpp2_root; + struct dentry *mvpp2_dir; int ret, i; - mvpp2_root = debugfs_lookup(MVPP2_DRIVER_NAME, NULL); if (!mvpp2_root) mvpp2_root = debugfs_create_dir(MVPP2_DRIVER_NAME, NULL); diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c index b84128b549b4..eaa51cd7456b 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c @@ -7706,7 +7706,18 @@ static struct platform_driver mvpp2_driver = { }, }; -module_platform_driver(mvpp2_driver); +static int __init mvpp2_driver_init(void) +{ + return platform_driver_register(&mvpp2_driver); +} +module_init(mvpp2_driver_init); + +static void __exit mvpp2_driver_exit(void) +{ + platform_driver_unregister(&mvpp2_driver); + mvpp2_dbgfs_exit(); +} +module_exit(mvpp2_driver_exit); MODULE_DESCRIPTION("Marvell PPv2 Ethernet Driver - www.marvell.com"); MODULE_AUTHOR("Marcin Wojtas <mw@semihalf.com>");
When mvpp2 is unloaded, the driver specific debugfs directory is not removed, which technically leads to a memory leak. However, this directory is only created when the first device is probed, so the hardware is present. Removing the module is only something a developer would to when e.g. testing out changes, so the module would be reloaded. So this memory leak is minor. The original attempt in commit fe2c9c61f668 ("net: mvpp2: debugfs: fix memory leak when using debugfs_lookup()") that was labelled as a memory leak fix was not, it fixed a refcount leak, but in doing so created a problem when the module is reloaded - the directory already exists, but mvpp2_root is NULL, so we lose all debugfs entries. This fix has been reverted. This is the alternative fix, where we remove the offending directory whenever the driver is unloaded. Fixes: 21da57a23125 ("net: mvpp2: add a debugfs interface for the Header Parser") Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 1 + drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c | 10 ++++++++-- drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 13 ++++++++++++- 3 files changed, 21 insertions(+), 3 deletions(-)