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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The dirty tracking structure `vbo->dirty` can be freed concurrently by `vmw_bo_dirty_release()` without holding the buffer object's reservation lock, causing a use-after-free in `vmw_bo_dirty_mkwrite()`.
- [High] Inverted error handling in ttm_bo_vm_fault_reserved masks primary fault errors (causing infinite page fault loops) and erroneously surfaces speculative prefault errors.
--

--- Patch [3]: [PATCH RFT 3/3] drm/ttm, drm/vmwgfx: directly create writable PTEs when mkwrite is in use ---
commit 894d8cfbec765789074f38bcd9f120fdcf49cba5
Author: Paolo Bonzini <pbonzini@redhat.com>

drm/ttm, drm/vmwgfx: directly create writable PTEs when mkwrite is in use

This commit changes the fault handler to directly insert writable PTEs for the
first page when write access is requested. It achieves this by manually
recording the write fault instead of relying on the .pfn_mkwrite() callback.

> diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> index a80510489c456..ef27a2d7afc07 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> @@ -263,7 +263,8 @@ 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 = vmf_insert_pfn_prot(vma, address, pfn, prot);
> +		ret = __vmf_insert_pfn_prot(vma, address, pfn, prot,
> +					    i == 0 && !!(vmf->flags & FAULT_FLAG_WRITE));
>  
>  		/* Never error on prefaulted PTEs */
>  		if (unlikely((ret & VM_FAULT_ERROR))) {
>  			if (i == 0)
>  				return VM_FAULT_NOPAGE;
>  			else
>  				break;
>  		}

[Severity: High]
This is a pre-existing issue, but could this error handling in
ttm_bo_vm_fault_reserved() lead to infinite page fault loops or unwarranted
SIGBUS crashes?

If __vmf_insert_pfn_prot() returns an error like VM_FAULT_SIGBUS or
VM_FAULT_OOM, and it is the primary fault (i == 0), this block returns
VM_FAULT_NOPAGE (success). Does this cause the kernel to infinitely retry
the faulting instruction? 

Also, if it is a speculative prefault (i > 0), the loop breaks and returns
the prefault error, crashing the application even though the primary fault
succeeded. Is this the intended behavior?

[ ... ]

> 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
> @@ -398,15 +398,33 @@ void vmw_bo_dirty_clear_res(struct vmw_resource *res)
>  		dirty->end = res_start;
>  }
>  
> +static vm_fault_t vmw_bo_dirty_mkwrite(struct vm_fault *vmf, struct ttm_buffer_object *bo)
> +{
> +	unsigned long page_offset;
> +	struct vmw_bo *vbo = to_vmw_bo(&bo->base);
> +
> +	page_offset = vmf->pgoff - drm_vma_node_start(&bo->base.vma_node);
> +	if (unlikely(page_offset >= PFN_UP(bo->resource->size)))
> +		return VM_FAULT_SIGBUS;
> +
> +	if (vbo->dirty && vbo->dirty->method == VMW_BO_DIRTY_MKWRITE &&
> +	    !test_bit(page_offset, &vbo->dirty->bitmap[0])) {
> +		struct vmw_bo_dirty *dirty = vbo->dirty;
> +
> +		__set_bit(page_offset, &dirty->bitmap[0]);
> +		dirty->start = min(dirty->start, page_offset);
> +		dirty->end = max(dirty->end, page_offset + 1);
> +	}

[Severity: Critical]
This isn't a bug introduced by this patch, but is it possible for vbo->dirty
to be freed concurrently without holding the buffer object's reservation lock?

If user space destroys the framebuffer, vmw_bo_dirty_release() is called,
which modifies and frees vbo->dirty:

void vmw_bo_dirty_release(struct vmw_bo *vbo)
{
	struct vmw_bo_dirty *dirty = vbo->dirty;

	if (dirty && kref_put(&dirty->ref_count, vmw_bo_dirty_free))
		vbo->dirty = NULL;
}

Since vmw_bo_dirty_release() doesn't appear to take the resv lock, can a
concurrent write to a mapped buffer object (which triggers the page fault
handler and calls vmw_bo_dirty_mkwrite() under the resv lock) dereference
the freed vbo->dirty memory here, leading to a use-after-free?

> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731164341.1109827-1-pbonzini@redhat.com?part=3