All of lore.kernel.org
 help / color / mirror / Atom feed
From: Oleg Nesterov <oleg@redhat.com>
To: Hugh Dickins <hughd@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Andrey Konovalov <andreyknvl@google.com>,
	Davidlohr Bueso <dave@stgolabs.net>,
	"Kirill A. Shutemov" <kirill@shutemov.name>,
	Sasha Levin <sasha.levin@oracle.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	Andrea Arcangeli <aarcange@redhat.com>,
	Michel Lespinasse <walken@google.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH 1/2] mm: fix the racy mm->locked_vm change in
Date: Thu, 1 Oct 2015 16:49:51 +0200	[thread overview]
Message-ID: <20151001144951.GA6781@redhat.com> (raw)
In-Reply-To: <alpine.LSU.2.11.1509301911320.4528@eggly.anvils>

On 09/30, Hugh Dickins wrote:
>
> On Tue, 29 Sep 2015, Oleg Nesterov wrote:
>
> > "mm->locked_vm += grow" and vm_stat_account() in acct_stack_growth()
> > are not safe; multiple threads using the same ->mm can do this at the
> > same time trying to expans different vma's under down_read(mmap_sem).
>                       expand
> > This means that one of the "locked_vm += grow" changes can be lost
> > and we can miss munlock_vma_pages_all() later.
>
> From the Cc list, I guess you are thinking this might be the fix to
> the "Bad state page (mlocked)" issues Andrey and Sasha have reported.

Yes, I found this when I tried to explain this problem, but I doubt
this change can fix it... Firstly I think it is very unlikely that
trinity hits this race. And even if mm->locked_vm is wrongly equal
to zero in exit_mmap(), it seems that page_remove_rmap() should do
clear_page_mlock(). But I do not understand this code enough. So if
this patch can actually help I would really like to know why ;)

And of course this can not explain other traces which look like
mm->mmap corruption.

> Acked-by: Hugh Dickins <hughd@google.com>

Thanks!

> with some hesitation.  I don't like very much that the preliminary
> mm->locked_vm + grow check is still done without complete locking,
> so racing threads could get more locked_vm than they're permitted;
> but I'm not sure that we care enough to put page_table_lock back
> over all of that (and security_vm_enough_memory wants to have final
> say on whether to go ahead); even if it was that way years ago.

Yes. Plus all these RLIMIT_MEMLOCK/etc and security_* checks assume
that we are going to expand current->mm, but this is not necessarily
true. Debugger or sys_process_vm_* can expand a foreign vma.

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

WARNING: multiple messages have this Message-ID (diff)
From: Oleg Nesterov <oleg@redhat.com>
To: Hugh Dickins <hughd@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Andrey Konovalov <andreyknvl@google.com>,
	Davidlohr Bueso <dave@stgolabs.net>,
	"Kirill A. Shutemov" <kirill@shutemov.name>,
	Sasha Levin <sasha.levin@oracle.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	Andrea Arcangeli <aarcange@redhat.com>,
	Michel Lespinasse <walken@google.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH 1/2] mm: fix the racy mm->locked_vm change in
Date: Thu, 1 Oct 2015 16:49:51 +0200	[thread overview]
Message-ID: <20151001144951.GA6781@redhat.com> (raw)
In-Reply-To: <alpine.LSU.2.11.1509301911320.4528@eggly.anvils>

On 09/30, Hugh Dickins wrote:
>
> On Tue, 29 Sep 2015, Oleg Nesterov wrote:
>
> > "mm->locked_vm += grow" and vm_stat_account() in acct_stack_growth()
> > are not safe; multiple threads using the same ->mm can do this at the
> > same time trying to expans different vma's under down_read(mmap_sem).
>                       expand
> > This means that one of the "locked_vm += grow" changes can be lost
> > and we can miss munlock_vma_pages_all() later.
>
> From the Cc list, I guess you are thinking this might be the fix to
> the "Bad state page (mlocked)" issues Andrey and Sasha have reported.

Yes, I found this when I tried to explain this problem, but I doubt
this change can fix it... Firstly I think it is very unlikely that
trinity hits this race. And even if mm->locked_vm is wrongly equal
to zero in exit_mmap(), it seems that page_remove_rmap() should do
clear_page_mlock(). But I do not understand this code enough. So if
this patch can actually help I would really like to know why ;)

And of course this can not explain other traces which look like
mm->mmap corruption.

> Acked-by: Hugh Dickins <hughd@google.com>

Thanks!

> with some hesitation.  I don't like very much that the preliminary
> mm->locked_vm + grow check is still done without complete locking,
> so racing threads could get more locked_vm than they're permitted;
> but I'm not sure that we care enough to put page_table_lock back
> over all of that (and security_vm_enough_memory wants to have final
> say on whether to go ahead); even if it was that way years ago.

Yes. Plus all these RLIMIT_MEMLOCK/etc and security_* checks assume
that we are going to expand current->mm, but this is not necessarily
true. Debugger or sys_process_vm_* can expand a foreign vma.

Oleg.


  reply	other threads:[~2015-10-01 14:53 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-09-29 18:27 [PATCH 0/2] mm: fix the racy mm->locked_vm change in acct_stack_growth() Oleg Nesterov
2015-09-29 18:27 ` [PATCH 1/2] mm: fix the racy mm->locked_vm change in Oleg Nesterov
2015-10-01  3:01   ` Hugh Dickins
2015-10-01  3:01     ` Hugh Dickins
2015-10-01 14:49     ` Oleg Nesterov [this message]
2015-10-01 14:49       ` Oleg Nesterov
2015-10-01 18:34       ` Hugh Dickins
2015-10-01 18:34         ` Hugh Dickins
2015-09-29 18:28 ` [PATCH 2/2] mm: add the "struct mm_struct *mm" local into Oleg Nesterov
2015-10-01  3:02   ` Hugh Dickins
2015-10-01  3:02     ` Hugh Dickins

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20151001144951.GA6781@redhat.com \
    --to=oleg@redhat.com \
    --cc=aarcange@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=andreyknvl@google.com \
    --cc=dave@stgolabs.net \
    --cc=hughd@google.com \
    --cc=kirill@shutemov.name \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=sasha.levin@oracle.com \
    --cc=vbabka@suse.cz \
    --cc=walken@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.