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: <5d04a880-ed06-49ff-888f-56052eb69319@lucifer.local>
Date: Fri, 9 Aug 2024 15:37:39 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>
Subject: Re: [PATCH 10/10] mm: rework vm_ops->close() handling on VMA merge

On Fri, Aug 09, 2024 at 04:25:53PM GMT, Vlastimil Babka wrote:
> On 8/5/24 14:13, Lorenzo Stoakes wrote:
> > In commit 714965ca8252 ("mm/mmap: start distinguishing if vma can be
> > removed in mergeability test") we relaxed the VMA merge rules for VMAs
> > possessing a vm_ops->close() hook, permitting this operation in instances
> > where we wouldn't delete the VMA as part of the merge operation.
> >
> > This was later corrected in commit fc0c8f9089c2 ("mm, mmap: fix vma_merge()
> > case 7 with vma_ops->close") to account for a subtle case that the previous
> > commit had not taken into account.
> >
> > In both instances, we first rely on is_mergeable_vma() to determine whether
> > we might be dealing with a VMA that might be removed, taking advantage of
> > the fact that a 'previous' VMA will never be deleted, only VMAs that follow
> > it.
> >
> > The second patch corrects the instance where a merge of the previous VMA
> > into a subsequent one did not correctly check whether the subsequent VMA
> > had a vm_ops->close() handler.
> >
> > Both changes prevent merge cases that are actually permissible (for
> > instance a merge of a VMA into a following VMA with a vm_ops->close(), but
> > with no previous VMA, which would result in the next VMA being extended,
> > not deleted).
> >
> > In addition, both changes fail to consider the case where a VMA that would
> > otherwise be merged with the previous and next VMA might have
> > vm_ops->close(), on the assumption that for this to be the case, all three
> > would have to have the same vma->vm_file to be mergeable and thus the same
> > vm_ops.
> >
> > And in addition both changes operate at 50,000 feet, trying to guess
> > whether a VMA will be deleted.
> >
> > As we have majorly refactored the VMA merge operation and de-duplicated
> > code to the point where we know precisely where deletions will occur, this
> > patch removes the aforementioned checks altogether and instead explicitly
> > checks whether a VMA will be deleted.
> >
> > In cases where a reduced merge is still possible (where we merge both
> > previous and next VMA but the next VMA has a vm_ops->close hook, meaning we
> > could just merge the previous and current VMA), we do so, otherwise the
> > merge is not permitted.
> >
> > We take advantage of our userland testing to assert that this functions
> > correctly - replacing the previous limited vm_ops->close() tests with tests
> > for every single case where we delete a VMA.
> >
> > We also update all testing for both new and modified VMAs to set
> > vma->vm_ops->close() in every single instance where this would not prevent
> > the merge, to assert that we never do so.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
>
> Amazing!
>
> Acked-by: Vlastimil Babka <vbabka@...e.cz>
>

Thanks! :)

> > @@ -710,9 +706,30 @@ static struct vm_area_struct *vma_merge_modified(struct vma_merge_struct *vmg)
> >
> >  	/* If we span the entire VMA, a merge implies it will be deleted. */
> >  	merge_will_delete_vma = left_side && right_side;
> > -	/* If we merge both VMAs, then next is also deleted. */
>
> Nit: This comment ...
>
> > +
> > +	/*
> > +	 * If we need to remove vma in its entirety but are unable to do so,
> > +	 * we have no sensible recourse but to abort the merge.
> > +	 */
> > +	if (merge_will_delete_vma && !can_merge_remove_vma(vma))
> > +		return NULL;
> > +
> > +	/*
> > +	 * If we merge both VMAs, then next is also deleted. This implies
> > +	 * merge_will_delete_vma also.
> > +	 */
>
> ... changed to this comment. Seems spurious, could have been like that
> before already? I don't see how the new "This implies" part became relevant
> now? We already tested merge_will_delete_vma above.

Will move to previous commit.

>
> >  	merge_will_delete_next = merge_both;
> >
> > +	/*
> > +	 * If we cannot delete next, then we can reduce the operation to merging
> > +	 * prev and vma (thereby deleting vma).
> > +	 */
> > +	if (merge_will_delete_next && !can_merge_remove_vma(next)) {
> > +		merge_will_delete_next = false;
> > +		merge_right = false;
> > +		merge_both = false;
> > +	}
> > +
> >  	/* No matter what happens, we will be adjusting vma. */
> >  	vma_start_write(vma);
> >
>