Re: [PATCH] mm: fix sleeping function warning in alloc_swap_info

[Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>]

Yang Shi wrote:
> On Tue, Jan 29, 2019 at 1:12 PM Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
> >
> > On 2019/01/30 4:13, Andrew Morton wrote:
> > > On Tue, 29 Jan 2019 20:43:20 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> > >
> > >> On 2019/01/29 16:21, Jiufei Xue wrote:
> > >>> Trinity reports BUG:
> > >>>
> > >>> sleeping function called from invalid context at mm/vmalloc.c:1477
> > >>> in_atomic(): 1, irqs_disabled(): 0, pid: 12269, name: trinity-c1
> > >>>
> > >>> [ 2748.573460] Call Trace:
> > >>> [ 2748.575935]  dump_stack+0x91/0xeb
> > >>> [ 2748.578512]  ___might_sleep+0x21c/0x250
> > >>> [ 2748.581090]  remove_vm_area+0x1d/0x90
> > >>> [ 2748.583637]  __vunmap+0x76/0x100
> > >>> [ 2748.586120]  __se_sys_swapon+0xb9a/0x1220
> > >>> [ 2748.598973]  do_syscall_64+0x60/0x210
> > >>> [ 2748.601439]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > >>>
> > >>> This is triggered by calling kvfree() inside spinlock() section in
> > >>> function alloc_swap_info().
> > >>> Fix this by moving the kvfree() after spin_unlock().
> > >>>
> > >>
> > >> Excuse me? But isn't kvfree() safe to be called with spinlock held?
> > >
> > > Yes, I'm having trouble spotting where kvfree() can sleep.  Perhaps it
> > > *used* to sleep on mutex_lock(vmap_purge_lock), but
> > > try_purge_vmap_area_lazy() is using mutex_trylock().  Confused.
> > >
> > > kvfree() darn well *shouldn't* sleep!
> > >
> >
> > If I recall correctly, there was an attempt to allow vfree() to sleep
> > but that attempt failed, and the change to allow vfree() to sleep was
> > reverted. Thus, vfree() had been "Context: Any context except NMI.".

That attempt was not reverted. Instead vfree_atomic() was added.

> >
> > If we want to allow vfree() to sleep, at least we need to test with
> > kvmalloc() == vmalloc() (i.e. force kvmalloc()/kvfree() users to use
> > vmalloc()/vfree() path). For now, reverting the
> > "Context: Either preemptible task context or not-NMI interrupt." change
> > will be needed for stable kernels.
> 
> So, the comment for vfree "May sleep if called *not* from interrupt
> context." is wrong?

Commit bf22e37a641327e3 ("mm: add vfree_atomic()") says

    We are going to use sleeping lock for freeing vmap.  However some
    vfree() users want to free memory from atomic (but not from interrupt)
    context.  For this we add vfree_atomic() - deferred variation of vfree()
    which can be used in any atomic context (except NMIs).

and commit 52414d3302577bb6 ("kvfree(): fix misleading comment") made

    - * Context: Any context except NMI.
    + * Context: Either preemptible task context or not-NMI interrupt.

change. But I think that we converted kmalloc() to kvmalloc() without checking
context of kvfree() callers. Therefore, I think that kvfree() needs to use
vfree_atomic() rather than just saying "vfree() might sleep if called not in
interrupt context."...