linux-kernel - Re: possible deadlock in sk_clone

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <06edda9a-dce9-accd-11a3-97f6d5243ed1@oracle.com>
Date:   Tue, 2 Mar 2021 13:19:34 -0800
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     Michal Hocko <mhocko@...e.com>, Shakeel Butt <shakeelb@...gle.com>
Cc:     syzbot <syzbot+506c8a2a115201881d45@...kaller.appspotmail.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux MM <linux-mm@...ck.org>,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Mina Almasry <almasrymina@...gle.com>
Subject: Re: possible deadlock in sk_clone_lock

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@...e.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@...e.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