[PATCH v2] mtd: spi-nor: fix memory leak when using debugfs_lookup()

[PATCH v2] mtd: spi-nor: fix memory leak when using debugfs_lookup()

[Greg Kroah-Hartman]

When calling debugfs_lookup() the result must have dput() called on it,
otherwise the memory will leak over time.

Cc: Tudor Ambarus <tudor.ambarus@microchip.com>
Cc: Pratyush Yadav <pratyush@kernel.org>
Cc: Michael Walle <michael@walle.cc>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Vignesh Raghavendra <vigneshr@ti.com>
Cc: linux-mtd@lists.infradead.org
Cc: stable <stable@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v2: fix up to work when module is removed and added, making the fix
    much simpler.

 drivers/mtd/spi-nor/debugfs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/spi-nor/debugfs.c b/drivers/mtd/spi-nor/debugfs.c
index ff895f6758ea..af41fbc09a97 100644
--- a/drivers/mtd/spi-nor/debugfs.c
+++ b/drivers/mtd/spi-nor/debugfs.c
@@ -242,6 +242,7 @@ void spi_nor_debugfs_register(struct spi_nor *nor)
 
 	d = debugfs_create_dir(dev_name(nor->dev), rootdir);
 	nor->debugfs_root = d;
+	dput(rootdir);
 
 	debugfs_create_file("params", 0444, d, nor, &spi_nor_params_fops);
 	debugfs_create_file("capabilities", 0444, d, nor,
-- 
2.39.1

Re: [PATCH v2] mtd: spi-nor: fix memory leak when using debugfs_lookup()

[Michael Walle]

Am 2023-02-08 13:57, schrieb Greg Kroah-Hartman:
> When calling debugfs_lookup() the result must have dput() called on it,
> otherwise the memory will leak over time.
> 
> Cc: Tudor Ambarus <tudor.ambarus@microchip.com>
> Cc: Pratyush Yadav <pratyush@kernel.org>
> Cc: Michael Walle <michael@walle.cc>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: Vignesh Raghavendra <vigneshr@ti.com>
> Cc: linux-mtd@lists.infradead.org
> Cc: stable <stable@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> v2: fix up to work when module is removed and added, making the fix
>     much simpler.
> 
>  drivers/mtd/spi-nor/debugfs.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mtd/spi-nor/debugfs.c 
> b/drivers/mtd/spi-nor/debugfs.c
> index ff895f6758ea..af41fbc09a97 100644
> --- a/drivers/mtd/spi-nor/debugfs.c
> +++ b/drivers/mtd/spi-nor/debugfs.c
> @@ -242,6 +242,7 @@ void spi_nor_debugfs_register(struct spi_nor *nor)
> 
>  	d = debugfs_create_dir(dev_name(nor->dev), rootdir);
>  	nor->debugfs_root = d;
> +	dput(rootdir);

rootdir might either be the return value of debugfs_lookup() or
debugfs_create_dir(). dput() is probably wrong for the latter,
right? Also there is an early return, where the dput() is missing,
too.

-michael

Re: [PATCH v2] mtd: spi-nor: fix memory leak when using debugfs_lookup()

[Greg Kroah-Hartman]

On Wed, Feb 08, 2023 at 02:36:23PM +0100, Michael Walle wrote:
> Am 2023-02-08 13:57, schrieb Greg Kroah-Hartman:
> > When calling debugfs_lookup() the result must have dput() called on it,
> > otherwise the memory will leak over time.
> > 
> > Cc: Tudor Ambarus <tudor.ambarus@microchip.com>
> > Cc: Pratyush Yadav <pratyush@kernel.org>
> > Cc: Michael Walle <michael@walle.cc>
> > Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> > Cc: Richard Weinberger <richard@nod.at>
> > Cc: Vignesh Raghavendra <vigneshr@ti.com>
> > Cc: linux-mtd@lists.infradead.org
> > Cc: stable <stable@kernel.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > v2: fix up to work when module is removed and added, making the fix
> >     much simpler.
> > 
> >  drivers/mtd/spi-nor/debugfs.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/mtd/spi-nor/debugfs.c
> > b/drivers/mtd/spi-nor/debugfs.c
> > index ff895f6758ea..af41fbc09a97 100644
> > --- a/drivers/mtd/spi-nor/debugfs.c
> > +++ b/drivers/mtd/spi-nor/debugfs.c
> > @@ -242,6 +242,7 @@ void spi_nor_debugfs_register(struct spi_nor *nor)
> > 
> >  	d = debugfs_create_dir(dev_name(nor->dev), rootdir);
> >  	nor->debugfs_root = d;
> > +	dput(rootdir);
> 
> rootdir might either be the return value of debugfs_lookup() or
> debugfs_create_dir(). dput() is probably wrong for the latter,
> right? Also there is an early return, where the dput() is missing,
> too.

{sigh}

Yeah, this is all wrong, sorry.  Let me fix this up again, properly.
And to do it properly, let's have the module remove the directory if it
is unloaded, like a good module should :)

greg k-h

Re: [PATCH v2] mtd: spi-nor: fix memory leak when using debugfs_lookup()

[Michael Walle]

