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