Re: [PATCH] drm/amdgpu: Fix NPA-REVOKE racing an in-flight UALink import

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] amdgpu: duplicate in-flight NPA-REVOKE bypasses early exit causing NULL deref
- [Medium] amdgpu: in-flight NPA-REVOKE fails to complete npa_done causing spurious timeout
--

commit 707623cd9d98bbd57c7b50f6dc2ad0e819566c82
Author: David Carlier <devnexen@gmail.com>

drm/amdgpu: Fix NPA-REVOKE racing an in-flight UALink import

This patch modifies the NPA-REVOKE handler to cleanly mark in-flight UALink
imports for teardown rather than crashing by dereferencing a NULL dmabuf.
The importer thread now checks for this teardown state and unwinds safely
if the handle was revoked before the import completed.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
> @@ -3288,7 +3289,19 @@ static void amdgpu_ualink_process_npa_revoke_msg(struct amdgpu_device *adev,
>  		return;
>  	}
>  
> +	node_state = READ_ONCE(imp_xa_node->node_state);
>  	WRITE_ONCE(imp_xa_node->node_state, AMDGPU_UALINK_NODE_TEARDOWN);
> +
> +	/* The import is still in flight: the dmabuf may not exist yet and
> +	 * nothing has been handed to user-space. Leave the node to the
> +	 * importing thread, which sees the teardown state and unwinds.
> +	 */
> +	if (node_state == AMDGPU_UALINK_NODE_NOT_READY ||
> +	    node_state == AMDGPU_UALINK_NODE_PENDING) {
> +		xa_unlock(&adev->ualink.imp_xa);
> +		goto send_release;
> +	}

[Severity: High]
If a duplicate NPA-REVOKE message arrives for an in-flight import, the
node_state is already AMDGPU_UALINK_NODE_TEARDOWN. Does this bypass the
early exit check here since it only checks for NOT_READY or PENDING?

If it bypasses this check, it will fall through to the GPUVM mappings
invalidation and unconditionally dereference imp_xa_node->dmabuf->priv
(which is NULL at this stage), causing a kernel panic.

[Severity: Medium]
When an NPA-REVOKE message arrives for an in-flight import, it sets the
node state to TEARDOWN just above, but does this strand the importing thread?

If we do not call complete(&imp_xa_node->npa_done) here, and the NPA-RSP
subsequently arrives, the RSP handler skips calling complete() because the
state is no longer NOT_READY:

amdgpu_ualink_process_npa_rsp_msg() {
    if (READ_ONCE(imp_xa_node->node_state) == AMDGPU_UALINK_NODE_NOT_READY) {
        WRITE_ONCE(imp_xa_node->node_state, AMDGPU_UALINK_NODE_PENDING);
        complete(&imp_xa_node->npa_done);
    } else {
        ...

Will this cause the importing thread to wait until the timeout expires,
resulting in a spurious timeout and an unnecessary full connection reset
of the vPod?

> +
>  	list_del_init(&imp_xa_node->list);
>  	xa_unlock(&adev->ualink.imp_xa);

[ ... ]

> @@ -3299,6 +3312,7 @@ static void amdgpu_ualink_process_npa_revoke_msg(struct amdgpu_device *adev,
>  	/* Drop the refcount for the node */
>  	amdgpu_ualink_imp_xa_entry_put(imp_xa_node);

[Severity: High]
If a duplicate revoke bypassed the early exit check above, does it also
drop the reference count a second time here?

This could lead to a double-free when the importing thread eventually cleans
up the node.

> +send_release:
>  	r = amdgpu_ualink_send_npa_release_msg(adev, remote_acc_id, handle);
>  	if (r)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260926171625.288519-1-devnexen@gmail.com?part=1