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@xxxxxxxxxxxxxxxxxxxxxxxxx> > 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
