linux-kernel - Re: [PATCH] staging: kpc2000: Convert put_page() to put_user

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: <82441723-f30e-5811-ab1c-dd9a4993d7df@nvidia.com>
Date:   Mon, 15 Jul 2019 15:01:43 -0700
From:   John Hubbard <jhubbard@...dia.com>
To:     Matt Sickler <Matt.Sickler@...tronics.com>,
        Bharath Vedartham <linux.bhar@...il.com>,
        "ira.weiny@...el.com" <ira.weiny@...el.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "jglisse@...hat.com" <jglisse@...hat.com>
CC:     "devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] staging: kpc2000: Convert put_page() to put_user_page*()

On 7/15/19 2:47 PM, Matt Sickler wrote:
> It looks like Outlook is going to absolutely trash this email.  Hopefully it comes through okay.
> 
...
>>
>> Because this is a common pattern, and because the code here doesn't likely
>> need to set page dirty before the dma_unmap_sg call, I think the following
>> would be better (it's untested), instead of the above diff hunk:
>>
>> diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c
>> b/drivers/staging/kpc2000/kpc_dma/fileops.c
>> index 48ca88bc6b0b..d486f9866449 100644
>> --- a/drivers/staging/kpc2000/kpc_dma/fileops.c
>> +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
>> @@ -211,16 +211,13 @@ void  transfer_complete_cb(struct aio_cb_data
>> *acd, size_t xfr_count, u32 flags)
>>        BUG_ON(acd->ldev == NULL);
>>        BUG_ON(acd->ldev->pldev == NULL);
>>
>> -       for (i = 0 ; i < acd->page_count ; i++) {
>> -               if (!PageReserved(acd->user_pages[i])) {
>> -                       set_page_dirty(acd->user_pages[i]);
>> -               }
>> -       }
>> -
>>        dma_unmap_sg(&acd->ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents, acd->ldev->dir);
>>
>>        for (i = 0 ; i < acd->page_count ; i++) {
>> -               put_page(acd->user_pages[i]);
>> +               if (!PageReserved(acd->user_pages[i])) {
>> +                       put_user_pages_dirty(&acd->user_pages[i], 1);
>> +               else
>> +                       put_user_page(acd->user_pages[i]);
>>        }
>>
>>        sg_free_table(&acd->sgt);
> 
> I don't think I ever really knew the right way to do this. 
> 
> The changes Bharath suggested look okay to me.  I'm not sure about the check for PageReserved(), though.  At first glance it appears to be equivalent to what was there before, but maybe I should learn what that Reserved page flag really means.
> From [1], the only comment that seems applicable is
> * - MMIO/DMA pages. Some architectures don't allow to ioremap pages that are
>  *   not marked PG_reserved (as they might be in use by somebody else who does
>  *   not respect the caching strategy).
> 
> These pages should be coming from anonymous (RAM, not file backed) memory in userspace.  Sometimes it comes from hugepage backed memory, though I don't think that makes a difference.  I should note that transfer_complete_cb handles both RAM to device and device to RAM DMAs, if that matters.
> 
> [1] https://elixir.bootlin.com/linux/v5.2/source/include/linux/page-flags.h#L17
> 

I agree: the PageReserved check looks unnecessary here, from my outside-the-kpc_2000-team
perspective, anyway. Assuming that your analysis above is correct, you could collapse that
whole think into just:

@@ -211,17 +209,8 @@ void  transfer_complete_cb(struct aio_cb_data *acd, size_t xfr_count, u32 flags)
        BUG_ON(acd->ldev == NULL);
        BUG_ON(acd->ldev->pldev == NULL);
 
-       for (i = 0 ; i < acd->page_count ; i++) {
-               if (!PageReserved(acd->user_pages[i])) {
-                       set_page_dirty(acd->user_pages[i]);
-               }
-       }
-
        dma_unmap_sg(&acd->ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents, acd->ldev->dir);
-
-       for (i = 0 ; i < acd->page_count ; i++) {
-               put_page(acd->user_pages[i]);
-       }
+       put_user_pages_dirty(&acd->user_pages[i], acd->page_count);
 
        sg_free_table(&acd->sgt);
 
(Also, Matt, I failed to Cc: you on a semi-related cleanup that I just sent out for this
driver, as long as I have your attention:

   https://lore.kernel.org/r/20190715212123.432-1-jhubbard@nvidia.com
)

thanks,
-- 
John Hubbard
NVIDIA