Re: [PATCH] Uprobes: Fix deadlock between delayed_uprobe_lock and fs_reclaim

Ravi Bangoria
2019-02-08T08:32:21Z

On 2/6/19 7:06 PM, Oleg Nesterov wrote:
> Ravi, I am on vacation till the end of this week, can't read your patch
> carefully.
>
> I am not sure I fully understand the problem, but shouldn't we change
> binder_alloc_free_page() to use mmput_async() ? Like it does if trylock
> fails.

I don't understand binderfs code much so I'll let Sherry comment on this.

>
> In any case, I don't think memalloc_nofs_save() is what we need, see below.
>
> On 02/04, Ravi Bangoria wrote:
>>
>> There can be a deadlock between delayed_uprobe_lock and
>> fs_reclaim like:
>>
>> CPU0 CPU1
>> ---- ----
>> lock(fs_reclaim);
>> lock(delayed_uprobe_lock);
>> lock(fs_reclaim);
>> lock(delayed_uprobe_lock);
>>
>> Here CPU0 is a file system code path which results in
>> mmput()->__mmput()->uprobe_clear_state() with fs_reclaim
>> locked. And, CPU1 is a uprobe event creation path.
>
> But this is false positive, right? if CPU1 calls update_ref_ctr() then
> either ->mm_users is already zero so binder_alloc_free_page()->mmget_not_zero()
> will fail, or the caller of update_ref_ctr() has a reference and thus
> binder_alloc_free_page()->mmput() can't trigger __mmput() ?

Yes, it seems so.

So, IIUC, even though the locking sequence are actually opposite, *actual*
instances of the locks will never be able to lock simultaneously on both
the code path as warned by lockdep. Please correct me if I misunderstood.

[...]

>> + nofs_flags = memalloc_nofs_save();
>> mutex_lock(&delayed_uprobe_lock);
>> if (d > 0)
>> ret = delayed_uprobe_add(uprobe, mm);
>> else
>> delayed_uprobe_remove(uprobe, mm);
>> mutex_unlock(&delayed_uprobe_lock);
>> + memalloc_nofs_restore(nofs_flags);
>
> PF_MEMALLOC_NOFS is only needed when we are going to call delayed_uprobe_add()
> which does kzalloc(GFP_KERNEL). Can't we simply change it tuse use use GFP_NOFS
> instead?

Yes, I can use GFP_NOFS. (and same was suggested by Aneesh as well)

But from https://lwn.net/Articles/710545/, I found that community
is planning to deprecate the GFP_NOFS flag?

-Ravi