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

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

[Qiliang Yuan]

serial_unlink_irq_chain() holds hash_mutex and calls free_irq() + kfree(i)
on an empty port list.  serial_link_irq_chain() used to release hash_mutex
after serial_get_or_create_irq_info() but before acquiring i->lock, so a
concurrent unlink could see list_empty() as true while a port was being
added, free i, and cause a use-after-free.

Dropping hash_mutex before request_irq() completes also lets another port
on the same IRQ join the chain and run the THRE test while IRQ startup is
still in progress, triggering "Unbalanced enable for IRQ" because
irq_shutdown() in the premature free_irq() path increments desc->depth and
breaks the disable_irq/enable_irq pairing in serial8250_THRE_test().

Hold hash_mutex across the whole first request_irq() completion (including
the error cleanup path) by taking it at the top of serial_link_irq_chain()
via guard(mutex)(&hash_mutex).  serial_unlink_irq_chain() already holds
hash_mutex throughout, so the race is closed.

With hash_mutex now taken by the caller, replace the guard in
serial_get_or_create_irq_info() with lockdep_assert_held() and add
__must_hold(&hash_mutex) for static analysis.

Fixes: 768aec0b5bcc ("serial: 8250: fix shared interrupts issues with SMP and RT kernels")
Reported-by: Wang Zhaolong <wangzhaolong@fnnas.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221579
Signed-off-by: Qiliang Yuan <realwujing@gmail.com>
---
V5 -> V6:
- Keep guard(mutex) and scoped_guard(spinlock_irq) style instead of
  manual locking; the lock lifecycle fits the scope-based model.

V4 -> V5:
- Add __must_hold(&hash_mutex) annotation to
  serial_get_or_create_irq_info() for static analysis.

V3 -> V4:
- Move cleanup under hash_mutex on request_irq() failure to prevent a
  second port from joining the chain before the irq_info is cleaned up.
- Fix inaccurate description of irq_shutdown() in commit message.

V2 -> V3:
- Hold hash_mutex across the first request_irq() completion to prevent a
  second port from joining the chain and running the shared-IRQ THRE test
  while IRQ startup is still in progress.

V1 -> V2:
- Add Reported-by tag from Wang Zhaolong.

v5: https://lore.kernel.org/r/20260624-bug-221579-8250-shared-irq-race-v5-1-15d841f89e1e@gmail.com
v4: https://lore.kernel.org/r/20260529-bug-221579-8250-shared-irq-race-v4-1-cfda63b4420f@gmail.com
v3: https://lore.kernel.org/r/20260529-bug-221579-8250-shared-irq-race-v3-1-fe4d430862a9@gmail.com
v2: https://lore.kernel.org/r/20260528-bug-221579-8250-shared-irq-race-v2-1-06531202e54d@gmail.com
v1: https://lore.kernel.org/r/20260528-bug-221579-8250-shared-irq-race-v1-1-30980cca02f3@gmail.com
---
 drivers/tty/serial/8250/8250_core.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index a428e88938eb7..6fd3bb2eee233 100644
--- 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);
+
 	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);
-
 			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;
 }
 
 static void serial_unlink_irq_chain(struct uart_8250_port *up)

---
base-commit: eb3f4b7426cfd2b79d65b7d37155480b32259a11
change-id: 20260528-bug-221579-8250-shared-irq-race-581e4900a178

Best regards,
-- 
Jing Wu <realwujing@gmail.com>

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

[Jiri Slaby]

Ah, now I see you are fixing the same thing as:
https://lore.kernel.org/all/20260708031115.3757150-1-wangzhaolong@fnnas.com/

I did not look up who of you was first.

But:
Reported-by: Wang Zhaolong <wangzhaolong@fnnas.com>
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

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

[Jing Wu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1681 bytes --]

From: Qiliang Yuan <realwujing@gmail.com>

On Wed, Jul 08, 2026 at 08:03:55AM +0200, Jiri Slaby wrote:
> Ah, now I see you are fixing the same thing as:
> https://lore.kernel.org/all/20260708031115.3757150-1-wangzhaolong@fnnas.com/
>
> I did not look up who of you was first.
>
> 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.

Wang reported Bugzilla #221579 and sent an initial patch on May 27.
I sent my v1 the next day (May 28), and have iterated with Wang's review
input across v1..v6 — the timeline shows Wang's v2 (Jul 8) was written
after and based on the direction of our v5/v6 series:

  v1: May 28, Qiliang    [PATCH] serial: 8250: fix use-after-free in IRQ chain handling
  v3: May 29, Wang       confirmed "v3 fixes the Bugzilla reproducer on my setup."
  v5: Jun 24, Qiliang    [PATCH v5] (with manual locking)
  v6: Jul 7,  Qiliang    [PATCH v6] (back to guard style)
  v2: Jul 8,  Wang       [PATCH v2] (same lock-move skeleton as v5/v6)

Wang's v2 has the same lock-move skeleton but is missing three pieces
we've added through review:

  - lockdep_assert_held() and __must_hold() in serial_get_or_create_irq_info()
  - hash_mutex covering the request_irq() error cleanup (use-after-free fix)
  - addressing both the use-after-free and "Unbalanced enable" races

I'll add Co-developed-by: Wang Zhaolong to the next revision.

> hash_mutex is no longer an appropriate name for this lock.

Ack, will rename in v7.

> Unrelated change.
> Unrelated and mainly unneeded change.

Ack, will drop both the blank-line removal and the return 0 change.

Thanks,
Qiliang

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

[Jing Wu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1681 bytes --]

From: Qiliang Yuan <realwujing@gmail.com>

On Wed, Jul 08, 2026 at 08:03:55AM +0200, Jiri Slaby wrote:
> Ah, now I see you are fixing the same thing as:
> https://lore.kernel.org/all/20260708031115.3757150-1-wangzhaolong@fnnas.com/
>
> I did not look up who of you was first.
>
> 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.

