Messages in this thread | | | Subject | Re: possible deadlock in sk_clone_lock | From | Mike Kravetz <> | Date | Tue, 2 Mar 2021 13:19:34 -0800 |
| |
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
| |