Re: [PATCH v4 2/6] PM: runtime: Add pm_runtime_if_active guard and conditional variant

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pm: TOCTOU race in pm_runtime_if_active_try allows hardware access on suspended device
- [Medium] pm: pm_runtime_if_active base guard causes usage_count underflow if used directly
--

commit 90c398317818a4245091ccf1d776dfe02e35d94e
Author: Golla Nagendra <nagendra.golla@amd.com>

PM: runtime: Add pm_runtime_if_active guard and conditional variant

This commit introduces a pm_runtime_if_active base guard and its _try
conditional variant to allow drivers to conditionally acquire a runtime
PM reference only when the device is already active.

> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index 322e3b17f9871..41804e8480427 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h

[ ... ]

> @@ -645,6 +645,13 @@ DEFINE_GUARD(pm_runtime_active, struct device *,
>  	     pm_runtime_get_sync(_T), pm_runtime_put(_T));
>  DEFINE_GUARD(pm_runtime_active_auto, struct device *,
>  	     pm_runtime_get_sync(_T), pm_runtime_put_autosuspend(_T));
> +/*
> + * Do not use directly -- the destructor calls pm_runtime_put()
> + * unconditionally, which underflows if no reference was acquired.
> + * Use only via the _try variant below.
> + */
> +DEFINE_GUARD(pm_runtime_if_active, struct device *,
> +	     pm_runtime_get_if_active(_T), pm_runtime_put(_T));

[Severity: Medium]
Is there a risk that this base guard will be used directly despite the
comment?

If used via guard(pm_runtime_if_active), pm_runtime_get_if_active() does
not acquire a reference when the device is suspended, but the destructor
unconditionally calls pm_runtime_put().

Can this underflow the usage_count and permanently break the device's
power management state?

Since the _try variant overrides the lock expression, could the base guard
be safely defined using pm_runtime_get_noresume() instead of
pm_runtime_get_if_active() to prevent this?

[ ... ]

> @@ -660,6 +667,23 @@ DEFINE_GUARD_COND(pm_runtime_active_auto, _try,
>  		  pm_runtime_get_active(_T, RPM_TRANSPARENT), _RET == 0)
>  DEFINE_GUARD_COND(pm_runtime_active_auto, _try_enabled,
>  		  pm_runtime_resume_and_get(_T), _RET == 0)
> +/*
> + * The _try contract is that no error is produced when runtime PM is disabled,
> + * so bypass the guard when pm_runtime_get_if_active() returns -EINVAL: take a
> + * reference with pm_runtime_get_noresume() to pre-balance the destructor's
> + * pm_runtime_put() and return 1 so the guard condition succeeds. Interrupt
> + * handlers then keep servicing interrupts when runtime PM is disabled for the
> + * device or CONFIG_PM is not enabled.
> + */
> +DEFINE_GUARD_COND(pm_runtime_if_active, _try,
> +		  ({
> +			int _r = pm_runtime_get_if_active(_T);
> +
> +			if (_r == -EINVAL)
> +				pm_runtime_get_noresume(_T);
> +			_r == -EINVAL ? 1 : (_r ?: -EAGAIN);
> +		  }),
> +		  _RET == 1)

[Severity: High]
Does this regression allow a race condition that leads to hardware access
on a suspended device?

pm_runtime_get_if_active() checks the state under dev->power.lock and then
drops the lock before returning -EINVAL.

Could a concurrent thread enable and suspend the device immediately after
the lock is dropped:

Thread 1
_r = pm_runtime_get_if_active(_T); // returns -EINVAL, drops lock

Thread 2
pm_runtime_enable(dev);  // disable_depth becomes 0
pm_runtime_suspend(dev); // device is now suspended

Thread 1 continues
if (_r == -EINVAL)
    pm_runtime_get_noresume(_T); // increments usage_count outside lock

The macro then evaluates to 1, indicating success.

Does this trick the caller into treating the device as active, causing
interrupt handlers to perform MMIO accesses on powered-down hardware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-adma-pm-fix-and-per-channel-reset-v4-0-55f5fe409c55@amd.com?part=2