Re: [PATCH v5 1/5] drm/ras: Cancel and free message on get counter failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing RCU read lock or refcounting causes a Use-After-Free of `drm_ras_node` in netlink callbacks.
--

--- Patch [1]: [PATCH v5 1/5] drm/ras: Cancel and free message on get counter failure ---
commit a7d45a29dc348f6234c9ec9f5c9ccdf5d73188b7
Author: Raag Jadav <raag.jadav@intel.com>

drm/ras: Cancel and free message on get counter failure

doit_reply_value() directly returns on get counter failure, which results
in stale sk_buff and genetlink header that aren't cleaned up. Fix it and
while at it, consolidate error handling using goto.

> diff --git a/drivers/gpu/drm/drm_ras.c b/drivers/gpu/drm/drm_ras.c
> index d6eab29a1394c..467a169026fc1 100644
> --- a/drivers/gpu/drm/drm_ras.c
> +++ b/drivers/gpu/drm/drm_ras.c
> @@ -201,25 +201,28 @@ static int doit_reply_value(struct genl_info *info, u32 node_id,
>  
>  	hdr = genlmsg_iput(msg, info);
>  	if (!hdr) {
> -		nlmsg_free(msg);
> -		return -EMSGSIZE;
> +		ret = -EMSGSIZE;
> +		goto free_msg;
>  	}
>  
>  	ret = get_node_error_counter(node_id, error_id,
>  				     &error_name, &value);

[Severity: High]
This is a pre-existing issue, but is there a missing RCU read lock around
these node accesses?

Since drm_ras_nl_family uses parallel_ops = true, callbacks execute
concurrently without the global genl_lock(). Looking at
get_node_error_counter(), it retrieves the node using xa_load() and then
dereferences it:

get_node_error_counter() {
    node = xa_load(&drm_ras_xa, node_id);
    if (!node || !node->query_error_counter)
        return -ENOENT;
    ...
    return node->query_error_counter(node, error_id, name, value);
}

Because xa_load() only holds the RCU read lock internally and drops it
before returning, the node pointer is unprotected here. If a driver
concurrently unregisters and frees the node memory, could this lead to a
use-after-free when the callback dereferences node->query_error_counter?

A similar issue appears to exist in drm_ras_nl_list_nodes_dumpit() where
xa_for_each_start() is used without an encompassing rcu_read_lock().

>  	if (ret)
> -		return ret;
> +		goto cancel_msg;
>  
>  	ret = msg_reply_value(msg, error_id, error_name, value);
> -	if (ret) {
> -		genlmsg_cancel(msg, hdr);
> -		nlmsg_free(msg);
> -		return ret;
> -	}
> +	if (ret)
> +		goto cancel_msg;
>  
>  	genlmsg_end(msg, hdr);
>  
>  	return genlmsg_reply(msg, info);
> +
> +cancel_msg:
> +	genlmsg_cancel(msg, hdr);
> +free_msg:
> +	nlmsg_free(msg);
> +	return ret;
>  }
>  
>  /**

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