| Message ID | 20170823070304.1336-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Accepted |
| Delegated to: | Brian Norris |
| Headers | show |
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
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)
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
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 >
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
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.
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)
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(+)