[PATCH] kcsan, debugfs: Move debugfs file creation out of early init

[PATCH] kcsan, debugfs: Move debugfs file creation out of early init

[Marco Elver]

Commit 56348560d495 ("debugfs: do not attempt to create a new file
before the filesystem is initalized") forbids creating new debugfs files
until debugfs is fully initialized. This breaks KCSAN's debugfs file
creation, which happened at the end of __init().

There is no reason to create the debugfs file during early
initialization. Therefore, move it into a late_initcall() callback.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: stable <stable@vger.kernel.org>
Fixes: 56348560d495 ("debugfs: do not attempt to create a new file before the filesystem is initalized")
Signed-off-by: Marco Elver <elver@google.com>
---
I've marked this for 'stable', since 56348560d495 is also intended for
stable, and would subsequently break KCSAN in all stable kernels where
KCSAN is available (since 5.8).
---
 kernel/kcsan/core.c    | 2 --
 kernel/kcsan/debugfs.c | 4 +++-
 kernel/kcsan/kcsan.h   | 5 -----
 3 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/kernel/kcsan/core.c b/kernel/kcsan/core.c
index 8c3867640c21..45c821d4e8bd 100644
--- a/kernel/kcsan/core.c
+++ b/kernel/kcsan/core.c
@@ -644,8 +644,6 @@ void __init kcsan_init(void)
 
 	BUG_ON(!in_task());
 
-	kcsan_debugfs_init();
-
 	for_each_possible_cpu(cpu)
 		per_cpu(kcsan_rand_state, cpu) = (u32)get_cycles();
 
diff --git a/kernel/kcsan/debugfs.c b/kernel/kcsan/debugfs.c
index c837ce6c52e6..c1dd02f3be8b 100644
--- a/kernel/kcsan/debugfs.c
+++ b/kernel/kcsan/debugfs.c
@@ -266,7 +266,9 @@ static const struct file_operations debugfs_ops =
 	.release = single_release
 };
 
-void __init kcsan_debugfs_init(void)
+static void __init kcsan_debugfs_init(void)
 {
 	debugfs_create_file("kcsan", 0644, NULL, NULL, &debugfs_ops);
 }
+
+late_initcall(kcsan_debugfs_init);
diff --git a/kernel/kcsan/kcsan.h b/kernel/kcsan/kcsan.h
index 594a5dd4842a..9881099d4179 100644
--- a/kernel/kcsan/kcsan.h
+++ b/kernel/kcsan/kcsan.h
@@ -31,11 +31,6 @@ extern bool kcsan_enabled;
 void kcsan_save_irqtrace(struct task_struct *task);
 void kcsan_restore_irqtrace(struct task_struct *task);
 
