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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] accel/qaic: NULL pointer dereference and race on qdev->tele_ch
- [High] accel/qaic: missing dev_lock SRCU sync and tele_ch state check in telemetry
- [High] accel/qaic: missing tele_wq flush and drmm/devm mismatch in create_qdev()
- [Medium] accel/qaic: qaic_telemetry_get() and qaic_telemetry_set() drop status replies on error
- [Medium] accel/qaic: silent u8 truncation of u32 telemetry field in telemetry_request()
- [Medium] accel/qaic: ignored qaic_telemetry_register() failure in qaic_init()
- [Medium] accel/qaic: telemetry_request() drops completed response and leaks -ERESTARTSYS to netlink
--

commit 089cd42088c5c813480c90b14aeb8fbd79e5ff35
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 during PCI probe so userspace can read and write device
telemetry fields over Generic Netlink.

> diff --git a/drivers/accel/qaic/qaic_drv.c b/drivers/accel/qaic/qaic_drv.c
> index 9e1e184efe0d1..e24a4f271a585 100644
> --- a/drivers/accel/qaic/qaic_drv.c
> +++ b/drivers/accel/qaic/qaic_drv.c
> @@ -32,6 +32,7 @@
>  #include "qaic_debugfs.h"
>  #include "qaic_ras.h"
>  #include "qaic_ssr.h"
> +#include "qaic_telemetry.h"
>  #include "qaic_timesync.h"
>  #include "sahara.h"
>  
> @@ -389,6 +390,7 @@ static void qaic_notify_reset(struct qaic_device *qdev)
>  	qdev->dev_state = QAIC_OFFLINE;
>  	/* wake up any waiters to avoid waiting for timeouts at sync */
>  	wake_all_cntl(qdev);
> +	wake_all_telemetry(qdev);
>  	for (i = 0; i < qdev->num_dbc; ++i)
>  		wakeup_dbc(qdev, i);
>  	synchronize_srcu(&qdev->dev_lock);
> @@ -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 this cause a use-after-free of qdev if the PCI device is removed while a
userspace process holds an open file descriptor to /dev/accel/accelN?

In create_qdev(), qdev is allocated with devm_kzalloc(&pdev->dev, ...),
whereas qdev->tele_mutex and qdev->tele_wq are managed by drmm on struct
drm_device. If /dev/accel/accelN is open when qaic_pci_remove() runs:

1. qaic_telemetry_mhi_remove() calls mhi_unprepare_from_transfer(qdev->tele_ch)
   without flushing qdev->tele_wq.
2. When qaic_pci_remove() returns, devres_release_all(&pdev->dev) drops the
   initial drm_device reference via devm_drm_dev_init_release(), but
   drm_managed_release() does not run yet because the open file descriptor
   still holds a reference on drm_device.
3. devres_release_all() immediately frees the devm-allocated qdev.

At that point, any pending resp_worker() work item on qdev->tele_wq will
dereference freed qdev memory (qdev->tele_mutex and qdev->tele_xfer_list),
and when the userspace file descriptor is finally closed,
__drmm_mutex_release() will call mutex_destroy(&qdev->tele_mutex) on the
already-freed qdev.

Should qaic_telemetry_mhi_remove() flush qdev->tele_wq, and should the
lifetime of qdev->tele_mutex and qdev->tele_wq match the devm lifetime of
qdev?

[ ... ]

> @@ -611,6 +620,13 @@ static int qaic_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		return ret;
>  	}
>  
> +	ret = qaic_drm_ras_register_node(qdev);
> +	if (ret) {
> +		qaic_mhi_free_controller(qdev->mhi_cntrl, link_up);
> +		qaic_destroy_drm_device(qdev, QAIC_NO_PARTITION);
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -624,6 +640,7 @@ static void qaic_pci_remove(struct pci_dev *pdev)
>  		return;
>  
>  	qaic_dev_reset_clean_local_state(qdev);
> +	qaic_drm_ras_unregister_node(qdev);
>  	qaic_mhi_free_controller(qdev->mhi_cntrl, link_up);
>  	qaic_destroy_drm_device(qdev, QAIC_NO_PARTITION);
>  }

[Severity: High]
Does qaic_pci_remove() risk a use-after-free on qdev and qdev->ras_node if a
netlink telemetry request is in flight during device removal or reset?

In qaic_notify_reset() (called by qaic_dev_reset_clean_local_state()), the
driver sets qdev->dev_state = QAIC_OFFLINE, calls wake_all_telemetry(qdev),
and then calls synchronize_srcu(&qdev->dev_lock) to wait for in-flight
readers.

However, unlike the control path in qaic_control.c, qaic_telemetry_get(),
qaic_telemetry_set(), and telemetry_request() never acquire
srcu_read_lock(&qdev->dev_lock) and never check whether qdev->dev_state is
QAIC_ONLINE. In addition, qaic_pci_remove() runs
qaic_dev_reset_clean_local_state(qdev) before qaic_drm_ras_unregister_node(),
and qaic_drm_ras_unregister_node() frees qdev->ras_node immediately after
drm_ras_node_unregister().

