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

[Ravi Bangoria <ravi.bangoria@linux.ibm.com>]

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