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 <[email protected]> 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/[email protected]?part=3
