linux-kernel - Re: [PATCH v4 38/66] coredump: Remove vma linked list walk

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, 19 Jan 2022 11:31:45 +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 38/66] coredump: Remove vma linked list walk

On 12/1/21 15:30, Liam Howlett wrote:
> From: "Matthew Wilcox (Oracle)" <willy@...radead.org>
> 
> Use the Maple Tree iterator instead.  This is too complicated for the
> VMA iterator to handle, so let's open-code it for now.  If this turns
> out to be a common pattern, we can migrate it to common code.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@...radead.org>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@...cle.com>
> ---
>  fs/coredump.c | 33 ++++++++++++---------------------
>  1 file changed, 12 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/coredump.c b/fs/coredump.c
> index a6b3c196cdef..59347e42048d 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -997,30 +997,20 @@ static unsigned long vma_dump_size(struct vm_area_struct *vma,
>  	return vma->vm_end - vma->vm_start;
>  }
>  
> -static struct vm_area_struct *first_vma(struct task_struct *tsk,
> -					struct vm_area_struct *gate_vma)
> -{
> -	struct vm_area_struct *ret = tsk->mm->mmap;
> -
> -	if (ret)
> -		return ret;
> -	return gate_vma;
> -}
> -
>  /*
>   * Helper function for iterating across a vma list.  It ensures that the caller
>   * will visit `gate_vma' prior to terminating the search.
>   */
> -static struct vm_area_struct *next_vma(struct vm_area_struct *this_vma,
> +static struct vm_area_struct *coredump_next_vma(struct ma_state *mas,
> +				       struct vm_area_struct *vma,
>  				       struct vm_area_struct *gate_vma)
>  {
> -	struct vm_area_struct *ret;
> -
> -	ret = this_vma->vm_next;
> -	if (ret)
> -		return ret;
> -	if (this_vma == gate_vma)
> +	if (vma == gate_vma)
>  		return NULL;
> +
> +	vma = mas_next(mas, ULONG_MAX);
> +	if (vma)
> +		return vma;

This looks suspicious. Before this patch if gate_vma was part of the linked
list, it was returned. Even more than once if it was not the last vma in the
list. After this patch, if it's part of the maple tree, it will not be
returned and the iteration will stop.

But I don't know what are the rules for gate_vma being part of linked
list/maple tree, thus whether this is a bug.

>  	return gate_vma;
>  }
>  
> @@ -1032,9 +1022,10 @@ int dump_vma_snapshot(struct coredump_params *cprm, int *vma_count,
>  		      struct core_vma_metadata **vma_meta,
>  		      size_t *vma_data_size_ptr)
>  {
> -	struct vm_area_struct *vma, *gate_vma;
> +	struct vm_area_struct *gate_vma, *vma = NULL;
>  	struct mm_struct *mm = current->mm;
> -	int i;
> +	MA_STATE(mas, &mm->mm_mt, 0, 0);
> +	int i = 0;
>  	size_t vma_data_size = 0;
>  
>  	/*
> @@ -1054,8 +1045,7 @@ int dump_vma_snapshot(struct coredump_params *cprm, int *vma_count,
>  		return -ENOMEM;
>  	}
>  
> -	for (i = 0, vma = first_vma(current, gate_vma); vma != NULL;
> -			vma = next_vma(vma, gate_vma), i++) {
> +	while ((vma = coredump_next_vma(&mas, vma, gate_vma)) != NULL) {
>  		struct core_vma_metadata *m = (*vma_meta) + i;
>  
>  		m->start = vma->vm_start;
> @@ -1064,6 +1054,7 @@ int dump_vma_snapshot(struct coredump_params *cprm, int *vma_count,
>  		m->dump_size = vma_dump_size(vma, cprm->mm_flags);
>  
>  		vma_data_size += m->dump_size;
> +		i++;
>  	}
>  
>  	mmap_write_unlock(mm);