linux-kernel - Re: [PATCH 0/4] further cleanup of vma

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: <20230320164707.zpjhcwplkrp4tvgf@revolver>
Date:   Mon, 20 Mar 2023 12:47:07 -0400
From:   "Liam R. Howlett" <Liam.Howlett@...cle.com>
To:     Lorenzo Stoakes <lstoakes@...il.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        David Hildenbrand <david@...hat.com>,
        Matthew Wilcox <willy@...radead.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        maple-tree@...ts.infradead.org
Subject: Re: [PATCH 0/4] further cleanup of vma_merge()

* Lorenzo Stoakes <lstoakes@...il.com> [230318 07:15]:
> Following on from Vlastimil Babka's patch series "cleanup vma_merge() and
> improve mergeability tests" which was in turn based on Liam's prior
> cleanups, this patch series introduces changes discussed in review of
> Vlastimil's series and goes further in attempting to make the logic as
> clear as possible.
> 
> Nearly all of this should have absolutely no functional impact, however it
> does add a singular VM_WARN_ON() case.

Thanks for looking at this function and adding more clarity.  I'm happy
to have comments within the code, especially tricky areas.  But I find
that adding almost 50 lines to this function makes it rather hard to
follow.

Can we remove the more obvious comments and possibly reduce the nesting
of others so there are less lines?

For example in patch 2:
        /*
         * If there is a previous VMA specified, find the next, otherwise find                                          
         * the first.                                                                                                   
         */ 
        vma = find_vma(mm, prev ? prev->vm_end : 0);

Is rather verbose for something that can be seen in the code itself.

I think we are risking over-documenting what is going on here which is
making the code harder to read; the function is pushing 200 lines now.

> 
> Lorenzo Stoakes (4):
>   mm/mmap/vma_merge: further improve prev/next VMA naming
>   mm/mmap/vma_merge: set next to NULL if not applicable
>   mm/mmap/vma_merge: extend invariants, avoid invalid res, vma
>   mm/mmap/vma_merge: be explicit about the non-mergeable case
> 
>  mm/mmap.c | 165 +++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 107 insertions(+), 58 deletions(-)
> 
> --
> 2.39.2