Re: [PATCH] mm owner: fix race between swapoff and exit

[Balbir Singh <balbir@linux.vnet.ibm.com>]

Jiri Slaby wrote:
> Hugh Dickins napsal(a):
>> From: Balbir Singh <balbir@linux.vnet.ibm.com>
>>
>> There's a race between mm->owner assignment and swapoff, more easily
>> seen when task slab poisoning is turned on.  The condition occurs when
>> try_to_unuse() runs in parallel with an exiting task.  A similar race
>> can occur with callers of get_task_mm(), such as /proc/<pid>/<mmstats>
>> or ptrace or page migration.
>>
>> CPU0                                    CPU1
>>                                         try_to_unuse
>>                                         looks at mm = task0->mm
>>                                         increments mm->mm_users
>> task 0 exits
>> mm->owner needs to be updated, but no
>> new owner is found (mm_users > 1, but
>> no other task has task->mm = task0->mm)
>> mm_update_next_owner() leaves
>>                                         mmput(mm) decrements mm->mm_users
>> task0 freed
>>                                         dereferencing mm->owner fails
>>
>> The fix is to notify the subsystem via mm_owner_changed callback(),
>> if no new owner is found, by specifying the new task as NULL.
>>
>> Jiri Slaby:
>> mm->owner was set to NULL prior to calling cgroup_mm_owner_callbacks(), but
>> must be set after that, so as not to pass NULL as old owner causing oops.
>>
>> Daisuke Nishimura:
>> mm_update_next_owner() may set mm->owner to NULL, but mem_cgroup_from_task()
>> and its callers need to take account of this situation to avoid oops.
> 
> What about
> memrlimit-setup-the-memrlimit-controller-mm_owner-fix
> ? It adds check for `old' being NULL.
> 

The memrlimit patches are not yet in mainline (they are in -mm).

> BTW there is also mm->owner = NULL; movement in the patch to the line before
> the callbacks are invoked which I don't understand much (why to inform
> anybody about NULL->NULL change?), but the first hunk seems reasonable to me.
> 

It is not in the hunk being posted for inclusion in 2.6.27-rc

> [...]
>> --- 2.6.27-rc7/kernel/cgroup.c	2008-08-06 08:36:20.000000000 +0100
>> +++ linux/kernel/cgroup.c	2008-09-24 17:17:32.000000000 +0100
>> @@ -2738,14 +2738,15 @@ void cgroup_fork_callbacks(struct task_s
>>   */
>>  void cgroup_mm_owner_callbacks(struct task_struct *old, struct task_struct *new)
>>  {
>> -	struct cgroup *oldcgrp, *newcgrp;
>> +	struct cgroup *oldcgrp, *newcgrp = NULL;
>>  
>>  	if (need_mm_owner_callback) {
>>  		int i;
>>  		for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
>>  			struct cgroup_subsys *ss = subsys[i];
>>  			oldcgrp = task_cgroup(old, ss->subsys_id);
>> -			newcgrp = task_cgroup(new, ss->subsys_id);
>> +			if (new)
>> +				newcgrp = task_cgroup(new, ss->subsys_id);
>>  			if (oldcgrp == newcgrp)
>>  				continue;
>>  			if (ss->mm_owner_changed)
>> --- 2.6.27-rc7/kernel/exit.c	2008-09-10 07:37:25.000000000 +0100
>> +++ linux/kernel/exit.c	2008-09-24 17:17:32.000000000 +0100
>> @@ -627,6 +625,16 @@ retry:
>>  	} while_each_thread(g, c);
>>  
>>  	read_unlock(&tasklist_lock);
>> +	/*
>> +	 * We found no owner yet mm_users > 1: this implies that we are
>> +	 * most likely racing with swapoff (try_to_unuse()) or /proc or
>> +	 * ptrace or page migration (get_task_mm()).  Mark owner as NULL,
>> +	 * so that subsystems can understand the callback and take action.
>> +	 */
>> +	down_write(&mm->mmap_sem);
>> +	cgroup_mm_owner_callbacks(mm->owner, NULL);
>> +	mm->owner = NULL;
>> +	up_write(&mm->mmap_sem);
>>  	return;
>>  
>>  assign_new_owner:
> 


-- 
	Balbir

--
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>