diff mbox series

mm: kfence: fix using kfence_metadata without initialization in show_object()

Message ID 20230315034441.44321-1-songmuchun@bytedance.com
State New
Headers
Series mm: kfence: fix using kfence_metadata without initialization in show_object() |

Commit Message

Muchun Song March 15, 2023, 3:44 a.m. UTC 
  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(-)

Comments

Marco Elver March 15, 2023, 8:07 a.m. UTC | #1 
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
>
Andrew Morton March 15, 2023, 7:54 p.m. UTC | #2 
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?
Marco Elver March 15, 2023, 9:04 p.m. UTC | #3 
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 mbox series

Patch

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;
 }