If a netlink caller is sleeping in
wait_for_completion_interruptible_timeout(&elem.xfer_done, HZ) inside
telemetry_request(), or enters telemetry_request() after wake_all_telemetry()
runs while qdev->ras_node is still registered, synchronize_srcu() will not
wait for it. When the waiter wakes up after qaic_pci_remove() completes,
telemetry_request() will call mutex_lock(&qdev->tele_mutex) and
list_del(&elem.list) on freed qdev memory.

Should qaic_drm_ras_unregister_node() be called before
qaic_dev_reset_clean_local_state(), and should the telemetry callbacks guard
access with srcu_read_lock(&qdev->dev_lock) and check qdev->dev_state?

> @@ -860,6 +877,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 +910,7 @@ static void __exit qaic_exit(void)
>  	 * reinitializing the link_up state after the cleanup is done.
>  	 */
>  	link_up = true;
> +	qaic_telemetry_unregister();
>  	qaic_ssr_unregister();
>  	qaic_ras_unregister();
>  	qaic_bootlog_unregister();

[Severity: Medium]
If qaic_telemetry_register() fails in qaic_init(), qaic_init() logs a debug
message and returns 0, leaving qaic_telemetry_mhi_driver unregistered
(drv->p == NULL). When the module is later unloaded, qaic_exit()
unconditionally calls qaic_telemetry_unregister() -> mhi_driver_unregister()
-> driver_unregister(), which triggers:

    WARN(1, "Unexpected driver unregister!\n");

in drivers/base/driver.c:driver_unregister(). Should qaic_init() unwind and
propagate the error when qaic_telemetry_register() fails, matching
qaic_ssr_register()?

> diff --git a/drivers/accel/qaic/qaic_telemetry.c b/drivers/accel/qaic/qaic_telemetry.c
> new file mode 100644
> index 0000000000000..fdce7a71d3eba
> --- /dev/null
> +++ b/drivers/accel/qaic/qaic_telemetry.c

[ ... ]

> +int telemetry_request(struct qaic_device *qdev, u8 cmd, u8 cmd_type, s64 *val)
> +{
> +	struct wrapper_msg *wrapper;
> +	struct xfer_queue_elem elem;
> +	struct telemetry_msg *resp;
> +	struct telemetry_msg *req;
> +	long ret = 0;
> +
> +	wrapper = kzalloc_obj(*wrapper);
> +	if (!wrapper)
> +		return -ENOMEM;
> +
> +	kref_init(&wrapper->ref_count);
> +	req = &wrapper->msg;
> +
> +	ret = mutex_lock_interruptible(&qdev->tele_mutex);
> +	if (ret)
> +		goto free_req;
> +
> +	req->hdr.magic = cpu_to_le16(MAGIC);
> +	req->hdr.ver = cpu_to_le16(VERSION);
> +	req->hdr.seq_num = cpu_to_le32(qdev->tele_next_seq_num++);
> +	req->hdr.type = MSG_REQ;
> +	req->hdr.id = 0;
> +	req->hdr.len = cpu_to_le16(sizeof(req->data));
> +
> +	req->data.cmd = cmd;
> +	req->data.cmd_type = cmd_type;
> +	req->data.status = 0;
> +	if (cmd_type == TYPE_READ)
> +		req->data.val = cpu_to_le64(0);
> +	else
> +		req->data.val = cpu_to_le64(*val);
> +
> +	elem.seq_num = qdev->tele_next_seq_num - 1;
> +	elem.buf = NULL;
> +	init_completion(&elem.xfer_done);
> +	if (likely(!qdev->tele_lost_buf)) {
> +		resp = kzalloc_obj(*resp);
> +		if (!resp) {
> +			mutex_unlock(&qdev->tele_mutex);
> +			ret = -ENOMEM;
> +			goto free_req;
> +		}
> +
> +		ret = mhi_queue_buf(qdev->tele_ch, DMA_FROM_DEVICE, resp, sizeof(*resp), MHI_EOT);
> +		if (ret) {
> +			mutex_unlock(&qdev->tele_mutex);
> +			goto free_resp;
> +		}
> +	} else {
> +		/*
> +		 * we lost a buffer because we queued a recv buf, but then
> +		 * queuing the corresponding tx buf failed. To try to avoid
> +		 * a memory leak, lets reclaim it and use it for this
> +		 * transaction.
> +		 */
> +		qdev->tele_lost_buf = false;
> +	}
> +
> +	kref_get(&wrapper->ref_count);
> +	ret = mhi_queue_buf(qdev->tele_ch, DMA_TO_DEVICE, req, sizeof(*req), MHI_EOT);

[Severity: High]
What happens if telemetry_request() is called when qdev->tele_ch is NULL, or
while qaic_telemetry_mhi_remove() is running concurrently?

In qaic_pci_probe(), qaic_drm_ras_register_node(qdev) exposes the telemetry
netlink callbacks immediately after qaic_create_drm_device() creates
/dev/accel/accelN. However, qdev->tele_ch is only set asynchronously in
qaic_telemetry_mhi_probe() once the device enters AMSS, and is reset to NULL
in qaic_telemetry_mhi_remove().

If userspace sends a DRM_RAS_CMD_TELEMETRY_GET or DRM_RAS_CMD_TELEMETRY_SET
request before qaic_telemetry_mhi_probe() runs (or after
qaic_telemetry_mhi_remove() clears qdev->tele_ch), telemetry_request() passes
qdev->tele_ch == NULL to mhi_queue_buf() -> mhi_queue(), which immediately
dereferences mhi_dev->mhi_cntrl and crashes.

Furthermore, qaic_telemetry_mhi_probe() and qaic_telemetry_mhi_remove()
modify qdev->tele_ch and qdev->tele_lost_buf and call
mhi_unprepare_from_transfer(qdev->tele_ch) without holding qdev->tele_mutex,
and qaic_telemetry_mhi_remove() does not wake waiters on
qdev->tele_xfer_list. Should telemetry_request() check if (!qdev->tele_ch)
under qdev->tele_mutex (returning -ENODEV), and should
qaic_telemetry_mhi_probe() and qaic_telemetry_mhi_remove() serialize against
qdev->tele_mutex and wake pending waiters?

> +	if (ret) {
> +		qdev->tele_lost_buf = true;
> +		kref_put(&wrapper->ref_count, free_wrapper);
> +		mutex_unlock(&qdev->tele_mutex);
> +		goto free_req;
> +	}
> +
> +	list_add_tail(&elem.list, &qdev->tele_xfer_list);
> +	mutex_unlock(&qdev->tele_mutex);
> +
> +	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);
> +	if (!ret && !elem.buf)
> +		ret = -ETIMEDOUT;
> +	else if (ret > 0 && !elem.buf)
> +		ret = -EIO;
> +	mutex_unlock(&qdev->tele_mutex);
> +
> +	resp = elem.buf;
> +
> +	if (ret < 0)
> +		goto free_resp;

