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