Wang reported Bugzilla #221579 and sent an initial patch on May 27.
I sent my v1 the next day (May 28), and have iterated with Wang's review
input across v1..v6 — the timeline shows Wang's v2 (Jul 8) was written
after and based on the direction of our v5/v6 series:

  v1: May 28, Qiliang    [PATCH] serial: 8250: fix use-after-free in IRQ chain handling
  v3: May 29, Wang       confirmed "v3 fixes the Bugzilla reproducer on my setup."
  v5: Jun 24, Qiliang    [PATCH v5] (with manual locking)
  v6: Jul 7,  Qiliang    [PATCH v6] (back to guard style)
  v2: Jul 8,  Wang       [PATCH v2] (same lock-move skeleton as v5/v6)

Wang's v2 has the same lock-move skeleton but is missing three pieces
we've added through review:

  - lockdep_assert_held() and __must_hold() in serial_get_or_create_irq_info()
  - hash_mutex covering the request_irq() error cleanup (use-after-free fix)
  - addressing both the use-after-free and "Unbalanced enable" races

I'll add Co-developed-by: Wang Zhaolong to the next revision.

> hash_mutex is no longer an appropriate name for this lock.

Ack, will rename in v7.

> Unrelated change.
> Unrelated and mainly unneeded change.

Ack, will drop both the blank-line removal and the return 0 change.

Thanks,
Qiliang

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

[Wang Zhaolong]

> From: "Jiri Slaby"<jirislaby@kernel.org>
> 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@gmail.com>, "Greg Kroah-Hartman"<gregkh@linuxfoundation.org>, "Anton Vorontsov"<avorontsov@ru.mvista.com>, "Alan Cox"<alan@redhat.com>
> Cc: <linux-kernel@vger.kernel.org>, <linux-serial@vger.kernel.org>, "Wang Zhaolong"<wangzhaolong@fnnas.com>
> Ah, now I see you are fixing the same thing as:
> https://lore.kernel.org/all/20260708031115.3757150-1-wangzhaolong@fnnas.com/
> 
> I did not look up who of you was first.
> 
> But:
> Reported-by: Wang Zhaolong <wangzhaolong@fnnas.com>
> 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@fnnas.com/

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

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

[Jing Wu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 997 bytes --]

On Wed, Jul 08, 2026 at 03:33:36PM +0800, Wang Zhaolong wrote:
> I do not want to make this a patch ownership discussion.

Let's be clear about the facts, then.

You reported the bug.  Our team fixed it — the next day.  May 28, our
v1.  You confirmed on May 29 that our v3 fixes your reproducer.

Since then, you've posted three versions — v1, v2, v3 — that
incrementally copy the work we developed and refined through seven
iterations.  Your v2 copied our lock-move skeleton.  Your v3 copied the
irq_chain_mutex rename, __must_hold(), and lockdep_assert_held() — all
written by us, all reviewed by Jiri on our patches.

You list these as your own "Changes in v3" with zero attribution.

This is not a "patch ownership discussion."  You copied our code.  The
kernel is GPLv2 — that means you can use the code, but you must
preserve the copyright notices.  Co-developed-by is the mechanism for
that.

You reported the bug — we fixed it first.  We credited you.  Now add
the tags.

Jing

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

[Wang Zhaolong]

> From: "Jing Wu"<realwujing@gmail.com>
> Date:  Wed, Jul 8, 2026, 3:57 PM
> Subject:  Re: [PATCH v6] serial: 8250: fix use-after-free in IRQ chain handling
> To: <wangzhaolong@fnnas.com>
> Cc: <jirislaby@kernel.org>, <gregkh@linuxfoundation.org>, <linux-serial@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <yuanql9@chinatelecom.cn>
> On Wed, Jul 08, 2026 at 03:33:36PM +0800, Wang Zhaolong wrote:
> > I do not want to make this a patch ownership discussion.
> 
> Let's be clear about the facts, then.
> 
> You reported the bug.  Our team fixed it — the next day.  May 28, our
> v1.  You confirmed on May 29 that our v3 fixes your reproducer.
> 
> Since then, you've posted three versions — v1, v2, v3 — that
> incrementally copy the work we developed and refined through seven
> iterations.  Your v2 copied our lock-move skeleton.  Your v3 copied the
> irq_chain_mutex rename, __must_hold(), and lockdep_assert_held() — all
> written by us, all reviewed by Jiri on our patches.
> 
> You list these as your own "Changes in v3" with zero attribution.
> 
> This is not a "patch ownership discussion."  You copied our code.  The
> kernel is GPLv2 — that means you can use the code, but you must
> preserve the copyright notices.  Co-developed-by is the mechanism for
> that.
> 
> You reported the bug — we fixed it first.  We credited you.  Now add
> the tags.
> 
> Jing
> 


Hi Jing,

This accusation is not acceptable.

My v1 was posted on May 27, before your v1, and it already made the core
locking change: move hash_mutex to serial_link_irq_chain() and keep it held
across irq_info lookup, i->head publication, the first request_irq(), and the
request_irq() failure cleanup:

  https://lore.kernel.org/r/20260527092052.2086342-1-wangzhaolong@fnnas.com

So the claim that my v2 copied your lock-move skeleton is false.

That same locking change also closes the irq_info UAF window, because
serial_unlink_irq_chain() takes the same mutex before free_irq() and
serial_do_unlink()/kfree(i).  My commit message focused on the IRQ warning
because that was the failure I reported and reproduced.

For v3, the irq_chain_mutex rename was made in response to Jiri's review
comment that hash_mutex was no longer an appropriate name.  The __must_hold()
and lockdep_assert_held() additions document the caller-held mutex after the
lock move; they are not the functional fix.

I do not agree to a Co-developed-by tag for my patch, and please do not add
Co-developed-by for me without my agreement and Signed-off-by.

I do not want to keep tracking or arguing about this issue.  My only goal is
to see this regression fixed upstream.

Thanks,
Wang

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

[Jing Wu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1921 bytes --]

On Wed, Jul 08, 2026 at 08:03:55AM +0200, Jiri Slaby wrote:
> Ah, now I see you are fixing the same thing as:
> https://lore.kernel.org/all/20260708031115.3757150-1-wangzhaolong@fnnas.com/
> I did not look up who of you was first.
> 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.

