diff mbox series

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

Message ID 20230208160230.2179905-1-gregkh@linuxfoundation.org
State New
Headers
Series [v4] mtd: spi-nor: fix memory leak when using debugfs_lookup() |

Commit Message

Greg KH Feb. 8, 2023, 4:02 p.m. UTC 
  When calling debugfs_lookup() the result must have dput() called on it,
otherwise the memory will leak over time.  To solve this, remove the
lookup and create the directory on the first device found, and then
remove it when the module is unloaded.

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
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v4: reverse the teardown order when the module is unloaded to ensure
    that the debugfs directory is still around when the device directory
    is removed.
v3: create the directory the first time it is used, and then remove it
    if it is present when the module is unloaded.  More complex than v2,
    but correct.
v2: fix up to work when module is removed and added, making the fix
    much simpler.
 drivers/mtd/spi-nor/core.c    | 14 +++++++++++++-
 drivers/mtd/spi-nor/core.h    |  2 ++
 drivers/mtd/spi-nor/debugfs.c | 12 +++++++++---
 3 files changed, 24 insertions(+), 4 deletions(-)

Comments

Michael Walle Feb. 8, 2023, 4:15 p.m. UTC | #1 
Am 2023-02-08 17:02, schrieb Greg Kroah-Hartman:
> When calling debugfs_lookup() the result must have dput() called on it,
> otherwise the memory will leak over time.  To solve this, remove the
> lookup and create the directory on the first device found, and then
> remove it when the module is unloaded.
> 
> 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
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Reviewed-by: Michael Walle <michael@walle.cc>

one nit below I didn't notice earlier, no need to send a new
patch version just for that.

..

