Re: [PATCH v3 2/3] drm/drm_ras: Extend DRM RAS interface to include telemetry nodes

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Sat, Nov 07, 2026 at 06:18:56PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issues to consider:
> - [High] drm/drm_ras: unsynchronized xa_load() node lookup in telemetry
>   handlers
> - [Medium] drm/drm_ras: broken status handling and non-standard UAPI in
>   telemetry ops
> --
> 
> commit 136edf60d4fd4d79aaeb6f30bb50fe8f150aa0fe
> Author: Zack McKevitt <zachary.mckevitt@oss.qualcomm.com>
> 
> drm/drm_ras: Extend DRM RAS interface to include telemetry nodes
> 
> This commit extends the DRM RAS Generic Netlink specification and driver
> interface with a telemetry node type and operations to list, read, and
> write device telemetry fields.
> 
> > diff --git a/Documentation/netlink/specs/drm_ras.yaml b/Documentation/netlink/specs/drm_ras.yaml
> > index a88fdab7a1c74..cf015bba020d0 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]
> Does the telemetry Netlink UAPI and its status handling work as intended
> when a device reports an error?
> 
> In drm_ras_nl_telemetry_get_doit() and drm_ras_nl_telemetry_set_doit(), a
> non-zero return from node->telemetry_get() or node->telemetry_set() frees
> msg and returns immediately without serializing
> DRM_RAS_A_TELEMETRY_ATTRS_STATUS.  Conversely, if the callback returns 0
> with a non-zero status, drm_ras_nl_telemetry_get_doit() still serializes a
> zero DRM_RAS_A_TELEMETRY_ATTRS_VALUE alongside status and bypasses standard
> Netlink NLMSG_ERROR and extack reporting:
> 
> drivers/gpu/drm/drm_ras.c:drm_ras_nl_telemetry_get_doit() {
>     ...
> 	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);
>     ...
> 	ret = nla_put_sint(msg, DRM_RAS_A_TELEMETRY_ATTRS_VALUE, value);
>     ...
> }
> 
> In addition, telemetry-set attaches a dump callback to a set command to
> enumerate writable fields, telemetry-get returns different objects in do
> ([value, status]) versus dump ([field, field-name]), and telemetry-attrs
> exposes board temperatures, voltages, power, and TDP limits as device-
> specific field IDs and unitless integers rather than through hwmon.
> 
> > diff --git a/drivers/gpu/drm/drm_ras.c b/drivers/gpu/drm/drm_ras.c
> > index 4f4777060a4e8..fda50563c4a54 100644
> > --- a/drivers/gpu/drm/drm_ras.c
> > +++ b/drivers/gpu/drm/drm_ras.c
> [ ... ]
> > @@ -617,6 +644,285 @@ int drm_ras_nl_set_error_threshold_doit(struct sk_buff *skb, struct genl_info *i
> > +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;
> > +}
> [ ... ]
> > +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;
> > +
> > +	if (!telemetry_fields_contains(node->telemetry_fields_gettable, field))
> > +		return -EINVAL;
> > +
> > +	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);
> 
> [Severity: High]
> Can the unlocked xa_load() lookups in the telemetry handlers race with
> drm_ras_node_unregister() and cause a use-after-free?
> 
> drm_ras_nl_family sets .parallel_ops = true, and xa_load() only holds the
> RCU read lock during the XArray walk itself.  drm_ras_nl_pre_telemetry(),
> drm_ras_nl_telemetry_get_doit(), drm_ras_nl_telemetry_get_dumpit(),
> drm_ras_nl_telemetry_set_doit(), and drm_ras_nl_telemetry_set_dumpit() use
> the returned node pointer without holding a lock or reference across
> GFP_KERNEL allocations and sleeping driver callbacks, while
> drm_ras_node_unregister() only calls xa_erase() before the caller frees the
> node:
> 
> CPU0                                  CPU1
> drm_ras_nl_telemetry_get_doit()
>   node = xa_load(&drm_ras_xa, id);
>   genlmsg_new(..., GFP_KERNEL);
>                                       drm_ras_node_unregister(node)
>                                         xa_erase(&drm_ras_xa, node->id);
>                                       kfree(node);
>   node->telemetry_get(node, ...);  <-- use-after-free
> 
> Because drm_ras_nl_pre_telemetry() and the doit handlers also perform
> separate unlocked xa_load() calls instead of pinning the node in pre_doit
> and releasing it in post_doit, node_id can be unregistered and reallocated
> to a different node between the permission check and the operation.

Perhaps we should only do xa_load in the pre_doit and stash it with a put
in a post_doit?! or something like that?!...

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