sched/numa: fix unsafe get_task_struct() in task_numa_assign()

(quoted diff) +0 -1

@@ -259,10 +259,15 @@ void __init fork_init(unsigned long mempages)
 #ifndef ARCH_MIN_TASKALIGN
 #define ARCH_MIN_TASKALIGN	L1_CACHE_BYTES
 #endif
-	/* create a slab on which task_structs can be allocated */

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.

Quoted 5 lines · Oleg Nesterov

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?

Quoted 40 lines · Oleg Nesterov

Quoted 5 lines · Oleg Nesterov

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

Quoted 5 lines

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.

I think generic helper is a good idea. The prototype looks OK.

But I'm a little doubt about retry loop. If this helper is generic and one day it may move to ./include directory,

isn't there a probability
people will use it wrong? This loop may bring delays or something bad.

And since we still depends on RCU, I'd suggest to add its lockdep assert.

Quoted 5 lines · Kirill Tkhai

I think generic helper is a good idea. The prototype looks OK.

But I'm a little doubt about retry loop. If this helper is generic and one day it may move to ./include directory,

Well, if we add a generic helper I think it should be exported even if it has a single caller. But I agree this probably needs a justification too.

Quoted 2 lines

isn't there a probability
people will use it wrong? This loop may bring delays or something bad.

Yes, I thought about livelock too. OK, we can remove it, just s/goto retry/return NULL/. Or, perhaps better, return ERR_PTR(-EAGAIN) so that the caller can know that retry is possible.

I do not really mind, and we can reconsider this later. And I will not argue if you prefer to add the rq->curr specific hack (like your patch does).

Quoted 1 line

And since we still depends on RCU, I'd suggest to add its lockdep assert.

Agreed.

Let me explain what I personally dislike in v3:

- I think that we do not have enough reasons for SLAB_DESTROY_BY_RCU. This is the serious change.

probe_kernel_read() looks better to me, and hopefully IS_ENABLED(DEBUG_PAGEALLOC) can make it conditional.

- PF_EXITING was fine in task_numa_compare(), but if we move this logic into a helper (even if it is not exported) then I think we need a more specific check. sighand == NULL looks better to me because it clearly connects to release_task() which makes this task_struct "rcu-unsafe".

- Again, perhaps we should start we a simple and stupid fix. We can do get_task_struct() under rq->lock or, if nothing else, just

raw_spin_lock_irq(&rq->lock);
cur = rq->curr;
if (is_idle_task(cur) || (cur->flags & PF_EXITING))
cur = NULL;
raw_spin_unlock_irq(&rq->lock);

Either way, I hope you will send v4 ;)

But probably you should wait for for Peter's opinion first.

No, I won't send. Please do this. Your idea and your patch is almost ready. Thanks :)

So please make a patch after we know what Peter thinks.

Let me explain what I personally dislike in v3:

- I think that we do not have enough reasons for SLAB_DESTROY_BY_RCU. This is the serious change.

	- Again, perhaps we should start we a simple and stupid fix.
	  We can do get_task_struct() under rq->lock or, if nothing
	  else, just

	  	raw_spin_lock_irq(&rq->lock);
	  	cur = rq->curr;
	  	if (is_idle_task(cur) || (cur->flags & PF_EXITING))
	  		cur = NULL;
	  	raw_spin_unlock_irq(&rq->lock);

Quoted 4 lines · Oleg Nesterov

Let me explain what I personally dislike in v3:

- I think that we do not have enough reasons for SLAB_DESTROY_BY_RCU. This is the serious change.

What exactly would the downsides be? SDBR has very limited space overhead iirc.

Quoted 9 lines

	- Again, perhaps we should start we a simple and stupid fix.
	  We can do get_task_struct() under rq->lock or, if nothing
	  else, just

	  	raw_spin_lock_irq(&rq->lock);
	  	cur = rq->curr;
	  	if (is_idle_task(cur) || (cur->flags & PF_EXITING))
	  		cur = NULL;
	  	raw_spin_unlock_irq(&rq->lock);

I think I agree with you, this is the simple safe option and is something we can easily backport. After that we can add creative bits on top.

I think I prefer the SLAB_DESTROY_BY_RCU thing over the probe_kernel thing

Quoted 9 lines · Peter Zijlstra

Quoted 4 lines · Oleg Nesterov

Let me explain what I personally dislike in v3:

- I think that we do not have enough reasons for SLAB_DESTROY_BY_RCU. This is the serious change.

What exactly would the downsides be? SDBR has very limited space overhead iirc.

Yes, SDBR is nice (and it could probably have more users), but my concern is not overhead. Please see below.

Quoted 10 lines

