Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1593136 > unrolled thread

perf: use-after-free in perf_release

Back to article view | Back to linux.kernel

Contents

  perf: use-after-free in perf_release Dmitry Vyukov <dvyukov@google.com> - 2017-03-06 11:00 +0100
    Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-06 13:20 +0100
      Re: perf: use-after-free in perf_release Dmitry Vyukov <dvyukov@google.com> - 2017-03-06 13:20 +0100
        Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-06 13:30 +0100
          Re: perf: use-after-free in perf_release Dmitry Vyukov <dvyukov@google.com> - 2017-03-06 13:40 +0100
            Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-06 13:50 +0100
    Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-06 14:30 +0100
      Re: perf: use-after-free in perf_release Dmitry Vyukov <dvyukov@google.com> - 2017-03-06 14:40 +0100
        Re: perf: use-after-free in perf_release Dmitry Vyukov <dvyukov@google.com> - 2017-03-07 10:30 +0100
          Re: perf: use-after-free in perf_release Dmitry Vyukov <dvyukov@google.com> - 2017-03-07 10:50 +0100
            Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-07 12:50 +0100
          Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-07 11:40 +0100
        Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-07 10:50 +0100
      Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-07 14:20 +0100
        Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-07 15:10 +0100
      Re: perf: use-after-free in perf_release Oleg Nesterov <oleg@redhat.com> - 2017-03-07 15:10 +0100
        Re: perf: use-after-free in perf_release Dmitry Vyukov <dvyukov@google.com> - 2017-03-07 15:30 +0100
          Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-07 18:40 +0100
          Re: perf: use-after-free in perf_release Oleg Nesterov <oleg@redhat.com> - 2017-03-07 18:50 +0100
            Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-14 14:00 +0100
              Re: perf: use-after-free in perf_release Oleg Nesterov <oleg@redhat.com> - 2017-03-14 14:30 +0100
                Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-14 14:50 +0100
              Re: perf: use-after-free in perf_release Oleg Nesterov <oleg@redhat.com> - 2017-03-14 15:10 +0100
                Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-14 15:10 +0100
                  Re: perf: use-after-free in perf_release Oleg Nesterov <oleg@redhat.com> - 2017-03-14 15:40 +0100
                    Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-14 16:10 +0100
                      Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-14 16:10 +0100
                        Re: perf: use-after-free in perf_release Oleg Nesterov <oleg@redhat.com> - 2017-03-14 16:40 +0100
                          Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-14 16:50 +0100
                      Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-14 16:30 +0100
                        Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-14 18:40 +0100
                          Re: perf: use-after-free in perf_release Oleg Nesterov <oleg@redhat.com> - 2017-03-15 17:50 +0100
                            Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-16 13:10 +0100
                            Re: perf: use-after-free in perf_release Peter Zijlstra <peterz@infradead.org> - 2017-03-16 15:00 +0100
                              Re: perf: use-after-free in perf_release Oleg Nesterov <oleg@redhat.com> - 2017-03-16 17:50 +0100
                      Re: perf: use-after-free in perf_release Oleg Nesterov <oleg@redhat.com> - 2017-03-14 16:30 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1600346

On 03/14, Peter Zijlstra wrote:
>
> On Tue, Mar 07, 2017 at 05:51:32PM +0100, Oleg Nesterov wrote:
>
> > inherit_event() returns NULL under is_orphaned_event() check, not ERR_PTR().
> > Is it correct?
>
> Yes. This is all a tad tricky, but it seems to be correct.
>
> By returning NULL, not an error, we affect the silent discard of
> orphaned events. This is correct, because otherwise
> perf_event_release_kernel() would have come by and explicitly discarded
> those events for us anyway.

Thanks... I'll try to understand this later.

