Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] drm/drm_ras: unsynchronized node lookup in telemetry netlink handlers - [Medium] drm/drm_ras: broken status handling and non-standard UAPI in telemetry ops - [Medium] drm/drm_ras: missing field validation in telemetry doit callbacks - [Low] drm/drm_ras: mismatched kernel-doc @field tag in drm_ras_telemetry_field --
commit dc8b379237033d84f80f290cec4514d0c3cc4442 Author: Zack McKevitt <[email protected]> drm/drm_ras: Extend DRM RAS interface to include telemetry nodes This commit adds a telemetry node type, netlink attribute set, and doit/dumpit callbacks to the DRM RAS interface to support reading and writing device telemetry fields. > diff --git a/Documentation/netlink/specs/drm_ras.yaml > b/Documentation/netlink/specs/drm_ras.yaml > index a88fdab7a1c74..a02aaed8772fc 100644 > --- a/Documentation/netlink/specs/drm_ras.yaml > +++ b/Documentation/netlink/specs/drm_ras.yaml [ ... ] > @@ -100,6 +100,34 @@ attribute-sets: > name: error-value > type: u32 > doc: Current value of the error counter. > + - > + name: telemetry-attrs > + attributes: > + - > + name: node-id > + type: u32 > + doc: >- > + Unique identifier for the node. > + Assigned dynamically by the DRM RAS core upon registration. > + - > + name: field > + type: u32 > + doc: Telemetry field to be read/written, e.g., SoC temp. > + - > + name: field-name > + type: string > + doc: Name of the telemetry field defined by the device's uapi. > + - > + name: value > + type: sint > + doc: Value of telemetry field that was read/written. > + - > + name: status > + type: s32 > + doc: >- > + Status of the telemetry operation. > + Populated by the device in response to the performed > + telemetry operation. Errors may be hardware specific. > > operations: > list: > @@ -198,6 +226,52 @@ operations: > - node-id > - error-id > - error-threshold > + - > + name: telemetry-get > + doc: Perform a read of the specified telemetry field. > + attribute-set: telemetry-attrs > + do: > + pre: drm-ras-nl-pre-telemetry > + request: > + attributes: > + - node-id > + - field > + reply: > + attributes: > + - value > + - status > + dump: > + request: > + attributes: > + - node-id > + reply: > + attributes: &telemetry-field-attrs > + - field > + - field-name > + - > + name: telemetry-set > + doc: >- > + Perform a write of the specified telemetry field. > + Telemetry writes are device specific, and may be used to set > + device limits or other capacities, for example. > + attribute-set: telemetry-attrs > + flags: [admin-perm] > + do: > + pre: drm-ras-nl-pre-telemetry > + request: > + attributes: > + - node-id > + - field > + - value > + reply: > + attributes: > + - status > + dump: > + request: > + attributes: > + - node-id > + reply: > + attributes: *telemetry-field-attrs [Severity: Medium] Is this Netlink uAPI structure and status attribute behavior intentional? In drm_ras_nl_telemetry_get_doit() and drm_ras_nl_telemetry_set_doit(), if node->telemetry_get() or node->telemetry_set() returns a non-zero value, the handler frees msg and returns ret directly without serializing DRM_RAS_A_TELEMETRY_ATTRS_STATUS: ret = node->telemetry_get(node, &field, &value, &status); if (ret) { nlmsg_free(msg); return ret; } In the consumer patch "accel/qaic: Add callbacks for DRM RAS netlink telemetry", qaic_telemetry_get() and qaic_telemetry_set() assign *status from telemetry_request() (which maps firmware errors to negative errno values) and return *status directly. Consequently, whenever an error occurs, the reply is aborted and DRM_RAS_A_TELEMETRY_ATTRS_STATUS is never sent, whereas whenever a reply is sent, status is always 0. Conversely, if a driver returns 0 while setting a non-zero *status, drm_ras_nl_telemetry_get_doit() unconditionally serializes the uninitialized or zero DRM_RAS_A_TELEMETRY_ATTRS_VALUE and bypasses standard Netlink error reporting. In addition, telemetry-set attaches a dump operation to a SET command to list writable field descriptors, and telemetry-get returns different object representations in do ([value, status]) versus dump ([field, field-name]), unlike get-error-counter where do and dump return the same attributes. Finally, telemetry-attrs exposes opaque device-specific field IDs and unitless values for thermal, power, and voltage sensors and limits (such as SoC/board temperatures and TDP in the qaic consumer). Should those metrics be exposed through hwmon rather than a device-specific key-value pass-through in DRM RAS? > diff --git a/drivers/gpu/drm/drm_ras.c b/drivers/gpu/drm/drm_ras.c > index 4f4777060a4e8..5b9c00f28834b 100644 > --- a/drivers/gpu/drm/drm_ras.c > +++ b/drivers/gpu/drm/drm_ras.c [ ... ] > @@ -617,6 +644,264 @@ int drm_ras_nl_set_error_threshold_doit(struct sk_buff > *skb, struct genl_info *i > return set_node_error_threshold(node_id, error_id, threshold); > } > > +/** > + * drm_ras_nl_pre_telemetry - Enforce permission checks > + * @ops: Generic netlink operations > + * @skb: Netlink message buffer > + * @info: Generic Netlink info containing attributes of the request > + * > + * Enforces a telemetry node's pre_doit conditions (if they exist). > + * > + * Returns 0 if no pre_doit is specified, otherwise returns the pre_doit. > + */ > +int drm_ras_nl_pre_telemetry(const struct genl_split_ops *ops, struct > sk_buff *skb, > + struct genl_info *info) > +{ > + struct drm_ras_node *node; > + u32 node_id; > + > + if (!info->attrs || GENL_REQ_ATTR_CHECK(info, > DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID)) > + return -EINVAL; > + > + node_id = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID]); > + node = xa_load(&drm_ras_xa, node_id); > + if (!node) > + return -ENOENT; > + > + if (node->telemetry_pre) > + return node->telemetry_pre(node); > + > + return 0; > +} [Severity: High] What prevents node from being unregistered and freed while drm_ras_nl_pre_telemetry(), drm_ras_nl_telemetry_get_doit(), drm_ras_nl_telemetry_get_dumpit(), drm_ras_nl_telemetry_set_doit(), or drm_ras_nl_telemetry_set_dumpit() is using it? The drm_ras Generic Netlink family sets .parallel_ops = true, and xa_load() only holds rcu_read_lock() internally during the lookup, returning an unpinned struct drm_ras_node pointer without a lock or reference count. In drm_ras_nl_telemetry_get_doit() and drm_ras_nl_telemetry_set_doit(), the handler holds node across a sleeping genlmsg_new(..., GFP_KERNEL) allocation and a sleeping driver callback (such as qaic_telemetry_get() -> telemetry_request()), while drm_ras_node_unregister() only calls xa_erase(&drm_ras_xa, node->id) and returns immediately before the driver frees node and the underlying device structure. Furthermore, because drm_ras_nl_pre_telemetry() and the doit handlers perform separate unlocked xa_load(&drm_ras_xa, node_id) lookups instead of pinning the validated node in pre_doit, can node_id be unregistered and reallocated to a different node between pre_doit and doit? [ ... ] > +int drm_ras_nl_telemetry_get_doit(struct sk_buff *skb, struct genl_info > *info) > +{ > + struct drm_ras_node *node; > + struct sk_buff *msg; > + struct nlattr *hdr; > + u32 node_id, field; > + s32 status = 0; > + s64 value = 0; > + int ret = 0; > + > + if (!info->attrs || > + GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID) || > + GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_FIELD)) > + return -EINVAL; > + > + field = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_FIELD]); > + node_id = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID]); > + > + node = xa_load(&drm_ras_xa, node_id); > + if (!node || !node->telemetry_get) > + return -ENOENT; > + > + msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL); > + if (!msg) > + return -ENOMEM; > + > + hdr = genlmsg_iput(msg, info); > + if (!hdr) { > + nlmsg_free(msg); > + return -EMSGSIZE; > + } > + > + ret = node->telemetry_get(node, &field, &value, &status); [ ... ] > +int drm_ras_nl_telemetry_set_doit(struct sk_buff *skb, struct genl_info > *info) > +{ > + struct drm_ras_node *node; > + struct sk_buff *msg; > + struct nlattr *hdr; > + u32 node_id, field; > + s32 status = 0; > + s64 value = 0; > + int ret = 0; > + > + if (!info->attrs || > + GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID) || > + GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_VALUE) || > + GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_TELEMETRY_ATTRS_FIELD)) > + return -EINVAL; > + > + value = nla_get_sint(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_VALUE]); > + field = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_FIELD]); > + node_id = nla_get_u32(info->attrs[DRM_RAS_A_TELEMETRY_ATTRS_NODE_ID]); > + > + node = xa_load(&drm_ras_xa, node_id); > + if (!node || !node->telemetry_set) > + return -ENOENT; [ ... ] > @@ -631,14 +916,21 @@ int drm_ras_node_register(struct drm_ras_node *node) > if (!node->device_name || !node->node_name) > return -EINVAL; > > - /* Currently, only Error Counter Endpoints are supported */ > - if (node->type != DRM_RAS_NODE_TYPE_ERROR_COUNTER) > - return -EINVAL; > - > - /* Mandatory entries for Error Counter Node */ > - if (node->type == DRM_RAS_NODE_TYPE_ERROR_COUNTER && > - (!node->error_counter_range.last || !node->query_error_counter)) > + /* Currently, only Error Counter and Telemetry endpoints are supported > */ > + switch (node->type) { > + case DRM_RAS_NODE_TYPE_ERROR_COUNTER: > + /* Mandatory entries for Error Counter Node */ > + if (!node->error_counter_range.last || > !node->query_error_counter) > + return -EINVAL; > + break; > + case DRM_RAS_NODE_TYPE_TELEMETRY: > + /* Must provide telemetry_get, telemetry_set is optional */ > + if (!node->telemetry_get || !node->telemetry_fields_gettable) > + return -EINVAL; > + break; [Severity: Medium] Should drm_ras_nl_telemetry_get_doit() and drm_ras_nl_telemetry_set_doit() validate the user-supplied field ID against node->telemetry_fields_gettable and node->telemetry_fields_settable before invoking the driver callbacks? Unlike the error counter handlers, which check error_id against node->error_counter_range, drm_ras_nl_telemetry_get_doit() and drm_ras_nl_telemetry_set_doit() pass the untrusted u32 field directly to node->telemetry_get() and node->telemetry_set(). In the qaic consumer patch ("accel/qaic: Add callbacks for DRM RAS netlink telemetry"), qaic_telemetry_get() and qaic_telemetry_set() pass *field directly to telemetry_request(struct qaic_device *qdev, u8 cmd, ...), which truncates the u32 value to u8 so out-of-range values like 0x101 alias to valid commands. Also, since the documentation comment states that settable telemetry fields are required when providing telemetry_set(), should drm_ras_node_register() reject a node that sets node->telemetry_set without node->telemetry_fields_settable, and should include/drm/drm_ras.h document that telemetry_fields_gettable and telemetry_fields_settable must be terminated by a NULL .name sentinel entry for telemetry_fields_dumpit()? > diff --git a/include/drm/drm_ras.h b/include/drm/drm_ras.h > index 1765a3130b09b..fb10238999ddb 100644 > --- a/include/drm/drm_ras.h > +++ b/include/drm/drm_ras.h > @@ -10,6 +10,16 @@ > > #include <uapi/drm/drm_ras.h> > > +/** > + * struct drm_ras_telemetry_field - A descriptor of a telemetry field. > + */ > +struct drm_ras_telemetry_field { > + /** @name: The enum or macro name defined by the device uAPI. */ > + const char *name; > + /** @field: The numerical value of the field. */ > + u32 value; > +}; [Severity: Low] In struct drm_ras_telemetry_field, should the kernel-doc tag @field be @value to match the struct member name? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
