Re: possible deadlock in sk_clone_lock

[Mike Kravetz <mike.kravetz@oracle.com>]

On 3/2/21 6:29 AM, Michal Hocko wrote:
> On Tue 02-03-21 06:11:51, Shakeel Butt wrote:
>> On Tue, Mar 2, 2021 at 1:44 AM Michal Hocko <mhocko@suse.com> wrote:
>>>
>>> On Mon 01-03-21 17:16:29, Mike Kravetz wrote:
>>>> On 3/1/21 9:23 AM, Michal Hocko wrote:
>>>>> On Mon 01-03-21 08:39:22, Shakeel Butt wrote:
>>>>>> On Mon, Mar 1, 2021 at 7:57 AM Michal Hocko <mhocko@suse.com> wrote:
>>>>> [...]
>>>>>>> Then how come this can ever be a problem? in_task() should exclude soft
>>>>>>> irq context unless I am mistaken.
>>>>>>>
>>>>>>
>>>>>> If I take the following example of syzbot's deadlock scenario then
>>>>>> CPU1 is the one freeing the hugetlb pages. It is in the process
>>>>>> context but has disabled softirqs (see __tcp_close()).
>>>>>>
>>>>>>         CPU0                    CPU1
>>>>>>         ----                    ----
>>>>>>    lock(hugetlb_lock);
>>>>>>                                 local_irq_disable();
>>>>>>                                 lock(slock-AF_INET);
>>>>>>                                 lock(hugetlb_lock);
>>>>>>    <Interrupt>
>>>>>>      lock(slock-AF_INET);
>>>>>>
> [...]
>>> Wouldn't something like this help? It is quite ugly but it would be
>>> simple enough and backportable while we come up with a more rigorous
>>> solution. What do you think?
>>>
>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>>> index 4bdb58ab14cb..c9a8b39f678d 100644
>>> --- a/mm/hugetlb.c
>>> +++ b/mm/hugetlb.c
>>> @@ -1495,9 +1495,11 @@ static DECLARE_WORK(free_hpage_work, free_hpage_workfn);
>>>  void free_huge_page(struct page *page)
>>>  {
>>>         /*
>>> -        * Defer freeing if in non-task context to avoid hugetlb_lock deadlock.
>>> +        * Defer freeing if in non-task context or when put_page is called
>>> +        * with IRQ disabled (e.g from via TCP slock dependency chain) to
>>> +        * avoid hugetlb_lock deadlock.
>>>          */
>>> -       if (!in_task()) {
>>> +       if (!in_task() || irqs_disabled()) {
>>
>> Does irqs_disabled() also check softirqs?
> 
> Nope it doesn't AFAICS. I was referring to the above lockdep splat which
> claims irq disabled to be the trigger. But now that you are mentioning
> that it would be better to replace in_task() along the way. We have
> discussed that in another email thread and I was suggesting to use
> in_atomic() which should catch also bh disabled situation. The big IF is
> that this needs preempt count to be enabled unconditionally. There are
> changes in the RCU tree heading that direction.

I have not been following developments in preemption and the RCU tree. 
The comment for in_atomic() says:

/*
 * Are we running in atomic context?  WARNING: this macro cannot
 * always detect atomic context; in particular, it cannot know about
 * held spinlocks in non-preemptible kernels.  Thus it should not be
 * used in the general case to determine whether sleeping is possible.
 * Do not use in_atomic() in driver code.
 */

That does seem to be the case.  I verified in_atomic can detect softirq
context even in non-preemptible kernels.  But, as the comment says it
will not detect a held spinlock in non-preemptible kernels.  So, I think
in_atomic would be better than the current check for !in_task.  That
would handle this syzbot issue, but we could still have issues if the
hugetlb put_page path is called while someone is holding a spinlock with
all interrupts enabled.  Looks like there is no way to detect this
today in non-preemptible kernels.  in_atomic does detect spinlocks held
in preemptible kernels.

I might suggest changing !in_task to in_atomic for now, and then work on
a more robust solution.  I'm afraid such a robust solution will
require considerable effort.  It would need to handle put_page being
called in any context: hardirq, softirq, spinlock held ...  The
put_page/free_huge_page path will need to offload (workqueue or
something else) any processing that can possibly sleep.

Is it worth making the in_atomic change now, or should we just start
working on the more robust complete solution?
-- 
Mike Kravetz