Re: [PATCH]: Clean up of __alloc_pages

[Nick Piggin <nickpiggin@yahoo.com.au>]

Paul Jackson wrote:
> Andi wrote:
> 
>>>The current code in the kernel does the following:
>>>  1) The cpuset_update_current_mems_allowed() calls in the
>>>     various alloc_page*() paths in mm/mempolicy.c:
>>>	* take the task_lock spinlock on the current task
>>
>>That needs to go imho.
> 
> 
> The comment for refresh_mems(), where this is happening, explains
> why this lock is needed:
> 
>  * The task_lock() is required to dereference current->cpuset safely.
>  * Without it, we could pick up the pointer value of current->cpuset
>  * in one instruction, and then attach_task could give us a different
>  * cpuset, and then the cpuset we had could be removed and freed,
>  * and then on our next instruction, we could dereference a no longer
>  * valid cpuset pointer to get its mems_generation field.
> 
> Hmmm ... on second thought ... damn ... you're right.
> 
> I can just flat out remove that task_lock - without penalty.
> 
> It's *OK* if I dereference a no longer valid cpuset pointer to get
> its (used to be) mems_generation field.  Either that field will have
> already changed, or it won't.
> 

I don't think so because if the cpuset can be freed, then its page
might be unmapped from the kernel address space if use-after-free
debugging is turned on. And this is a use after free :)

Also, it may be reused for something else far into the future without
having its value changed - is this OK?

Anyway, I think the first problem is a showstopper. I'd look into
Hugh's SLAB_DESTROY_BY_RCU for this, which sounds like a good fit
if you need to go down this path (although I only had a quick skim
over the cpusets code).

-- 
SUSE Labs, Novell Inc.

Send instant messages to your online friends http://au.messenger.yahoo.com