From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx149.postini.com [74.125.245.149]) by kanga.kvack.org (Postfix) with SMTP id 229B46B004A for ; Fri, 24 Feb 2012 12:07:02 -0500 (EST) Received: by pbcwz17 with SMTP id wz17so3433483pbc.14 for ; Fri, 24 Feb 2012 09:07:01 -0800 (PST) Message-ID: <4F47C3B7.20109@gmail.com> Date: Fri, 24 Feb 2012 12:07:03 -0500 From: KOSAKI Motohiro MIME-Version: 1.0 Subject: Re: [RFC][PATCH] fix move/migrate_pages() race on task struct References: <20120223180740.C4EC4156@kernel> <4F468F09.5050200@linux.vnet.ibm.com> <4F469BC7.50705@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Christoph Lameter Cc: "Eric W. Biederman" , Dave Hansen , linux-kernel@vger.kernel.org, linux-mm@kvack.org, kosaki.motohiro@gmail.com (2/24/12 10:20 AM), Christoph Lameter wrote: > On Thu, 23 Feb 2012, Eric W. Biederman wrote: > >>> The bug in migrate_pages() is that we do a rcu_unlock and a rcu_lock. If >>> we drop those then we should be safe if the use of a task pointer within a >>> rcu section is safe without taking a refcount. >> >> Yes the user of a task_struct pointer found via a userspace pid is valid >> for the life of an rcu critical section, and the bug is indeed that we >> drop the rcu_lock and somehow expect the task to remain valid. >> >> The guarantee comes from release_task. In release_task we call >> __exit_signal which calls __unhash_process, and then we call >> delayed_put_task to guarantee that the task lives until the end of the >> rcu interval. > > Ah. Ok. Great. > >> In migrate_pages we have a lot of task accesses outside of the rcu >> critical section, and without a reference count on task. > > Yes but that is only of interesting for setup and verification of > permissions. What matters during migration is that the mm_struct does not > go away and we take a refcount on that one. > >> I tell you the truth trying to figure out what that code needs to be >> correct if task != current makes my head hurt. > > Hmm... > >> I think we need to grab a reference on task_struct, to stop the task >> from going away, and in addition we need to hold task_lock. To keep >> task->mm from changing (see exec_mmap). But we can't do that and sleep >> so I think the entire function needs to be rewritten, and the need for >> task deep in the migrate_pages path needs to be removed as even with the >> reference count held we can race with someone calling exec. > > We dont need the task during migration. We only need the mm. The task > is safe until rcu_read_unlock therefore maybe the following should fix > migrate pages: > > > Subject: migration: Do not do rcu_read_unlock until the last time we need the task_struct pointer > > Migration functions perform the rcu_read_unlock too early. As a result the > task pointed to may change. Bugs were introduced when adding security checks > because rcu_unlock/lock sequences were inserted. Plus the security checks > and do_move_pages used the task_struct pointer after rcu_unlock. > > Fix those issues by removing the unlock/lock sequences and moving the > rcu_read_unlock after the last use of the task struct pointer. > > Signed-off-by: Christoph Lameter > > Acked-by: KOSAKI Motohiro -- 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/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: email@kvack.org