linux-kernel - Re: [PATCH] Uprobes: Fix deadlock between delayed_uprobe_lock and fs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 8 Feb 2019 14:03:55 +0530
From:   Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
To:     Oleg Nesterov <oleg@...hat.com>, Sherry Yang <sherryy@...roid.com>
Cc:     Michal Hocko <mhocko@...e.com>, srikar@...ux.vnet.ibm.com,
        songliubraving@...com, peterz@...radead.org, mingo@...hat.com,
        acme@...nel.org, alexander.shishkin@...ux.intel.com,
        jolsa@...hat.com, namhyung@...nel.org,
        linux-kernel@...r.kernel.org, aneesh.kumar@...ux.ibm.com,
        syzbot+1068f09c44d151250c33@...kaller.appspotmail.com
Subject: Re: [PATCH] Uprobes: Fix deadlock between delayed_uprobe_lock and
 fs_reclaim



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