[PATCH] mtd: nandsim: remove debugfs entries in error path

[PATCH] mtd: nandsim: remove debugfs entries in error path

[Uwe Kleine-König]

The debugfs entries must be removed before an error is returned in the
probe function. Otherwise another try to load the module fails and when
the debugfs files are accessed without the module loaded, the kernel
still tries to call a function in that module.

Fixes: 5346c27c5fed ("mtd: nandsim: Introduce debugfs infrastructure")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/mtd/nand/nandsim.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
index 03a0d057bf2f..e4211c3cc49b 100644
--- a/drivers/mtd/nand/nandsim.c
+++ b/drivers/mtd/nand/nandsim.c
@@ -2373,6 +2373,7 @@ static int __init ns_init_module(void)
         return 0;
 
 err_exit:
+	nandsim_debugfs_remove(nand);
 	free_nandsim(nand);
 	nand_release(nsmtd);
 	for (i = 0;i < ARRAY_SIZE(nand->partitions); ++i)
-- 
2.11.0

Re: [PATCH] mtd: nandsim: remove debugfs entries in error path

[Richard Weinberger]

Am Mittwoch, 23. August 2017, 09:03:04 CEST schrieb Uwe Kleine-König:
> The debugfs entries must be removed before an error is returned in the
> probe function. Otherwise another try to load the module fails and when
> the debugfs files are accessed without the module loaded, the kernel
> still tries to call a function in that module.
> 
> Fixes: 5346c27c5fed ("mtd: nandsim: Introduce debugfs infrastructure")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH] mtd: nandsim: remove debugfs entries in error path

[Boris Brezillon]

On Wed, 23 Aug 2017 09:03:04 +0200
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> The debugfs entries must be removed before an error is returned in the
> probe function. Otherwise another try to load the module fails and when
> the debugfs files are accessed without the module loaded, the kernel
> still tries to call a function in that module.

Unfortunately this patch conflicts with [1], but feel free to send it to
stable with my

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Regards,

Boris

[1]http://git.infradead.org/l2-mtd.git/commit/e8e3edb95ce6a146bc774b6cfad3553f4383edc8

> 
> Fixes: 5346c27c5fed ("mtd: nandsim: Introduce debugfs infrastructure")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/mtd/nand/nandsim.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
> index 03a0d057bf2f..e4211c3cc49b 100644
> --- a/drivers/mtd/nand/nandsim.c
> +++ b/drivers/mtd/nand/nandsim.c
> @@ -2373,6 +2373,7 @@ static int __init ns_init_module(void)
>          return 0;
>  
>  err_exit:
> +	nandsim_debugfs_remove(nand);
>  	free_nandsim(nand);
>  	nand_release(nsmtd);
>  	for (i = 0;i < ARRAY_SIZE(nand->partitions); ++i)

Re: [PATCH] mtd: nandsim: remove debugfs entries in error path

[Uwe Kleine-König]

Hello,

On Wed, Aug 23, 2017 at 10:40:52AM +0200, Boris Brezillon wrote:
> On Wed, 23 Aug 2017 09:03:04 +0200
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> 
> > The debugfs entries must be removed before an error is returned in the
> > probe function. Otherwise another try to load the module fails and when
> > the debugfs files are accessed without the module loaded, the kernel
> > still tries to call a function in that module.
> 
> Unfortunately this patch conflicts with [1], but feel free to send it to
> stable with my
> 
> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>
> [1]http://git.infradead.org/l2-mtd.git/commit/e8e3edb95ce6a146bc774b6cfad3553f4383edc8

I didn't test, but I think e8e3edb95ce6a146bc774b6c should fix the
issue, too.

