Re: [PATCH v3 3/3] accel/qaic: Add callbacks for DRM RAS netlink telemetry

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential
issues to consider:
- [High] accel/qaic: missing tele_wq flush and drmm/devm mismatch in
  create_qdev()
- [Medium] accel/qaic: telemetry_request() returns positive jiffies on
  firmware error
- [Medium] accel/qaic: missing cleanup on error in
  qaic_telemetry_mhi_probe()
- [Medium] accel/qaic: unsynchronized tele_ch teardown in
  qaic_telemetry_mhi_remove()
- [Medium] accel/qaic: use-after-free of ras_node in
  qaic_drm_ras_unregister_node()
- [Low] accel/qaic: ignored qaic_telemetry_register() failure in
  qaic_init()
- [Low] accel/qaic: missing static on file-local telemetry arrays and
  helpers
- [Low] accel/qaic: commit message states RAS node is created in PCI
  probe
--

commit aee476e17f4e0ed7de92c77cb028e7ff6d94ed60
Author: Zack McKevitt <zachary.mckevitt@oss.qualcomm.com>

accel/qaic: Add callbacks for DRM RAS netlink telemetry

This commit adds MHI telemetry channel support and registers a DRM RAS
telemetry node with read and write callbacks for QAIC devices.

> During PCI probe, create a DRM RAS node with DRM_RAS_NODE_TYPE_TELEMETRY
> type and add callbacks to telemetry_request for read and write operations.

[Severity: Low]
This isn't a bug, but the commit message says the DRM RAS node is created
during PCI probe, whereas qaic_drm_ras_register_node() is actually called
from qaic_telemetry_mhi_probe() when the QAIC_TELEMETRY MHI channel is
probed.

> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
> index 9e1e184efe0d1..c2a30cb9af467 100644
> --- a/drivers/accel/qaic/qaic_drv.c
> +++ b/drivers/accel/qaic/qaic_drv.c

[ ... ]

> @@ -439,6 +441,9 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev,
>  	if (ret)
>  		return NULL;
>  	ret = drmm_mutex_init(drm, &qdev->bootlog_mutex);
> +	if (ret)
> +		return NULL;
> +	ret = drmm_mutex_init(drm, &qdev->tele_mutex);
>  	if (ret)
>  		return NULL;
>  
> @@ -451,6 +456,9 @@ static struct qaic_device *create_qdev(struct pci_dev *pdev,
>  	qdev->ssr_wq = qaicm_wq_init(drm, "qaic_ssr");
>  	if (IS_ERR(qdev->ssr_wq))
>  		return NULL;
> +	qdev->tele_wq = qaicm_wq_init(drm, "qaic_tele");
> +	if (IS_ERR(qdev->tele_wq))
> +		return NULL;

[Severity: High]
Can managing qdev->tele_mutex and qdev->tele_wq with drmm while qdev is
allocated with devm_kzalloc() cause a use-after-free on PCI removal?

If a process holds an open file descriptor to /dev/accel/accelN when the
PCI device is removed, drm_device outlives qaic_pci_remove(), while
devres frees qdev as soon as qaic_pci_remove() returns.  Because
qaic_telemetry_mhi_remove() also does not flush qdev->tele_wq, a queued
resp_worker() item can run after qdev is freed, and closing the file
descriptor later runs __drmm_mutex_release() on the freed
qdev->tele_mutex:

CPU0                                  CPU1 (kworker)
qaic_pci_remove()
  ...
  qaic_telemetry_mhi_dl_xfer_cb()
    queue_work(qdev->tele_wq, &resp->work)
  qaic_mhi_free_controller()
    qaic_telemetry_mhi_remove()
  devres_release_all(&pdev->dev)
    kfree(qdev)
                                      resp_worker()
                                        mutex_lock(&qdev->tele_mutex)
                                          <-- use-after-free

Flushing qdev->tele_wq in qaic_telemetry_mhi_remove() after
mhi_unprepare_from_transfer() and tying qdev's lifetime to drmm (or
managing tele_mutex and tele_wq with devm) would avoid this.

[ ... ]

> @@ -860,6 +869,10 @@ static int __init qaic_init(void)
>  		goto free_bootlog;
>  	}
>  
> +	ret = qaic_telemetry_register();
> +	if (ret)
> +		pr_debug("qaic: qaic_telemetry_register failed %d\n", ret);
> +
>  	return 0;
>  
>  free_bootlog:
> @@ -889,6 +902,7 @@ static void __exit qaic_exit(void)
>  	 * reinitializing the link_up state after the cleanup is done.
>  	 */
>  	link_up = true;
> +	qaic_telemetry_unregister();

