linux-kernel - Re: [PATCH] mm: hugetlb: kill set_huge_swap_pte

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]

Date:   Mon, 27 Jun 2022 12:44:19 +0530
From:   Anshuman Khandual <anshuman.khandual@....com>
To:     Qi Zheng <zhengqi.arch@...edance.com>, mike.kravetz@...cle.com,
        songmuchun@...edance.com, akpm@...ux-foundation.org,
        catalin.marinas@....com, will@...nel.org
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org
Subject: Re: [PATCH] mm: hugetlb: kill set_huge_swap_pte_at()



On 6/27/22 12:25, Qi Zheng wrote:
> 
> 
> On 2022/6/27 14:18, Anshuman Khandual wrote:
>>
>>
>> On 6/26/22 20:27, Qi Zheng wrote:
>>> The commit e5251fd43007 ("mm/hugetlb: introduce set_huge_swap_pte_at()
>>> helper") add set_huge_swap_pte_at() to handle swap entries on
>>> architectures that support hugepages consisting of contiguous ptes.
>>> And currently the set_huge_swap_pte_at() is only overridden by arm64.
>>>
>>> The set_huge_swap_pte_at() provide a sz parameter to help determine
>>> the number of entries to be updated. But in fact, all hugetlb swap
>>> entries contain pfn information, so we can find the corresponding
>>> folio through the pfn recorded in the swap entry, then the folio_size()
>>> is the number of entries that need to be updated.
>>>
>>> And considering that users will easily cause bugs by ignoring the
>>> difference between set_huge_swap_pte_at() and set_huge_pte_at().
>>> Let's handle swap entries in set_huge_pte_at() and remove the
>>> set_huge_swap_pte_at(), then we can call set_huge_pte_at()
>>> anywhere, which simplifies our coding.
>>>
>>> Signed-off-by: Qi Zheng <zhengqi.arch@...edance.com>
>>> ---
>>>   arch/arm64/include/asm/hugetlb.h |  3 ---
>>>   arch/arm64/mm/hugetlbpage.c      | 34 ++++++++++++++++----------------
>>>   include/linux/hugetlb.h          | 13 ------------
>>>   mm/hugetlb.c                     |  8 +++-----
>>>   mm/rmap.c                        | 11 +++--------
>>>   5 files changed, 23 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/hugetlb.h b/arch/arm64/include/asm/hugetlb.h
>>> index 1fd2846dbefe..d20f5da2d76f 100644
>>> --- a/arch/arm64/include/asm/hugetlb.h
>>> +++ b/arch/arm64/include/asm/hugetlb.h
>>> @@ -46,9 +46,6 @@ extern void huge_pte_clear(struct mm_struct *mm, unsigned long addr,
>>>                  pte_t *ptep, unsigned long sz);
>>>   #define __HAVE_ARCH_HUGE_PTEP_GET
>>>   extern pte_t huge_ptep_get(pte_t *ptep);
>>> -extern void set_huge_swap_pte_at(struct mm_struct *mm, unsigned long addr,
>>> -                 pte_t *ptep, pte_t pte, unsigned long sz);
>>> -#define set_huge_swap_pte_at set_huge_swap_pte_at
>>>     void __init arm64_hugetlb_cma_reserve(void);
>>>   diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
>>> index c9e076683e5d..58b89b9d13e0 100644
>>> --- a/arch/arm64/mm/hugetlbpage.c
>>> +++ b/arch/arm64/mm/hugetlbpage.c
>>> @@ -238,6 +238,13 @@ static void clear_flush(struct mm_struct *mm,
>>>       flush_tlb_range(&vma, saddr, addr);
>>>   }
>>>   +static inline struct folio *hugetlb_swap_entry_to_folio(swp_entry_t entry)
>>> +{
>>> +    VM_BUG_ON(!is_migration_entry(entry) && !is_hwpoison_entry(entry));
>>> +
>>> +    return page_folio(pfn_to_page(swp_offset(entry)));
>>> +}
>>
>> Extracting this huge page size from swap entry is an additional operation which
>> will increase the over all cost for set_huge_swap_pte_at(). At present the size
> 
> Hmm, I think this cost is very small. And replacing
> set_huge_swap_pte_at() by transparently handling swap entries helps
> reduce possible bugs, which is worthwhile.

Possible bugs ? There are just six call sites for this function.
Although this proposed patch is functionally correct, I dont see
a valid enough reason to increase the overall cost in the path.