The key question now is: Is my fix
(http://patchwork.ozlabs.org/patch/804837/) important enough to still go
into 4.13 (e8e3edb95ce6a146bc774b6c certainly isn't suitable here)? If
yes, it "only" creates a merge conflict that should be trivial to fix.
If it's to late for 4.13 then my patch should go into stable with an
updated comment that references e8e3edb95ce6a146bc774b6c.

What do you think?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] mtd: nandsim: remove debugfs entries in error path

[Brian Norris]

Hi,

On Wed, Aug 23, 2017 at 09:03:04AM +0200, Uwe Kleine-König wrote:
> The debugfs entries must be removed before an error is returned in the
> probe function. Otherwise another try to load the module fails and when
> the debugfs files are accessed without the module loaded, the kernel
> still tries to call a function in that module.
> 
> Fixes: 5346c27c5fed ("mtd: nandsim: Introduce debugfs infrastructure")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/mtd/nand/nandsim.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
> index 03a0d057bf2f..e4211c3cc49b 100644
> --- a/drivers/mtd/nand/nandsim.c
> +++ b/drivers/mtd/nand/nandsim.c
> @@ -2373,6 +2373,7 @@ static int __init ns_init_module(void)
>          return 0;
>  
>  err_exit:
> +	nandsim_debugfs_remove(nand);

The 'err_exit' label is used by error paths that occur before the
debugfs entries are registered. But I guess that's OK, since
debugfs_remove_recursive() handles NULL arguments fine?

We also have the same mismatch with init_nandsim()/free_nandsim(), which
is also similarly safe because of the nature of kfree(), vfree(), and
the fact that most of those are guarded with NULL checks anyway... So
it's just confusing and potentially dangerous for future developers --
but not wrong.

Given this code will promptly go away with the for-4.14 material, I
suppose this isn't that bad.

I'll look to queue this up and send to Linus within a day.

Thanks,
Brian

>  	free_nandsim(nand);
>  	nand_release(nsmtd);
>  	for (i = 0;i < ARRAY_SIZE(nand->partitions); ++i)
> -- 
> 2.11.0
> 

Re: [PATCH] mtd: nandsim: remove debugfs entries in error path

[Uwe Kleine-König]

Hello Brian,

On Thu, Aug 24, 2017 at 09:22:03PM -0700, Brian Norris wrote:
> On Wed, Aug 23, 2017 at 09:03:04AM +0200, Uwe Kleine-König wrote:
> > The debugfs entries must be removed before an error is returned in the
> > probe function. Otherwise another try to load the module fails and when
> > the debugfs files are accessed without the module loaded, the kernel
> > still tries to call a function in that module.
> > 
> > Fixes: 5346c27c5fed ("mtd: nandsim: Introduce debugfs infrastructure")
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/mtd/nand/nandsim.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
> > index 03a0d057bf2f..e4211c3cc49b 100644
> > --- a/drivers/mtd/nand/nandsim.c
> > +++ b/drivers/mtd/nand/nandsim.c
> > @@ -2373,6 +2373,7 @@ static int __init ns_init_module(void)
> >          return 0;
> >  
> >  err_exit:
> > +	nandsim_debugfs_remove(nand);
> 
> The 'err_exit' label is used by error paths that occur before the
> debugfs entries are registered. But I guess that's OK, since
> debugfs_remove_recursive() handles NULL arguments fine?

Yes. 
 
> We also have the same mismatch with init_nandsim()/free_nandsim(), which
> is also similarly safe because of the nature of kfree(), vfree(), and
> the fact that most of those are guarded with NULL checks anyway... So
> it's just confusing and potentially dangerous for future developers --
> but not wrong.

Ack, I took me some time to find a right place for
nandsim_debugfs_remove because I had problems to match the cleanup
functions in the error path to the setup functions. I then stopped
checking the already existing code for correctness and just put
nandsim_debugfs_remove to a place that works.

> I'll look to queue this up and send to Linus within a day.

\o/

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] mtd: nandsim: remove debugfs entries in error path

[Brian Norris]

On Thu, Aug 24, 2017 at 09:22:03PM -0700, Brian Norris wrote:
> I'll look to queue this up and send to Linus within a day.

Applied to linux-mtd.git.