Re: [PATCH v3 1/1] platform/x86/intel/pmt: Notify userspace when crashlogs complete

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] platform/x86/intel/pmt: control_mutex lock inversion in
  pmt_crashlog_probe()
--

commit ff673d94c967284b95f515a0ddd6add0a4afc80c
Author: David E. Box <david.e.box@linux.intel.com>

platform/x86/intel/pmt: Notify userspace when crashlogs complete

This commit emits a KOBJ_CHANGE uevent on the crashlog device when a
completed crashlog is found at probe or after a manual trigger finishes
polling.

> diff --git a/drivers/platform/x86/intel/pmt/crashlog.c b/drivers/platform/x86/intel/pmt/crashlog.c
> index d7018b495217c..1796023a2f0e6 100644
> --- a/drivers/platform/x86/intel/pmt/crashlog.c
> +++ b/drivers/platform/x86/intel/pmt/crashlog.c

[ ... ]

> @@ -502,9 +616,17 @@ trigger_store(struct device *dev, struct device_attribute *attr,
>  
>  	if (!trigger) {
>  		ret = pmt_crashlog_set_clear(crashlog);
> -		return ret ?: count;
> +		if (ret)
> +			return ret;
> +
> +		pmt_crashlog_end_poll(crashlog);
> +		return count;
>  	}
>  
> +	/* reject triggers until probe has finished setting up this entry */
> +	if (!crashlog->trigger_ready)
> +		return -EBUSY;

[ ... ]

> @@ -661,7 +801,14 @@ static int pmt_crashlog_probe(struct auxiliary_device *auxdev,
>  	auxiliary_set_drvdata(auxdev, priv);
>  
>  	for (i = 0; i < intel_vsec_dev->num_resources; i++) {
> -		struct intel_pmt_entry *entry = &priv->entry[priv->num_entries].entry;
> +		struct crashlog_entry *crashlog = &priv->entry[priv->num_entries];
> +		struct intel_pmt_entry *entry = &crashlog->entry;
> +
> +		/* trigger_store() rejects triggers until trigger_ready is set below */
> +		INIT_WORK(&crashlog->pending_work, pmt_crashlog_pending_fn);
> +		INIT_WORK(&crashlog->uevent_work, pmt_crashlog_uevent_fn);
> +		INIT_DELAYED_WORK(&crashlog->uevent_poll_work,
> +				  pmt_crashlog_uevent_poll_fn);
>  
>  		ret = intel_pmt_dev_create(entry, &pmt_crashlog_ns, intel_vsec_dev, i);
>  		if (ret < 0)
> @@ -672,6 +819,27 @@ static int pmt_crashlog_probe(struct auxiliary_device *auxdev,
>  		priv->num_entries++;
>  	}
>  
> +	/*
> +	 * Don't read crashlog registers from probe. The read may go through the
> +	 * parent's read_reg() callback, which can take a lock that the parent
> +	 * holds across intel_vsec_register() and so across a synchronous probe.
> +	 * From a work item, the read just waits for registration to finish.
> +	 *
> +	 * Enable triggers and queue the work only once every entry is created.
> +	 * abort_probe waits for the work items in pmt_crashlog_remove(), which
> +	 * would deadlock if one were already blocked on a lock held by our
> +	 * caller. For the same reason, a parent must not hold that lock while
> +	 * removing the device.
> +	 */
> +	for (i = 0; i < priv->num_entries; i++) {
> +		struct crashlog_entry *crashlog = &priv->entry[i];
> +
> +		scoped_guard(mutex, &crashlog->control_mutex)
> +			crashlog->trigger_ready = true;
> +
> +		queue_work(system_freezable_wq, &crashlog->pending_work);
> +	}

[Severity: High]
Does taking crashlog->control_mutex in pmt_crashlog_probe() invert the lock
order against the parent's register-access lock?

When the parent holds its register lock across intel_vsec_register(),
pmt_crashlog_probe() takes crashlog->control_mutex under that lock, whereas
pmt_crashlog_pending_fn(), pmt_crashlog_uevent_poll_fn() and the sysfs
store handlers take crashlog->control_mutex first and then call
entry->cb->read_reg() or entry->cb->write_reg().  This triggers a lockdep
splat when pending_work runs, and because intel_pmt_dev_create() has
already exposed the sysfs attributes and emitted KOBJ_ADD in the first
loop, a concurrent sysfs store can ABBA deadlock with probe:

CPU0                              CPU1
----                              ----
lock(parent_lock)
                                  lock(&crashlog->control_mutex)
lock(&crashlog->control_mutex)
                                  lock(parent_lock)

 *** DEADLOCK ***

CPU0: intel_vsec_register() -> ... -> pmt_crashlog_probe()
CPU1: trigger_store() -> pmt_crashlog_disabled() -> entry->cb->read_reg()

In addition, trigger_store() calls pmt_crashlog_disabled() and (when
trigger is false) pmt_crashlog_set_clear() before checking
crashlog->trigger_ready, so a write to trigger during probe still accesses
registers and can clear the crashlog before setup finishes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794444671.git.david.e.box@linux.intel.com?part=1