Am 2023-02-08 15:12, schrieb Greg Kroah-Hartman:
> On Wed, Feb 08, 2023 at 02:36:23PM +0100, Michael Walle wrote:
>> Am 2023-02-08 13:57, schrieb Greg Kroah-Hartman:
>> > When calling debugfs_lookup() the result must have dput() called on it,
>> > otherwise the memory will leak over time.
>> >
>> > Cc: Tudor Ambarus <tudor.ambarus@microchip.com>
>> > Cc: Pratyush Yadav <pratyush@kernel.org>
>> > Cc: Michael Walle <michael@walle.cc>
>> > Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>> > Cc: Richard Weinberger <richard@nod.at>
>> > Cc: Vignesh Raghavendra <vigneshr@ti.com>
>> > Cc: linux-mtd@lists.infradead.org
>> > Cc: stable <stable@kernel.org>
>> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> > ---
>> > v2: fix up to work when module is removed and added, making the fix
>> >     much simpler.
>> >
>> >  drivers/mtd/spi-nor/debugfs.c | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/drivers/mtd/spi-nor/debugfs.c
>> > b/drivers/mtd/spi-nor/debugfs.c
>> > index ff895f6758ea..af41fbc09a97 100644
>> > --- a/drivers/mtd/spi-nor/debugfs.c
>> > +++ b/drivers/mtd/spi-nor/debugfs.c
>> > @@ -242,6 +242,7 @@ void spi_nor_debugfs_register(struct spi_nor *nor)
>> >
>> >  	d = debugfs_create_dir(dev_name(nor->dev), rootdir);
>> >  	nor->debugfs_root = d;
>> > +	dput(rootdir);
>> 
>> rootdir might either be the return value of debugfs_lookup() or
>> debugfs_create_dir(). dput() is probably wrong for the latter,
>> right? Also there is an early return, where the dput() is missing,
>> too.
> 
> {sigh}
> 
> Yeah, this is all wrong, sorry.  Let me fix this up again, properly.
> And to do it properly, let's have the module remove the directory if it
> is unloaded, like a good module should :)

There were some complications. IIRC you'd need to do reference counting,
to determine whether you are the last user of the rootdir. Other subsys
create an empty rootdir in their .init(). But that was hard to do in 
MTD.
Again memory is hazy.. Therefore, I resorted to create it on the fly if
there isn't already one.

Maybe you got some better/simpler idea :)

-michael

Re: [PATCH v2] mtd: spi-nor: fix memory leak when using debugfs_lookup()

[Greg Kroah-Hartman]

On Wed, Feb 08, 2023 at 03:24:31PM +0100, Michael Walle wrote:
> Am 2023-02-08 15:12, schrieb Greg Kroah-Hartman:
> > On Wed, Feb 08, 2023 at 02:36:23PM +0100, Michael Walle wrote:
> > > Am 2023-02-08 13:57, schrieb Greg Kroah-Hartman:
> > > > When calling debugfs_lookup() the result must have dput() called on it,
> > > > otherwise the memory will leak over time.
> > > >
> > > > Cc: Tudor Ambarus <tudor.ambarus@microchip.com>
> > > > Cc: Pratyush Yadav <pratyush@kernel.org>
> > > > Cc: Michael Walle <michael@walle.cc>
> > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > Cc: Richard Weinberger <richard@nod.at>
> > > > Cc: Vignesh Raghavendra <vigneshr@ti.com>
> > > > Cc: linux-mtd@lists.infradead.org
> > > > Cc: stable <stable@kernel.org>
> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > ---
> > > > v2: fix up to work when module is removed and added, making the fix
> > > >     much simpler.
> > > >
> > > >  drivers/mtd/spi-nor/debugfs.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/mtd/spi-nor/debugfs.c
> > > > b/drivers/mtd/spi-nor/debugfs.c
> > > > index ff895f6758ea..af41fbc09a97 100644
> > > > --- a/drivers/mtd/spi-nor/debugfs.c
> > > > +++ b/drivers/mtd/spi-nor/debugfs.c
> > > > @@ -242,6 +242,7 @@ void spi_nor_debugfs_register(struct spi_nor *nor)
> > > >
> > > >  	d = debugfs_create_dir(dev_name(nor->dev), rootdir);
> > > >  	nor->debugfs_root = d;
> > > > +	dput(rootdir);
> > > 
> > > rootdir might either be the return value of debugfs_lookup() or
> > > debugfs_create_dir(). dput() is probably wrong for the latter,
> > > right? Also there is an early return, where the dput() is missing,
> > > too.
> > 
> > {sigh}
> > 
> > Yeah, this is all wrong, sorry.  Let me fix this up again, properly.
> > And to do it properly, let's have the module remove the directory if it
> > is unloaded, like a good module should :)
> 
> There were some complications. IIRC you'd need to do reference counting,
> to determine whether you are the last user of the rootdir. Other subsys
> create an empty rootdir in their .init(). But that was hard to do in MTD.
> Again memory is hazy.. Therefore, I resorted to create it on the fly if
> there isn't already one.
> 
> Maybe you got some better/simpler idea :)

Yup, just do it normally like other drivers, I'll send a v3 now.

thanks,

greg k-h