Messages in this thread |  | | Date | Mon, 20 Oct 2014 20:27:48 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH v3] sched/numa: fix unsafe get_task_struct() in task_numa_assign() |
| |
On 10/20, Oleg Nesterov wrote: > > On 10/20, Oleg Nesterov wrote: > > > > Again, perhaps we will need to change the lifetime rules for task_struct > > anyway, if we have more problems like this. But until then this looks like > > an overkill to me. Plus rq_curr_if_not_put() looks too subtle, and it is > > not generic. > > Yes... otoh, perhaps we can do something more generic? Something like > > struct task_struct *xxx(struct task_struct **ptask) > { > struct task_struct *task; > void *sighand; > retry: > task = ACCESS_ONCE(*ptask); > if (!task) > return NULL; > > if (IS_ENABLED(CONFIG_DEBUG_PAGEALLOC)) { > if (probe_kernel_read(&sighand, &task->sighand, sizeof(sighand))) > goto retry; > } else { > sighand = task->sighand; > } > > if (!sighand) > return NULL; > /* > * Pairs with atomic_dec_and_test() in put_task_struct(task). > * If we have read the freed/reused memory, we must see that > * the pointer was updated. > */ > smp_rmb(); > if (task != ACCESS_ONCE(*ptask)) > goto retry; > > return task; > } > > task_numa_compare() can do cur = xxx(&rc->curr), but this helper can work > with any "task_struct *" pointer assuming that somehow this pointer is > cleared or changed before the final put_task_struct(). > > What do you think? Peter?
And if we introduce this helper, it would better to check "sighand != NULL" after "task != *ptask":
struct task_struct *xxx(struct task_struct **ptask) { struct task_struct *task; struct sighand_struct *sighand;
retry: task = ACCESS_ONCE(*ptask); if (!task) return task;
if (IS_ENABLED(CONFIG_DEBUG_PAGEALLOC)) { if (probe_kernel_read(&sighand, &task->sighand, sizeof(sighand))) goto retry; } else { sighand = task->sighand; } /* * Pairs with atomic_dec_and_test() in put_task_struct(task). * If we have read the freed/reused memory, we must see that * the pointer was updated. */ smp_rmb(); if (unlikely(task != ACCESS_ONCE(*ptask))) goto retry; /* * release_task(task) was already called, potentially before * the caller took rcu_read_lock() and in this case it can be * freed before rcu_read_unlock(). */ if (!sighand) return NULL; return task; }
Of course, task_numa_compare() do not really need "retry", and task == NULL is not possible. But this way the new helper can (probably) have more users, and this just looks better imo.
Oleg.
|  |