Re: [PATCH 2/3] genirq: Export NMI APIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Exporting NMI APIs to loadable modules exposes an unmitigated Use-After-Free and kernel panic during module unload due to the lack of NMI synchronization.
- [High] Lack of request_mutex locking in free_nmi() and the request_nmi() error path leads to data races, resource double-frees, and refcount underflows.
--

--- Patch [2]: [PATCH 2/3] genirq: Export NMI APIs ---
commit dfe5c7d2a97396d411087d0eb1dca59bbf190e69
Author: Mayank Rungta <mrungta@google.com>

genirq: Export NMI APIs

Currently, request_nmi(), free_nmi(), enable_nmi() and disable_nmi_nosync()
are restricted to built-in kernel code because they are not exported to
loadable modules. Export these APIs to allow loadable modules to register
and manage NMIs.

> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 2fbff2618a1e2..fb0b8da32f4c1 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
[ ... ]
> @@ -2080,6 +2082,7 @@ const void *free_nmi(unsigned int irq, void *dev_id)
>  
>  	return __cleanup_nmi(irq, desc);
>  }
> +EXPORT_SYMBOL_GPL(free_nmi);

[Severity: Critical]
This is a pre-existing issue, but does exporting this API to loadable modules
expose an unmitigated use-after-free and kernel panic during module unload?

When a module unloads and calls free_nmi(), __cleanup_nmi() uninstalls the
handler and calls kfree(action) without waiting for executing NMI handlers to
finish, such as via synchronize_rcu():

kernel/irq/manage.c:__cleanup_nmi() {
    ...
    unregister_handler_proc(irq, action);
    kfree(action);
    ...
}

Concurrent NMIs on other CPUs bypass normal IRQ locks and can continue to
execute the handler locklessly:

kernel/irq/chip.c:handle_fasteoi_nmi() {
    ...
    struct irqaction *action = desc->action;
    ...
    res = action->handler(irq, action->dev_id);
    ...
}

Since the module text section is unmapped upon unload, couldn't an actively
executing concurrent NMI jump into unmapped memory, resulting in a fatal
panic?

[ ... ]
> @@ -2342,6 +2345,7 @@ int request_nmi(unsigned int irq, irq_handler_t handler,
>  
>  	return retval;
>  }
> +EXPORT_SYMBOL_GPL(request_nmi);

[Severity: High]
This is a pre-existing issue, but do request_nmi() and free_nmi() lack
request_mutex locking around their cleanup paths?

According to IRQ subsystem locking rules, desc->request_mutex serializes
resource allocation and teardown. However, both free_nmi() and the failure
path of request_nmi() invoke __cleanup_nmi() without holding this mutex.

In request_nmi():

kernel/irq/manage.c:request_nmi() {
    ...
    if (retval) {
        __cleanup_nmi(irq, desc);
        return -EINVAL;
    }
    ...
}

Inside __cleanup_nmi(), resource teardowns are executed unconditionally
outside of desc->lock:

kernel/irq/manage.c:__cleanup_nmi() {
    ...
    irq_release_resources(desc);
    irq_chip_pm_put(&desc->irq_data);
    module_put(desc->owner);
    ...
}

If request_nmi() fails concurrently with another request_nmi() or free_nmi(),
won't this lead to resource double-frees and unbalanced reference counts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-qcom-wdt-nmi-series-v1-0-3aa86d162914@google.com?part=2