I sent a v7 [1] addressing the three review items you raised, and added
Co-developed-by: Wang Zhaolong and Qiliang Yuan.

Wang has just posted a v3 [2] which copies the very same changes from
our series — irq_chain_mutex rename, __must_hold(), lockdep_assert_held()
— without any acknowledgment.  No Co-developed-by, no mention of our
series in the changelog.  It presents these as his own improvements.

The full timeline, for the record:

  May 27  Wang v1 — only the THRE test race, no use-after-free fix
  May 28  Our v1 — both races, full fix
  May 29  Wang on our v3 thread: "v3 fixes the Bugzilla reproducer"
  Jun 24  Our v5 — added __must_hold() and lockdep_assert_held()
  Jul  7  Our v6 — back to guard style
  Jul  8  Our v7 — renamed hash_mutex to irq_chain_mutex (per Jiri)
  Jul  8  Wang v3 — copies the rename, __must_hold(), lockdep_assert_held()
                  from our v5/v7, claims as own work

Wang's v2 (Jul 8) was posted after our v5 and v6 and uses the same
lock-move skeleton.  His v3 (Jul 8) was posted after our v7 and copies
the irq_chain_mutex rename and __must_hold()/lockdep_assert_held() that
we added in v5 and v7 — yet his changelog gives no credit.

I've been happy to add him as co-developer on our v7.  I would expect
the same basic courtesy in return, but apparently that's too much to
ask.

[1] https://lore.kernel.org/r/20260708-bug-221579-8250-shared-irq-race-v7-1-xxx@gmail.com
[2] https://lore.kernel.org/all/20260708072306.3921604-1-wangzhaolong@fnnas.com/

Thanks,
Jing

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

[Wang Zhaolong]

> From: "Jing Wu"<realwujing@gmail.com>
> Date:  Wed, Jul 8, 2026, 3:39 PM
> Subject:  Re: [PATCH v6] serial: 8250: fix use-after-free in IRQ chain handling
> To: <jirislaby@kernel.org>
> Cc: <gregkh@linuxfoundation.org>, <avorontsov@ru.mvista.com>, <alan@redhat.com>, <linux-kernel@vger.kernel.org>, <linux-serial@vger.kernel.org>, <wangzhaolong@fnnas.com>, <yuanql9@chinatelecom.cn>
> On Wed, Jul 08, 2026 at 08:03:55AM +0200, Jiri Slaby wrote:
> > Ah, now I see you are fixing the same thing as:
> > https://lore.kernel.org/all/20260708031115.3757150-1-wangzhaolong@fnnas.com/
> > I did not look up who of you was first.
> > 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.
> 
> I sent a v7 [1] addressing the three review items you raised, and added
> Co-developed-by: Wang Zhaolong and Qiliang Yuan.
> 
> Wang has just posted a v3 [2] which copies the very same changes from
> our series — irq_chain_mutex rename, __must_hold(), lockdep_assert_held()
> — without any acknowledgment.  No Co-developed-by, no mention of our
> series in the changelog.  It presents these as his own improvements.
> 
> The full timeline, for the record:
> 
>   May 27  Wang v1 — only the THRE test race, no use-after-free fix

Hi Jing,

Just one technical clarification: my v1 also moved hash_mutex to
serial_link_irq_chain() and kept it held across irq_info lookup, i->head
publication, the first request_irq(), and the request_irq() failure cleanup:

  https://lore.kernel.org/r/20260527092052.2086342-1-wangzhaolong@fnnas.com

Since serial_unlink_irq_chain() takes the same mutex before free_irq() and
serial_do_unlink()/kfree(i), that also closes the irq_info UAF window.  My
commit message focused on the IRQ warning because that was the failure I
reported and reproduced.

>   May 28  Our v1 — both races, full fix
>   May 29  Wang on our v3 thread: "v3 fixes the Bugzilla reproducer"
>   Jun 24  Our v5 — added __must_hold() and lockdep_assert_held()
>   Jul  7  Our v6 — back to guard style
>   Jul  8  Our v7 — renamed hash_mutex to irq_chain_mutex (per Jiri)
>   Jul  8  Wang v3 — copies the rename, __must_hold(), lockdep_assert_held()
>                   from our v5/v7, claims as own work
> 
> Wang's v2 (Jul 8) was posted after our v5 and v6 and uses the same
> lock-move skeleton.  His v3 (Jul 8) was posted after our v7 and copies
> the irq_chain_mutex rename and __must_hold()/lockdep_assert_held() that
> we added in v5 and v7 — yet his changelog gives no credit.
> 
> I've been happy to add him as co-developer on our v7.  I would expect
> the same basic courtesy in return, but apparently that's too much to
> ask.
> 
> [1] https://lore.kernel.org/r/20260708-bug-221579-8250-shared-irq-race-v7-1-xxx@gmail.com
> [2] https://lore.kernel.org/all/20260708072306.3921604-1-wangzhaolong@fnnas.com/
> 
> Thanks,
> Jing
> 

 I do not want to keep tracking or arguing about this issue. My only goal is to get this
regression fixed upstream.

Thanks,
Wang

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

[Jing Wu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1231 bytes --]

On Wed, Jul 08, 2026 at 03:53:26PM +0800, Wang Zhaolong wrote:
> Just one technical clarification: my v1 also moved hash_mutex to
> serial_link_irq_chain() and kept it held across irq_info lookup, i->head
> publication, the first request_irq(), and the request_irq() failure cleanup

Your v1 commit message discusses only the "Unbalanced enable for IRQ"
warning — it never mentions the use-after-free.  Our v1, one day later,
was the first to identify and document *both* races.  You confirmed on
May 29 that our v3 fixes your reproducer.

But the v1 lock-move is not the issue.  The issue is what happened after.