[Severity: Medium]
What happens here when wait_for_completion_interruptible_timeout() (or the
earlier mutex_lock_interruptible()) is interrupted by a signal and returns
-ERESTARTSYS?

First, if resp_worker() already completed the transfer and set elem.buf
before telemetry_request() acquired qdev->tele_mutex, ret is still
-ERESTARTSYS, so if (ret < 0) branches to free_resp and discards the valid
response (unlike msg_xfer() in qaic_control.c, which resets ret = 0 when
elem.buf is present).

Second, because telemetry_request() is called from Generic Netlink .doit
callbacks (drm_ras_nl_telemetry_get_doit() and
drm_ras_nl_telemetry_set_doit()), returning -ERESTARTSYS causes
netlink_rcv_skb() -> netlink_ack() to serialize the kernel-internal
-ERESTARTSYS (-512) value directly into the NLMSG_ERROR payload sent to
userspace. Should this check for a completed elem.buf and translate any
remaining -ERESTARTSYS to -EINTR?

[ ... ]

> +static int qaic_telemetry_get(struct drm_ras_node *ep, u32 *field, s64 *value, s32 *status)
> +{
> +	struct qaic_device *qdev = ep->priv;
> +
> +	*status = (s32)telemetry_request(qdev, *field, TYPE_READ, value);
> +
> +	return *status;
> +}
> +
> +static int qaic_telemetry_set(struct drm_ras_node *ep, u32 *field, s64 *value, s32 *status)
> +{
> +	struct qaic_device *qdev = ep->priv;
> +
> +	*status = (s32)telemetry_request(qdev, *field, TYPE_WRITE, value);
> +
> +	return *status;
> +}

[Severity: Medium]
In qaic_telemetry_get() and qaic_telemetry_set(), *field is a u32 coming from
the netlink DRM_RAS_A_TELEMETRY_ATTRS_FIELD attribute (which drm_ras does not
validate against telemetry_fields_gettable or telemetry_fields_settable in
the doit path), whereas telemetry_request() takes u8 cmd.

Doesn't passing *field directly to telemetry_request() silently truncate
32-bit field IDs modulo 256 (for example, turning 0x101 into 1,
QAIC_DEVINFO_THERMAL_SOC_MAX_TEMP) instead of rejecting out-of-range or
unsupported field IDs?

[Severity: Medium]
Also in qaic_telemetry_get() and qaic_telemetry_set(), both functions assign
the return value of telemetry_request() to *status and then return *status.
Looking at drm_ras_nl_telemetry_get_doit() and
drm_ras_nl_telemetry_set_doit() in drivers/gpu/drm/drm_ras.c:

	ret = node->telemetry_get(node, &field, &value, &status);
	if (ret) {
		nlmsg_free(msg);
		return ret;
	}

	ret = nla_put_s32(msg, DRM_RAS_A_TELEMETRY_ATTRS_STATUS, status);

When telemetry_request() returns a non-zero error code (such as when the
device returns a firmware status error in resp->data.status), returning
*status from the callback causes drm_ras to free the reply message and abort
before encoding DRM_RAS_A_TELEMETRY_ATTRS_STATUS. Is *status intended to be
reported in the netlink reply when the device returns a status error (with
the callback returning 0)?

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