-/*
- * Initialize debugfs file.
- */
-void kcsan_debugfs_init(void);
-
 /*
  * Statistics counters displayed via debugfs; should only be modified in
  * slow-paths.
-- 
2.30.1.766.gb4fecdf3b7-goog

Re: [PATCH] kcsan, debugfs: Move debugfs file creation out of early init

[Greg KH]

On Wed, Mar 03, 2021 at 10:38:45AM +0100, Marco Elver wrote:
> Commit 56348560d495 ("debugfs: do not attempt to create a new file
> before the filesystem is initalized") forbids creating new debugfs files
> until debugfs is fully initialized. This breaks KCSAN's debugfs file
> creation, which happened at the end of __init().

How did it "break" it?  The files shouldn't have actually been created,
right?

> There is no reason to create the debugfs file during early
> initialization. Therefore, move it into a late_initcall() callback.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: stable <stable@vger.kernel.org>
> Fixes: 56348560d495 ("debugfs: do not attempt to create a new file before the filesystem is initalized")
> Signed-off-by: Marco Elver <elver@google.com>
> ---
> I've marked this for 'stable', since 56348560d495 is also intended for
> stable, and would subsequently break KCSAN in all stable kernels where
> KCSAN is available (since 5.8).

No objection from me, just odd that this actually fixes anything :)

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

Re: [PATCH] kcsan, debugfs: Move debugfs file creation out of early init

[Marco Elver]

On Wed, 3 Mar 2021 at 10:57, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Mar 03, 2021 at 10:38:45AM +0100, Marco Elver wrote:
> > Commit 56348560d495 ("debugfs: do not attempt to create a new file
> > before the filesystem is initalized") forbids creating new debugfs files
> > until debugfs is fully initialized. This breaks KCSAN's debugfs file
> > creation, which happened at the end of __init().
>
> How did it "break" it?  The files shouldn't have actually been created,
> right?

Right, with 56348560d495 the debugfs file isn't created anymore, which
is the problem. Before 56348560d495 the file exists (syzbot wants the
file to exist.)

> > There is no reason to create the debugfs file during early
> > initialization. Therefore, move it into a late_initcall() callback.
> >
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: stable <stable@vger.kernel.org>
> > Fixes: 56348560d495 ("debugfs: do not attempt to create a new file before the filesystem is initalized")
> > Signed-off-by: Marco Elver <elver@google.com>
> > ---
> > I've marked this for 'stable', since 56348560d495 is also intended for
> > stable, and would subsequently break KCSAN in all stable kernels where
> > KCSAN is available (since 5.8).
>
> No objection from me, just odd that this actually fixes anything :)

56348560d495 causes the file to just not be created if we try to
create at the end of __init(). Having it created as late as
late_initcall() gets us the file back.

When you say "fixes anything", should the file be created even though
it's at the end of __init()? Perhaps I misunderstood what 56348560d495
changes, but I verified it to be the problem by reverting (upon which
the file exists as expected).

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

Thanks! Would it be possible to get this into 5.12?

-- Marco

Re: [PATCH] kcsan, debugfs: Move debugfs file creation out of early init

[Greg KH]

On Wed, Mar 03, 2021 at 11:18:06AM +0100, Marco Elver wrote:
> On Wed, 3 Mar 2021 at 10:57, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Mar 03, 2021 at 10:38:45AM +0100, Marco Elver wrote:
> > > Commit 56348560d495 ("debugfs: do not attempt to create a new file
> > > before the filesystem is initalized") forbids creating new debugfs files
> > > until debugfs is fully initialized. This breaks KCSAN's debugfs file
> > > creation, which happened at the end of __init().
> >
> > How did it "break" it?  The files shouldn't have actually been created,
> > right?
> 
> Right, with 56348560d495 the debugfs file isn't created anymore, which
> is the problem. Before 56348560d495 the file exists (syzbot wants the
> file to exist.)
> 
> > > There is no reason to create the debugfs file during early
> > > initialization. Therefore, move it into a late_initcall() callback.
> > >
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Cc: stable <stable@vger.kernel.org>
> > > Fixes: 56348560d495 ("debugfs: do not attempt to create a new file before the filesystem is initalized")
> > > Signed-off-by: Marco Elver <elver@google.com>
> > > ---
> > > I've marked this for 'stable', since 56348560d495 is also intended for
> > > stable, and would subsequently break KCSAN in all stable kernels where
> > > KCSAN is available (since 5.8).
> >
> > No objection from me, just odd that this actually fixes anything :)
> 
> 56348560d495 causes the file to just not be created if we try to
> create at the end of __init(). Having it created as late as
> late_initcall() gets us the file back.
> 
> When you say "fixes anything", should the file be created even though
> it's at the end of __init()? Perhaps I misunderstood what 56348560d495
> changes, but I verified it to be the problem by reverting (upon which
> the file exists as expected).

All my change did is explicitly not allow you to create a file if
debugfs had not been initialized.  If you tried to do that before, you
should have gotten an error from the vfs layer that the file was not
created, as otherwise how would it have succeeded?

I just moved the check up higher in the "stack" to the debugfs code, and
not relied on the vfs layer to do a lot of work only to reject things
later on.

So there "should" not have been any functional change with this patch.
If there was, then something is really odd as how can the vfs layer
create a file for a filesystem _before_ that filesystem has been
registered with the vfs layer?

thanks,

greg k-h

Re: [PATCH] kcsan, debugfs: Move debugfs file creation out of early init

[Marco Elver]

On Wed, 3 Mar 2021 at 11:24, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Mar 03, 2021 at 11:18:06AM +0100, Marco Elver wrote:
> > On Wed, 3 Mar 2021 at 10:57, Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Mar 03, 2021 at 10:38:45AM +0100, Marco Elver wrote:
> > > > Commit 56348560d495 ("debugfs: do not attempt to create a new file
> > > > before the filesystem is initalized") forbids creating new debugfs files
> > > > until debugfs is fully initialized. This breaks KCSAN's debugfs file
> > > > creation, which happened at the end of __init().
> > >
> > > How did it "break" it?  The files shouldn't have actually been created,
> > > right?
> >
> > Right, with 56348560d495 the debugfs file isn't created anymore, which
> > is the problem. Before 56348560d495 the file exists (syzbot wants the
> > file to exist.)
> >
> > > > There is no reason to create the debugfs file during early
> > > > initialization. Therefore, move it into a late_initcall() callback.
> > > >
> > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > > Cc: stable <stable@vger.kernel.org>
> > > > Fixes: 56348560d495 ("debugfs: do not attempt to create a new file before the filesystem is initalized")
> > > > Signed-off-by: Marco Elver <elver@google.com>
> > > > ---
> > > > I've marked this for 'stable', since 56348560d495 is also intended for
> > > > stable, and would subsequently break KCSAN in all stable kernels where
> > > > KCSAN is available (since 5.8).
> > >
> > > No objection from me, just odd that this actually fixes anything :)
> >
> > 56348560d495 causes the file to just not be created if we try to
> > create at the end of __init(). Having it created as late as
> > late_initcall() gets us the file back.
> >
> > When you say "fixes anything", should the file be created even though
> > it's at the end of __init()? Perhaps I misunderstood what 56348560d495
> > changes, but I verified it to be the problem by reverting (upon which
> > the file exists as expected).
>
> All my change did is explicitly not allow you to create a file if
> debugfs had not been initialized.  If you tried to do that before, you
> should have gotten an error from the vfs layer that the file was not
> created, as otherwise how would it have succeeded?
>
> I just moved the check up higher in the "stack" to the debugfs code, and
> not relied on the vfs layer to do a lot of work only to reject things
> later on.
>
> So there "should" not have been any functional change with this patch.
> If there was, then something is really odd as how can the vfs layer
> create a file for a filesystem _before_ that filesystem has been
> registered with the vfs layer?

Ah, I see. I do confirm that the file has been created until
56348560d495, without any errors.

Thanks,
-- Marco