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 for Android: free password hash cracker in your pocket

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

Message-ID: <e055b3e4-22c3-c0e2-fb8b-81ba8b97382d@redhat.com>
Date:   Thu, 29 Jun 2023 07:35:37 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Sumitra Sharma <sumitraartsy@...il.com>,
        Matthew Wilcox <willy@...radead.org>
Cc:     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 Sumitra,

On 6/29/23 06:30, Sumitra Sharma wrote:
> On Tue, Jun 27, 2023 at 06:48:20PM +0100, Matthew Wilcox wrote:
>> On Tue, Jun 27, 2023 at 06:51:15AM -0700, Sumitra Sharma wrote:
>>> +++ b/fs/vboxsf/file.c
>>> @@ -234,7 +234,7 @@ static int vboxsf_read_folio(struct file *file, struct folio *folio)
>>>  	u8 *buf;
>>>  	int err;
>>>  
>>> -	buf = kmap(page);
>>> +	buf = kmap_local_folio(folio, off);
>>
>> Did you test this?  'off' is the offset in the _file_.  Whereas
>> kmap_local_folio() takes the offset within the _folio_.  They have
>> different types (loff_t vs size_t) to warn you that they're different
>> things.
>>
> 
> Hi Matthew,
> 
> When creating this patch, I read and searched about the loff_t vs size_t.
> By mistake, I implemented it in the wrong way.
> 
> Also, I did not test it and just compiled it. I apologise for doing so.
> 
> And for the other points you have put as feedback. I will take some time to understand
> it. And would like to work on the changes you suggest.

If you work further on this please make sure that you actually test your
changes. Submitting untested fs changes is a really bad idea. People don't
like it when their data gets corrupted.

Note vboxsf can be tested easily by setting up a VirtualBox guest and then
using the shared folder features. Note do *not* use the VirtualBox provided
guest utils they contain their own out of tree vboxsf implementation and
will use that.

To avoid this you could e.g. use Fedora inside the guest and install
the Fedora packaged vbox guest utils which does not replace the mainline
vboxsf.

Regards,

Hans