| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANLsYkxPgQtJJWk=17mF0TgRNj+nH1ePgMMtYdek9Pero-C0rQ@mail.gmail.com>
Date: Tue, 7 Feb 2017 10:50:50 -0700
From: Mathieu Poirier <mathieu.poirier@...aro.org>
To: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
Cc: Peter Zijlstra <a.p.zijlstra@...llo.nl>,
Ingo Molnar <mingo@...hat.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Vince Weaver <vince@...ter.net>,
Stephane Eranian <eranian@...gle.com>,
Arnaldo Carvalho de Melo <acme@...radead.org>,
Will Deacon <will.deacon@....com>,
Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH 1/3] perf, pt, coresight: Clean up address filter structure
On 2 February 2017 at 09:22, Alexander Shishkin
<alexander.shishkin@...ux.intel.com> wrote:
> Mathieu Poirier <mathieu.poirier@...aro.org> writes:
>
>> If this is what you want to convey then
>>
>> + * @action: filter/start/stop
>>
>> needs to be fixed. This can be interpreted as "use range filter,
>> start filter or stop filter" - which is exactly what I did. Something
>> like
>
> I was beginning to think that the 'correct' way only existed in my head,
> but then I noticed elsewhere there's a comment:
>
> | where ACTION is one of the
> | * "filter": limit the trace to this region
> | * "start": start tracing from this address
> | * "stop": stop tracing at this address/region;
>
> so it is sort of explained. Maybe I also need to spell it out in the
> structure. And the man page, of course, when I get to it.
>
>>> The fact that the CS driver gets it wrong
>>> just proves the point that "filter->filter" is confusing and misleading
>>> and needs to be replaced.
>>>
>>
>> I could not agree more.
>>
>> On the flip side it doesn't change anything to my original argument:
>> the code should not be made to be smart. If a range filter is used
>> then a size of zero should be treated as an error.
>
> It actually does, because when you say 'range filter' you really mean
> 'filter filter' and we need to get this difference straight as I also
> mentioned in the other email. ACTION=="filter" does not make sense with
> size==0, this much is true.
I've always been in agreement with that statement.
> The other two are fine, though.
>
> So considering all of the above, I have amended the patch to something
> like the following. I'm not 100% sure that the CS side is accurate, but
> I'm hoping you can have a look.
>
> Side note: while looking through address comparator code, I noticed your
> catch-all filter gets the end address wrong if compiled in 64bit mode.
I'm interested in your point of view on this - can you elaborate?
>
> From 0a549c016591b470fc46b79664ec761db1db6666 Mon Sep 17 00:00:00 2001
> From: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
> Date: Mon, 23 Jan 2017 17:29:20 +0200
> Subject: [PATCH] perf, pt, coresight: Clean up address filter structure
>
> $WRITEME
>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
> Cc: Mathieu Poirier <mathieu.poirier@...aro.org>
> ---
> arch/x86/events/intel/pt.c | 13 ++++--
> drivers/hwtracing/coresight/coresight-etm-perf.c | 59 +++++++++++-------------
> include/linux/perf_event.h | 14 ++++--
> kernel/events/core.c | 26 +++++++----
> 4 files changed, 63 insertions(+), 49 deletions(-)
>
> diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
> index 1c1b9fe705..f693c8ae75 100644
> --- a/arch/x86/events/intel/pt.c
> +++ b/arch/x86/events/intel/pt.c
> @@ -1100,8 +1100,12 @@ static int pt_event_addr_filters_validate(struct list_head *filters)
> int range = 0;
>
> list_for_each_entry(filter, filters, entry) {
> - /* PT doesn't support single address triggers */
> - if (!filter->range || !filter->size)
> + /*
> + * PT doesn't support single address triggers and
> + * 'start' filters.
> + */
> + if (!filter->size ||
> + filter->action == PERF_ADDR_FILTER_ACTION_START)
> return -EOPNOTSUPP;
>
> if (!filter->inode) {
> @@ -1141,7 +1145,10 @@ static void pt_event_addr_filters_sync(struct perf_event *event)
>
> filters->filter[range].msr_a = msr_a;
> filters->filter[range].msr_b = msr_b;
> - filters->filter[range].config = filter->filter ? 1 : 2;
> + if (filter->action == PERF_ADDR_FILTER_ACTION_FILTER)
> + filters->filter[range].config = 1;
> + else
> + filters->filter[range].config = 2;
> range++;
> }
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> index 1774196902..70eaa74dc2 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -392,35 +392,26 @@ static int etm_addr_filters_validate(struct list_head *filters)
> if (++index > ETM_ADDR_CMP_MAX)
> return -EOPNOTSUPP;
>
> + /* filter::size==0 means single address trigger */
> + if (filter->size) {
> + /*
> + * The existing code relies on START/STOP filters
> + * being address filters.
> + */
The above comment is not needed.
> + if (filter->action == PERF_ADDR_FILTER_ACTION_START ||
> + filter->action == PERF_ADDR_FILTER_ACTION_STOP)
> + return -EOPNOTSUPP;
> +
> + range = true;
> + } else
> + address = true;
> +
> /*
> - * As taken from the struct perf_addr_filter documentation:
> - * @range: 1: range, 0: address
> - *
> * At this time we don't allow range and start/stop filtering
> * to cohabitate, they have to be mutually exclusive.
> */
> - if ((filter->range == 1) && address)
> + if (range && address)
> return -EOPNOTSUPP;
> -
> - if ((filter->range == 0) && range)
> - return -EOPNOTSUPP;
> -
> - /*
> - * For range filtering, the second address in the address
> - * range comparator needs to be higher than the first.
> - * Invalid otherwise.
> - */
> - if (filter->range && filter->size == 0)
> - return -EINVAL;
I'm good with that as this check is now done in perf_event_parse_addr_filter().
> -
> - /*
> - * Everything checks out with this filter, record what we've
> - * received before moving on to the next one.
> - */
> - if (filter->range)
> - range = true;
> - else
> - address = true;
> }
>
> return 0;
> @@ -440,18 +431,20 @@ static void etm_addr_filters_sync(struct perf_event *event)
> stop = start + filter->size;
> etm_filter = &filters->etm_filter[i];
>
> - if (filter->range == 1) {
> + switch (filter->action) {
> + case PERF_ADDR_FILTER_ACTION_FILTER:
> etm_filter->start_addr = start;
> etm_filter->stop_addr = stop;
> etm_filter->type = ETM_ADDR_TYPE_RANGE;
> - } else {
> - if (filter->filter == 1) {
> - etm_filter->start_addr = start;
> - etm_filter->type = ETM_ADDR_TYPE_START;
> - } else {
> - etm_filter->stop_addr = stop;
> - etm_filter->type = ETM_ADDR_TYPE_STOP;
> - }
> + break;
> + case PERF_ADDR_FILTER_ACTION_START:
> + etm_filter->start_addr = start;
> + etm_filter->type = ETM_ADDR_TYPE_START;
> + break;
> + case PERF_ADDR_FILTER_ACTION_STOP:
> + etm_filter->stop_addr = stop;
> + etm_filter->type = ETM_ADDR_TYPE_STOP;
> + break;
That is correct and reflects the current implementation.
> }
> i++;
> }
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 1432df5a82..c09aff84f7 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -457,14 +457,19 @@ struct pmu {
> int (*filter_match) (struct perf_event *event); /* optional */
> };
>
> +enum perf_addr_filter_action_t {
> + PERF_ADDR_FILTER_ACTION_STOP = 0,
> + PERF_ADDR_FILTER_ACTION_START,
> + PERF_ADDR_FILTER_ACTION_FILTER,
> +};
> +
> /**
> * struct perf_addr_filter - address range filter definition
> * @entry: event's filter list linkage
> * @inode: object file's inode for file-based filters
> * @offset: filter range offset
> - * @size: filter range size
> - * @range: 1: range, 0: address
> - * @filter: 1: filter/start, 0: stop
> + * @size: filter range size (size==0 means single address trigger)
> + * @action: filter/start/stop
> *
> * This is a hardware-agnostic filter configuration as specified by the user.
> */
> @@ -473,8 +478,7 @@ struct perf_addr_filter {
> struct inode *inode;
> unsigned long offset;
> unsigned long size;
> - unsigned int range : 1,
> - filter : 1;
> + enum perf_addr_filter_action_t action;
> };
>
> /**
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 39106ae61b..d7a11faac1 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8194,7 +8194,8 @@ static void perf_event_addr_filters_apply(struct perf_event *event)
> * * for kernel addresses: <start address>[/<size>]
> * * for object files: <start address>[/<size>]@</path/to/object/file>
> *
> - * if <size> is not specified, the range is treated as a single address.
> + * if <size> is not specified or is zero, the range is treated as a single
> + * address; not valid for ACTION=="filter".
Now that a size of 0 can't be specified with a "filter" action, I'm
good with that statement.
Thanks,
Mathieu
> */
> enum {
> IF_ACT_NONE = -1,
> @@ -8244,6 +8245,11 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
> return -ENOMEM;
>
> while ((start = strsep(&fstr, " ,\n")) != NULL) {
> + static const enum perf_addr_filter_action_t actions[] = {
> + [IF_ACT_FILTER] = PERF_ADDR_FILTER_ACTION_FILTER,
> + [IF_ACT_START] = PERF_ADDR_FILTER_ACTION_START,
> + [IF_ACT_STOP] = PERF_ADDR_FILTER_ACTION_STOP,
> + };
> ret = -EINVAL;
>
> if (!*start)
> @@ -8260,12 +8266,11 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
> switch (token) {
> case IF_ACT_FILTER:
> case IF_ACT_START:
> - filter->filter = 1;
> -
> case IF_ACT_STOP:
> if (state != IF_STATE_ACTION)
> goto fail;
>
> + filter->action = actions[token];
> state = IF_STATE_SOURCE;
> break;
>
> @@ -8278,15 +8283,12 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
> if (state != IF_STATE_SOURCE)
> goto fail;
>
> - if (token == IF_SRC_FILE || token == IF_SRC_KERNEL)
> - filter->range = 1;
> -
> *args[0].to = 0;
> ret = kstrtoul(args[0].from, 0, &filter->offset);
> if (ret)
> goto fail;
>
> - if (filter->range) {
> + if (token == IF_SRC_KERNEL || token == IF_SRC_FILE) {
> *args[1].to = 0;
> ret = kstrtoul(args[1].from, 0, &filter->size);
> if (ret)
> @@ -8294,7 +8296,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
> }
>
> if (token == IF_SRC_FILE || token == IF_SRC_FILEADDR) {
> - int fpos = filter->range ? 2 : 1;
> + int fpos = token == IF_SRC_FILE ? 2 : 1;
>
> filename = match_strdup(&args[fpos]);
> if (!filename) {
> @@ -8319,6 +8321,14 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
> if (kernel && event->attr.exclude_kernel)
> goto fail;
>
> + /*
> + * ACTION "filter" must have a non-zero length region
> + * specified.
> + */
> + if (filter->action == PERF_ADDR_FILTER_ACTION_FILTER &&
> + !filter->size)
> + goto fail;
> +
> if (!kernel) {
> if (!filename)
> goto fail;
> --
> 2.11.0
>
Powered by blists - more mailing lists