Our v5 (Jun 24) added lockdep_assert_held() and __must_hold() — reviewed
by Jiri on our v4.  Our v7 (Jul 8) renamed hash_mutex to irq_chain_mutex
— per Jiri's review on our v6.  Your v3 (Jul 8, after our v7) copies all
three of these improvements and presents them in your changelog as:

  "Changes in v3:
   - Rename hash_mutex to irq_chain_mutex
   - Add __must_hold() and lockdep_assert_held()"

That is not a technical clarification.  That is taking work we wrote,
reviewed by Jiri on our patches, and claiming it as your own.

Add the Co-developed-by tags.  We already added them for you on our v7.

Jing

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

[Wang Zhaolong]

> From: "Jing Wu"<realwujing@gmail.com>
> Date:  Wed, Jul 8, 2026, 4:08 PM
> Subject:  Re: [PATCH v6] serial: 8250: fix use-after-free in IRQ chain handling
> To: <wangzhaolong@fnnas.com>
> Cc: <jirislaby@kernel.org>, <gregkh@linuxfoundation.org>, <linux-serial@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <yuanql9@chinatelecom.cn>
> On Wed, Jul 08, 2026 at 03:53:26PM +0800, Wang Zhaolong wrote:
> > Just one technical clarification: my v1 also moved hash_mutex to
> > serial_link_irq_chain() and kept it held across irq_info lookup, i->head
> > publication, the first request_irq(), and the request_irq() failure cleanup
> 
> Your v1 commit message discusses only the "Unbalanced enable for IRQ"
> warning — it never mentions the use-after-free.  Our v1, one day later,
> was the first to identify and document *both* races.  You confirmed on
> May 29 that our v3 fixes your reproducer.
> 
> But the v1 lock-move is not the issue.  The issue is what happened after.
> 
> Our v5 (Jun 24) added lockdep_assert_held() and __must_hold() — reviewed
> by Jiri on our v4.  Our v7 (Jul 8) renamed hash_mutex to irq_chain_mutex
> — per Jiri's review on our v6.  Your v3 (Jul 8, after our v7) copies all
> three of these improvements and presents them in your changelog as:
> 
>   "Changes in v3:
>    - Rename hash_mutex to irq_chain_mutex
>    - Add __must_hold() and lockdep_assert_held()"
> 
> That is not a technical clarification.  That is taking work we wrote,
> reviewed by Jiri on our patches, and claiming it as your own.
> 
> Add the Co-developed-by tags.  We already added them for you on our v7.
> 
> Jing
> 

Hi Jing,

No.

A changelog entry describes what changed between my v2 and my v3.  It is not
a claim that nobody else ever wrote a similar line in another patch.

The functional fix in my series was already in my v1 on May 27.  Whether my
v1 commit message named the UAF or not does not change the code: holding the
same mutex across link and unlink closes that window too.  My v1 commit
message intentionally described the failure mode I had reproduced and
validated: the unbalanced IRQ enable warning.  I did not want to overstate the
impact beyond the evidence I had at the time.

The v3 rename follows Jiri's review comment that hash_mutex is no longer an
appropriate name.  The __must_hold() and lockdep_assert_held() annotations
document the locking rule after the mutex was moved to the caller.  They are
not the functional fix.

I do not agree that Co-developed-by is appropriate for my patch, and I do not
authorize adding a Co-developed-by tag for me without my Signed-off-by.

I am not going to continue this argument.  Maintainers can decide which patch,
if any, to take.

Thanks,
Wang

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

[Greg KH]

On Wed, Jul 08, 2026 at 04:30:21PM +0800, Wang Zhaolong wrote:
> Hi Jing,
> 
> No.
> 
> A changelog entry describes what changed between my v2 and my v3.  It is not
> a claim that nobody else ever wrote a similar line in another patch.
> 
> The functional fix in my series was already in my v1 on May 27.  Whether my
> v1 commit message named the UAF or not does not change the code: holding the
> same mutex across link and unlink closes that window too.  My v1 commit
> message intentionally described the failure mode I had reproduced and
> validated: the unbalanced IRQ enable warning.  I did not want to overstate the
> impact beyond the evidence I had at the time.
> 
> The v3 rename follows Jiri's review comment that hash_mutex is no longer an
> appropriate name.  The __must_hold() and lockdep_assert_held() annotations
> document the locking rule after the mutex was moved to the caller.  They are
> not the functional fix.
> 
> I do not agree that Co-developed-by is appropriate for my patch, and I do not
> authorize adding a Co-developed-by tag for me without my Signed-off-by.
> 
> I am not going to continue this argument.  Maintainers can decide which patch,
> if any, to take.

As I said before, I'm not going to take either until you all can agree.

And yes, a co-developed-by does require a signed-off-by.  Without you
all agreeing, none of this is going to be acceptable, sorry.

Also, all of these really look like they were created/found/whatever by
a LLM, which is not being documented, and for that reason alone I think
I need to reject all of these until that is properly addressed.

thanks,

greg k-h

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

[Wang Zhaolong]

Hi Greg,

> 
> As I said before, I'm not going to take either until you all can agree.
> 
> And yes, a co-developed-by does require a signed-off-by.  Without you
> all agreeing, none of this is going to be acceptable, sorry.
> 

Understood.

I do not agree to a Co-developed-by tag with Jing/Qiliang.  My v1 was posted
before that series and already contained the functional lock move.  I
reviewed/tested parts of the other series against my reproducer, but I do not
consider this a jointly developed patch.

I do not require my patch to be the one accepted upstream.  If another version
is used, I am fine with keeping Reported-by and a Link to my original patch,
and I can retest the final version against my QEMU ttyS1/ttyS3 shared IRQ
reproducer if needed.  But I do not provide my Signed-off-by for a
Co-developed-by tag.

> Also, all of these really look like they were created/found/whatever by
> a LLM, which is not being documented, and for that reason alone I think
> I need to reject all of these until that is properly addressed.
> 

Regarding LLM assistance, my v1 explicitly included:

Assisted-by: Codex:gpt-5

