Re: [PATCH v6] serial: 8250: fix use-after-free in IRQ chain handling

[Wang Zhaolong]

> From: "Jiri Slaby"<jirislaby@xxxxxxxxxx>
> Date:  Wed, Jul 8, 2026, 2:04 PM
> Subject:  Re: [PATCH v6] serial: 8250: fix use-after-free in IRQ chain handling
> To: "Qiliang Yuan"<realwujing@xxxxxxxxx>, "Greg Kroah-Hartman"<gregkh@xxxxxxxxxxxxxxxxxxx>, "Anton Vorontsov"<avorontsov@xxxxxxxxxxxxx>, "Alan Cox"<alan@xxxxxxxxxx>
> Cc: <linux-kernel@xxxxxxxxxxxxxxx>, <linux-serial@xxxxxxxxxxxxxxx>, "Wang Zhaolong"<wangzhaolong@xxxxxxxxx>
> Ah, now I see you are fixing the same thing as:
> https://lore.kernel.org/all/20260708031115.3757150-1-wangzhaolong@xxxxxxxxx/
> 
> I did not look up who of you was first.
> 
> But:
> Reported-by: Wang Zhaolong <wangzhaolong@xxxxxxxxx>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221579
> 
> Looks like he reported and apparently tries to fix that too ;). You were 
> obviously CCed, talk to them and don't send two patches for the same issue.
> 
> On 07. 07. 26, 16:06, Qiliang Yuan wrote:
> ...
> > --- a/drivers/tty/serial/8250/8250_core.c
> > +++ b/drivers/tty/serial/8250/8250_core.c
> > @@ -131,10 +131,11 @@ static void serial_do_unlink(struct irq_info *i, struct uart_8250_port *up)
> >    * - allocate a new one, add it to the hashtable and return it.
> >    */
> >   static struct irq_info *serial_get_or_create_irq_info(const struct uart_8250_port *up)
> > +        __must_hold(&hash_mutex)
> >   {
> >           struct irq_info *i;
> >   
> > -        guard(mutex)(&hash_mutex);
> > +        lockdep_assert_held(&hash_mutex);
> >   
> >           hash_for_each_possible(irq_lists, i, node, up->port.irq)
> >                   if (i->irq == up->port.irq)
> > @@ -151,19 +152,37 @@ static struct irq_info *serial_get_or_create_irq_info(const struct uart_8250_por
> >           return i;
> >   }
> >   
> > +/*
> > + * serial_link_irq_chain() hooks the given 8250 port into the IRQ chain.
> > + *
> > + * hash_mutex must be held from the hash lookup through the first
> > + * request_irq() completion.  Dropping it earlier allows a concurrent
> > + * serial_unlink_irq_chain() to race in after i->head is published but
> > + * before the IRQ is fully set up — another port sharing the IRQ can then
> > + * join the chain and run the shared-IRQ THRE test while IRQ startup is
> > + * still in progress, triggering an "Unbalanced enable for IRQ" warning
> > + * in kernel/irq/manage.c.
> > + */
> >   static int serial_link_irq_chain(struct uart_8250_port *up)
> >   {
> >           struct irq_info *i;
> >           int ret;
> >   
> > +        guard(mutex)(&hash_mutex);
> 
> hash_mutex is no longer an appropriate name for this lock.
> 
> > +
> >           i = serial_get_or_create_irq_info(up);
> >           if (IS_ERR(i))
> >                   return PTR_ERR(i);
> >   
> > +        /*
> > +         * Serialise against the list manipulation in the interrupt handler
> > +         * and in serial_unlink_irq_chain().  hash_mutex is still held which
> > +         * prevents serial_unlink_irq_chain() from entering and freeing the
> > +         * irq_info until the first request_irq() completes.
> > +         */
> >           scoped_guard(spinlock_irq, &i->lock) {
> >                   if (i->head) {
> >                           list_add(&up->list, i->head);
> > -
> 
> Unrelated change.
> 
> >                           return 0;
> >                   }
> >   
> > @@ -171,11 +190,14 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
> >                   i->head = &up->list;
> >           }
> >   
> > -        ret = request_irq(up->port.irq, serial8250_interrupt, up->port.irqflags, up->port.name, i);
> > -        if (ret < 0)
> > +        ret = request_irq(up->port.irq, serial8250_interrupt,
> > +                          up->port.irqflags, up->port.name, i);
> > +        if (ret < 0) {
> >                   serial_do_unlink(i, up);
> > +                return ret;
> > +        }
> >   
> > -        return ret;
> > +        return 0;
> 
> Unrelated and mainly unneeded change.
> 
> thanks,
> -- 
> js
> suse labs
> 

Hi Jiri, Qiliang,

Thanks for looking at this.

>From my side, I do not want to make this a patch ownership discussion.  I have
sent my v3 here:

  https://lore.kernel.org/all/20260708072306.3921604-1-wangzhaolong@xxxxxxxxx/

It addresses the lock rename comment, keeps the change focused on the required
locking, and has been verified again with the QEMU ttyS1/ttyS3 shared IRQ
reproducer.

My main goal is simply to get this reproducible mainline regression fixed
soon, because with panic_on_warn=1 it breaks my regular build/regression
testing.  I do not have any promotion or credit pressure around which patch is
taken.

So I think the choice should be left to the maintainers.  Please take whichever
version you think is the better basis for mainline.

Thanks,
Wang Zhaolong