Re: [PATCH 1/7] Revert "drm/amdgpu: mark A+A VF VRAM mappings as system memory"

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] drm/amdgpu: revert commit message cites unmerged SHA and lacks
  Fixes tag
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] drm/amdgpu: use-after-free on
  amdgpu_ualink_peer_remote_init() error [1]
--

commit 6fcaa0941d3baaab33a7b2b5dd02cf2d392bc896
Author: Zhigang Luo <Zhigang.Luo@amd.com>

> Revert "drm/amdgpu: mark A+A VF VRAM mappings as system memory"
>
> This reverts commit 521bedfc94cc.
>
> The patch sets AMDGPU_PTE_SYSTEM in amdgpu_ttm_tt_pde_flags() and
> converts the VRAM address with amdgpu_gmc_vram_mc2pa() inside
> amdgpu_gmc_get_pde_for_bo(). gmc_v12_1_get_vm_pde() already converts
> an MC address to a physical address and skips that conversion when
> the SYSTEM bit is set, so setting the bit first bypasses the
> translation.
>
> The bit belongs in the GFX 12.1 helpers, after that translation.
> PDB0 is written directly and does not go through get_vm_pde(), and
> NPA still requires PTE.S=0, so those two pieces come back in the
> following patch.
>
> Signed-off-by: Zhigang Luo <zhigang.luo@amd.com>

[Severity: Low]
In the merged tree, the reverted change was committed as 010e510347f5
("drm/amdgpu: mark A+A VF VRAM mappings as system memory") rather than the
pre-signoff commit cited above.  Since this revert fixes broken PDE address
translation, should the commit message reference 010e510347f5 ("drm/amdgpu:
mark A+A VF VRAM mappings as system memory") and include:

  Fixes: 010e510347f5 ("drm/amdgpu: mark A+A VF VRAM mappings as system memory")

[ ... ]

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
> index d025b4183b296..9655758fa11ad 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c
> @@ -6173,7 +6173,6 @@ static int amdgpu_ualink_peer_remote_init(struct amdgpu_device *adev)
>  	flags = amdgpu_ttm_tt_pte_flags(adev, bo->tbo.ttm, bo->tbo.resource);
>  	flags |= AMDGPU_PTE_SNOOPED | AMDGPU_PTE_PRT_GFX12 | AMDGPU_PTE_BUS_ATOMICS;
>  	flags &= ~AMDGPU_PTE_VALID;
> -	flags &= ~AMDGPU_PTE_SYSTEM;
>  	/* PTE.X=0 turn off RPC checks for RBs, wptr and doorbell NPA address */
>  	flags &= ~AMDGPU_PTE_EXECUTABLE;

[Severity: Critical]
This problem wasn't introduced by this patch, but can a failure in
amdgpu_ualink_peer_remote_init() cause a use-after-free on
adev->ualink.remote [1]?

For each active remote peer, amdgpu_ualink_peer_remote_init() calls
amdgpu_ualink_gart_map() several times, inserting the embedded drm_mm_node
fields (mm_node_rb, mm_node_wptr and mm_node_doorbell) into
adev->mman.gtt_mgr.mm.  If a later amdgpu_ualink_gart_map() call fails,
amdgpu_ualink_peer_remote_init() breaks out and returns the error without
unmapping the earlier nodes:

drivers/gpu/drm/amd/amdgpu/amdgpu_ualink.c:amdgpu_ualink_peer_remote_init() {
	...
	for_each_set_bit(dst_accel_id, remote->active_accel_bits, AMDGPU_UALINK_ACCEL_MAX) {
		...
		r = amdgpu_ualink_gart_map(adev, rb_pages, npa, &interrupt->mm_node_rb, flags);
		if (r)
			break;
		...
		r = amdgpu_ualink_gart_map(adev, rb_pages, npa, &shootdown->mm_node_rb, flags);
		if (r)
			break;
	...
}

amdgpu_ualink_sw_init() then jumps to out_sdma_entities_fini and frees
adev->ualink.remote with kfree(), leaving the freed drm_mm_node objects
linked in adev->mman.gtt_mgr.mm and corrupting the GTT allocator on later
GTT operations or driver teardown.

[1] https://sashiko.dev/#/bug/linux-104150f2-9bfd-48ac-8dd1-d6f510b9e3c1

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113145702.323968-1-Zhigang.Luo@amd.com?part=1