That tag was dropped in later revisions while refreshing the patch. The bug itself
was found and reproduced by me, not by an LLM.  I used the tool to help refine
the patch text and write the reproducer/test code, then reviewed the code
myself and validated the result with the QEMU ttyS1/ttyS3 shared IRQ
reproducer before submitting.  I also spent time testing/reviewing the other
series; for example, one earlier revision did not pass the reproducer, which I
reported in that thread。

I understand this may mean my patch is not taken.  I am fine with that.  I do not want
to spend more community time on this dispute.

I will stop here and leave the technical record as-is.  I hope the regression
can still be fixed upstream without spending more time on the dispute.

Best regards,
Wang Zhaolong

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

[Greg KH]

On Fri, Jul 10, 2026 at 09:15:19PM +0800, Wang Zhaolong wrote:
> Hi Greg,
> 
> > 
> > As I said before, I'm not going to take either until you all can agree.
> > 
> > And yes, a co-developed-by does require a signed-off-by.  Without you
> > all agreeing, none of this is going to be acceptable, sorry.
> > 
> 
> Understood.
> 
> I do not agree to a Co-developed-by tag with Jing/Qiliang.  My v1 was posted
> before that series and already contained the functional lock move.  I
> reviewed/tested parts of the other series against my reproducer, but I do not
> consider this a jointly developed patch.
> 
> I do not require my patch to be the one accepted upstream.  If another version
> is used, I am fine with keeping Reported-by and a Link to my original patch,
> and I can retest the final version against my QEMU ttyS1/ttyS3 shared IRQ
> reproducer if needed.  But I do not provide my Signed-off-by for a
> Co-developed-by tag.
> 
> > Also, all of these really look like they were created/found/whatever by
> > a LLM, which is not being documented, and for that reason alone I think
> > I need to reject all of these until that is properly addressed.
> > 
> 
> Regarding LLM assistance, my v1 explicitly included:
> 
> Assisted-by: Codex:gpt-5
> 
> That tag was dropped in later revisions while refreshing the patch. The bug itself
> was found and reproduced by me, not by an LLM.  I used the tool to help refine
> the patch text and write the reproducer/test code, then reviewed the code
> myself and validated the result with the QEMU ttyS1/ttyS3 shared IRQ
> reproducer before submitting.  I also spent time testing/reviewing the other
> series; for example, one earlier revision did not pass the reproducer, which I
> reported in that thread。

That's fine, but you still need to keep the Assisted-by: tag.  For that
reason alone I will not take this.

> I understand this may mean my patch is not taken.  I am fine with that.  I do not want
> to spend more community time on this dispute.
> 
> I will stop here and leave the technical record as-is.  I hope the regression
> can still be fixed upstream without spending more time on the dispute.

Hopefully someone sends a fix for this in a format that can be
accepted...

thanks,

greg k-h

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

[Jing Wu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2401 bytes --]

> This accusation is not acceptable.

It's not an accusation.  It's a timeline.

Your v1 predates ours by one day — not in dispute.  But your v1 didn't
work.  You confirmed this yourself on May 29 when you wrote "v3 fixes
the Bugzilla reproducer on my setup" — on *our* v3 thread, not yours.
If your v1 already fixed the problem, you wouldn't have needed to test
our v3 and report back.

> That same locking change also closes the irq_info UAF window

Your v1 commit message never mentions use-after-free.  It discusses only
"Unbalanced enable for IRQ."  Our v1 was the first to identify and
document both races.  Claiming retroactively that your v1 "also" fixed
UAF is revisionist.

> For v3, the irq_chain_mutex rename was made in response to Jiri's review
> comment

Jiri made that comment on *our* v6, not yours.  He reviewed our code.
You took a review comment from our thread and applied it to your patch
as if it were your own insight.

> The __must_hold() and lockdep_assert_held() additions ... are not the
> functional fix.

They were added in our v5, reviewed by Greg on our v4.  You copied them
into your v3 and listed them as "Changes in v3" with zero attribution.

Now look at the timeline:

  Jul 7 22:06  Our v6 — posted to the list
  Jul 8 11:11  Your v2 — next morning, same lock-move skeleton
  Jul 8 15:23  Your v3 — hours later, copies irq_chain_mutex rename,
               __must_hold(), lockdep_assert_held() from our v5/v7

If we hadn't replied to Jiri and sent v6, would your v2 exist the next
morning?  If we hadn't renamed hash_mutex in v7, would your v3 appear
hours later with the same rename?  The answer is obvious.

> I do not agree to a Co-developed-by tag for my patch

We added Co-developed-by for you on our v7.  The only reason we did so
is that you confirmed the fix worked on your setup — nothing more.  You
reported the bug, you tested the fix, we credited you.  That's what
open source contributors do.

You, on the other hand, copied our lockdep annotations, our static
analysis annotations, our mutex rename — all reviewed by Jiri and Greg
on *our* patches — and passed them off as your own changelog entries.
And now you're saying you won't extend the same basic courtesy in return.
Copying is fine under GPLv2.  Presenting it as your own work is not.

You reported the bug.  We fixed it.  You copied our improvements.  Add
the tags.

Jing

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

[Jing Wu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2401 bytes --]

> This accusation is not acceptable.

It's not an accusation.  It's a timeline.

Your v1 predates ours by one day — not in dispute.  But your v1 didn't
work.  You confirmed this yourself on May 29 when you wrote "v3 fixes
the Bugzilla reproducer on my setup" — on *our* v3 thread, not yours.
If your v1 already fixed the problem, you wouldn't have needed to test
our v3 and report back.

> That same locking change also closes the irq_info UAF window

Your v1 commit message never mentions use-after-free.  It discusses only
"Unbalanced enable for IRQ."  Our v1 was the first to identify and
document both races.  Claiming retroactively that your v1 "also" fixed
UAF is revisionist.

> For v3, the irq_chain_mutex rename was made in response to Jiri's review
> comment

Jiri made that comment on *our* v6, not yours.  He reviewed our code.
You took a review comment from our thread and applied it to your patch
as if it were your own insight.

> The __must_hold() and lockdep_assert_held() additions ... are not the
> functional fix.