> +void spi_nor_debugfs_shutdown(void)
> +{
> +	if (rootdir)
> +		debugfs_remove(rootdir);

debugfs_remove() already has a check for NULL.
Greg KH Feb. 8, 2023, 5:30 p.m. UTC | #2 
On Wed, Feb 08, 2023 at 05:15:41PM +0100, Michael Walle wrote:
> Am 2023-02-08 17:02, schrieb Greg Kroah-Hartman:
> > When calling debugfs_lookup() the result must have dput() called on it,
> > otherwise the memory will leak over time.  To solve this, remove the
> > lookup and create the directory on the first device found, and then
> > remove it when the module is unloaded.
> > 
> > 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
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Reviewed-by: Michael Walle <michael@walle.cc>
> 
> one nit below I didn't notice earlier, no need to send a new
> patch version just for that.
> 
> ..
> 
> > +void spi_nor_debugfs_shutdown(void)
> > +{
> > +	if (rootdir)
> > +		debugfs_remove(rootdir);
> 
> debugfs_remove() already has a check for NULL.
> 

Argh, I knew that, for some reason I went back and added that check as I
was thinking this would have been an issue if rootdir was never created.
You are right, it isn't a problem, oh well.

The simplest patches sometimes take the longest time to get right, I'll
stop now :)

thanks so much for the reviews,

greg k-h
Greg KH March 6, 2023, 6:52 a.m. UTC | #3 
On Wed, Feb 08, 2023 at 05:15:41PM +0100, Michael Walle wrote:
> Am 2023-02-08 17:02, schrieb Greg Kroah-Hartman:
> > When calling debugfs_lookup() the result must have dput() called on it,
> > otherwise the memory will leak over time.  To solve this, remove the
> > lookup and create the directory on the first device found, and then
> > remove it when the module is unloaded.
> > 
> > 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
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Reviewed-by: Michael Walle <michael@walle.cc>
> 
> one nit below I didn't notice earlier, no need to send a new
> patch version just for that.
> 
> ..
> 
> > +void spi_nor_debugfs_shutdown(void)
> > +{
> > +	if (rootdir)
> > +		debugfs_remove(rootdir);
> 
> debugfs_remove() already has a check for NULL.
> 

Ah, good catch, I merged this in when I applied it to my tree, thanks!

greg k-h
Miquel Raynal March 6, 2023, 8:33 a.m. UTC | #4 
Hi Greg,

gregkh@linuxfoundation.org wrote on Mon, 6 Mar 2023 07:52:38 +0100:

> On Wed, Feb 08, 2023 at 05:15:41PM +0100, Michael Walle wrote:
> > Am 2023-02-08 17:02, schrieb Greg Kroah-Hartman:  
> > > When calling debugfs_lookup() the result must have dput() called on it,
> > > otherwise the memory will leak over time.  To solve this, remove the
> > > lookup and create the directory on the first device found, and then
> > > remove it when the module is unloaded.
> > > 
> > > 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
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>  
> > 
> > Reviewed-by: Michael Walle <michael@walle.cc>
> > 
> > one nit below I didn't notice earlier, no need to send a new
> > patch version just for that.
> > 
> > ..
> >   
> > > +void spi_nor_debugfs_shutdown(void)
> > > +{
> > > +	if (rootdir)
> > > +		debugfs_remove(rootdir);  
> > 
> > debugfs_remove() already has a check for NULL.
> >   
> 
> Ah, good catch, I merged this in when I applied it to my tree, thanks!

Any reasons why you did apply this patch to your tree? It is a spi-nor
fix, I would have expected it to go through mtd.

Thanks,
Miquèl
Greg KH March 6, 2023, 9:13 a.m. UTC | #5 
On Mon, Mar 06, 2023 at 09:33:36AM +0100, Miquel Raynal wrote:
> Hi Greg,
> 
> gregkh@linuxfoundation.org wrote on Mon, 6 Mar 2023 07:52:38 +0100:
> 
> > On Wed, Feb 08, 2023 at 05:15:41PM +0100, Michael Walle wrote:
> > > Am 2023-02-08 17:02, schrieb Greg Kroah-Hartman:  
> > > > When calling debugfs_lookup() the result must have dput() called on it,
> > > > otherwise the memory will leak over time.  To solve this, remove the
> > > > lookup and create the directory on the first device found, and then
> > > > remove it when the module is unloaded.
> > > > 
> > > > 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
> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>  
> > > 
> > > Reviewed-by: Michael Walle <michael@walle.cc>
> > > 
> > > one nit below I didn't notice earlier, no need to send a new
> > > patch version just for that.
> > > 
> > > ..
> > >   
> > > > +void spi_nor_debugfs_shutdown(void)
> > > > +{
> > > > +	if (rootdir)
> > > > +		debugfs_remove(rootdir);  
> > > 
> > > debugfs_remove() already has a check for NULL.
> > >   
> > 
> > Ah, good catch, I merged this in when I applied it to my tree, thanks!
> 
> Any reasons why you did apply this patch to your tree? It is a spi-nor
> fix, I would have expected it to go through mtd.

It's been sitting around for a month, I assumed it was lost, so I picked
it up.  I can revert it if you don't want me to take it for 6.3-final
through my driver core tree.

thanks,

greg k-h
Miquel Raynal March 6, 2023, 11:05 a.m. UTC | #6 
Hi Greg,

gregkh@linuxfoundation.org wrote on Mon, 6 Mar 2023 10:13:33 +0100:

> On Mon, Mar 06, 2023 at 09:33:36AM +0100, Miquel Raynal wrote:
> > Hi Greg,
> > 
> > gregkh@linuxfoundation.org wrote on Mon, 6 Mar 2023 07:52:38 +0100:
> >   
> > > On Wed, Feb 08, 2023 at 05:15:41PM +0100, Michael Walle wrote:  
> > > > Am 2023-02-08 17:02, schrieb Greg Kroah-Hartman:    
> > > > > When calling debugfs_lookup() the result must have dput() called on it,
> > > > > otherwise the memory will leak over time.  To solve this, remove the
> > > > > lookup and create the directory on the first device found, and then
> > > > > remove it when the module is unloaded.
> > > > > 
> > > > > 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
> > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>    
> > > > 
> > > > Reviewed-by: Michael Walle <michael@walle.cc>
> > > > 
> > > > one nit below I didn't notice earlier, no need to send a new
> > > > patch version just for that.
> > > > 
> > > > ..
> > > >     
> > > > > +void spi_nor_debugfs_shutdown(void)
> > > > > +{
> > > > > +	if (rootdir)
> > > > > +		debugfs_remove(rootdir);    
> > > > 
> > > > debugfs_remove() already has a check for NULL.
> > > >     
> > > 
> > > Ah, good catch, I merged this in when I applied it to my tree, thanks!  
> > 
> > Any reasons why you did apply this patch to your tree? It is a spi-nor
> > fix, I would have expected it to go through mtd.  
> 
> It's been sitting around for a month, I assumed it was lost, so I picked
> it up.

Sorry if it took too long, the merge window also happened during that
time, we are collecting patches now that 6.3-rc1 has been released.
Next time don't hesitate to ping first ;-)

> I can revert it if you don't want me to take it for 6.3-final
> through my driver core tree.

I'll let spi-nor maintainers decide what they prefer.

Thanks,
Miquèl
Tudor Ambarus March 17, 2023, 8:44 a.m. UTC | #7 
On 06.03.2023 13:05, Miquel Raynal wrote:
> Hi Greg,
> 
> gregkh@linuxfoundation.org wrote on Mon, 6 Mar 2023 10:13:33 +0100:
> 
>> On Mon, Mar 06, 2023 at 09:33:36AM +0100, Miquel Raynal wrote:
>>> Hi Greg,
>>>
>>> gregkh@linuxfoundation.org wrote on Mon, 6 Mar 2023 07:52:38 +0100:
>>>   
>>>> On Wed, Feb 08, 2023 at 05:15:41PM +0100, Michael Walle wrote:  
>>>>> Am 2023-02-08 17:02, schrieb Greg Kroah-Hartman:    
>>>>>> When calling debugfs_lookup() the result must have dput() called on it,
>>>>>> otherwise the memory will leak over time.  To solve this, remove the
>>>>>> lookup and create the directory on the first device found, and then
>>>>>> remove it when the module is unloaded.
>>>>>>
>>>>>> 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
>>>>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>    
>>>>>
>>>>> Reviewed-by: Michael Walle <michael@walle.cc>
>>>>>
>>>>> one nit below I didn't notice earlier, no need to send a new
>>>>> patch version just for that.
>>>>>
>>>>> ..
>>>>>     
>>>>>> +void spi_nor_debugfs_shutdown(void)
>>>>>> +{
>>>>>> +	if (rootdir)
>>>>>> +		debugfs_remove(rootdir);    
>>>>>
>>>>> debugfs_remove() already has a check for NULL.
>>>>>     
>>>>
>>>> Ah, good catch, I merged this in when I applied it to my tree, thanks!  
>>>
>>> Any reasons why you did apply this patch to your tree? It is a spi-nor
>>> fix, I would have expected it to go through mtd.  
>>
>> It's been sitting around for a month, I assumed it was lost, so I picked
>> it up.
> 
> Sorry if it took too long, the merge window also happened during that
> time, we are collecting patches now that 6.3-rc1 has been released.
> Next time don't hesitate to ping first ;-)
> 
>> I can revert it if you don't want me to take it for 6.3-final
>> through my driver core tree.
>

It's fine, I don't expect other changes on these chunks of code. Anyway,
if there will be merge conflicts I'll solve them locally by merging the
-rc containing the fix.

BTW, you forgot to cc stable and add a fixes tag :).

> I'll let spi-nor maintainers decide what they prefer.
> 

If it'll be a next time, a ping would be better.

Cheers,
ta
diff mbox series

Patch

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index d67c926bca8b..1452ed6fe5d1 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3335,7 +3335,19 @@  static struct spi_mem_driver spi_nor_driver = {
 	.remove = spi_nor_remove,
 	.shutdown = spi_nor_shutdown,
 };
-module_spi_mem_driver(spi_nor_driver);
+
+static int __init spi_nor_module_init(void)
+{
+	return spi_mem_driver_register(&spi_nor_driver);
+}
+module_init(spi_nor_module_init);
+
+static void __exit spi_nor_module_exit(void)
+{
+	spi_mem_driver_unregister(&spi_nor_driver);
+	spi_nor_debugfs_shutdown();
+}
+module_exit(spi_nor_module_exit);
 
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Huang Shijie <shijie8@gmail.com>");
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index f03b55cf7e6f..e62cd9964456 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -713,8 +713,10 @@  static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
 
 #ifdef CONFIG_DEBUG_FS
 void spi_nor_debugfs_register(struct spi_nor *nor);
+void spi_nor_debugfs_shutdown(void);
 #else
 static inline void spi_nor_debugfs_register(struct spi_nor *nor) {}
+static inline void spi_nor_debugfs_shutdown(void) {}
 #endif
 
 #endif /* __LINUX_MTD_SPI_NOR_INTERNAL_H */
diff --git a/drivers/mtd/spi-nor/debugfs.c b/drivers/mtd/spi-nor/debugfs.c
index ff895f6758ea..cd9c2dc07509 100644
--- a/drivers/mtd/spi-nor/debugfs.c
+++ b/drivers/mtd/spi-nor/debugfs.c
@@ -226,13 +226,13 @@  static void spi_nor_debugfs_unregister(void *data)
 	nor->debugfs_root = NULL;
 }
 
+static struct dentry *rootdir;
+
 void spi_nor_debugfs_register(struct spi_nor *nor)
 {
-	struct dentry *rootdir, *d;
+	struct dentry *d;
 	int ret;
 
-	/* Create rootdir once. Will never be deleted again. */
-	rootdir = debugfs_lookup(SPI_NOR_DEBUGFS_ROOT, NULL);
 	if (!rootdir)
 		rootdir = debugfs_create_dir(SPI_NOR_DEBUGFS_ROOT, NULL);
 
@@ -247,3 +247,9 @@  void spi_nor_debugfs_register(struct spi_nor *nor)
 	debugfs_create_file("capabilities", 0444, d, nor,
 			    &spi_nor_capabilities_fops);
 }
+
+void spi_nor_debugfs_shutdown(void)
+{
+	if (rootdir)
+		debugfs_remove(rootdir);
+}