> @@ -10608,7 +10627,6 @@ inherit_task_group(struct perf_event *event, struct task_struct *parent,
>  		 * First allocate and initialize a context for the
>  		 * child.
>  		 */
> -
>  		child_ctx = alloc_perf_context(parent_ctx->pmu, child);
>  		if (!child_ctx)
>  			return -ENOMEM;
> @@ -10670,7 +10688,7 @@ static int perf_event_init_context(struct task_struct *child, int ctxn)
>  		ret = inherit_task_group(event, parent, parent_ctx,
>  					 child, ctxn, &inherited_all);
>  		if (ret)
> -			break;
> +			goto out_unlock;
>  	}
>  
>  	/*
> @@ -10686,7 +10704,7 @@ static int perf_event_init_context(struct task_struct *child, int ctxn)
>  		ret = inherit_task_group(event, parent, parent_ctx,
>  					 child, ctxn, &inherited_all);
>  		if (ret)
> -			break;
> +			goto out_unlock;

With this change you can also simplify inherit_task_group() a little bit,
it no longer needs to nullify *inherited_all if inherit_group() fails.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1600418

On Tue, Mar 14, 2017 at 02:24:27PM +0100, Oleg Nesterov wrote:
> On 03/14, Peter Zijlstra wrote:
> >
> > On Tue, Mar 07, 2017 at 05:51:32PM +0100, Oleg Nesterov wrote:
> >
> > > inherit_event() returns NULL under is_orphaned_event() check, not ERR_PTR().
> > > Is it correct?
> >
> > Yes. This is all a tad tricky, but it seems to be correct.
> >
> > By returning NULL, not an error, we affect the silent discard of
> > orphaned events. This is correct, because otherwise
> > perf_event_release_kernel() would have come by and explicitly discarded
> > those events for us anyway.
> 
> Thanks... I'll try to understand this later.
> 
> > @@ -10608,7 +10627,6 @@ inherit_task_group(struct perf_event *event, struct task_struct *parent,
> >  		 * First allocate and initialize a context for the
> >  		 * child.
> >  		 */
> > -
> >  		child_ctx = alloc_perf_context(parent_ctx->pmu, child);
> >  		if (!child_ctx)
> >  			return -ENOMEM;
> > @@ -10670,7 +10688,7 @@ static int perf_event_init_context(struct task_struct *child, int ctxn)
> >  		ret = inherit_task_group(event, parent, parent_ctx,
> >  					 child, ctxn, &inherited_all);
> >  		if (ret)
> > -			break;
> > +			goto out_unlock;
> >  	}
> >  
> >  	/*
> > @@ -10686,7 +10704,7 @@ static int perf_event_init_context(struct task_struct *child, int ctxn)
> >  		ret = inherit_task_group(event, parent, parent_ctx,
> >  					 child, ctxn, &inherited_all);
> >  		if (ret)
> > -			break;
> > +			goto out_unlock;
> 
> With this change you can also simplify inherit_task_group() a little bit,
> it no longer needs to nullify *inherited_all if inherit_group() fails.

Ah, that last one is broken because then we forget to re-enable
parent_ctx->rotate_disable.

So if we keep that a break, we still need that inherited_all thing as
well.

[toc] | [prev] | [next] | [standalone]

#1600447

On 03/14, Peter Zijlstra wrote:
>
> Yes, this looks buggy. But I cannot explain how that would result in the
> observed use-after-free.

Yes...

Suppose that copy_process() fails after perf_event_init_task(). In this
case perf_event_free_task() does put_ctx(), but if this ctx has another
reference (ctx->refcount > 1) then ctx->task will point to the already
freed task, copy_process() does free_task() at the end of error path.
And we can't replace it with put_task_struct().

I am looking at TASK_TOMBSTONE, perhaps perf_event_free_task() should
use it too?

Oleg.

[toc] | [prev] | [next] | [standalone]

#1600452

On Tue, Mar 14, 2017 at 03:03:02PM +0100, Oleg Nesterov wrote:
> On 03/14, Peter Zijlstra wrote:
> >
> > Yes, this looks buggy. But I cannot explain how that would result in the
> > observed use-after-free.
> 
> Yes...
> 
> Suppose that copy_process() fails after perf_event_init_task(). In this
> case perf_event_free_task() does put_ctx(), but if this ctx has another
> reference (ctx->refcount > 1) then ctx->task will point to the already
> freed task, copy_process() does free_task() at the end of error path.
> And we can't replace it with put_task_struct().
> 
> I am looking at TASK_TOMBSTONE, perhaps perf_event_free_task() should
> use it too?

The idea was that the task isn't visible when we use
perf_event_free_task(). But I'll have a look.

[toc] | [prev] | [next] | [standalone]

#1600502

On 03/14, Peter Zijlstra wrote:
>
> On Tue, Mar 14, 2017 at 03:03:02PM +0100, Oleg Nesterov wrote:
> > On 03/14, Peter Zijlstra wrote:
> > >
> > > Yes, this looks buggy. But I cannot explain how that would result in the
> > > observed use-after-free.
> >
> > Yes...
> >
> > Suppose that copy_process() fails after perf_event_init_task(). In this
> > case perf_event_free_task() does put_ctx(), but if this ctx has another
> > reference (ctx->refcount > 1) then ctx->task will point to the already
> > freed task, copy_process() does free_task() at the end of error path.
> > And we can't replace it with put_task_struct().
> >
> > I am looking at TASK_TOMBSTONE, perhaps perf_event_free_task() should
> > use it too?
>
> The idea was that the task isn't visible when we use
> perf_event_free_task(). But I'll have a look.

I can be easily wrong, I do not understans this code.

But. perf_event_init_task() adds child_event to parent_event->child_list.

If perf_event_release_kernel(parent_event) is called before copy_process()
does perf_event_free_task() which (in particular) removes it from child_list,
perf_event_release_kernel() can find this child_event and do get_ctx(ctx)
(under the list_for_each_entry(child, &event->child_list, child_list) loop).

Then it does put_ctx(ctx), but ctx->task can be already freed by
copy_process()->free_task() in this case.

No?

Oleg.

[toc] | [prev] | [next] | [standalone]

#1600546

On Tue, Mar 14, 2017 at 03:30:11PM +0100, Oleg Nesterov wrote:

> But. perf_event_init_task() adds child_event to parent_event->child_list.
> 
> If perf_event_release_kernel(parent_event) is called before copy_process()
> does perf_event_free_task() which (in particular) removes it from child_list,
> perf_event_release_kernel() can find this child_event and do get_ctx(ctx)
> (under the list_for_each_entry(child, &event->child_list, child_list) loop).

Right; the child_list is the only thing that is exposed. And yes, it
looks like that can interleave just right.

> Then it does put_ctx(ctx), but ctx->task can be already freed by
> copy_process()->free_task() in this case.


	Task1				Task2

	fork()
	  perf_event_init_task()
	  /* ... */
	  goto bad_fork_$foo;
	  /* ... */
	  perf_event_free_task()
	    mutex_lock(ctx->lock)
	    perf_free_event(B)

					perf_event_release_kernel(A)
					  mutex_lock(A->child_mutex)
					  list_for_each_entry(child, ...) {
					    /* child == B */
					    ctx = B->ctx;
					    get_ctx(ctx);
					    mutex_unlock(A->child_mutex);

	      mutex_lock(A->child_mutex)
	      list_del_init(B->child_list)
	      mutex_unlock(A->child_mutex)

	      /* ... */

	    mutex_unlock(ctx->lock);
	    put_ctx() /* >0 */
	  free_task();
					    mutex_lock(ctx->lock);
					    mutex_lock(A->child_mutex);
					    /* ... */
					    mutex_unlock(A->child_mutex);
					    mutex_unlock(ctx->lock)
					    put_ctx() /* 0 */
					      ctx->task && !TOMBSTONE
					        put_task_struct() /* UAF */


Something like that, right?


Let me see if it makes sense to retain perf_event_free_task() at all;
maybe we should always do perf_event_exit_task().

[toc] | [prev] | [next] | [standalone]

#1600555

On Tue, Mar 14, 2017 at 04:02:41PM +0100, Peter Zijlstra wrote:
> On Tue, Mar 14, 2017 at 03:30:11PM +0100, Oleg Nesterov wrote:
> 
> > But. perf_event_init_task() adds child_event to parent_event->child_list.
> > 
> > If perf_event_release_kernel(parent_event) is called before copy_process()
> > does perf_event_free_task() which (in particular) removes it from child_list,
> > perf_event_release_kernel() can find this child_event and do get_ctx(ctx)
> > (under the list_for_each_entry(child, &event->child_list, child_list) loop).
> 
> Right; the child_list is the only thing that is exposed. And yes, it
> looks like that can interleave just right.
> 
> > Then it does put_ctx(ctx), but ctx->task can be already freed by
> > copy_process()->free_task() in this case.
> 
> 
> 	Task1				Task2
> 
> 	fork()
> 	  perf_event_init_task()
> 	  /* ... */
> 	  goto bad_fork_$foo;
> 	  /* ... */
> 	  perf_event_free_task()
> 	    mutex_lock(ctx->lock)
> 	    perf_free_event(B)
> 
> 					perf_event_release_kernel(A)
> 					  mutex_lock(A->child_mutex)
> 					  list_for_each_entry(child, ...) {
> 					    /* child == B */
> 					    ctx = B->ctx;
> 					    get_ctx(ctx);
> 					    mutex_unlock(A->child_mutex);
> 
> 	      mutex_lock(A->child_mutex)
> 	      list_del_init(B->child_list)
> 	      mutex_unlock(A->child_mutex)
> 
> 	      /* ... */
> 
> 	    mutex_unlock(ctx->lock);
> 	    put_ctx() /* >0 */
> 	  free_task();
> 					    mutex_lock(ctx->lock);
> 					    mutex_lock(A->child_mutex);
> 					    /* ... */
> 					    mutex_unlock(A->child_mutex);
> 					    mutex_unlock(ctx->lock)
> 					    put_ctx() /* 0 */
> 					      ctx->task && !TOMBSTONE
> 					        put_task_struct() /* UAF */
> 
> 
> Something like that, right?
> 
> 
> Let me see if it makes sense to retain perf_event_free_task() at all;
> maybe we should always do perf_event_exit_task().

Do we want a WARN_ON_ONCE(atomic_read(&tsk->usage)); in free_task()?
Because in the above scenario we're freeing it with references on.

[toc] | [prev] | [next] | [standalone]

#1600620

On 03/14, Peter Zijlstra wrote:
>
> Do we want a WARN_ON_ONCE(atomic_read(&tsk->usage)); in free_task()?
> Because in the above scenario we're freeing it with references on.

Not sure, in this case copy_process() should decrement tsk->usage
before free_task(), note the atomic_set(&tsk->usage, 2) in
dup_task_struct().

Perhaps we should just add WARN_ON(tsk->usage != 2) into copy_process()
right before free_task() ?

On the other hand, WARN_ON(atomic_read(&tsk->usage)) looks pointless,
the only caller is put_task_struct().

Oleg.

[toc] | [prev] | [next] | [standalone]

#1600630

On Tue, Mar 14, 2017 at 04:37:05PM +0100, Oleg Nesterov wrote:
> On 03/14, Peter Zijlstra wrote:
> >
> > Do we want a WARN_ON_ONCE(atomic_read(&tsk->usage)); in free_task()?
> > Because in the above scenario we're freeing it with references on.
> 
> Not sure, in this case copy_process() should decrement tsk->usage
> before free_task(), note the atomic_set(&tsk->usage, 2) in
> dup_task_struct().
> 
> Perhaps we should just add WARN_ON(tsk->usage != 2) into copy_process()
> right before free_task() ?

Sure; that works. I'll try that once I'm back home again, to see if
there's unexpected fail because other things increment it.

[toc] | [prev] | [next] | [standalone]

#1600596

On Tue, Mar 14, 2017 at 04:19:10PM +0100, Oleg Nesterov wrote:
> On 03/14, Peter Zijlstra wrote:
> >
> > 	    mutex_unlock(ctx->lock);
> > 	    put_ctx() /* >0 */
> > 	  free_task();
> > 					    mutex_lock(ctx->lock);
> > 					    mutex_lock(A->child_mutex);
> > 					    /* ... */
> > 					    mutex_unlock(A->child_mutex);
> > 					    mutex_unlock(ctx->lock)
> > 					    put_ctx() /* 0 */
> > 					      ctx->task && !TOMBSTONE
> > 					        put_task_struct() /* UAF */
> >
> >
> > Something like that, right?
> 
> Yes, exactly.
> 
> > Let me see if it makes sense to retain perf_event_free_task() at all;
> > maybe we should always do perf_event_exit_task().
> 
> Yes, perhaps... but this needs changes too. Say, WARN_ON_ONCE(child != current)
> in perf_event_exit_task_context(). And even perf_event_task(new => F) does not
> look right in this case. In fact it would be simply buggy to do this, this task
> was not fully constructed yet, so even perf_event_pid(task) is not safe.

Yeah; there's a fair amount of stuff like that. I'm afraid crafting
exceptions for all that will just end up with more of a mess than we
safe by merging the two :/

A well.. I'll go do the 'trivial' patch then.

[toc] | [prev] | [next] | [standalone]

#1600758

On Tue, Mar 14, 2017 at 04:26:25PM +0100, Peter Zijlstra wrote:
> A well.. I'll go do the 'trivial' patch then.

A little like so; completely untested.

---
 kernel/events/core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 110b38a58493..6576449b6029 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -10346,6 +10346,17 @@ void perf_event_free_task(struct task_struct *task)
 			continue;
 
 		mutex_lock(&ctx->mutex);
+		raw_spin_lock_irq(&ctx->lock);
+		/*
+		 * Destroy the task <-> ctx relation and mark the context dead.
+		 *
+		 * This is important because even though the task hasn't been
+		 * exposed yet the context has been (through child_list).
+		 */
+		RCU_INIT_POINTER(task->perf_event_ctxp[ctxn], NULL);
+		WRITE_ONCE(ctx->task, TASK_TOMBSTONE);
+		put_task_struct(task); /* cannot be last */
+		raw_spin_unlock_irq(&ctx->lock);
 again:
 		list_for_each_entry_safe(event, tmp, &ctx->pinned_groups,
 				group_entry)

[toc] | [prev] | [next] | [standalone]

#1601551

On 03/14, Peter Zijlstra wrote:
>
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -10346,6 +10346,17 @@ void perf_event_free_task(struct task_struct *task)
>  			continue;
>
>  		mutex_lock(&ctx->mutex);
> +		raw_spin_lock_irq(&ctx->lock);
> +		/*
> +		 * Destroy the task <-> ctx relation and mark the context dead.
> +		 *
> +		 * This is important because even though the task hasn't been
> +		 * exposed yet the context has been (through child_list).
> +		 */
> +		RCU_INIT_POINTER(task->perf_event_ctxp[ctxn], NULL);
> +		WRITE_ONCE(ctx->task, TASK_TOMBSTONE);
> +		put_task_struct(task); /* cannot be last */
> +		raw_spin_unlock_irq(&ctx->lock);

Agreed, this is what I had in mind. Although you know, I spent 3
hours looking at your patch and I still can't convince myself I am
really sure it closes all races ;)

OK, I believe this is correct. And iiuc both RCU_INIT_POINTER(NULL)
and put_task_struct() are not strictly necessary? At least until we
add WARN_ON(tsk->usage != 2) before free_task() in copy process().


---------------------------------------------------------------------
This is off-topic, but to me list_for_each_entry(event->child_list)
in perf_event_release_kernel() looks very confusing and misleading.
And list_first_entry_or_null(), we do not really need NULL if list
is empty, tmp == child should be F even if we use list_first_entry().
And given that we already have list_is_last(), it would be nice to
add list_is_first() and cleanup perf_event_release_kernel() a bit:

	--- x/kernel/events/core.c
	+++ x/kernel/events/core.c
	@@ -4152,7 +4152,7 @@ static void put_event(struct perf_event 
	 int perf_event_release_kernel(struct perf_event *event)
	 {
		struct perf_event_context *ctx = event->ctx;
	-	struct perf_event *child, *tmp;
	+	struct perf_event *child;
	 
		/*
		 * If we got here through err_file: fput(event_file); we will not have
	@@ -4190,8 +4190,9 @@ int perf_event_release_kernel(struct per
	 
	 again:
		mutex_lock(&event->child_mutex);
	-	list_for_each_entry(child, &event->child_list, child_list) {
	-
	+	if (!list_empty(&event->child_list)) {
	+		child = list_first_entry(&event->child_list,
	+					 struct perf_event, child_list);
			/*
			 * Cannot change, child events are not migrated, see the
			 * comment with perf_event_ctx_lock_nested().
	@@ -4221,9 +4222,7 @@ again:
			 * state, if child is still the first entry, it didn't get freed
			 * and we can continue doing so.
			 */
	-		tmp = list_first_entry_or_null(&event->child_list,
	-					       struct perf_event, child_list);
	-		if (tmp == child) {
	+		if (list_is_first(child, &event->child_list)) {
				perf_remove_from_context(child, DETACH_GROUP);
				list_del(&child->child_list);
				free_event(child);

But we can't, because

	static inline int list_is_first(const struct list_head *list,
					const struct list_head *head)
	{
		return list->prev == head;
	}

won't work, "child" can be freed so we can't dereference it, and

	static inline int list_is_first(const struct list_head *list,
					const struct list_head *head)
	{
		return head->next == list;
	}

won't be symmetrical with list_is_last() we already have.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1602213

On Wed, Mar 15, 2017 at 05:43:02PM +0100, Oleg Nesterov wrote:
> On 03/14, Peter Zijlstra wrote:
> >
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -10346,6 +10346,17 @@ void perf_event_free_task(struct task_struct *task)
> >  			continue;
> >
> >  		mutex_lock(&ctx->mutex);
> > +		raw_spin_lock_irq(&ctx->lock);
> > +		/*
> > +		 * Destroy the task <-> ctx relation and mark the context dead.
> > +		 *
> > +		 * This is important because even though the task hasn't been
> > +		 * exposed yet the context has been (through child_list).
> > +		 */
> > +		RCU_INIT_POINTER(task->perf_event_ctxp[ctxn], NULL);
> > +		WRITE_ONCE(ctx->task, TASK_TOMBSTONE);
> > +		put_task_struct(task); /* cannot be last */
> > +		raw_spin_unlock_irq(&ctx->lock);
> 
> Agreed, this is what I had in mind. Although you know, I spent 3
> hours looking at your patch and I still can't convince myself I am
> really sure it closes all races ;)

Ha; yes I know that feeling. I used to have a few sheets of paper filled
with diagrams. Sadly I could not find them again. Must've been over
eager cleaning my desk at some point.

> 
> OK, I believe this is correct. And iiuc both RCU_INIT_POINTER(NULL)
> and put_task_struct() are not strictly necessary? At least until we
> add WARN_ON(tsk->usage != 2) before free_task() in copy process().

Right; I just kept the code similar to the other location. I even
considered making a helper function to not duplicate, but in the end
decided against it.

> ---------------------------------------------------------------------
> This is off-topic, but to me list_for_each_entry(event->child_list)
> in perf_event_release_kernel() looks very confusing and misleading.
> And list_first_entry_or_null(), we do not really need NULL if list
> is empty, tmp == child should be F even if we use list_first_entry().
> And given that we already have list_is_last(), it would be nice to
> add list_is_first() and cleanup perf_event_release_kernel() a bit:
> 

Agreed; its a bit of a weird one.

Let me go write proper patches for the things we have so far though.

[toc] | [prev] | [next] | [standalone]

#1602299

On Wed, Mar 15, 2017 at 05:43:02PM +0100, Oleg Nesterov wrote:
> 	static inline int list_is_first(const struct list_head *list,
> 					const struct list_head *head)
> 	{
> 		return head->next == list;
> 	}
> 
> won't be symmetrical with list_is_last() we already have.

This is the one that makes sense to me though; that is, the current
list_is_last() doesn't make sense to me.

I would expect:

static inline int list_is_last(const struct list_head *list,
				const struct list_head *head)
{
	return head->prev == list
}

because @head is the list argument (yes, I know, horrible naming!).

[toc] | [prev] | [next] | [standalone]

#1602641

On 03/16, Peter Zijlstra wrote:
>
> On Wed, Mar 15, 2017 at 05:43:02PM +0100, Oleg Nesterov wrote:
> > 	static inline int list_is_first(const struct list_head *list,
> > 					const struct list_head *head)
> > 	{
> > 		return head->next == list;
> > 	}
> >
> > won't be symmetrical with list_is_last() we already have.
>
> This is the one that makes sense to me though; that is, the current
> list_is_last() doesn't make sense to me.
>
> I would expect:
>
> static inline int list_is_last(const struct list_head *list,
> 				const struct list_head *head)
> {
> 	return head->prev == list
> }

Yes!

> because @head is the list argument (yes, I know, horrible naming!).

and perhaps it could have more users if we redefine it to dereference
"head" which is likely more "stable", iow less likely can go away.

But after the quick grep I came to conclusion it is not possible to
audit the users it already has.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1600597

On 03/14, Peter Zijlstra wrote:
>
> 	    mutex_unlock(ctx->lock);
> 	    put_ctx() /* >0 */
> 	  free_task();
> 					    mutex_lock(ctx->lock);
> 					    mutex_lock(A->child_mutex);
> 					    /* ... */
> 					    mutex_unlock(A->child_mutex);
> 					    mutex_unlock(ctx->lock)
> 					    put_ctx() /* 0 */
> 					      ctx->task && !TOMBSTONE
> 					        put_task_struct() /* UAF */
>
>
> Something like that, right?

Yes, exactly.

> Let me see if it makes sense to retain perf_event_free_task() at all;
> maybe we should always do perf_event_exit_task().

Yes, perhaps... but this needs changes too. Say, WARN_ON_ONCE(child != current)
in perf_event_exit_task_context(). And even perf_event_task(new => F) does not
look right in this case. In fact it would be simply buggy to do this, this task
was not fully constructed yet, so even perf_event_pid(task) is not safe.

Oleg.

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web