linux-kernel - Re: [PATCH] fs/vboxsf: Replace kmap() with kmap_local

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <3baed0e0-db2c-906c-5256-1d83d59794e9@redhat.com>
Date:   Tue, 27 Jun 2023 20:10:23 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     Sumitra Sharma <sumitraartsy@...il.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Ira Weiny <ira.weiny@...el.com>,
        Fabio <fmdefrancesco@...il.com>, Deepak R Varma <drv@...lo.com>
Subject: Re: [PATCH] fs/vboxsf: Replace kmap() with kmap_local_{page, folio}()

Hi Matthew,

On 6/27/23 19:46, Matthew Wilcox wrote:
> On Tue, Jun 27, 2023 at 04:34:51PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 6/27/23 15:51, Sumitra Sharma wrote:
>>> kmap() has been deprecated in favor of the kmap_local_page() due to high
>>> cost, restricted mapping space, the overhead of a global lock for
>>> synchronization, and making the process sleep in the absence of free
>>> slots.
>>>
>>> kmap_local_{page, folio}() is faster than kmap() and offers thread-local
>>> and CPU-local mappings, can take pagefaults in a local kmap region and
>>> preserves preemption by saving the mappings of outgoing tasks and
>>> restoring those of the incoming one during a context switch.
>>>
>>> The difference between kmap_local_page() and kmap_local_folio() consist
>>> only in the first taking a pointer to a page and the second taking two
>>> arguments, a pointer to a folio and the byte offset within the folio which
>>> identifies the page.
>>>
>>> The mappings are kept thread local in the functions 'vboxsf_read_folio',
>>> 'vboxsf_writepage', 'vboxsf_write_end' in file.c
>>>
>>> Suggested-by: Ira Weiny <ira.weiny@...el.com>
>>> Signed-off-by: Sumitra Sharma <sumitraartsy@...il.com>
>>
>> Thanks, patch looks good to me:
> 
> It doesn't look great to me, tbh.  It's generally an antipattern to map
> the page/folio up at the top and then pass the virtual address down to
> the bottom.  Usually we want to work in terms of physical addresses
> as long as possible.  I see the vmmdev_hgcm_function_parameter can
> take physical addresses; does it work to simply use the phys_addr
> instead of the linear_addr?  I see this commentary:
> 
>        /** Deprecated Doesn't work, use PAGELIST. */
>         VMMDEV_HGCM_PARM_TYPE_PHYSADDR           = 3,
> 
> so, um, can we use
>         /** Physical addresses of locked pages for a buffer. */
>         VMMDEV_HGCM_PARM_TYPE_PAGELIST           = 10,
> 
> and convert vboxsf_read_folio() to pass the folio down to vboxsf_read()
> which converts it to a PAGELIST (however one does that)?


It has been a long time since I looked at this code in detail. I don't
think you can just use different types when making virtualbox hypervisor
calls and then expect the hypervisor to say sure that another way to
represent a memory buffer, I'll take that instead.

After I upstreamed vboxsf support VirtualBox upstream did do some
further optimizations to speed up vboxsf. So there may be something
there which allows passing a physical address to the hypervisor,
but I don't have the time to dive into this.

When I upstreamed this the idea was that VirtualBox upstream
would see the benefits of having the guest drivers upstream and would
help with upstream maintenance. But unfortunately this never materialized
and they are still doing their own out of tree thing even for
their guest drivers.

TL;DR: for now I believe that it is best to just keep the code as
is and do a straight forward folio conversion.

Regards,

Hans