linux-kernel - Re: [PATCH 10/10] mm: rework vm_ops->close() handling on VMA merge

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: <20240806162149.5d1de9e1@mordecai.tesarici.cz>
Date: Tue, 6 Aug 2024 16:21:49 +0200
From: Petr Tesařík <petr@...arici.cz>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org, Andrew Morton
 <akpm@...ux-foundation.org>, "Liam R . Howlett" <Liam.Howlett@...cle.com>,
 Vlastimil Babka <vbabka@...e.cz>
Subject: Re: [PATCH 10/10] mm: rework vm_ops->close() handling on VMA merge

On Tue, 6 Aug 2024 15:08:33 +0100
Lorenzo Stoakes <lorenzo.stoakes@...cle.com> wrote:

> On Tue, Aug 06, 2024 at 03:55:55PM GMT, Petr Tesařík wrote:
> > On Mon,  5 Aug 2024 13:13:57 +0100
> > Lorenzo Stoakes <lorenzo.stoakes@...cle.com> wrote:
>[...]
> > > @@ -886,6 +894,8 @@ struct vm_area_struct *vma_merge_new_vma(struct vma_merge_struct *vmg)
> > >  	unsigned long end = vmg->end;
> > >  	pgoff_t pgoff = vmg->pgoff;
> > >  	pgoff_t pglen = PHYS_PFN(end - start);
> > > +	bool merge_next = false;
> > > +	struct anon_vma *anon_vma = vmg->anon_vma;  
> >
> > Calling this "anon_vma" feels a bit too generic. IIUC you want to save
> > the original vmg->anon_vma in case the VMA turns out to be ummergeable
> > with the next VMA after vmg->anon_vma has already been modified.
> >
> > What about calling it "orig_anon_vma"?  
> 
> I disagree, that'd be unnecessary noise (and this is applicable to _all_
> the fields).

I'm afraid I don't understand what you mean with _all_ fields. FWIW my
comment concerns a local variable called "anon_vma", not a struct
member called "anon_vma".

> 
> Again we come to some trade-off between readability and inherent
> complexity. I am not a fan of making variable names unnecessarily
> overwrought.

Then call it "a". ;-)

See additional comments below:

> 
> In this case it's just a short-hand, as the only instance where we'd retry
> the operation anon_vma would be NULL (from mmap_region()), so we reset that
> to NULL, however strictly we should reset to anon_vma.
> 
> I'll change that on the next respin just to be strict.
> 
> >
> > Petr T
> >  
> > >
> > >  	VM_WARN_ON(vmg->vma);
> > >
> > > @@ -916,8 +926,9 @@ struct vm_area_struct *vma_merge_new_vma(struct vma_merge_struct *vmg)
> > >  		vmg->end = next->vm_end;
> > >  		vmg->vma = next;
> > >  		vmg->pgoff = next->vm_pgoff - pglen;
> > > -
> > >  		vmg->anon_vma = next->anon_vma;

Here, vmg->anon_vma is modified. Original value is lost.

> > > +
> > > +		merge_next = true;
> > >  	}
> > >
> > >  	/* If we can merge with the previous VMA, adjust vmg accordingly. */
> > > @@ -925,6 +936,16 @@ struct vm_area_struct *vma_merge_new_vma(struct vma_merge_struct *vmg)
> > >  		vmg->start = prev->vm_start;
> > >  		vmg->vma = prev;
> > >  		vmg->pgoff = prev->vm_pgoff;
> > > +
> > > +		/*
> > > +		 * If this merge would result in removal of the next VMA but we
> > > +		 * are not permitted to do so, reduce the operation to merging
> > > +		 * prev and vma.
> > > +		 */
> > > +		if (merge_next && !can_merge_remove_vma(next)) {
> > > +			vmg->end = end;
> > > +			vmg->anon_vma = anon_vma;

But here you need to restore the original value of vmg->anon_vma.

Isn't this why you introduced the local variable "anon_vma"? I believe
it would be easier to understand its purpose if it includes the "orig_"
prefix.

Just my two eurocents.

Petr T