[Severity: Low]
If qaic_telemetry_register() fails in qaic_init(), won't
qaic_telemetry_unregister() in qaic_exit() hit WARN(1, "Unexpected driver
unregister!\n") in driver_unregister() because drv->p is NULL?

> diff --git a/drivers/accel/qaic/qaic_telemetry.c b/drivers/accel/qaic/qaic_telemetry.c
> new file mode 100644
> index 0000000000000..eb86982f659d7
> --- /dev/null
> +++ b/drivers/accel/qaic/qaic_telemetry.c
> @@ -0,0 +1,563 @@

[ ... ]

> +const struct drm_ras_telemetry_field qaic_telemetry_fields_gettable[] = {
> +	{"QAIC_DEVINFO_THERMAL_SOC_MAX_TEMP", QAIC_DEVINFO_THERMAL_SOC_MAX_TEMP},

[ ... ]

> +};
> +
> +const struct drm_ras_telemetry_field qaic_telemetry_fields_settable[] = {
> +	{"QAIC_DEVINFO_THERMAL_WARNING_TEMP", QAIC_DEVINFO_THERMAL_WARNING_TEMP},

[ ... ]

> +	{"QAIC_DEVINFO_SOC_TDP", QAIC_DEVINFO_SOC_TDP},
> +	{},
> +};

[Severity: Low]
Should qaic_telemetry_fields_gettable and qaic_telemetry_fields_settable
be marked static?  They are only used within qaic_telemetry.c and have no
header declarations, which triggers sparse warnings.

Similarly, telemetry_request(), qaic_drm_ras_register_node(), and
qaic_drm_ras_unregister_node() have no callers outside qaic_telemetry.c
and could be made static and dropped from qaic_telemetry.h.

[ ... ]

> +	ret = wait_for_completion_interruptible_timeout(&elem.xfer_done, HZ);
> +	/*
> +	 * not using _interruptable because we have to cleanup or we'll
> +	 * likely cause memory corruption
> +	 */
> +	mutex_lock(&qdev->tele_mutex);
> +	if (!list_empty(&elem.list))
> +		list_del(&elem.list);
> +	/* resp_worker() processed the response but the wait was interrupted */
> +	else if (ret == -ERESTARTSYS)
> +		ret = 0;
> +	if (!ret && !elem.buf)
> +		ret = -ETIMEDOUT;
> +	else if (ret > 0 && !elem.buf)
> +		ret = -EIO;
> +	mutex_unlock(&qdev->tele_mutex);
> +
> +	resp = elem.buf;
> +
> +	/*
> +	 * telemetry_request() is called from Generic Netlink .doit callbacks
> +	 * rather than a syscall boundary, so -ERESTARTSYS is never translated
> +	 * to -EINTR before it would otherwise be serialized as-is into the
> +	 * netlink error ack.
> +	 */
> +	if (ret == -ERESTARTSYS)
> +		ret = -EINTR;
> +
> +	if (ret < 0)
> +		goto free_resp;
> +
> +	if (le16_to_cpu(resp->hdr.magic) != MAGIC || le16_to_cpu(resp->hdr.ver) != VERSION ||
> +	    resp->hdr.type != MSG_RESP || resp->hdr.id != 0 ||
> +	    le16_to_cpu(resp->hdr.len) != sizeof(resp->data) || resp->data.cmd != cmd ||
> +	    resp->data.cmd_type != cmd_type) {
> +		ret = -EINVAL;
> +		goto free_resp;
> +	}
> +
> +	if (resp->data.status) {
> +		switch ((int8_t) resp->data.status) {
> +		case ERR_READ_ONLY:
> +			*status = -EROFS;
> +			break;
> +		case ERR_NOT_SUPPORTED:
> +			*status = -EOPNOTSUPP;
> +			break;
> +		case ERR_WHILE_PROCESSING:
> +			*status = -EREMOTEIO;
> +			break;
> +		case ERR_INVALID_VALUE:
> +			*status = -EINVAL;
> +			break;
> +		case ERR_INVALID_HEADER:
> +			*status = -EBADR;
> +			break;
> +		default:
> +			*status = -EPROTO;
> +		}
> +		goto free_resp;
> +	}
> +
> +	if (cmd_type == TYPE_READ)
> +		*val = le64_to_cpu(resp->data.val);
> +
> +	ret = 0;
> +
> +free_resp:
> +	kfree(resp);
> +free_req:
> +	kref_put(&wrapper->ref_count, free_wrapper);
> +
> +	return ret;
> +}

[Severity: Medium]
Does telemetry_request() return positive remaining jiffies when
resp->data.status is non-zero?