They were added in our v5, reviewed by Greg on our v4.  You copied them
into your v3 and listed them as "Changes in v3" with zero attribution.

Now look at the timeline:

  Jul 7 22:06  Our v6 — posted to the list
  Jul 8 11:11  Your v2 — next morning, same lock-move skeleton
  Jul 8 15:23  Your v3 — hours later, copies irq_chain_mutex rename,
               __must_hold(), lockdep_assert_held() from our v5/v7

If we hadn't replied to Jiri and sent v6, would your v2 exist the next
morning?  If we hadn't renamed hash_mutex in v7, would your v3 appear
hours later with the same rename?  The answer is obvious.

> I do not agree to a Co-developed-by tag for my patch

We added Co-developed-by for you on our v7.  The only reason we did so
is that you confirmed the fix worked on your setup — nothing more.  You
reported the bug, you tested the fix, we credited you.  That's what
open source contributors do.

You, on the other hand, copied our lockdep annotations, our static
analysis annotations, our mutex rename — all reviewed by Jiri and Greg
on *our* patches — and passed them off as your own changelog entries.
And now you're saying you won't extend the same basic courtesy in return.
Copying is fine under GPLv2.  Presenting it as your own work is not.

You reported the bug.  We fixed it.  You copied our improvements.  Add
the tags.

Jing

[PATCH] serial: 8250: serialize shared IRQ startup

[Wang Zhaolong]

Concurrent startup of two 8250 ports sharing the same IRQ can trigger an
IRQ core warning:

  Unbalanced enable for IRQ 3
  WARNING: CPU: 0 PID: 580 at kernel/irq/manage.c:774 __enable_irq+0x3b/0x60
  Call Trace:
   enable_irq+0x8d/0x120
   serial8250_do_startup+0x80d/0xa80
   uart_port_startup+0x13d/0x440
   uart_port_activate+0x5b/0xb0
   tty_port_open+0xa1/0x120
   uart_open+0x1e/0x30
   tty_open+0x140/0x7a0

The second port can then run the shared-IRQ startup test while the IRQ core
is still enabling the line for the first port.  The local
disable_irq_nosync()/enable_irq() pair is balanced, but the interleaving can
still unbalance the IRQ core disable depth.

That makes the QEMU legacy serial ports enter the shared-IRQ THRE test path:

  serial8250_do_startup()
    if (port->irqflags & IRQF_SHARED)
      disable_irq_nosync(port->irq)
    ...
    if (port->irqflags & IRQF_SHARED)
      enable_irq(port->irq)

One possible interleaving is:

  CPU0, ttyS1                         CPU1, ttyS3

  serial_link_irq_chain()
    hash_add(i)
    i->head = &ttyS1
    request_irq()
                                        serial_link_irq_chain()
                                          find i in irq_lists
                                          list_add(&ttyS3, i->head)
                                        serial8250_do_startup()
                                          disable_irq_nosync(irq)
    irq_startup()
      desc->depth = 0
                                          enable_irq(irq)
                                            WARN: Unbalanced enable for IRQ 3

Keep hash_mutex held in serial_link_irq_chain() until the first request_irq()
has completed.  This prevents another 8250 port sharing the IRQ from joining
the chain and running the THRE test while the IRQ core is still starting the
interrupt.

This was reproduced in QEMU with ttyS1 and ttyS3 sharing IRQ 3.  With this
change, 100000 synchronized open/close iterations on /dev/ttyS1 and /dev/ttyS3
completed without the warning.

