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