Re: [PATCH v3 1/2] irqchip: irq-qcom-mpm: Register MPM with cpu_pm for RPM notification

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] irqchip/qcom-mpm: broken error handling and UAF in qcom_mpm_probe()
- [High] irqchip/qcom-mpm: cpus_in_pm not decremented on failure in mpm_cpu_pm_callback()
- [High] irqchip/qcom-mpm: invalid spinlock_t nesting in mpm_cpu_pm_callback()
- [High] irqchip: irq-qcom-mpm: cpus_in_pm underflow when probing with idle CPUs
--

commit 0b0c9281cd528ac3d6a6f2d4a69ee74beee11616
Author: Sneh Mankad <sneh.mankad@oss.qualcomm.com>

irqchip: irq-qcom-mpm: Register MPM with cpu_pm for RPM notification

This commit updates the Qualcomm MPM irqchip driver to register a CPU PM
notifier when the device tree node does not specify #power-domain-cells,
notifying the RPM processor when the last online CPU enters idle.

> diff --git a/drivers/irqchip/irq-qcom-mpm.c b/drivers/irqchip/irq-qcom-mpm.c
> index 8c575d7ce8a17..b6caea90e9f41 100644
> --- a/drivers/irqchip/irq-qcom-mpm.c
> +++ b/drivers/irqchip/irq-qcom-mpm.c

[ ... ]

