Re: [PATCH] mm/vmalloc: fix data race in show_numa_info()

[Eric Dumazet <edumazet@google.com>]

On Mon, May 5, 2025 at 11:06 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Mon, May 5, 2025 at 10:20 AM Jeongjun Park <aha310510@gmail.com> wrote:
> >
> > The following data-race was found in show_numa_info():
> >
> > ==================================================================
> > BUG: KCSAN: data-race in vmalloc_info_show / vmalloc_info_show
> >
> > r
> > value changed: 0x0000008f -> 0x00000000
> >
> > Reported by Kernel Concurrency Sanitizer on:
> > CPU: 1 UID: 0 PID: 8287 Comm: syz.0.411 Not tainted 6.15.0-rc4-00256-g95d3481af6dc-dirty #1 PREEMPT(voluntary)
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
> > ==================================================================
> >
> > There is a read/write data-race in counter[]. This seems to be happening
> > because only read memory barriers are currently applied, so we need to
> > modify the write operation to counters[] to be handled atomically.
> >
> > Fixes: a47a126ad5ea ("vmallocinfo: add NUMA information")
> > Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> > ---
> >  mm/vmalloc.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index 3ed720a787ec..d93fa535bc21 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -4917,7 +4917,8 @@ bool vmalloc_dump_obj(void *object)
> >  static void show_numa_info(struct seq_file *m, struct vm_struct *v)
> >  {
> >         if (IS_ENABLED(CONFIG_NUMA)) {
> > -               unsigned int nr, *counters = m->private;
> > +               atomic_t *counters = m->private;
> > +               unsigned int nr;
> >                 unsigned int step = 1U << vm_area_page_order(v);
> >
> >                 if (!counters)
> > @@ -4931,10 +4932,10 @@ static void show_numa_info(struct seq_file *m, struct vm_struct *v)
> >                 memset(counters, 0, nr_node_ids * sizeof(unsigned int));
> >
> >                 for (nr = 0; nr < v->nr_pages; nr += step)
> > -                       counters[page_to_nid(v->pages[nr])] += step;
> > +                       atomic_add(step, &counters[page_to_nid(v->pages[nr])]);
> >                 for_each_node_state(nr, N_HIGH_MEMORY)
> > -                       if (counters[nr])
> > -                               seq_printf(m, " N%u=%u", nr, counters[nr]);
> > +                       if (atomic_read(&counters[nr]))
> > +                               seq_printf(m, " N%u=%u", nr, atomic_read(&counters[nr]));
> >         }
> >  }
> >
> > --
>
> This patch looks bogus to me.
>
> The race is about using m->private for storage, while the same file
> can be read from multiple threads.
>
> Using atomic_t is going to silence syzbot, but the bug is still there.

A more correct fix would be :

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index a6e7acebe9adf5e6c8abd52dcf7d02a6a1bc3030..cb69b44587d2032a6192f3ceb518490a05eff541
100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -4921,24 +4921,24 @@ bool vmalloc_dump_obj(void *object)
 static void show_numa_info(struct seq_file *m, struct vm_struct *v)
 {
        if (IS_ENABLED(CONFIG_NUMA)) {
-               unsigned int nr, *counters = m->private;
+               unsigned int nr, *counters;
                unsigned int step = 1U << vm_area_page_order(v);

+               if (v->flags & VM_UNINITIALIZED)
+                       return;
+               counters = kcalloc(nr_node_ids, sizeof(unsigned int),
GFP_KERNEL);
                if (!counters)
                        return;

-               if (v->flags & VM_UNINITIALIZED)
-                       return;
                /* Pair with smp_wmb() in clear_vm_uninitialized_flag() */
                smp_rmb();

-               memset(counters, 0, nr_node_ids * sizeof(unsigned int));
-
                for (nr = 0; nr < v->nr_pages; nr += step)
                        counters[page_to_nid(v->pages[nr])] += step;
                for_each_node_state(nr, N_HIGH_MEMORY)
                        if (counters[nr])
                                seq_printf(m, " N%u=%u", nr, counters[nr]);
+               kfree(counters);
        }
 }

@@ -5032,13 +5032,7 @@ static int vmalloc_info_show(struct seq_file *m, void *p)

 static int __init proc_vmalloc_init(void)
 {
-       void *priv_data = NULL;
-
-       if (IS_ENABLED(CONFIG_NUMA))
-               priv_data = kmalloc(nr_node_ids * sizeof(unsigned
int), GFP_KERNEL);
-
-       proc_create_single_data("vmallocinfo",
-               0400, NULL, vmalloc_info_show, priv_data);
+       proc_create_single("vmallocinfo", 0400, NULL, vmalloc_info_show);

        return 0;
 }