| Message ID | 20220902134111.280657-1-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fe2c9c61f668cde28dac2b188028c5299cedcc1e |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: mvpp2: debugfs: fix memory leak when using debugfs_lookup() | expand |
Hello: This patch was applied to netdev/net.git (master) by David S. Miller <davem@davemloft.net>: 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 <mw@semihalf.com> > Cc: Russell King <linux@armlinux.org.uk> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Paolo Abeni <pabeni@redhat.com> > Cc: netdev@vger.kernel.org > Cc: stable <stable@kernel.org> > Fixes: 21da57a23125 ("net: mvpp2: add a debugfs interface for the Header Parser") > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > [...] 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!
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.
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
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?
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);
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 <mw@semihalf.com> Cc: Russell King <linux@armlinux.org.uk> Cc: "David S. Miller" <davem@davemloft.net> Cc: Eric Dumazet <edumazet@google.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Paolo Abeni <pabeni@redhat.com> Cc: netdev@vger.kernel.org Cc: stable <stable@kernel.org> Fixes: 21da57a23125 ("net: mvpp2: add a debugfs interface for the Header Parser") Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/net/ethernet/marvell/mvpp2/mvpp2_debugfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)