> @@ -307,10 +308,52 @@ static int mpm_pd_power_off(struct generic_pm_domain *genpd)
>  		return ret;
>  
>  	mbox_client_txdone(priv->mbox_chan, 0);
> -
>  	return 0;
>  }
>  
> +static int mpm_pd_power_off(struct generic_pm_domain *genpd)
> +{
> +	struct qcom_mpm_priv *priv = container_of(genpd, struct qcom_mpm_priv,
> +						  genpd);
> +
> +	return handle_rpm_notification(priv);
> +}
> +
> +static int mpm_cpu_pm_callback(struct notifier_block *nfb, unsigned long action, void *v)
> +{
> +	struct qcom_mpm_priv *priv = container_of(nfb, struct qcom_mpm_priv, mpm_pm);
> +	int cpus_in_pm;
> +
> +	switch (action) {
> +	case CPU_PM_ENTER:
> +		cpus_in_pm = atomic_inc_return(&priv->cpus_in_pm);
> +		/*
> +		 * NOTE: comments for num_online_cpus() point out that it's
> +		 * only a snapshot so we need to be careful. It should be OK
> +		 * for us to use, though.  It's important for us not to miss
> +		 * if we're the last CPU going down so it would only be a
> +		 * problem if a CPU went offline right after we did the check
> +		 * AND that CPU was not idle AND that CPU was the last non-idle
> +		 * CPU. That can't happen. CPUs would have to come out of idle
> +		 * before the CPU could go offline.
> +		 */
> +		if (cpus_in_pm < num_online_cpus())
> +			return NOTIFY_OK;
> +		break;
> +	case CPU_PM_ENTER_FAILED:
> +	case CPU_PM_EXIT:
> +		atomic_dec(&priv->cpus_in_pm);
> +		return NOTIFY_OK;

[Severity: High]
Can priv->cpus_in_pm underflow if other CPUs are already in idle when
qcom_mpm_probe() registers this notifier?

Because MPM is no longer a power domain provider for the PSCI cluster domain,
psci_cpuidle_domain_probe() and psci_cpuidle_probe() in
drivers/cpuidle/cpuidle-psci.c can initialize cpuidle before qcom_mpm_probe()
runs (for example, when CONFIG_QCOM_MPM, CONFIG_QCOM_SMD_RPM, or
CONFIG_QCOM_APCS_IPC is built as a module or defers probe).

If one or more CPUs have already entered idle via cpu_pm_enter() when
qcom_mpm_probe() calls cpu_pm_register_notifier(&priv->mpm_pm) with
priv->cpus_in_pm initialized to 0, those idle CPUs will later wake up and
invoke cpu_pm_exit() -> mpm_cpu_pm_callback(CPU_PM_EXIT) without a matching
CPU_PM_ENTER.

That decrements priv->cpus_in_pm below zero when all CPUs are active, so
subsequent CPU_PM_ENTER transitions will never reach num_online_cpus() and
handle_rpm_notification() will never be called.

> +	default:
> +		return NOTIFY_DONE;
> +	}
> +
> +	if (handle_rpm_notification(priv))
> +		return NOTIFY_BAD;

[Severity: High]
If handle_rpm_notification(priv) fails here in mpm_cpu_pm_callback() in
drivers/irqchip/irq-qcom-mpm.c, does priv->cpus_in_pm remain permanently
incremented by one?

On the CPU_PM_ENTER path, priv->cpus_in_pm is incremented before calling
handle_rpm_notification(priv). In kernel/cpu_pm.c, cpu_pm_enter() calls
cpu_pm_notify_robust(), which uses raw_notifier_call_chain_robust() to send
CPU_PM_ENTER_FAILED only to the callbacks that succeeded prior to the failing
callback:

kernel/notifier.c:notifier_call_chain_robust()
    ret = nb->notifier_call(nb, val_up, v);
    if (ret & NOTIFY_STOP_MASK) {
        notifier_call_chain(nl, val_down, v, nr - 1, NULL);
        ...
    }

Because mpm_cpu_pm_callback() will not receive CPU_PM_ENTER_FAILED (and the
caller aborts idle entry without calling cpu_pm_exit()), returning NOTIFY_BAD
without calling atomic_dec(&priv->cpus_in_pm) leaves priv->cpus_in_pm skewed
by +1, causing future idle entries to trigger handle_rpm_notification() while
a CPU is still active.

[Severity: High]
Also, is it valid to call handle_rpm_notification(priv) from
mpm_cpu_pm_callback() while holding the raw spinlock in cpu_pm_notify_robust()?

In kernel/cpu_pm.c, cpu_pm_notify_robust() holds cpu_pm_notifier.lock (a
raw_spinlock_t) across the entire notifier chain:

kernel/cpu_pm.c:cpu_pm_notify_robust()
    raw_spin_lock_irqsave(&cpu_pm_notifier.lock, flags);
    ret = raw_notifier_call_chain_robust(&cpu_pm_notifier.chain, event_up, event_down, NULL);
    raw_spin_unlock_irqrestore(&cpu_pm_notifier.lock, flags);

Inside mpm_cpu_pm_callback(), handle_rpm_notification() invokes
mbox_send_message() and mbox_client_txdone() in drivers/mailbox/mailbox.c,
where add_to_rbuf(), msg_submit(), and tx_tick() acquire chan->lock (a
non-raw spinlock_t). In addition, msg_submit() calls
qcom_apcs_ipc_send_data() in drivers/mailbox/qcom-apcs-ipc-mailbox.c, which
calls regmap_write() on a regmap without use_raw_spinlock set, acquiring
map->spinlock (another non-raw spinlock_t).

Acquiring a spinlock_t while holding a raw_spinlock_t violates lockdep wait-type
nesting rules under CONFIG_PROVE_RAW_LOCK_NESTING=y and sleeps in atomic idle
context on PREEMPT_RT kernels.

> +
> +	return NOTIFY_OK;
> +}
> +
>  static bool gic_hwirq_is_mapped(struct mpm_gic_map *maps, int cnt, u32 hwirq)
>  {
>  	int i;

[ ... ]

> @@ -444,6 +467,35 @@ static int qcom_mpm_probe(struct platform_device *pdev, struct device_node *pare
>  		return ret;
>  	}
>  
> +	if (of_find_property(np, "#power-domain-cells", NULL)) {
> +		genpd = &priv->genpd;
> +		genpd->flags = GENPD_FLAG_IRQ_SAFE;
> +		genpd->power_off = mpm_pd_power_off;
> +
> +		genpd->name = devm_kasprintf(dev, GFP_KERNEL, "%s", dev_name(dev));
> +		if (!genpd->name)
> +			return -ENOMEM;
> +
> +		ret = pm_genpd_init(genpd, NULL, false);
> +		if (ret) {
> +			dev_err(dev, "failed to init genpd: %d\n", ret);
> +			return ret;
> +		}
> +
> +		ret = of_genpd_add_provider_simple(np, genpd);
> +		if (ret) {
> +			dev_err(dev, "failed to add genpd provider: %d\n", ret);
> +			goto remove_genpd;
> +		}
> +	} else {
> +		priv->mpm_pm.notifier_call = mpm_cpu_pm_callback;
> +		ret = cpu_pm_register_notifier(&priv->mpm_pm);
> +		if (ret) {
> +			dev_err(dev, "failed to register for CPU_PM notifications: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
>  	parent_domain = irq_find_host(parent);
>  	if (!parent_domain) {
>  		dev_err(dev, "failed to find MPM parent domain\n");
> @@ -471,10 +523,10 @@ static int qcom_mpm_probe(struct platform_device *pdev, struct device_node *pare
>  
>  remove_domain:
>  	irq_domain_remove(priv->domain);
> -free_mbox:
> -	mbox_free_channel(priv->mbox_chan);
>  remove_genpd:
>  	pm_genpd_remove(genpd);
> +free_mbox:
> +	mbox_free_channel(priv->mbox_chan);
>  	return ret;
>  }

[Severity: Critical]
Does the error cleanup in qcom_mpm_probe() in drivers/irqchip/irq-qcom-mpm.c
mishandle failure paths in both the genpd and cpu_pm branches?

There appear to be four related issues on probe failure:

1. In qcom_mpm_probe(), the local variable genpd is declared without
initialization:

    struct generic_pm_domain *genpd;

When #power-domain-cells is absent, the else branch is taken and genpd remains
uninitialized. If devm_request_irq() subsequently fails and jumps to
remove_domain, execution falls through to remove_genpd and calls
pm_genpd_remove(genpd) with an uninitialized pointer.

2. In the else branch, cpu_pm_register_notifier(&priv->mpm_pm) registers the
notifier block embedded in devm-allocated priv into the global cpu_pm_notifier
chain. If irq_find_host(), irq_domain_create_hierarchy(), or
devm_request_irq() fails, cpu_pm_unregister_notifier(&priv->mpm_pm) is never
called before devres frees priv, leaving a dangling notifier in the global
chain.

3. Because remove_genpd now precedes free_mbox, if #power-domain-cells is
present and irq_find_host() or irq_domain_create_hierarchy() fails, jumping to
free_mbox skips remove_genpd (and of_genpd_del_provider(np)), leaving
priv->genpd registered after priv is freed.

4. Because mbox_request_channel() is now called before the genpd and cpu_pm
setup, the direct return statements on !genpd->name, pm_genpd_init() failure,
and cpu_pm_register_notifier() failure return without calling
mbox_free_channel(priv->mbox_chan), leaking the mailbox channel with a
dangling client pointer once priv is freed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-b4-shikra_lpm_addition-v3-0-424abcdcf824@oss.qualcomm.com?part=1