From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/ras: Introduce the DRM RAS infrastructure over generic netlink
Date: Tue, 24 Feb 2026 10:45:41 +1000
Message-ID: <review-patch1-20260223060541.526397-8-riana.tauro@intel.com>
In-Reply-To: <20260223060541.526397-8-riana.tauro@intel.com>
References: <20260223060541.526397-7-riana.tauro@intel.com> <20260223060541.526397-8-riana.tauro@intel.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

**Uninitialized return value in `drm_ras_nl_list_nodes_dumpit()`:**

> +	int ret;
> +
> +	xa_for_each_start(&drm_ras_xa, id, node, ctx->restart) {
> +		...
> +		genlmsg_end(skb, hdr);
> +	}
> +
> +	if (ret == -EMSGSIZE)
> +		ctx->restart = id;
> +
> +	return ret;

If the xarray is empty, or if `ctx->restart` is past all entries (which can happen on the continuation call after the last batch of entries exactly filled the previous skb), the loop body never executes and `ret` is used uninitialized. This is undefined behavior. `ret` should be initialized to 0 so the function returns 0 (dump complete) when there are no entries to process.

The same issue exists in `drm_ras_nl_get_error_counter_dumpit()` -- if `first > last` or all error IDs return `-ENOENT`, the for loop completes without setting `ret`.

**skb leak in `doit_reply_value()`:**

> +	msg = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	hdr = genlmsg_iput(msg, info);
> +	if (!hdr) {
> +		nlmsg_free(msg);
> +		return -EMSGSIZE;
> +	}
> +
> +	ret = get_node_error_counter(node_id, error_id,
> +				     &error_name, &value);
> +	if (ret)
> +		return ret;

When `get_node_error_counter` fails, the function returns without freeing `msg`. This leaks the skb allocated by `genlmsg_new`. It should do `nlmsg_free(msg)` before returning.

**Self-include in auto-generated header:**

> +#include <uapi/drm/drm_ras.h>
> +#include <drm/drm_ras_nl.h>

`drm_ras_nl.h` includes itself. The header guard prevents infinite recursion so it's harmless, but it's clearly wrong in the auto-generated output. Might indicate a YAML configuration issue (the `kernel-family.headers` includes the generated header itself).

**`netnsok: true` in the genl family:**

> +	.netnsok	= true,

Hardware RAS data is global to the system, not per-network-namespace. Is there a reason to make this namespace-aware? If userspace in a non-init netns can query hardware error counters, that might be undesirable from a security/isolation perspective. Consider whether this should be `false`.

---
Generated by Claude Code Patch Reviewer