When calling debugfs_lookup() the result must have dput() called on it,
otherwise the memory will leak over time. Fix this up to be much
simpler logic and only create the root debugfs directory once when the
driver is first accessed. That resolves the memory leak and makes
things more obvious as to what the intent is.
Cc: Marcin Wojtas <[email protected]>
Cc: Russell King <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: [email protected]
Cc: stable <[email protected]>
Fixes: 21da57a23125 ("net: mvpp2: add a debugfs interface for the Header Parser")
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c
index 4a3baa7e0142..0eec05d905eb 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c
@@ -700,10 +700,10 @@ void mvpp2_dbgfs_cleanup(struct mvpp2 *priv)
void mvpp2_dbgfs_init(struct mvpp2 *priv, const char *name)
{
- struct dentry *mvpp2_dir, *mvpp2_root;
+ static struct dentry *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);
--
2.37.3
Hello:
This patch was applied to netdev/net.git (master)
by David S. Miller <[email protected]>:
On Fri, 2 Sep 2022 15:41:11 +0200 you wrote:
> When calling debugfs_lookup() the result must have dput() called on it,
> otherwise the memory will leak over time. Fix this up to be much
> simpler logic and only create the root debugfs directory once when the
> driver is first accessed. That resolves the memory leak and makes
> things more obvious as to what the intent is.
>
> Cc: Marcin Wojtas <[email protected]>
> Cc: Russell King <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Eric Dumazet <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Paolo Abeni <[email protected]>
> Cc: [email protected]
> Cc: stable <[email protected]>
> Fixes: 21da57a23125 ("net: mvpp2: add a debugfs interface for the Header Parser")
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
>
> [...]
Here is the summary with links:
- [net] net: mvpp2: debugfs: fix memory leak when using debugfs_lookup()
https://git.kernel.org/netdev/net/c/fe2c9c61f668
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
On Fri, Sep 02, 2022 at 03:41:11PM +0200, Greg Kroah-Hartman wrote:
> When calling debugfs_lookup() the result must have dput() called on it,
> otherwise the memory will leak over time. Fix this up to be much
> simpler logic and only create the root debugfs directory once when the
> driver is first accessed. That resolves the memory leak and makes
> things more obvious as to what the intent is.
To clarify a bit more on the original patch rather than one of the
backported stable patches of this.
This patch introduces a bug, whereby if the driver is a module, and
is inserted, binds to a device, then is removed and re-inserted,
mvpp2_root will be NULL on the first call to mvpp2_dbgfs_init(),
so we will attempt to call debugfs_create_dir(). However, the
directory was already previously created, so this will fail, and
mvpp2_root will be the EEXIST error pointer.
Since we never clean up this directory, the original code does NOT
result in a memory leak - since the increase in refcount caused by
debugfs_lookup() has absolutely no effect - because we never remove
this directory once it's been created.
If the driver /did/ remove the directory when the module is removed,
then yes, maybe there's an argument for this fix. However, as things
currently stand, this is in no way a fix, but actually introduces a
debugfs regression.
Please can the change be reverted in mainline and all stable trees.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Sep 14, 2022 at 08:03:08PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Sep 13, 2022 at 05:55:52PM +0100, Russell King (Oracle) wrote:
> > On Fri, Sep 02, 2022 at 03:41:11PM +0200, Greg Kroah-Hartman wrote:
> > > When calling debugfs_lookup() the result must have dput() called on it,
> > > otherwise the memory will leak over time. Fix this up to be much
> > > simpler logic and only create the root debugfs directory once when the
> > > driver is first accessed. That resolves the memory leak and makes
> > > things more obvious as to what the intent is.
> >
> > To clarify a bit more on the original patch rather than one of the
> > backported stable patches of this.
> >
> > This patch introduces a bug, whereby if the driver is a module, and
> > is inserted, binds to a device, then is removed and re-inserted,
> > mvpp2_root will be NULL on the first call to mvpp2_dbgfs_init(),
> > so we will attempt to call debugfs_create_dir(). However, the
> > directory was already previously created, so this will fail, and
> > mvpp2_root will be the EEXIST error pointer.
> >
> > Since we never clean up this directory, the original code does NOT
> > result in a memory leak - since the increase in refcount caused by
> > debugfs_lookup() has absolutely no effect - because we never remove
> > this directory once it's been created.
> >
> > If the driver /did/ remove the directory when the module is removed,
> > then yes, maybe there's an argument for this fix. However, as things
> > currently stand, this is in no way a fix, but actually introduces a
> > debugfs regression.
> >
> > Please can the change be reverted in mainline and all stable trees.
>
> I never considered the 'rmmod the driver and then load it again' as a
> valid thing to worry about. And I doubt that many others would either :)
>
> Given that the current code does NOT clean up when it is removed, I
> assumed that no one cared abou this, but yes, it is crazy but the
> current code does work, but it leaks a dentry. I'll send a follow-on
> patch to do this "correctly" when I return from the Plumbers conference
> next week.
>
> But for now, this patch is correct, and does not leak memory anymore
> like the code without this change currently does, so I think it should
> stay.
Please can you explain which memory isn't leaked as a result of the
patch?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Tue, Sep 13, 2022 at 05:55:52PM +0100, Russell King (Oracle) wrote:
> On Fri, Sep 02, 2022 at 03:41:11PM +0200, Greg Kroah-Hartman wrote:
> > When calling debugfs_lookup() the result must have dput() called on it,
> > otherwise the memory will leak over time. Fix this up to be much
> > simpler logic and only create the root debugfs directory once when the
> > driver is first accessed. That resolves the memory leak and makes
> > things more obvious as to what the intent is.
>
> To clarify a bit more on the original patch rather than one of the
> backported stable patches of this.
>
> This patch introduces a bug, whereby if the driver is a module, and
> is inserted, binds to a device, then is removed and re-inserted,
> mvpp2_root will be NULL on the first call to mvpp2_dbgfs_init(),
> so we will attempt to call debugfs_create_dir(). However, the
> directory was already previously created, so this will fail, and
> mvpp2_root will be the EEXIST error pointer.
>
> Since we never clean up this directory, the original code does NOT
> result in a memory leak - since the increase in refcount caused by
> debugfs_lookup() has absolutely no effect - because we never remove
> this directory once it's been created.
>
> If the driver /did/ remove the directory when the module is removed,
> then yes, maybe there's an argument for this fix. However, as things
> currently stand, this is in no way a fix, but actually introduces a
> debugfs regression.
>
> Please can the change be reverted in mainline and all stable trees.
I never considered the 'rmmod the driver and then load it again' as a
valid thing to worry about. And I doubt that many others would either :)
Given that the current code does NOT clean up when it is removed, I
assumed that no one cared abou this, but yes, it is crazy but the
current code does work, but it leaks a dentry. I'll send a follow-on
patch to do this "correctly" when I return from the Plumbers conference
next week.
But for now, this patch is correct, and does not leak memory anymore
like the code without this change currently does, so I think it should
stay.
thanks,
greg k-h