linux-kernel - Re: [PATCH 1/5] mm/vmscan: put the redirtied MADV

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: <ff349ed2-14b5-8583-0c0c-725990d965e0@huawei.com>
Date:   Tue, 13 Jul 2021 21:16:50 +0800
From:   Miaohe Lin <linmiaohe@...wei.com>
To:     Yu Zhao <yuzhao@...gle.com>
CC:     Andrew Morton <akpm@...ux-foundation.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Michal Hocko <mhocko@...e.com>, Jens Axboe <axboe@...nel.dk>,
        Joonsoo Kim <iamjoonsoo.kim@....com>,
        Alex Shi <alexs@...nel.org>, <apopple@...dia.com>,
        Matthew Wilcox <willy@...radead.org>,
        Minchan Kim <minchan@...nel.org>,
        David Hildenbrand <david@...hat.com>, <shli@...com>,
        <hillf.zj@...baba-inc.com>, Linux-MM <linux-mm@...ck.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/5] mm/vmscan: put the redirtied MADV_FREE pages back to
 anonymous LRU list

On 2021/7/13 15:25, Yu Zhao wrote:
> On Mon, Jul 12, 2021 at 1:12 AM Miaohe Lin <linmiaohe@...wei.com> wrote:
>>
>> On 2021/7/11 7:22, Yu Zhao wrote:
>>> On Sat, Jul 10, 2021 at 4:03 AM Miaohe Lin <linmiaohe@...wei.com> wrote:
>>>>
>>>> If the MADV_FREE pages are redirtied before they could be reclaimed, put
>>>> the pages back to anonymous LRU list by setting SwapBacked flag and the
>>>> pages will be reclaimed in normal swapout way. Otherwise MADV_FREE pages
>>>> won't be reclaimed as expected.
>>>>
>>>> Fixes: 802a3a92ad7a ("mm: reclaim MADV_FREE pages")
>>>
>>> This is not a bug -- the dirty check isn't needed but it was copied
>>> from __remove_mapping().
>>
>> Yes, this is not a bug and harmless. When we reach here, page should not be
>> dirtied because PageDirty is handled above and there is no way to redirty it
>> again as pagetable references are all gone and it's not in the swap cache.
>>
>>>
>>> The page has only one reference left, which is from the isolation.
>>> After the caller puts the page back on lru and drops the reference,
>>> the page will be freed anyway. It doesn't matter which lru it goes.
>>
>> But it looks buggy as it didn't perform the expected ops from code view.
>> Should I drop the Fixes tag and send a v2 version?
> 
> I don't understand the logic here -- it looks pretty obvious to me
> that, if we want to change anything, we should delete the dirty check,
> not add another line that would enforce the belief that the dirty
> check is needed.
> 

The dirty check could be removed even with the page_ref_freeze check because no one can grab
the page refcnt after the page is successfully unmapped.

Does the change below makes sense for you?

Many Thanks.

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 6e26b3c93242..c31925320b33 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1624,15 +1624,11 @@ static unsigned int shrink_page_list(struct list_head *page_list,
                }

                if (PageAnon(page) && !PageSwapBacked(page)) {
-                       /* follow __remove_mapping for reference */
-                       if (!page_ref_freeze(page, 1))
-                               goto keep_locked;
-                       if (PageDirty(page)) {
-                               SetPageSwapBacked(page);
-                               page_ref_unfreeze(page, 1);
-                               goto keep_locked;
-                       }
-
+                       /*
+                        * No one can grab the page refcnt or redirty the page
+                        * after the page is successfully unmapped.
+                        */
+                       WARN_ON_ONCE(!page_ref_freeze(page, 1));
                        count_vm_event(PGLAZYFREED);
                        count_memcg_page_event(page, PGLAZYFREED);
                } else if (!mapping || !__remove_mapping(mapping, page, true,

>>
>> Many thanks for reply!
>>
>>>
>>>> Signed-off-by: Miaohe Lin <linmiaohe@...wei.com>
>>>> ---
>>>>  mm/vmscan.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>>> index a7602f71ec04..6483fe0e2065 100644
>>>> --- a/mm/vmscan.c
>>>> +++ b/mm/vmscan.c
>>>> @@ -1628,6 +1628,7 @@ static unsigned int shrink_page_list(struct list_head *page_list,
>>>>                         if (!page_ref_freeze(page, 1))
>>>>                                 goto keep_locked;
>>>>                         if (PageDirty(page)) {
>>>> +                               SetPageSwapBacked(page);
>>>>                                 page_ref_unfreeze(page, 1);
>>>>                                 goto keep_locked;
>>>>                         }
>>>> --
>>>> 2.23.0
>>>>
>>>>
>>> .
>>>
>>
> .
>