Quoted 9 lines

	- Again, perhaps we should start we a simple and stupid fix.
	  We can do get_task_struct() under rq->lock or, if nothing
	  else, just

	  	raw_spin_lock_irq(&rq->lock);
	  	cur = rq->curr;
	  	if (is_idle_task(cur) || (cur->flags & PF_EXITING))
	  		cur = NULL;
	  	raw_spin_unlock_irq(&rq->lock);

Quoted 3 lines

I think I agree with you, this is the simple safe option and is something we can easily backport. After that we can add creative bits on top.

Agreed.

Kirill, could you please make a patch?

Quoted 2 lines

I think I prefer the SLAB_DESTROY_BY_RCU thing over the probe_kernel thing

I won't really insist, but let me try to explain why I dislike it in this particular case.

- It is not clear who else (except task_numa_compare) will need it. And it looks at bit strange to make task_struct SLAB_DESTROY_BY_RCU just to read a word in task_numa_compare().

- In some sense, the usage of SDBR looks simply "wrong" in this case. IOW, I agree that probe_kernel_read() is ugly, but in this case SDBR acts exactly the same way as probe_kernel_read().

SDBR does not make the object rcu-safe, it only protects the object type plus ensures that this memory can't go away. It was designed for the case when you read the stable members initialized in ctor (usually a lock) and verify/lock the object.

But in this case we can not detect that the object is still alive without the additional trick, so when you read ->sighand or ->flags, the fact that this memory is still "struct task_struct" doesn't help and doesn't matter at all. Only the subsequent "task == rq->curr" check proves that yes, this is task_struct.

OTOH, (afaics) we only need probe_kernel_read() if CONFIG_DEBUG_SLAB. And in fact I think that "read the valid but potentially freed kernel pointer" deserves another helper, it can have more users. For example, please look at get_freepointer_safe().

What if we add get_kernel(x, ptr) macro to factor out the IS_ENABLED() of ifdef hack? Or inline function... This way the new xxx() helper we discussed won't look that bad.

But again, I agree that this subjective, I won't really argue.

Quoted 24 lines · Oleg Nesterov

Quoted 9 lines · Peter Zijlstra

Quoted 4 lines · Oleg Nesterov

Let me explain what I personally dislike in v3:

- I think that we do not have enough reasons for SLAB_DESTROY_BY_RCU. This is the serious change.

What exactly would the downsides be? SDBR has very limited space overhead iirc.

Yes, SDBR is nice (and it could probably have more users), but my concern is not overhead. Please see below.

Quoted 10 lines

Quoted 9 lines

	- Again, perhaps we should start we a simple and stupid fix.
	  We can do get_task_struct() under rq->lock or, if nothing
	  else, just

	  	raw_spin_lock_irq(&rq->lock);
	  	cur = rq->curr;
	  	if (is_idle_task(cur) || (cur->flags & PF_EXITING))
	  		cur = NULL;
	  	raw_spin_unlock_irq(&rq->lock);

Quoted 7 lines

Quoted 3 lines

I think I agree with you, this is the simple safe option and is something we can easily backport. After that we can add creative bits on top.

Agreed.

Kirill, could you please make a patch?

Yeah, I'll send it tomorrow.

Quoted 30 lines

Quoted 2 lines

I think I prefer the SLAB_DESTROY_BY_RCU thing over the probe_kernel thing

I won't really insist, but let me try to explain why I dislike it in this particular case.

- It is not clear who else (except task_numa_compare) will need it. And it looks at bit strange to make task_struct SLAB_DESTROY_BY_RCU just to read a word in task_numa_compare().

- In some sense, the usage of SDBR looks simply "wrong" in this case. IOW, I agree that probe_kernel_read() is ugly, but in this case SDBR acts exactly the same way as probe_kernel_read().

SDBR does not make the object rcu-safe, it only protects the object type plus ensures that this memory can't go away. It was designed for the case when you read the stable members initialized in ctor (usually a lock) and verify/lock the object.

But in this case we can not detect that the object is still alive without the additional trick, so when you read ->sighand or ->flags, the fact that this memory is still "struct task_struct" doesn't help and doesn't matter at all. Only the subsequent "task == rq->curr" check proves that yes, this is task_struct.

OTOH, (afaics) we only need probe_kernel_read() if CONFIG_DEBUG_SLAB. And in fact I think that "read the valid but potentially freed kernel pointer" deserves another helper, it can have more users. For example, please look at get_freepointer_safe().

Quoted 5 lines

What if we add get_kernel(x, ptr) macro to factor out the IS_ENABLED() of ifdef hack? Or inline function... This way the new xxx() helper we discussed won't look that bad.

But again, I agree that this subjective, I won't really argue.

So this patch we fix task_numa_compare(). We need remember to fix remaining later:

