Re: [PATCH v2 3/6] drm/ttm, drm/vmwgfx: directly create writable PTEs when mkwrite is in use

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

New issues:
- [High] Missing dirty page tracking in vmw_bo_vm_fault() when prefaulting =
fails, leading to silent data loss.

Pre-existing issues:
- [High] Inverted error handling logic in ttm_bo_vm_fault_reserved() hides =
primary fault errors and propagates prefault errors.
--

--- Patch [3]: [PATCH v2 3/6] drm/ttm, drm/vmwgfx: directly create writable=
 PTEs when mkwrite is in use ---
commit 2f0ecb57a3fc3ac93f0458269dd5ea847c608a25
Author: Paolo Bonzini <[email protected]>
Subject: drm/ttm, drm/vmwgfx: directly create writable PTEs when mkwrite is=
 in use

This patch modifies TTM BO fault handling to explicitly request writable
PTEs for the primary page when mkwrite is in use. It also updates vmwgfx
to manually record write faults since the .pfn_mkwrite() callback is no
longer invoked for the primary page.

> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo=
_vm.c
> index a80510489c456..3ebde936ce608 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c

[ ... ]

> @@ -242,6 +243,7 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *=
vmf,
>  	 * Speculatively prefault a number of pages. Only error on
>  	 * first page.
>  	 */
> +	mkwrite =3D !!(vmf->flags & FAULT_FLAG_WRITE);
>  	for (i =3D 0; i < num_prefault; ++i) {
>  		if (bo->resource->bus.is_iomem) {
>  			pfn =3D ttm_bo_io_mem_pfn(bo, page_offset);
> @@ -263,9 +265,10 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault =
*vmf,
>  		 * at arbitrary times while the data is mmap'ed.
>  		 * See vmf_insert_pfn_prot() for a discussion.
>  		 */
> -		ret =3D vmf_insert_pfn_prot(vma, address, pfn, prot);
> +		ret =3D vmf_insert_pfn_prot_mkwrite(vma, address, pfn, prot, mkwrite);
> =20
> -		/* Never error on prefaulted PTEs */
> +		/* Never error on prefaulted PTEs and never map them writable */
> +		mkwrite =3D false;
>  		if (unlikely((ret & VM_FAULT_ERROR))) {
>  			if (i =3D=3D 0)
>  				return VM_FAULT_NOPAGE;
>  			else
>  				break;
>  		}

[Severity: High]
This is a pre-existing issue, but does this error handling logic for
ttm_bo_vm_fault_reserved() behave backwards?

If vmf_insert_pfn_prot_mkwrite() fails for the primary page (i =3D=3D 0),
returning VM_FAULT_NOPAGE hides the error from the fault handler,
potentially causing an infinite page fault loop.

Conversely, if a speculative prefault (i > 0) fails, the loop breaks
and the error is propagated to the fault handler, which might cause
a spurious process crash even though the primary fault succeeded.


> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c b/drivers/gpu/drm=
/vmwgfx/vmwgfx_page_dirty.c
> index 45561bc1c9eff..3099558c0762f 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c

[ ... ]

> @@ -484,6 +487,9 @@ vm_fault_t vmw_bo_vm_fault(struct vm_fault *vmf)
>  		prot =3D vm_get_page_prot(vma->vm_flags);
> =20
>  	ret =3D ttm_bo_vm_fault_reserved(vmf, prot, num_prefault);
> +	if (ret =3D=3D VM_FAULT_NOPAGE && (vmf->flags & FAULT_FLAG_WRITE))
> +		WARN_ON_ONCE(vmw_bo_dirty_mkwrite(vmf, bo));
> +
>  	if (ret =3D=3D VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT=
))
>  		return ret;
> =20

[Severity: High]
If a speculative prefault fails inside ttm_bo_vm_fault_reserved(), will
this skip tracking the dirty page and lead to silent data loss?

Because the primary page is successfully mapped writable before the prefault
loop continues:

ttm_bo_vm_fault_reserved()
    ret =3D vmf_insert_pfn_prot_mkwrite(vma, address, pfn, prot, mkwrite);
    // ... prefault loops to next page and fails ...
    break; // returns the error

The error is returned to vmw_bo_vm_fault(), meaning ret is not
VM_FAULT_NOPAGE.

This check is skipped, leaving the primary page mapped writable without the
driver marking it dirty.

If the process survives the fault (e.g., by handling SIGBUS), could it
then modify data without the driver tracking it?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260804120529.1730=
[email protected]?part=3D3