Fixes: 64c79dfbc458 ("serial: 8250_pnp: Support configurable reg shift property")
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221579
Cc: stable@vger.kernel.org # 6.10+
Assisted-by: Codex:gpt-5
Signed-off-by: Wang Zhaolong <wangzhaolong@fnnas.com>
---
 drivers/tty/serial/8250/8250_core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index a428e88938eb..64eed4dc343f 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -132,12 +132,10 @@ static void serial_do_unlink(struct irq_info *i, struct uart_8250_port *up)
  */
 static struct irq_info *serial_get_or_create_irq_info(const struct uart_8250_port *up)
 {
 	struct irq_info *i;
 
-	guard(mutex)(&hash_mutex);
-
 	hash_for_each_possible(irq_lists, i, node, up->port.irq)
 		if (i->irq == up->port.irq)
 			return i;
 
 	i = kzalloc_obj(*i);
@@ -154,10 +152,12 @@ static struct irq_info *serial_get_or_create_irq_info(const struct uart_8250_por
 static int serial_link_irq_chain(struct uart_8250_port *up)
 {
 	struct irq_info *i;
 	int ret;
 
+	guard(mutex)(&hash_mutex);
+
 	i = serial_get_or_create_irq_info(up);
 	if (IS_ERR(i))
 		return PTR_ERR(i);
 
 	scoped_guard(spinlock_irq, &i->lock) {
@@ -169,10 +169,15 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
 
 		INIT_LIST_HEAD(&up->list);
 		i->head = &up->list;
 	}
 
+	/*
+	 * Keep the shared-IRQ chain locked until the first handler is installed.
+	 * Otherwise another UART can join early and run startup IRQ masking while
+	 * the IRQ core is still enabling the line, unbalancing the disable depth.
+	 */
 	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;
-- 
2.54.0

[PATCH v2] serial: 8250: fix shared IRQ startup race causing IRQ warning

[Wang Zhaolong]

Concurrent startup of two 8250 ports sharing the same IRQ can trigger an
IRQ core warning:

  Unbalanced enable for IRQ 3
  WARNING: CPU: 0 PID: 580 at kernel/irq/manage.c:774 __enable_irq+0x3b/0x60
  Call Trace:
   enable_irq+0x8d/0x120
   serial8250_do_startup+0x80d/0xa80
   uart_port_startup+0x13d/0x440
   uart_port_activate+0x5b/0xb0
   tty_port_open+0xa1/0x120
   uart_open+0x1e/0x30
   tty_open+0x140/0x7a0

This is reproducible in QEMU with four legacy 8250/16550 ports where ttyS1
and ttyS3 share IRQ 3.  A small userspace reproducer that synchronizes two
threads before open(), waits for both open attempts, and then closes both file
descriptors can trigger the warning almost immediately.

The regression was bisected to commit 64c79dfbc458 ("serial: 8250_pnp:
Support configurable reg shift property").  That change made QEMU's legacy
PNP serial ports take the shared-IRQ THRE test path in
serial8250_do_startup():

  if (port->irqflags & IRQF_SHARED)
    disable_irq_nosync(port->irq)
  ...
  if (port->irqflags & IRQF_SHARED)
    enable_irq(port->irq)

The disable_irq_nosync()/enable_irq() pair is locally balanced, but it can
race with the IRQ core startup path for the first 8250 port on the same IRQ.
One possible interleaving is:

  CPU0, ttyS1                         CPU1, ttyS3

  serial_link_irq_chain()
    hash_add(i)
    i->head = &ttyS1
    request_irq()
                                        serial_link_irq_chain()
                                          find i in irq_lists
                                          list_add(&ttyS3, i->head)
                                        serial8250_do_startup()
                                          disable_irq_nosync(irq)
    irq_startup()
      desc->depth = 0
                                          enable_irq(irq)
                                            WARN: Unbalanced enable for IRQ 3

Hold hash_mutex in serial_link_irq_chain() until the first request_irq() has
completed.  This prevents another 8250 port sharing the IRQ from joining the
chain and running the THRE test while the IRQ core is still starting the
interrupt.  The request_irq() failure cleanup also remains covered by
hash_mutex, so the just-published irq_info cannot be observed by another link
attempt before it is unlinked again.

Fixes: 64c79dfbc458 ("serial: 8250_pnp: Support configurable reg shift property")
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221579
Cc: stable@vger.kernel.org # 6.10+
Signed-off-by: Wang Zhaolong <wangzhaolong@fnnas.com>
---

Changes in v2:
  - Retitle the patch to describe the unbalanced IRQ enable warning.
  - Move the code comment to the hash_mutex acquisition site to document why the
    lock must cover the first request_irq() completion.
  - Drop the Assisted-by tag.

 drivers/tty/serial/8250/8250_core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index a428e88938eb..dd202032cc7c 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -132,12 +132,10 @@ static void serial_do_unlink(struct irq_info *i, struct uart_8250_port *up)
  */
 static struct irq_info *serial_get_or_create_irq_info(const struct uart_8250_port *up)
 {
 	struct irq_info *i;
 
-	guard(mutex)(&hash_mutex);
-
 	hash_for_each_possible(irq_lists, i, node, up->port.irq)
 		if (i->irq == up->port.irq)
 			return i;
 
 	i = kzalloc_obj(*i);
@@ -154,10 +152,18 @@ static struct irq_info *serial_get_or_create_irq_info(const struct uart_8250_por
 static int serial_link_irq_chain(struct uart_8250_port *up)
 {
 	struct irq_info *i;
 	int ret;
 
+	/*
+	 * Keep the hash lock held until the first request_irq() completes.
+	 * The first port publishes i->head before request_irq() starts the IRQ;
+	 * a second port sharing the IRQ must not join the chain and run the
+	 * THRE test while the IRQ core is still bringing the line up.
+	 */
+	guard(mutex)(&hash_mutex);
+
 	i = serial_get_or_create_irq_info(up);
 	if (IS_ERR(i))
 		return PTR_ERR(i);
 
 	scoped_guard(spinlock_irq, &i->lock) {
-- 
2.54.0

[PATCH v3] serial: 8250: fix shared IRQ startup race causing IRQ warning

[Wang Zhaolong]

Concurrent startup of two 8250 ports sharing the same IRQ can trigger an
IRQ core warning:

  Unbalanced enable for IRQ 3
  WARNING: CPU: 0 PID: 580 at kernel/irq/manage.c:774 __enable_irq+0x3b/0x60
  Call Trace:
   enable_irq+0x8d/0x120
   serial8250_do_startup+0x80d/0xa80
   uart_port_startup+0x13d/0x440
   uart_port_activate+0x5b/0xb0
   tty_port_open+0xa1/0x120
   uart_open+0x1e/0x30
   tty_open+0x140/0x7a0

This is reproducible in QEMU with four legacy 8250/16550 ports.  In that
setup, ttyS1 and ttyS3 share IRQ 3.  A small userspace reproducer
synchronizes two threads before open(), waits for both open attempts, and
then closes both file descriptors.  It can trigger the warning almost
immediately.

The regression was bisected to commit 64c79dfbc458 ("serial: 8250_pnp:
Support configurable reg shift property").  That change made QEMU's legacy
PNP serial ports take the shared-IRQ THRE test path in
serial8250_do_startup():

  if (port->irqflags & IRQF_SHARED)
    disable_irq_nosync(port->irq)
  ...
  if (port->irqflags & IRQF_SHARED)
    enable_irq(port->irq)

The disable_irq_nosync()/enable_irq() pair is locally balanced, but it can
race with the IRQ core startup path for the first 8250 port on the same
IRQ.  One possible interleaving is:

  CPU0, ttyS1                         CPU1, ttyS3

  serial_link_irq_chain()
    hash_add(i)
    i->head = &ttyS1
    request_irq()
                                        serial_link_irq_chain()
                                          find i in irq_lists
                                          list_add(&ttyS3, i->head)
                                        serial8250_do_startup()
                                          disable_irq_nosync(irq)
    irq_startup()
      desc->depth = 0
                                          enable_irq(irq)
                                            WARN: Unbalanced enable for IRQ 3

Hold irq_chain_mutex in serial_link_irq_chain() until the first request_irq()
has completed.  This prevents another 8250 port sharing the IRQ from joining
the chain and running the THRE test while the IRQ core is still starting the
interrupt.  The request_irq() failure cleanup also remains covered by
irq_chain_mutex, so the just-published irq_info cannot be observed by another
link attempt before it is unlinked again.

The lock used to only protect irq_lists hash walks, but it now also serializes
IRQ chain publication and the first request_irq() completion.  Rename it to
irq_chain_mutex and document the locking requirement for
serial_get_or_create_irq_info() with __must_hold() and lockdep_assert_held().

Fixes: 64c79dfbc458 ("serial: 8250_pnp: Support configurable reg shift property")
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=221579
Cc: stable@vger.kernel.org # 6.10+
Signed-off-by: Wang Zhaolong <wangzhaolong@fnnas.com>
---
Changes in v3:
  - Rename hash_mutex to irq_chain_mutex now that it also serializes IRQ chain
    publication and first request_irq() completion.
  - Add __must_hold() and lockdep_assert_held() to document the locking
    requirement for serial_get_or_create_irq_info().
  - Verify again with the QEMU ttyS1/ttyS3 shared IRQ reproducer.

Changes in v2:
  - Retitle the patch to describe the unbalanced IRQ enable warning.
  - Move the code comment to the mutex acquisition site to document why the
    lock must cover the first request_irq() completion.
  - Drop the Assisted-by tag.

v2: https://lore.kernel.org/all/20260708031115.3757150-1-wangzhaolong@fnnas.com/
v1: https://lore.kernel.org/all/20260527092052.2086342-1-wangzhaolong@fnnas.com/

 drivers/tty/serial/8250/8250_core.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index f49862d90eeb..41d87cdc69d7 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -48,11 +48,11 @@ struct irq_info {
 	struct list_head	*head;
 };
 
 #define IRQ_HASH_BITS		5	/* Can be adjusted later */
 static DEFINE_HASHTABLE(irq_lists, IRQ_HASH_BITS);
-static DEFINE_MUTEX(hash_mutex);	/* Used to walk the hash */
+static DEFINE_MUTEX(irq_chain_mutex);
 
 static bool skip_txen_test;
 module_param(skip_txen_test, bool, 0644);
 MODULE_PARM_DESC(skip_txen_test, "Skip checking for the TXEN bug at init time");
 
@@ -129,14 +129,15 @@ static void serial_do_unlink(struct irq_info *i, struct uart_8250_port *up)
  * Either:
  * - find the corresponding info in the hashtable and return it, or
  * - 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(&irq_chain_mutex)
 {
 	struct irq_info *i;
 
-	guard(mutex)(&hash_mutex);
+	lockdep_assert_held(&irq_chain_mutex);
 
 	hash_for_each_possible(irq_lists, i, node, up->port.irq)
 		if (i->irq == up->port.irq)
 			return i;
 
@@ -154,10 +155,18 @@ static struct irq_info *serial_get_or_create_irq_info(const struct uart_8250_por
 static int serial_link_irq_chain(struct uart_8250_port *up)
 {
 	struct irq_info *i;
 	int ret;
 
+	/*
+	 * Keep the IRQ chain lock held until the first request_irq() completes.
+	 * The first port publishes i->head before request_irq() starts the IRQ;
+	 * a second port sharing the IRQ must not join the chain and run the
+	 * THRE test while the IRQ core is still bringing the line up.
+	 */
+	guard(mutex)(&irq_chain_mutex);
+
 	i = serial_get_or_create_irq_info(up);
 	if (IS_ERR(i))
 		return PTR_ERR(i);
 
 	scoped_guard(spinlock_irq, &i->lock) {
@@ -180,11 +189,11 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
 
 static void serial_unlink_irq_chain(struct uart_8250_port *up)
 {
 	struct irq_info *i;
 
-	guard(mutex)(&hash_mutex);
+	guard(mutex)(&irq_chain_mutex);
 
 	hash_for_each_possible(irq_lists, i, node, up->port.irq)
 		if (i->irq == up->port.irq) {
 			if (WARN_ON(i->head == NULL))
 				return;
-- 
2.54.0

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

[Jing Wu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2401 bytes --]

> This accusation is not acceptable.

It's not an accusation.  It's a timeline.

Your v1 predates ours by one day — not in dispute.  But your v1 didn't
work.  You confirmed this yourself on May 29 when you wrote "v3 fixes
the Bugzilla reproducer on my setup" — on *our* v3 thread, not yours.
If your v1 already fixed the problem, you wouldn't have needed to test
our v3 and report back.

> That same locking change also closes the irq_info UAF window

Your v1 commit message never mentions use-after-free.  It discusses only
"Unbalanced enable for IRQ."  Our v1 was the first to identify and
document both races.  Claiming retroactively that your v1 "also" fixed
UAF is revisionist.

> For v3, the irq_chain_mutex rename was made in response to Jiri's review
> comment

Jiri made that comment on *our* v6, not yours.  He reviewed our code.
You took a review comment from our thread and applied it to your patch
as if it were your own insight.

> The __must_hold() and lockdep_assert_held() additions ... are not the
> functional fix.

They were added in our v5, reviewed by Greg on our v4.  You copied them
into your v3 and listed them as "Changes in v3" with zero attribution.

Now look at the timeline:

  Jul 7 22:06  Our v6 — posted to the list
  Jul 8 11:11  Your v2 — next morning, same lock-move skeleton
  Jul 8 15:23  Your v3 — hours later, copies irq_chain_mutex rename,
               __must_hold(), lockdep_assert_held() from our v5/v7

If we hadn't replied to Jiri and sent v6, would your v2 exist the next
morning?  If we hadn't renamed hash_mutex in v7, would your v3 appear
hours later with the same rename?  The answer is obvious.

> I do not agree to a Co-developed-by tag for my patch

We added Co-developed-by for you on our v7.  The only reason we did so
is that you confirmed the fix worked on your setup — nothing more.  You
reported the bug, you tested the fix, we credited you.  That's what
open source contributors do.

You, on the other hand, copied our lockdep annotations, our static
analysis annotations, our mutex rename — all reviewed by Jiri and Greg
on *our* patches — and passed them off as your own changelog entries.
And now you're saying you won't extend the same basic courtesy in return.
Copying is fine under GPLv2.  Presenting it as your own work is not.

You reported the bug.  We fixed it.  You copied our improvements.  Add
the tags.

Jing