wait_for_completion_interruptible_timeout() sets ret to the remaining
jiffies (> 0) when the completion signals before the timeout, and the
"if (resp->data.status)" branch jumps to free_resp before "ret = 0;".
drm_ras_nl_telemetry_get_doit() and drm_ras_nl_telemetry_set_doit() treat
any non-zero return from telemetry_get() or telemetry_set() as a callback
error and free the reply message instead of sending *status to userspace,
while netlink_rcv_skb() only sends an NLMSG_ERROR ack for negative
returns, so userspace receives neither the status attribute nor an error
ack.

> +static int qaic_telemetry_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_id *id)
> +{
> +	struct qaic_device *qdev = pci_get_drvdata(to_pci_dev(mhi_dev->mhi_cntrl->cntrl_dev));
> +	int ret;
> +
> +	ret = mhi_prepare_for_transfer(mhi_dev);
> +	if (ret)
> +		return ret;
> +
> +	dev_set_drvdata(&mhi_dev->dev, qdev);
> +	qdev->tele_ch = mhi_dev;
> +	qdev->tele_lost_buf = false;
> +
> +	ret = qaic_drm_ras_register_node(qdev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

[Severity: Medium]
If qaic_drm_ras_register_node() fails here, does
qaic_telemetry_mhi_probe() leak the prepared MHI channel state and leave
qdev->tele_ch dangling?  Since probe fails, qaic_telemetry_mhi_remove()
will not run, so mhi_unprepare_from_transfer(mhi_dev) and clearing
qdev->tele_ch need to happen on this error path.

> +static void qaic_telemetry_mhi_remove(struct mhi_device *mhi_dev)
> +{
> +	struct qaic_device *qdev = dev_get_drvdata(&mhi_dev->dev);
> +
> +	qaic_drm_ras_unregister_node(qdev);
> +	mhi_unprepare_from_transfer(qdev->tele_ch);
> +	qdev->tele_ch = NULL;
> +}

[Severity: Medium]
Can qaic_telemetry_mhi_remove() race with an in-flight
telemetry_request() on qdev->tele_ch?

During module unload in qaic_exit() or MHI driver unbind,
qaic_telemetry_mhi_remove() runs while qdev->dev_state is still
QAIC_ONLINE, and drm_ras_node_unregister() only erases the node from
drm_ras_xa without waiting for running netlink callbacks:

CPU0                                  CPU1
qaic_telemetry_get()
  telemetry_request()
    mutex_lock_interruptible(&qdev->tele_mutex)
    if (!qdev->tele_ch)
                                      qaic_telemetry_mhi_remove()
                                        mhi_unprepare_from_transfer(...)
                                        qdev->tele_ch = NULL;
    mhi_queue_buf(qdev->tele_ch, ...)
      mhi_queue(NULL, ...)  <-- NULL pointer dereference

In addition, when mhi_unprepare_from_transfer() resets the DL channel,
qaic_telemetry_mhi_dl_xfer_cb() sees transaction_status != 0 and drops the
buffer without completing elem.xfer_done.  Because
qaic_telemetry_mhi_remove() does not call wake_all_telemetry(qdev) and
telemetry_request() does not check qdev->dev_state under qdev->tele_mutex,
in-flight requests wait for the full timeout.

[ ... ]

> +void qaic_drm_ras_unregister_node(struct qaic_device *qdev)
> +{
> +	drm_ras_node_unregister(qdev->ras_node);
> +	kfree(qdev->ras_node->node_name);
> +	kfree(qdev->ras_node->device_name);
> +	kfree(qdev->ras_node);
> +}

[Severity: Medium]
Can freeing qdev->ras_node immediately after drm_ras_node_unregister()
cause a use-after-free in concurrent drm_ras netlink handlers?

drm_ras_nl_family enables .parallel_ops = true and looks up nodes in
drm_ras_xa via xa_load() or xa_for_each_start() without holding a lock or
reference across the callback, while drm_ras_node_unregister() only calls
xa_erase():

CPU0                                  CPU1
drm_ras_nl_pre_telemetry()
  node = xa_load(&drm_ras_xa, ...)
                                      qaic_telemetry_mhi_remove()
                                        qaic_drm_ras_unregister_node()
                                          drm_ras_node_unregister(ras_node)
                                          kfree(ras_node->node_name)
                                          kfree(ras_node->device_name)
                                          kfree(ras_node)
  node->telemetry_pre(node)  <-- use-after-free

Even on reset paths that call qaic_notify_reset() before MHI teardown,
synchronize_srcu(&qdev->dev_lock) runs before
qaic_drm_ras_unregister_node() erases the node from drm_ras_xa, and
qaic_telemetry_pre(), qaic_telemetry_get(), and qaic_telemetry_set() all
dereference ep before acquiring qdev->dev_lock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106181225.3333260-1-zachary.mckevitt@oss.qualcomm.com?part=3