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
