Re: [ 11/48] mm: correctly synchronize rss-counters at exit/exec

Re: [ 11/48] mm: correctly synchronize rss-counters at exit/exec

[Hugh Dickins]

On Sun, 1 Jul 2012, Ben Hutchings wrote:

> 3.2-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Konstantin Khlebnikov <khlebnikov@openvz.org>
> 
> commit 4fe7efdbdfb1c7e7a7f31decfd831c0f31d37091 upstream.
> 
> do_exit() and exec_mmap() call sync_mm_rss() before mm_release() does
> put_user(clear_child_tid) which can update task->rss_stat and thus make
> mm->rss_stat inconsistent.  This triggers the "BUG:" printk in check_mm().
> 
> Let's fix this bug in the safest way, and optimize/cleanup this later.
> 
> Reported-by: Markus Trippelsdorf <markus@trippelsdorf.de>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Hugh Dickins <hughd@google.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> [bwh: Backported to 3.2: sync_mm_rss() still takes a struct task_struct *]
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

If you or Konstantin or Oleg have done full diligence on this,
to ensure that it is really applicable to 3.2 (not just that
the patch applies without rejects), fair enough.

But I'd be cautious about it: it went through enough alternatives
and revisions that I wouldn't call it trivial; it's easy for me to
imagine that some of the affected paths were actually slightly different
in 3.2 days than they were in 3.4 days; and the disturbing warning that
these mods silence ("BUG: Bad rss-counter state ") did not exist before
3.4 - unless you've ported that too?

That's not to assert that we had no rss problem at all before 3.4,
but we've not heard of any trouble from it.  Caution tells me that
this patch might cause more trouble than it's worth.

Hugh

> ---
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -820,10 +820,10 @@
>  	/* Notify parent that we're no longer interested in the old VM */
>  	tsk = current;
>  	old_mm = current->mm;
> -	sync_mm_rss(tsk, old_mm);
>  	mm_release(tsk, old_mm);
>  
>  	if (old_mm) {
> +		sync_mm_rss(tsk, old_mm);
>  		/*
>  		 * Make sure that if there is a core dump in progress
>  		 * for the old mm, we get out and die instead of going
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -641,6 +641,7 @@
>  	mm_release(tsk, mm);
>  	if (!mm)
>  		return;
> +	sync_mm_rss(tsk, mm);
>  	/*
>  	 * Serialize with any possible pending coredump.
>  	 * We must hold mmap_sem around checking core_state
> 
> 
> 

Re: [ 11/48] mm: correctly synchronize rss-counters at exit/exec

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2200 bytes --]

On Sun, 2012-07-01 at 12:02 -0700, Hugh Dickins wrote:
> On Sun, 1 Jul 2012, Ben Hutchings wrote:
> 
> > 3.2-stable review patch.  If anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: Konstantin Khlebnikov <khlebnikov@openvz.org>
> > 
> > commit 4fe7efdbdfb1c7e7a7f31decfd831c0f31d37091 upstream.
> > 
> > do_exit() and exec_mmap() call sync_mm_rss() before mm_release() does
> > put_user(clear_child_tid) which can update task->rss_stat and thus make
> > mm->rss_stat inconsistent.  This triggers the "BUG:" printk in check_mm().
> > 
> > Let's fix this bug in the safest way, and optimize/cleanup this later.
> > 
> > Reported-by: Markus Trippelsdorf <markus@trippelsdorf.de>
> > Signed-off-by: Konstantin Khlebnikov <khlebnikov@openvz.org>
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > Cc: Hugh Dickins <hughd@google.com>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > [bwh: Backported to 3.2: sync_mm_rss() still takes a struct task_struct *]
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> 
> If you or Konstantin or Oleg have done full diligence on this,
> to ensure that it is really applicable to 3.2 (not just that
> the patch applies without rejects), fair enough.

I can't claim that, no.

> But I'd be cautious about it: it went through enough alternatives
> and revisions that I wouldn't call it trivial; it's easy for me to
> imagine that some of the affected paths were actually slightly different
> in 3.2 days than they were in 3.4 days; and the disturbing warning that
> these mods silence ("BUG: Bad rss-counter state ") did not exist before
> 3.4 - unless you've ported that too?

No.

> That's not to assert that we had no rss problem at all before 3.4,
> but we've not heard of any trouble from it.  Caution tells me that
> this patch might cause more trouble than it's worth.

Then I'll assume this should be dropped unless Konstantin or Oleg speaks
up for it.

Ben.

-- 
Ben Hutchings
When in doubt, use brute force. - Ken Thompson

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [ 11/48] mm: correctly synchronize rss-counters at exit/exec

[Oleg Nesterov]

On 07/02, Ben Hutchings wrote:
>
> Then I'll assume this should be dropped unless Konstantin or Oleg speaks
> up for it.

Just in case, I agree with Hugh.

The patch should shut up check_mm() which doesn't exist in 3.2

Oleg.

Re: [ 11/48] mm: correctly synchronize rss-counters at exit/exec

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 408 bytes --]

On Mon, 2012-07-02 at 18:46 +0200, Oleg Nesterov wrote:
> On 07/02, Ben Hutchings wrote:
> >
> > Then I'll assume this should be dropped unless Konstantin or Oleg speaks
> > up for it.
> 
> Just in case, I agree with Hugh.
> 
> The patch should shut up check_mm() which doesn't exist in 3.2

Thanks; I've dropped this.

Ben.

-- 
Ben Hutchings
When in doubt, use brute force. - Ken Thompson

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]