linux-kernel - Re: [PATCH v4 17/66] mmap: Change zeroing of maple tree in __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: <7bd61a52-57ef-04e4-6298-039308bb8f86@suse.cz>
Date:   Wed, 12 Jan 2022 15:55:59 +0100
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Liam Howlett <liam.howlett@...cle.com>,
        "maple-tree@...ts.infradead.org" <maple-tree@...ts.infradead.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     Song Liu <songliubraving@...com>,
        Davidlohr Bueso <dave@...olabs.net>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Matthew Wilcox <willy@...radead.org>,
        Laurent Dufour <ldufour@...ux.ibm.com>,
        David Rientjes <rientjes@...gle.com>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Rik van Riel <riel@...riel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Michel Lespinasse <walken.cr@...il.com>,
        Jerome Glisse <jglisse@...hat.com>,
        Minchan Kim <minchan@...gle.com>,
        Joel Fernandes <joelaf@...gle.com>,
        Rom Lemarchand <romlem@...gle.com>
Subject: Re: [PATCH v4 17/66] mmap: Change zeroing of maple tree in
 __vma_adjust

On 12/1/21 15:29, Liam Howlett wrote:
> Only write to the maple tree if we are not inserting or the insert isn't
> going to overwrite the area to clear.  This avoids spanning writes and
> node coealescing when unnecessary.
> 
> Signed-off-by: Liam R. Howlett <Liam.Howlett@...cle.com>
> ---
>  mm/mmap.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 93ed19b2c6ce..c5f92666d145 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -615,6 +615,7 @@ int __vma_adjust(struct vm_area_struct *vma, unsigned long start,
>  	bool vma_changed = false;
>  	long adjust_next = 0;
>  	int remove_next = 0;
> +	unsigned long old_start;
>  
>  	if (next && !insert) {
>  		struct vm_area_struct *exporter = NULL, *importer = NULL;
> @@ -740,25 +741,29 @@ int __vma_adjust(struct vm_area_struct *vma, unsigned long start,
>  			vma_interval_tree_remove(next, root);
>  	}
>  
> +	old_start = vma->vm_start;
>  	if (start != vma->vm_start) {
> -		if (vma->vm_start < start)
> -			vma_mt_szero(mm, vma->vm_start, start);
> -		else
> -			vma_changed = true;
> +		vma_changed = true;

This says vma_changed = true even if vma is shrinking, so below we might do
an unnecessary vma_store(), no?

>  		vma->vm_start = start;
>  	}
>  	if (end != vma->vm_end) {
> -		if (vma->vm_end > end)
> -			vma_mt_szero(mm, end, vma->vm_end);
> -		else
> +		if (vma->vm_end > end) {

In contrast to the above, here vma_changed is only set when expanding
('vma_expand' would be a more descriptive name maybe?). So why are the two
cases handled differently, am I missing something?

> +			if (!insert || (insert && (insert->vm_start != end)))

Note: thanks to lazy evaluation, "insert &&" should be unnecessary?
More importantly: is "insert->vm_start == end" a guarantee that insert
covers the whole interval from end to vma->vm_end? Probably yes, but a
VM_WARN_ON would be in order?

> +				vma_mt_szero(mm, end, vma->vm_end);

I guess it can't happen that insert would cover a later part of this
interval, so we could zero only between end vna insert->vm_start?

> +		} else
>  			vma_changed = true;

Same nit about { } block as previously.

>  		vma->vm_end = end;
>  		if (!next)
>  			mm->highest_vm_end = vm_end_gap(vma);
>  	}
>  
> -	if (vma_changed)
> +	if (vma_changed) {
>  		vma_store(mm, vma);
> +		if (old_start < start) {
> +			if (insert && (insert->vm_start != old_start))
> +				vma_mt_szero(mm, old_start, start);

This condition looks actively wrong, no zeroing at all if insert is NULL?

> +		}
> +	}
>  
>  	vma->vm_pgoff = pgoff;
>  	if (adjust_next) {