linux-kernel - Re: [PATCH v10 3/6] fs/proc/task_mmu: Implement IOCTL to get and/or the clear info about PTEs

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:   Wed, 22 Feb 2023 19:10:31 +0000
From:   Nadav Amit <namit@...are.com>
To:     Muhammad Usama Anjum <usama.anjum@...labora.com>
CC:     Andrew Morton <akpm@...ux-foundation.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Michał Mirosław <emmir@...gle.com>,
        Cyrill Gorcunov <gorcunov@...il.com>,
        Paul Gofman <pgofman@...eweavers.com>,
        Danylo Mocherniuk <mdanylo@...gle.com>,
        Shuah Khan <shuah@...nel.org>,
        Christian Brauner <brauner@...nel.org>,
        Peter Xu <peterx@...hat.com>, Yang Shi <shy828301@...il.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Yun Zhou <yun.zhou@...driver.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Alex Sierra <alex.sierra@....com>,
        Matthew Wilcox <willy@...radead.org>,
        Pasha Tatashin <pasha.tatashin@...een.com>,
        Mike Rapoport <rppt@...nel.org>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        "Gustavo A . R . Silva" <gustavoars@...nel.org>,
        Dan Williams <dan.j.williams@...el.com>,
        kernel list <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-mm <linux-mm@...ck.org>,
        linux-kselftest <linux-kselftest@...r.kernel.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        "kernel@...labora.com" <kernel@...labora.com>,
        David Hildenbrand <david@...hat.com>,
        Andrei Vagin <avagin@...il.com>
Subject: Re: [PATCH v10 3/6] fs/proc/task_mmu: Implement IOCTL to get and/or
 the clear info about PTEs



> On Feb 20, 2023, at 5:24 AM, Muhammad Usama Anjum <usama.anjum@...labora.com> wrote:
> 
>>> +static inline int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
>>> +                     unsigned long end, struct mm_walk *walk)
>>> +{
>>> +    struct pagemap_scan_private *p = walk->private;
>>> +    struct vm_area_struct *vma = walk->vma;
>>> +    unsigned long addr = end;
>>> +    spinlock_t *ptl;
>>> +    int ret = 0;
>>> +    pte_t *pte;
>>> +
>>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>> +    ptl = pmd_trans_huge_lock(pmd, vma);
>>> +    if (ptl) {
>>> +        bool pmd_wt;
>>> +
>>> +        pmd_wt = !is_pmd_uffd_wp(*pmd);
>>> +        /*
>>> +         * Break huge page into small pages if operation needs to be
>>> performed is
>>> +         * on a portion of the huge page.
>>> +         */
>>> +        if (pmd_wt && IS_WP_ENGAGE_OP(p) && (end - start < HPAGE_SIZE)) {
>>> +            spin_unlock(ptl);
>>> +            split_huge_pmd(vma, pmd, start);
>>> +            goto process_smaller_pages;
>> I think that such goto's are really confusing and should be avoided. And
>> using 'else' (could have easily prevented the need for goto). It is not the
>> best solution though, since I think it would have been better to invert the
>> conditions.
> Yeah, else can be used here. But then we'll have to add a tab to all the
> code after adding else. We have already so many tabs and very less space to
> right code. Not sure which is better.

goto’s are usually not the right solution. You can extract things into a different
function if you have to.

I’m not sure why IS_GET_OP(p) might be false and what’s the meaning of taking the
lock and dropping it in such a case. I think that the code can be simplified and
additional condition nesting can be avoided.

>>> --- a/include/uapi/linux/fs.h
>>> +++ b/include/uapi/linux/fs.h
>>> @@ -305,4 +305,54 @@ typedef int __bitwise __kernel_rwf_t;
>>>  #define RWF_SUPPORTED    (RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT |\
>>>               RWF_APPEND)
>>>  +/* Pagemap ioctl */
>>> +#define PAGEMAP_SCAN    _IOWR('f', 16, struct pagemap_scan_arg)
>>> +
>>> +/* Bits are set in the bitmap of the page_region and masks in
>>> pagemap_scan_args */
>>> +#define PAGE_IS_WRITTEN        (1 << 0)
>>> +#define PAGE_IS_FILE        (1 << 1)
>>> +#define PAGE_IS_PRESENT        (1 << 2)
>>> +#define PAGE_IS_SWAPPED        (1 << 3)
>> 
>> These names are way too generic and are likely to be misused for the wrong
>> purpose. The "_IS_" part seems confusing as well. So I think the naming
>> needs to be fixed and some new type (using typedef) or enum should be
>> introduced to hold these flags. I understand it is part of uapi and it is
>> less common there, but it is not unheard of and does make things clearer.
> Do you think PM_SCAN_PAGE_IS_* work here?

Can we lose the IS somehow?

> 
>> 
>> 
>>> +
>>> +/*
>>> + * struct page_region - Page region with bitmap flags
>>> + * @start:    Start of the region
>>> + * @len:    Length of the region
>>> + * bitmap:    Bits sets for the region
>>> + */
>>> +struct page_region {
>>> +    __u64 start;
>>> +    __u64 len;
>> 
>> I presume in bytes. Would be useful to mention.
> Length of region in pages.

Very unintuitive to me I must say. If the start is an address, I would expect
the len to be in bytes.