Re: possible deadlock in sk_clone_lock

On Mon 01-03-21 07:10:11, Shakeel Butt wrote:

On Mon, Mar 1, 2021 at 4:12 AM Michal Hocko [off-list ref] wrote:

quoted

On Fri 26-02-21 16:00:30, Shakeel Butt wrote:

quoted

On Fri, Feb 26, 2021 at 3:14 PM Mike Kravetz [off-list ref] wrote:

quoted

Cc: Michal

On 2/26/21 2:44 PM, Shakeel Butt wrote:

quoted

On Fri, Feb 26, 2021 at 2:09 PM syzbot
[off-list ref] wrote:

<snip>

quoted

quoted

other info that might help us debug this:

 Possible interrupt unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(hugetlb_lock);
                               local_irq_disable();
                               lock(slock-AF_INET);
                               lock(hugetlb_lock);
  <Interrupt>
    lock(slock-AF_INET);

 *** DEADLOCK ***

This has been reproduced on 4.19 stable kernel as well [1] and there
is a reproducer as well.

It seems like sendmsg(MSG_ZEROCOPY) from a buffer backed by hugetlb. I
wonder if we just need to make hugetlb_lock softirq-safe.

[1] https://syzkaller.appspot.com/bug?extid=6383ce4b0b8ec575ad93

Thanks Shakeel,

Commit c77c0a8ac4c5 ("mm/hugetlb: defer freeing of huge pages if in non-task
context") attempted to address this issue.  It uses a work queue to
acquire hugetlb_lock if the caller is !in_task().

In another recent thread, there was the suggestion to change the
!in_task to in_atomic.

I need to do some research on the subtle differences between in_task,
in_atomic, etc.  TBH, I 'thought' !in_task would prevent the issue
reported here.  But, that obviously is not the case.

I think the freeing is happening in the process context in this report
but it is creating the lock chain from softirq-safe slock to
irq-unsafe hugetlb_lock. So, two solutions I can think of are: (1)
always defer the freeing of hugetlb pages to a work queue or (2) make
hugetlb_lock softirq-safe.

There is __do_softirq so this should be in the soft IRQ context no?
Is this really reproducible with kernels which have c77c0a8ac4c5
applied?

Yes this is softirq context and syzbot has reproduced this on
linux-next 20210224.

Then how come this can ever be a problem? in_task() should exclude soft
irq context unless I am mistaken.
 

quoted

Btw. making hugetlb lock irq safe has been already discussed and it
seems to be much harder than expected as some heavy operations are done
under the lock. This is really bad.

What about just softirq-safe i.e. spin_[un]lock_bh()? Will it still be that bad?

This would be a similar problem to the irq variant. It would just result
in soft irq being delayed potentially.

quoted

Postponing the whole freeing
operation into a worker context is certainly possible but I would
consider it rather unfortunate. We would have to add some sync mechanism
to wait for hugetlb pages in flight to prevent from external
observability to the userspace. E.g. when shrinking the pool.

I think in practice recycling of hugetlb pages is a rare event, so we
might get away without the sync mechanism. How about start postponing
the freeing without sync mechanism and add it later if there are any
user reports complaining?

I think this should be a last resort. Maybe we can come up with
something better. E.g. break down the hugetlb_lock and use something
different for expensive operations.
-- 
Michal Hocko
SUSE Labs