| Message ID | 20210303093845.2743309-1-elver@google.com |
|---|---|
| State | Accepted |
| Commit | e36299efe7d749976fbdaaf756dee6ef32543c2c |
| Headers | show |
| Series | kcsan, debugfs: Move debugfs file creation out of early init | expand |
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>
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
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
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
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.
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(-)