$ git grep ACCESS_ONCE kernel/sched/ | grep "\->curr"
kernel/sched/deadline.c:	curr = ACCESS_ONCE(rq->curr); /*
kernel/sched/fair.c:	cur = ACCESS_ONCE(dst_rq->curr);
kernel/sched/fair.c:	tsk = ACCESS_ONCE(cpu_rq(cpu)->curr);
kernel/sched/rt.c:	curr = ACCESS_ONCE(rq->curr); /* unlocked

Quoted 2 lines

I think I prefer the SLAB_DESTROY_BY_RCU thing over the probe_kernel thing

I won't really insist, but let me try to explain why I dislike it in this particular case.

- It is not clear who else (except task_numa_compare) will need it. And it looks at bit strange to make task_struct SLAB_DESTROY_BY_RCU just to read a word in task_numa_compare().

- In some sense, the usage of SDBR looks simply "wrong" in this case. IOW, I agree that probe_kernel_read() is ugly, but in this case SDBR acts exactly the same way as probe_kernel_read().

SDBR does not make the object rcu-safe, it only protects the object type plus ensures that this memory can't go away. It was designed for the case when you read the stable members initialized in ctor (usually a lock) and verify/lock the object.

But in this case we can not detect that the object is still alive without the additional trick, so when you read ->sighand or ->flags, the fact that this memory is still "struct task_struct" doesn't help and doesn't matter at all. Only the subsequent "task == rq->curr" check proves that yes, this is task_struct.

OTOH, (afaics) we only need probe_kernel_read() if CONFIG_DEBUG_SLAB. And in fact I think that "read the valid but potentially freed kernel pointer" deserves another helper, it can have more users. For example, please look at get_freepointer_safe().

What if we add get_kernel(x, ptr) macro to factor out the IS_ENABLED() of ifdef hack? Or inline function... This way the new xxx() helper we discussed won't look that bad.

But again, I agree that this subjective, I won't really argue.

So I worry about cache aliasing (not an issue on x86), so by touching 'random' pages that might be freed and reissued to back userspace, we could be accessing the one page through multiple virtual mappings which therefore result in aliases.

SDBR avoids this issue by guaranteeing the page is not reissued for another purpose.

I'm not sure I can convince myself SLUB is correct here. How do we avoid cache aliasing.

Hmm. so perhaps I misunderstood your concern...

Do you mean that on !x86 a plain LOAD can "corrupt" the memory as it seen from another vaddr?

Quoted 4 lines · Oleg Nesterov

Hmm. so perhaps I misunderstood your concern...

Do you mean that on !x86 a plain LOAD can "corrupt" the memory as it seen from another vaddr?

I'm not sure. Stores for sure, loads I'm not sure about.

I suspect loads are OK, the aliasing cacheline will be !modified and therefore later eviction should discard (not write back). But like said, I'm not at all sure.

I would hesitate to put such assumptions into generic code -- although it appears we already have.

So perhaps something like this makes sense?

If some arch has problems with D-cache aliasing (because the freed page can be already mapped by user-space or vmalloc'ed), it can redefine this helper.

Do you think we can use it to access rq->curr? (although let me repeat that I won't really argue with SLAB_DESTROY_BY_RCU).

From: Oleg Nesterov <[email protected]>
Date: Wed, 22 Oct 2014 20:14:12 +0200

Quoted 2 lines

So perhaps something like this makes sense?

Quoted 6 lines

If some arch has problems with D-cache aliasing (because the freed page can be already mapped by user-space or vmalloc'ed), it can redefine this helper.

Do you think we can use it to access rq->curr? (although let me repeat that I won't really argue with SLAB_DESTROY_BY_RCU).

Do we really need this?

We fully initialize and read from the area using the same virtual address, there is no possiblity for corruption from SLAB's perspective.

It's when you store at vaddr X then read at vaddr Y and expect to see what you wrote at X that you have problems.

That is very much not what is happening here.

The lifetime is contained to SLAB's usage at one single virtual address.

Now the question: is this LOAD is safe in case when this (freed) page already has another mapping? This is black magic to me, I do not know. And Peter has some concerns.

And, say, copy_from_user_page() on sparc does

	flush_cache_page();
	memcpy();
	flush_ptrace_access();

From: Oleg Nesterov <[email protected]>
Date: Wed, 22 Oct 2014 21:42:28 +0200

Quoted 3 lines

Now the question: is this LOAD is safe in case when this (freed) page already has another mapping? This is black magic to me, I do not know. And Peter has some concerns.

It is immatieral, because you can read garbage and it's "don't care" in this context.

And later if it is used again at this virtual address, it will be initialized with stores at that virtual address first.

So no problem.

Quoted 5 lines

And, say, copy_from_user_page() on sparc does

	flush_cache_page();
	memcpy();
	flush_ptrace_access();

In this case, as I tried to explain, it matters because the physical address is being accessed from two virtual address at the same time "for the same usage".

That's what distinguishes this from the SLAB and RCU cases you cite.

It's when you store at vaddr X then read at vaddr Y and expect to see what you wrote at X that you have problems.

That is very much not what is happening here.