Re: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT.

From: Andi Kleen <ak@linux.intel.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Milind Chabbi <chabbi.milind@gmail.com>,
	jolsa@redhat.com, Ingo Molnar <mingo@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Hari Bathini <hbathini@linux.vnet.ibm.com>,
	Jin Yao <yao.jin@linux.intel.com>,
	Kan Liang <kan.liang@intel.com>,
	Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] perf/core: fast breakpoint modification via _IOC_MODIFY_BREAKPOINT.
Date: Tue, 7 Nov 2017 09:09:13 -0800	[thread overview]
Message-ID: <20171107170913.GF5320@tassilo.jf.intel.com> (raw)
In-Reply-To: <20171107081541.GF3326@worktop>

On Tue, Nov 07, 2017 at 09:15:41AM +0100, Peter Zijlstra wrote:
> On Mon, Nov 06, 2017 at 03:16:58PM -0800, Andi Kleen wrote:
> > > +static int _perf_event_modify_breakpoint(struct perf_event *bp,
> > > +					 struct perf_event_attr *attr)
> > > +{
> > > +	u64 old_addr = bp->attr.bp_addr;
> > > +	u64 old_len = bp->attr.bp_len;
> > > +	int old_type = bp->attr.bp_type;
> > > +	int err = 0;
> > > +
> > > +	_perf_event_disable(bp);
> > > +
> > > +	bp->attr.bp_addr = attr->bp_addr;
> > > +	bp->attr.bp_type = attr->bp_type;
> > > +	bp->attr.bp_len = attr->bp_len;
> > 
> > You don't check any of the other fields, so user space is free
> > to fill in junk. That means they can never be used for anything.
> > It would be better to check at least some of them for being
> > zero, and also that the type matches the break point.
> 
> Yes, the values should at the very least get the exact same validation
> they would get on creating an event with those values.

In this case the ioctl could be also generalized. Not call it _BREAKPOINT,
just _MODIFY. Just for now it would be only implemented for break points,
but that could be potentially extended later.

-Andi