[PATCH 5.19 005/192] net: mvpp2: debugfs: fix memory leak when using debugfs_lookup()

[PATCH 5.19 005/192] net: mvpp2: debugfs: fix memory leak when using debugfs_lookup()

[Greg Kroah-Hartman]

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

commit fe2c9c61f668cde28dac2b188028c5299cedcc1e upstream.

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: David S. Miller <davem@davemloft.net>
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(-)

--- 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 *p
 
 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);
 

Re: [PATCH 5.19 005/192] net: mvpp2: debugfs: fix memory leak when using debugfs_lookup()

[Russell King (Oracle)]

On Tue, Sep 13, 2022 at 04:01:51PM +0200, Greg Kroah-Hartman wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> commit fe2c9c61f668cde28dac2b188028c5299cedcc1e upstream.
> 
> 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: David S. Miller <davem@davemloft.net>
> 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(-)
> 
> --- 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 *p
>  
>  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);

This looks broken to me.

What happens if this is built as a module, and the module is loaded,
binds (and creates the directory), then is removed, and then re-
inserted?  Nothing removes the old directory, so doesn't
debugfs_create_dir() fail, resulting in subsequent failure to add
any subsequent debugfs entries?

I don't think this patch should be backported to stable trees until
this point is addressed.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 5.19 005/192] net: mvpp2: debugfs: fix memory leak when using debugfs_lookup()

[Greg Kroah-Hartman]

On Tue, Sep 13, 2022 at 05:48:58PM +0100, Russell King (Oracle) wrote:
> On Tue, Sep 13, 2022 at 04:01:51PM +0200, Greg Kroah-Hartman wrote:
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > 
> > commit fe2c9c61f668cde28dac2b188028c5299cedcc1e upstream.
> > 
> > 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: David S. Miller <davem@davemloft.net>
> > 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(-)
> > 
> > --- 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 *p
> >  
> >  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);
> 
> This looks broken to me.
> 
> What happens if this is built as a module, and the module is loaded,
> binds (and creates the directory), then is removed, and then re-
> inserted?  Nothing removes the old directory, so doesn't
> debugfs_create_dir() fail, resulting in subsequent failure to add
> any subsequent debugfs entries?
> 
> I don't think this patch should be backported to stable trees until
> this point is addressed.

I'll drop this for now and get back to the fixup next week.

thanks,

greg k-h