Re: [PATCH 2/8] perf: Sanitize get_callchain_buffer()

[Frederic Weisbecker <fweisbec@gmail.com>]

On Thu, Aug 01, 2013 at 03:51:02PM +0200, Jiri Olsa wrote:
> On Thu, Aug 01, 2013 at 03:42:28PM +0200, Frederic Weisbecker wrote:
> > On Thu, Aug 01, 2013 at 03:29:34PM +0200, Jiri Olsa wrote:
> > > On Tue, Jul 23, 2013 at 02:31:00AM +0200, Frederic Weisbecker wrote:
> > > SNIP
> > > 
> > > >  		if (event->attach_state & PERF_ATTACH_TASK)
> > > >  			static_key_slow_inc(&perf_sched_events.key);
> > > >  		if (event->attr.mmap || event->attr.mmap_data)
> > > > @@ -6572,16 +6570,19 @@ done:
> > > >  				atomic_inc(&per_cpu(perf_branch_stack_events,
> > > >  						    event->cpu));
> > > >  		}
> > > > -		if (event->attr.sample_type & PERF_SAMPLE_CALLCHAIN) {
> > > > -			err = get_callchain_buffers();
> > > > -			if (err) {
> > > > -				free_event(event);
> > > > -				return ERR_PTR(err);
> > > > -			}
> > > > -		}
> > > >  	}
> > > >  
> > > >  	return event;
> > > > +
> > > > +err_pmu:
> > > > +	if (event->destroy)
> > > > +		event->destroy(event);
> > > > +err_ns:
> > > > +	if (event->ns)
> > > > +		put_pid_ns(event->ns);
> > > > +	kfree(event);
> > > > +
> > > > +	return ERR_PTR(err);
> > > 
> > > could we call __free_filter(event) here?
> > 
> > Hmm, the filters are installed from ioctl time so there shouldn't be any yet. But there should be
> > an exception with inherited events. I fail to find where the filter is inherited though. Do
> > we actually inherit those?
> 
> ouch.. last I checked was freeing filter before writing this... :)
> 
> what I meant was the __free_event(event)

free_event() doesn't work either because we want several level of rollback depending
of where the error triggered:

   +err_pmu:
           if (event->destroy)
                 event->destroy(event);
   +err_ns:
           if (event->ns)
                 put_pid_ns(event->ns);
           kfree(event);

           return ERR_PTR(err)

If we fail after pmu init we want to call destroy, free pid ns and the event.
If we fail before the pmu init, we want to only free pid ns and the event, ...

_free_event() does the whole in any case, which is not what we want.

But...

OTOH it might work due to the if (event->destroy) and if (event->ns) before freeing the
resource associated.

So may be I can replace the labels with a single call to __free_event() after all as it
checks what needs to be freed.  What do you think?