On 3/7/19 6:24 PM, Aaron Lu wrote:
> On Thu, Mar 07, 2019 at 05:47:13PM +0300, Andrey Ryabinin wrote:
>>
>>
>> On 3/7/19 5:43 PM, Aaron Lu wrote:
>>> On Tue, Jan 29, 2019 at 05:01:50PM -0800, Andrew Morton wrote:
>>>> On Wed, 30 Jan 2019 09:42:06 +0900 Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> wrote:
>>>>
>>>>>>>
>>>>>>> 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."...
>>>>
>>>> Whereabouts in the vfree() path can the kernel sleep?
>>>
>>> (Sorry for the late reply.)
>>>
>>> Adding Andrey Ryabinin, author of commit 52414d3302577bb6
>>> ("kvfree(): fix misleading comment"), maybe Andrey remembers
>>> where vfree() can sleep.
>>>
>>> In the meantime, does "cond_resched_lock(&vmap_area_lock);" in
>>> __purge_vmap_area_lazy() count as a sleep point?
>>
>> Yes, this is the place (the only one) where vfree() can sleep.
>
> OK, thanks for the quick confirm.
>
> So what about this: use __vfree_deferred() when:
> - in_interrupt(), because we can't use mutex_trylock() as pointed out
> by Tetsuo;
> - in_atomic(), because cond_resched_lock();
> - irqs_disabled(), as smp_call_function_many() will deadlock.
>
> An untested diff to show the idea(not sure if warn is needed):
>
It was discussed before. You're not the first one suggesting something like this.
There is the comment near in_atomic() explaining well why and when your patch won't work.
The easiest way of making vfree() to be safe in atomic contexts is this patch:
http://lkml.kernel.org/r/20170330102719.13119-1-aryabinin@xxxxxxxxxxxxx
But the final decision at that time was to fix caller so the call vfree from sleepable context instead:
http://lkml.kernel.org/r/20170330152229.f2108e718114ed77acae7405@xxxxxxxxxxxxxxxxxxxx
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index e86ba6e74b50..28d200f054b0 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1578,7 +1578,7 @@ void vfree_atomic(const void *addr)
>
> static void __vfree(const void *addr)
> {
> - if (unlikely(in_interrupt()))
> + if (unlikely(in_interrupt() || in_atomic() || irqs_disabled()))
> __vfree_deferred(addr);
> else
> __vunmap(addr, 1);
> @@ -1606,8 +1606,6 @@ void vfree(const void *addr)
>
> kmemleak_free(addr);
>
> - might_sleep_if(!in_interrupt());
> -
> if (!addr)
> return;
>
>
