linux-kernel - Re: [PATCH] mm: Ignore VM_SOFTDIRTY on VMA merging, v2

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: <52E15A53.5020007@parallels.com>
Date:	Thu, 23 Jan 2014 22:07:15 +0400
From:	Pavel Emelyanov <xemul@...allels.com>
To:	Cyrill Gorcunov <gorcunov@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>
CC:	Mel Gorman <mgorman@...e.de>, <gnome@...t.net>,
	<grawoc@...krefraction.com>, <alan@...rguk.ukuu.org.uk>,
	<linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
	<bugzilla-daemon@...zilla.kernel.org>
Subject: Re: [PATCH] mm: Ignore VM_SOFTDIRTY on VMA merging, v2

On 01/23/2014 07:14 PM, Cyrill Gorcunov wrote:

> I think setting up dirty bit inside vma_merge() body is a big hammer
> which should not be used, but it's up to caller of vma_merge() to figure
> out if dirty bit should be set or not if merge successed. Thus softdirty
> vma bit should be (and it already is) set at the end of mmap_region and do_brk
> routines. So patch could be simplified (below). Pavel, what do you think?

Looks correct, thank you!

Acked-by: Pavel Emelyanov <xemul@...allels,com>

> ---
> From: Cyrill Gorcunov <gorcunov@...il.com>
> Subject: [PATCH] mm: Ignore VM_SOFTDIRTY on VMA merging, v2
> 
> VM_SOFTDIRTY bit affects vma merge routine: if two VMAs has all
> bits in vm_flags matched except dirty bit the kernel can't longer
> merge them and this forces the kernel to generate new VMAs instead.
> 
> It finally may lead to the situation when userspace application
> reaches vm.max_map_count limit and get crashed in worse case
> 
>  | (gimp:11768): GLib-ERROR **: gmem.c:110: failed to allocate 4096 bytes
>  |
>  | (file-tiff-load:12038): LibGimpBase-WARNING **: file-tiff-load: gimp_wire_read(): error
>  | xinit: connection to X server lost
>  |
>  | waiting for X server to shut down
>  | /usr/lib64/gimp/2.0/plug-ins/file-tiff-load terminated: Hangup
>  | /usr/lib64/gimp/2.0/plug-ins/script-fu terminated: Hangup
>  | /usr/lib64/gimp/2.0/plug-ins/script-fu terminated: Hangup
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=67651
> https://bugzilla.gnome.org/show_bug.cgi?id=719619#c0
> 
> Initial problem came from missed VM_SOFTDIRTY in do_brk() routine
> but even if we would set up VM_SOFTDIRTY here, there is still a way to
> prevent VMAs from merging: one can call
> 
>  | echo 4 > /proc/$PID/clear_refs
> 
> and clear all VM_SOFTDIRTY over all VMAs presented in memory map,
> then new do_brk() will try to extend old VMA and finds that dirty
> bit doesn't match thus new VMA will be generated.
> 
> As discussed to Pavel, the right approach should be to ignore
> VM_SOFTDIRTY bit when we're trying to merge VMAs and if merge
> successed we mark extended VMA with dirty bit where needed.
> 
> v2: Don't mark VMA as dirty inside vma_merge() body, it's up
>     to calling code to set up dirty bit where needed.
> 
> Reported-by: Mel Gorman <mgorman@...e.de>
> Signed-off-by: Cyrill Gorcunov <gorcunov@...nvz.org>
> CC: Pavel Emelyanov <xemul@...allels.com>
> CC: Andrew Morton <akpm@...ux-foundation.org>
> ---
>  mm/mmap.c |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6.git/mm/mmap.c
> ===================================================================
> --- linux-2.6.git.orig/mm/mmap.c
> +++ linux-2.6.git/mm/mmap.c
> @@ -893,7 +893,15 @@ again:			remove_next = 1 + (end > next->
>  static inline int is_mergeable_vma(struct vm_area_struct *vma,
>  			struct file *file, unsigned long vm_flags)
>  {
> -	if (vma->vm_flags ^ vm_flags)
> +	/*
> +	 * VM_SOFTDIRTY should not prevent from VMA merging, if we
> +	 * match the flags but dirty bit -- the caller should mark
> +	 * merged VMA as dirty. If dirty bit won't be excluded from
> +	 * comparison, we increase pressue on the memory system forcing
> +	 * the kernel to generate new VMAs when old one could be
> +	 * extended instead.
> +	 */
> +	if ((vma->vm_flags ^ vm_flags) & ~VM_SOFTDIRTY)
>  		return 0;
>  	if (vma->vm_file != file)
>  		return 0;
> @@ -1082,7 +1090,7 @@ static int anon_vma_compatible(struct vm
>  	return a->vm_end == b->vm_start &&
>  		mpol_equal(vma_policy(a), vma_policy(b)) &&
>  		a->vm_file == b->vm_file &&
> -		!((a->vm_flags ^ b->vm_flags) & ~(VM_READ|VM_WRITE|VM_EXEC)) &&
> +		!((a->vm_flags ^ b->vm_flags) & ~(VM_READ|VM_WRITE|VM_EXEC|VM_SOFTDIRTY)) &&
>  		b->vm_pgoff == a->vm_pgoff + ((b->vm_start - a->vm_start) >> PAGE_SHIFT);
>  }
>  
> .
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/