Re: [PATCH] mm: fix racing of vb->va when kasan enabled

[Zhaoyang Huang <huangzhaoyang@gmail.com>]

On Wed, Jun 22, 2022 at 11:15 AM Zhaoyang Huang <huangzhaoyang@gmail.com> wrote:
>
> On Tue, Jun 21, 2022 at 10:29 PM Uladzislau Rezki <urezki@gmail.com> wrote:
> >
> > > On Tue, Jun 21, 2022 at 5:27 PM Uladzislau Rezki <urezki@gmail.com> wrote:
> > > >
> > > > > On Mon, Jun 20, 2022 at 6:44 PM Uladzislau Rezki <urezki@gmail.com> wrote:
> > > > > >
> > > > > > > > >
> > > > > > > > Is it easy to reproduce? If so could you please describe the steps? As i see
> > > > > > > > the freeing of the "vb" is RCU safe whereas vb->va is not. But from the first
> > > > > > > > glance i do not see how it can accessed twice. Hm..
> > > > > > > It was raised from a monkey test on A13_k515 system and got 1/20 pcs
> > > > > > > failed. IMO, vb->va which out of vmap_purge_lock protection could race
> > > > > > > with a concurrent ra freeing within __purge_vmap_area_lazy.
> > > > > > >
> > > > > > Do you have exact steps how you run "monkey" test?
> > > > > There are about 30+ kos inserted during startup which could be a
> > > > > specific criteria for reproduction. Do you have doubts about the test
> > > > > result or the solution?
> > > > > >
> > > > I do not have any doubt about your test results, so if you can trigger it
> > > > then there is an issue at least on the 5.4.161-android12 kernel.
> > > >
> > > > 1. With your fix we get expanded mutex range, thus the worst case of vmalloc
> > > > allocation can be increased when it fails and repeat. Because it also invokes
> > > > the purge_vmap_area_lazy() that access the same mutex.
> > > I am not sure I get your point. _vm_unmap_aliases calls
> > > _purge_vmap_area_lazy instead of purge_vmap_area_lazy. Do you have any
> > > other solutions? I really don't think my patch is the best way as I
> > > don't have a full view of vmalloc mechanism.
> > >
> > Yep, but it holds the mutex:
I still don't get how _purge_vmap_area_lazy hold vmap_purge_lock?
> >
> > <snip>
> > mutex_lock(&vmap_purge_lock);
> > purge_fragmented_blocks_allcpus();
> > if (!__purge_vmap_area_lazy(start, end) && flush)
> >         flush_tlb_kernel_range(start, end);
> > mutex_unlock(&vmap_purge_lock);
> > <snip>
> >
> > I do not have a solution yet. I am trying still to figure out how you can
> > trigger it.
> >
> > <snip>
> >         rcu_read_lock();
> >         list_for_each_entry_rcu(vb, &vbq->free, free_list) {
> >                 spin_lock(&vb->lock);
> >                 if (vb->dirty && vb->dirty != VMAP_BBMAP_BITS) {
> >                         unsigned long va_start = vb->va->va_start;
> > <snip>
> >
> > so you say that "vb->va->va_start" can be accessed twice. I do not see
> > how it can happen. The purge_fragmented_blocks() removes "vb" from the
> > free_list and set vb->dirty to the VMAP_BBMAP_BITS to prevent purging
> > it again. It is protected by the spin_lock(&vb->lock):
> >
> > <snip>
> > spin_lock(&vb->lock);
> > if (vb->free + vb->dirty == VMAP_BBMAP_BITS && vb->dirty != VMAP_BBMAP_BITS) {
> >         vb->free = 0; /* prevent further allocs after releasing lock */
> >         vb->dirty = VMAP_BBMAP_BITS; /* prevent purging it again */
> >         vb->dirty_min = 0;
> >         vb->dirty_max = VMAP_BBMAP_BITS;
> > <snip>
> >
> > so the VMAP_BBMAP_BITS is set under spinlock. The _vm_unmap_aliases() checks it:
> >
> > <snip>
> > list_for_each_entry_rcu(vb, &vbq->free, free_list) {
> >         spin_lock(&vb->lock);
> >         if (vb->dirty && vb->dirty != VMAP_BBMAP_BITS) {
> >                 unsigned long va_start = vb->va->va_start;
> >                 unsigned long s, e;
> > <snip>
> >
> > if the "vb->dirty != VMAP_BBMAP_BITS". I am missing your point here?
> Could the racing be like bellowing scenario?  vb->va accessed in [2]
> has been freed in [1]
>
> _vm_unmap_aliases
>                 _vm_unmap_aliases
> {
>                                {
>               list_for_each_entry_rcu(vb, &vbq->free, free_list) {
>              __purge_vmap_area_lazy
>                      spin_lock(&vb->lock);
>                                 merge_or_add_vmap_area
>                      if (vb->dirty) {
>
> kmem_cache_free(vmap_area_cachep, va)[1]
>                             unsigned long va_start = vb->va->va_start;
> [2]

reformat the racing graph
 _vm_unmap_aliases
                             _vm_unmap_aliases
 {
                                              {
            list_for_each_entry_rcu(vb, &vbq->free, free_list) {
                   __purge_vmap_area_lazy
                      spin_lock(&vb->lock);
                                        merge_or_add_vmap_area
                      if (vb->dirty) {

kmem_cache_free(vmap_area_cachep, va)[1]
                             unsigned long va_start = vb->va->va_start; [2]

> >
> > > >
> > > > 2. You run 5.4.161-android12 kernel what is quite old. Could you please
> > > > retest with latest kernel? I am asking because on the latest kernel with
> > > > CONFIG_KASAN i am not able to reproduce it.
> > > >
> > > > I do a lot of: vm_map_ram()/vm_unmap_ram()/vmalloc()/vfree() in parallel
> > > > by 64 kthreads on my 64 CPUs test system.
> > > The failure generates at 20s from starting up, I think it is a rare timing.
> > > >
> > > > Could you please confirm that you can trigger an issue on the latest kernel?
> > > Sorry, I don't have an available latest kernel for now.
> > >
> > Can you do: "gdb ./vmlinux", execute "l *_vm_unmap_aliases+0x164" and provide
> > output?
> Sorry, I have lost the vmlinux with KASAN enabled and just got some
> instructions from logs.
>
> 0xffffffd010678da8 <_vm_unmap_aliases+0x134>: sub x22, x26, #0x28
>                x26 vbq->free
> 0xffffffd010678dac <_vm_unmap_aliases+0x138>: lsr x8, x22, #3
> 0xffffffd010678db0 <_vm_unmap_aliases+0x13c>: ldrb w8, [x8,x24]
> 0xffffffd010678db4 <_vm_unmap_aliases+0x140>: cbz w8,
> 0xffffffd010678dc0 <_vm_unmap_aliases+0x14c>
> 0xffffffd010678db8 <_vm_unmap_aliases+0x144>: mov x0, x22
> 0xffffffd010678dbc <_vm_unmap_aliases+0x148>: bl 0xffffffd0106c9a34
> <__asan_report_load8_noabort>
> 0xffffffd010678dc0 <_vm_unmap_aliases+0x14c>: ldr x22, [x22]
> 0xffffffd010678dc4 <_vm_unmap_aliases+0x150>: lsr x8, x22, #3
> 0xffffffd010678dc8 <_vm_unmap_aliases+0x154>: ldrb w8, [x8,x24]
> 0xffffffd010678dcc <_vm_unmap_aliases+0x158>: cbz w8,
> 0xffffffd010678dd8 <_vm_unmap_aliases+0x164>
> 0xffffffd010678dd0 <_vm_unmap_aliases+0x15c>: mov x0, x22
> 0xffffffd010678dd4 <_vm_unmap_aliases+0x160>: bl 0xffffffd0106c9a34
> <__asan_report_load8_noabort>
>
> >
> > --
> > Uladzislau Rezki