Re: [PATCH v5 2/5] drm/ras: Introduce error threshold

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The `set-error-threshold` command only ACKs on success instead of replying with a full message, violating Netlink design rules.

Pre-existing issues:
- [High] Lockless access to the `drm_ras_xa` XArray without RCU protection or reference counting leads to a Use-After-Free vulnerability when accessing `drm_ras_node`.
--

--- Patch [2]: [PATCH v5 2/5] drm/ras: Introduce error threshold ---
commit c7d3a70f53c077dda897bf8be8b57f1fb2216708
Author: Raag Jadav <raag.jadav@intel.com>

drm/ras: Introduce error threshold

Adds get-error-threshold and set-error-threshold netlink commands to query
and set RAS error thresholds for hardware error accumulation control.

> diff --git a/Documentation/netlink/specs/drm_ras.yaml b/Documentation/netlink/specs/drm_ras.yaml
> index e113056f8c016..9cf7f9cde2425 100644
> --- a/Documentation/netlink/specs/drm_ras.yaml
> +++ b/Documentation/netlink/specs/drm_ras.yaml
> @@ -124,3 +128,31 @@ operations:
[ ... ]
> +    -
> +      name: set-error-threshold
> +      doc: >-
> +           Set error threshold of a given counter.
> +      attribute-set: error-counter-attrs
> +      flags: [admin-perm]
> +      do:
> +        request:
> +          attributes:
> +            - node-id
> +            - error-id
> +            - error-threshold

[Severity: Medium]
Should this operation include a reply definition?

The Netlink uAPI guidelines for new families state that all operations,
especially SET or ADD commands, must reply with a full message rather than
relying on a standard ACK. 

Once a command only ACKs, it becomes uAPI and cannot be fixed later. Would
it be better to return the updated state of the threshold here instead of
relying on a separate GET request?

> diff --git a/drivers/gpu/drm/drm_ras.c b/drivers/gpu/drm/drm_ras.c
> index 467a169026fc1..9a7b3d2acd25d 100644
> --- a/drivers/gpu/drm/drm_ras.c
> +++ b/drivers/gpu/drm/drm_ras.c
[ ... ]
> @@ -168,6 +186,40 @@ static int get_node_error_counter(u32 node_id, u32 error_id,
> +static int get_node_error_threshold(u32 node_id, u32 error_id, const char **name, u32 *threshold)
> +{
> +	struct drm_ras_node *node;
> +
> +	node = xa_load(&drm_ras_xa, node_id);
> +	if (!node)
> +		return -ENOENT;
> +
> +	if (!node->query_error_threshold)

[Severity: High]
This is a pre-existing issue with how this file accesses the xarray, but
is it safe to dereference the node pointer here without RCU protection or
a reference count?

The generic netlink family drm_ras_nl_family sets .parallel_ops = true,
meaning doit callbacks run without genl_mutex. While xa_load() takes the
RCU read lock internally, it drops it before returning.

If a DRM device is concurrently unbound and freed, couldn't this memory be
freed before we call node->query_error_threshold(), resulting in a
use-after-free?

> +		return -EOPNOTSUPP;
[ ... ]
> @@ -358,6 +463,59 @@ int drm_ras_nl_clear_error_counter_doit(struct sk_buff *skb,
[ ... ]
> +int drm_ras_nl_set_error_threshold_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	u32 node_id, error_id, threshold;
> +
> +	if (!info->attrs ||
> +	    GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_ERROR_COUNTER_ATTRS_NODE_ID) ||
> +	    GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_ERROR_COUNTER_ATTRS_ERROR_ID) ||
> +	    GENL_REQ_ATTR_CHECK(info, DRM_RAS_A_ERROR_COUNTER_ATTRS_ERROR_THRESHOLD))
> +		return -EINVAL;
> +
> +	node_id = nla_get_u32(info->attrs[DRM_RAS_A_ERROR_COUNTER_ATTRS_NODE_ID]);
> +	error_id = nla_get_u32(info->attrs[DRM_RAS_A_ERROR_COUNTER_ATTRS_ERROR_ID]);
> +	threshold = nla_get_u32(info->attrs[DRM_RAS_A_ERROR_COUNTER_ATTRS_ERROR_THRESHOLD]);
> +
> +	return set_node_error_threshold(node_id, error_id, threshold);
> +}

[Severity: Medium]
Does this operation need to construct and return a reply message?

By returning directly, this handler only emits a standard generic netlink
ACK on success. As mentioned above, the Netlink design guidelines require
operations to reply with a full message rather than just an ACK.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722034228.662746-1-raag.jadav@intel.com?part=2