| Message ID | 20230315034441.44321-1-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: kfence: fix using kfence_metadata without initialization in show_object() | expand |
On Wed, 15 Mar 2023 at 04:45, Muchun Song <songmuchun@bytedance.com> wrote: > > The variable kfence_metadata is initialized in kfence_init_pool(), then, it is > not initialized if kfence is disabled after booting. In this case, kfence_metadata > will be used (e.g. ->lock and ->state fields) without initialization when reading > /sys/kernel/debug/kfence/objects. There will be a warning if you enable > CONFIG_DEBUG_SPINLOCK. Fix it by creating debugfs files when necessary. > > Fixes: 0ce20dd84089 ("mm: add Kernel Electric-Fence infrastructure") > Signed-off-by: Muchun Song <songmuchun@bytedance.com> Tested-by: Marco Elver <elver@google.com> Reviewed-by: Marco Elver <elver@google.com> Good catch! > --- > mm/kfence/core.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/mm/kfence/core.c b/mm/kfence/core.c > index 5349c37a5dac..79c94ee55f97 100644 > --- a/mm/kfence/core.c > +++ b/mm/kfence/core.c > @@ -726,10 +726,14 @@ static const struct seq_operations objects_sops = { > }; > DEFINE_SEQ_ATTRIBUTE(objects); > > -static int __init kfence_debugfs_init(void) > +static int kfence_debugfs_init(void) > { > - struct dentry *kfence_dir = debugfs_create_dir("kfence", NULL); > + struct dentry *kfence_dir; > > + if (!READ_ONCE(kfence_enabled)) > + return 0; > + > + kfence_dir = debugfs_create_dir("kfence", NULL); > debugfs_create_file("stats", 0444, kfence_dir, NULL, &stats_fops); > debugfs_create_file("objects", 0400, kfence_dir, NULL, &objects_fops); > return 0; > @@ -883,6 +887,8 @@ static int kfence_init_late(void) > } > > kfence_init_enable(); > + kfence_debugfs_init(); > + > return 0; > } > > -- > 2.11.0 >
On Wed, 15 Mar 2023 09:07:40 +0100 Marco Elver <elver@google.com> wrote: > On Wed, 15 Mar 2023 at 04:45, Muchun Song <songmuchun@bytedance.com> wrote: > > > > The variable kfence_metadata is initialized in kfence_init_pool(), then, it is > > not initialized if kfence is disabled after booting. In this case, kfence_metadata > > will be used (e.g. ->lock and ->state fields) without initialization when reading > > /sys/kernel/debug/kfence/objects. There will be a warning if you enable > > CONFIG_DEBUG_SPINLOCK. Fix it by creating debugfs files when necessary. > > > > Fixes: 0ce20dd84089 ("mm: add Kernel Electric-Fence infrastructure") > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > Tested-by: Marco Elver <elver@google.com> > Reviewed-by: Marco Elver <elver@google.com> Thanks, I'll add cc:stable to this. I assume the warning is the only known adverse effect of this bug?
On Wed, 15 Mar 2023 at 20:54, Andrew Morton <akpm@linux-foundation.org> wrote: > > On Wed, 15 Mar 2023 09:07:40 +0100 Marco Elver <elver@google.com> wrote: > > > On Wed, 15 Mar 2023 at 04:45, Muchun Song <songmuchun@bytedance.com> wrote: > > > > > > The variable kfence_metadata is initialized in kfence_init_pool(), then, it is > > > not initialized if kfence is disabled after booting. In this case, kfence_metadata > > > will be used (e.g. ->lock and ->state fields) without initialization when reading > > > /sys/kernel/debug/kfence/objects. There will be a warning if you enable > > > CONFIG_DEBUG_SPINLOCK. Fix it by creating debugfs files when necessary. > > > > > > Fixes: 0ce20dd84089 ("mm: add Kernel Electric-Fence infrastructure") > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > > > Tested-by: Marco Elver <elver@google.com> > > Reviewed-by: Marco Elver <elver@google.com> > > Thanks, I'll add cc:stable to this. > > I assume the warning is the only known adverse effect of this bug? For architectures where the initial spinlock state is 0, the warning is the only issue. For architectures where that's not the case, it might result in lockup of the task querying the 'objects' file -- which isn't the case for any arch that supports KFENCE by the looks of it (last I checked 'sh' and 'parisc' don't support KFENCE). Thanks, -- Marco
diff --git a/mm/kfence/core.c b/mm/kfence/core.c index 5349c37a5dac..79c94ee55f97 100644 --- a/mm/kfence/core.c +++ b/mm/kfence/core.c @@ -726,10 +726,14 @@ static const struct seq_operations objects_sops = { }; DEFINE_SEQ_ATTRIBUTE(objects); -static int __init kfence_debugfs_init(void) +static int kfence_debugfs_init(void) { - struct dentry *kfence_dir = debugfs_create_dir("kfence", NULL); + struct dentry *kfence_dir; + if (!READ_ONCE(kfence_enabled)) + return 0; + + kfence_dir = debugfs_create_dir("kfence", NULL); debugfs_create_file("stats", 0444, kfence_dir, NULL, &stats_fops); debugfs_create_file("objects", 0400, kfence_dir, NULL, &objects_fops); return 0; @@ -883,6 +887,8 @@ static int kfence_init_late(void) } kfence_init_enable(); + kfence_debugfs_init(); + return 0; }
The variable kfence_metadata is initialized in kfence_init_pool(), then, it is not initialized if kfence is disabled after booting. In this case, kfence_metadata will be used (e.g. ->lock and ->state fields) without initialization when reading /sys/kernel/debug/kfence/objects. There will be a warning if you enable CONFIG_DEBUG_SPINLOCK. Fix it by creating debugfs files when necessary. Fixes: 0ce20dd84089 ("mm: add Kernel Electric-Fence infrastructure") Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- mm/kfence/core.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)