Re: [PATCH] mm/page_alloc: fix potential deadlock on zonelist_update_seq seqlock

Petr Mladek
2023-04-04T07:43:23Z

On Tue 2023-04-04 09:37:25, Tetsuo Handa wrote:
> syzbot is reporting circular locking dependency which involves
> zonelist_update_seq seqlock [1], for this lock is checked by memory
> allocation requests which do not need to be retried.
>
> We somehow need to prevent __alloc_pages_slowpath() from checking
> this lock. Since Petr Mladek thinks that __build_all_zonelists() can
> become a candidate for deferring printk() [2], let's make sure that
> current CPU/thread won't reach __alloc_pages_slowpath() while this lock
> is in use.
>
> Reported-by: syzbot <syzbot+223c7461c58c58a4cb10@syzkaller.appspotmail.com>
> Link: https://syzkaller.appspot.com/bug?extid=223c7461c58c58a4cb10 [1]
> Fixes: 3d36424b3b58 ("mm/page_alloc: fix race condition between build_all_zonelists and page allocation")
> Link: https://lkml.kernel.org/r/ZCrs+1cDqPWTDFNM@alley [2]

From the description is far from obvious how printk() is involved.
It might make sense to paste the entire lockdep splat. The links
are not guaranteed to stay around.

> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -6632,7 +6632,21 @@ static void __build_all_zonelists(void *data)
> int nid;
> int __maybe_unused cpu;
> pg_data_t *self = data;
> + unsigned long flags;
>
> + /*
> + * Since __alloc_pages_slowpath() spins if zonelist_update_seq.seqcount
> + * is odd, any memory allocation while zonelist_update_seq.seqcount is
> + * odd have to be avoided.
> + *
> + * Explicitly disable local irqs in order to avoid calling
> + * kmalloc(GFP_ATOMIC) from e.g. timer interrupt handler.
> + * Also, explicitly prevent printk() from synchronously waiting for
> + * port->lock because tty_insert_flip_string_and_push_buffer() might
> + * call kmalloc(GFP_ATOMIC | __GFP_NOWARN) while holding port->lock.
> + */
> + local_irq_save(flags);

The comment above printk_deferred_enter definition in
include/linux/printk.h says that interrupts need to be disabled.

But strictly speaking, it should be enough to disable preemption
there days. The reason is that is uses per-CPU reference counter.

Note that it used to be really important to disable interrupts
in the past. The messages were temporary stored in a per-CPU buffer
and the lockless algorithm was not safe for reentrancy.

> + printk_deferred_enter();
> write_seqlock(&zonelist_update_seq);
>
> #ifdef CONFIG_NUMA
> @@ -6671,6 +6685,8 @@ static void __build_all_zonelists(void *data)
> }
>
> write_sequnlock(&zonelist_update_seq);
> + printk_deferred_exit();
> + local_irq_restore(flags);
> }
>
> static noinline void __init

Otherwise, it looks fine from the printk() POV.

Best Regards,
Petr