linux-kernel - Re: [PATCH] drm/virtio: Support partial maps of GEM objects

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1a4386bf-a707-45fb-a699-ee3847a98aec@collabora.com>
Date: Sat, 25 Jan 2025 00:22:21 +0300
From: Dmitry Osipenko <dmitry.osipenko@...labora.com>
To: Sasha Finkelstein <fnkl.kernel@...il.com>
Cc: David Airlie <airlied@...hat.com>, Gerd Hoffmann <kraxel@...hat.com>,
 Gurchetan Singh <gurchetansingh@...omium.org>, Chia-I Wu
 <olvaffe@...il.com>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 Simona Vetter <simona@...ll.ch>, dri-devel@...ts.freedesktop.org,
 virtualization@...ts.linux.dev, linux-kernel@...r.kernel.org,
 asahi@...ts.linux.dev
Subject: Re: [PATCH] drm/virtio: Support partial maps of GEM objects

On 1/19/25 23:23, Dmitry Osipenko wrote:
> On 1/19/25 23:02, Dmitry Osipenko wrote:
>> On 1/19/25 19:18, Sasha Finkelstein wrote:
>>> On Sun, 19 Jan 2025 at 12:50, Dmitry Osipenko
>>> <dmitry.osipenko@...labora.com> wrote:
>>>>>       ret = io_remap_pfn_range(vma, vma->vm_start,
>>>>> -                              vram->vram_node.start >> PAGE_SHIFT,
>>>>> +                              (vram->vram_node.start >> PAGE_SHIFT) + vma->vm_pgoff,
>>>>>                                vm_size, vma->vm_page_prot);
>>>>>       return ret;
>>>>>  }
>>>>
>>>> The vma->vm_pgoff is fake in DRM, it's used for looking up DRM GEM
>>>> object based on the vma->vm_pgoff value when mmap is invoked.
>>>
>>> If my understanding is correct, vm_pgoff gets "unfaked" by
>>> https://elixir.bootlin.com/linux/v6.12.6/source/drivers/gpu/drm/virtio/virtgpu_vram.c#L48
>>>
>>>> vma->vm_pgoff should be treated as zero here. Hence we can map a part of
>>>> GEM, but only from its start. See drm_gem_mmap().
>>>
>>> I've had a "v0" (not on ml) of this patch that always treated vma->vm_pgoff as
>>> zero. This broke when anything tried to mmap with a non-zero offset. Adding
>>> vm_pgoff made it work correctly.
>>
>> I've tested this patch. Partial mapping with a non-zero offset doesn't
>> work because drm_gem_mmap() rejects it. I'd want to see your sample code
>> that performs mmaping, maybe I'm missing something.
>>
>>>> Please correct vma->vm_pgoff in v2.
>>>
>>> I need apps to be able to mmap with a non-zero offset for my usecase.
>>> While the correct value may be something else other than what is in
>>> the current patch, 0 is definitely incorrect for at least some workloads.
>>
>> drm_gem_mmap() uses drm_vma_offset_exact_lookup_locked() that doesn't
>> allow vma->vm_pgoff != node.start. AFAICT, no one driver supports
>> mapping with a non-zero offset, perhaps for a reason that I don't know
>> about.
> 
> See now that a non-zero mapping of a dmabuf might work. Will test it.

Works for dmabuf

> 
> -	/* Partial mappings of GEM buffers don't happen much in practice. */
> -	if (vm_size != vram->vram_node.size)
> +	if (vm_size > vram->vram_node.size)
>  		return -EINVAL;

This check should include the vm_pgoff, like that:

if (check_add_overflow(vma->vm_pgoff << PAGE_SHIFT, vm_size, &vm_end))
	return -EINVAL;

if (vm_end > vram->vram_node.size)
	return -EINVAL;

The size and offset are actually validated before this code is reached,
but doesn't hurt to keep the check around.

I corrected the check and applied patch to misc-next, thanks!

-- 
Best regards,
Dmitry