Re: possible deadlock in sk_clone_lock

[Michal Hocko <mhocko@suse.com>]

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.
-- 
Michal Hocko
SUSE Labs