All of lore.kernel.org
 help / color / mirror / Atom feed
From: Oleg Nesterov <oleg@redhat.com>
To: David Miller <davem@davemloft.net>
Cc: peterz@infradead.org, cl@linux.com, tkhai@yandex.ru,
	ktkhai@parallels.com, linux-kernel@vger.kernel.org,
	mingo@redhat.com, vdavydov@parallels.com
Subject: Re: introduce probe_slab_address?
Date: Wed, 22 Oct 2014 21:42:28 +0200	[thread overview]
Message-ID: <20141022194228.GA10995@redhat.com> (raw)
In-Reply-To: <20141022.145950.1585224138048242923.davem@davemloft.net>

On 10/22, David Miller wrote:
>
> From: Oleg Nesterov <oleg@redhat.com>
> Date: Wed, 22 Oct 2014 20:14:12 +0200
>
> > 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).
>
> Do we really need this?

Sorry, I was not clear. let me first explain why do we want this helper,
(although we can avoid it if we make task_struct_cachep SLAB_DESTROY_BY_RCU).

To simplify the discussion, lets ignore the actuall problem we are
trying to solve. Suppose that we want a trivial function, say,

	int pid_of_curr_task_on_cpu(int cpu)
	{
		 struct rq *rq = cpu_rq(cpu);
		 int pid;

		 rcu_read_lock();
		 pid = rq->curr->pid;
		 rcu_read_unlock();
	}

and we do not really care if it returns the wrong/random result.

The problem is that rcu_read_lock() can not help (unless we add
SLAB_DESTROY_BY_RCU), rq->curr is not protected by RCU. So rq->curr can
be freed. Again, we do not care, but CONFIG_DEBUG_PAGEALLOC can ummap
this page.

So we can use ifdef(CONFIG_DEBUG_PAGEALLOC) but it would be better to
have a helper to hide this ugliness, and (at least) slub can use it too.

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();

Again, I simply do not know if this is relevant or not, probably this
is because "illegal alias" was created by kmap() which can use the
same vaddr to access different pages.

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

OK, so iiuc this should be safe.

Thanks!

Oleg.


  reply	other threads:[~2014-10-22 19:46 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-10-20 10:15 [PATCH v3] sched/numa: fix unsafe get_task_struct() in task_numa_assign() Kirill Tkhai
2014-10-20 14:47 ` Oleg Nesterov
2014-10-20 16:56   ` Oleg Nesterov
2014-10-20 18:27     ` Oleg Nesterov
2014-10-20 20:18       ` Kirill Tkhai
2014-10-20 20:50         ` Oleg Nesterov
2014-10-20 21:05           ` Kirill Tkhai
2014-10-20 21:34             ` Oleg Nesterov
2014-10-20 22:57               ` Peter Zijlstra
2014-10-21  9:45           ` Peter Zijlstra
2014-10-21 19:03             ` Oleg Nesterov
2014-10-21 20:03               ` Kirill Tkhai
2014-10-21 20:10                 ` Kirill Tkhai
2014-10-22  9:09               ` Peter Zijlstra
2014-10-22 16:14                 ` Oleg Nesterov
2014-10-22 16:37                   ` Peter Zijlstra
2014-10-22 18:14                     ` introduce probe_slab_address? (Was: sched/numa: fix unsafe get_task_struct() in task_numa_assign()) Oleg Nesterov
2014-10-22 18:59                       ` introduce probe_slab_address? David Miller
2014-10-22 19:42                         ` Oleg Nesterov [this message]
2014-10-22 20:08                           ` David Miller
2014-10-22 20:20                             ` Oleg Nesterov
2014-10-24  9:44                         ` Peter Zijlstra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20141022194228.GA10995@redhat.com \
    --to=oleg@redhat.com \
    --cc=cl@linux.com \
    --cc=davem@davemloft.net \
    --cc=ktkhai@parallels.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=tkhai@yandex.ru \
    --cc=vdavydov@parallels.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.