| 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: <20170126182645.GA1991@linaro.org>
Date: Thu, 26 Jan 2017 11:26:45 -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,
vince@...ter.net, 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
Hi Alex,
On Thu, Jan 26, 2017 at 11:40:55AM +0200, Alexander Shishkin wrote:
> This is a cosmetic patch that deals with the address filter structure's
> ambiguous fields 'filter' and 'range'. The former stands to mean that the
> filter's *action* should be to filter the traces to its address range if
> it's set or stop tracing if it's unset. This is confusing and hard on the
> eyes, so this patch replaces it with 'action' enum. The 'range' field is
> completely redundant (meaning that the filter is an address range as
> opposed to a single address trigger), as we can use zero size to mean the
> same thing.
>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
> Cc: Mathieu Poirier <mathieu.poirier@...aro.org>
> ---
> arch/x86/events/intel/pt.c | 7 +++--
> drivers/hwtracing/coresight/coresight-etm-perf.c | 33 ++++++------------------
> include/linux/perf_event.h | 14 ++++++----
> kernel/events/core.c | 12 ++++-----
> 4 files changed, 27 insertions(+), 39 deletions(-)
>
> diff --git a/arch/x86/events/intel/pt.c b/arch/x86/events/intel/pt.c
> index 5dba5468f8..5ac0911ac6 100644
> --- a/arch/x86/events/intel/pt.c
> +++ b/arch/x86/events/intel/pt.c
> @@ -1141,7 +1141,7 @@ static int pt_event_addr_filters_validate(struct list_head *filters)
>
> list_for_each_entry(filter, filters, entry) {
> /* PT doesn't support single address triggers */
> - if (!filter->range || !filter->size)
> + if (!filter->size)
> return -EOPNOTSUPP;
>
> if (!filter->inode) {
> @@ -1181,7 +1181,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..62ae652664 100644
> --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> @@ -392,35 +392,18 @@ 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)
> + 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;
This changes the behavior we used to have. Now a range filter with a size of 0
will be treated as start filter rather than an error. See below on a possible
way of fixing this.
> -
> - /*
> - * 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;
> @@ -445,7 +428,7 @@ static void etm_addr_filters_sync(struct perf_event *event)
> etm_filter->stop_addr = stop;
> etm_filter->type = ETM_ADDR_TYPE_RANGE;
> } else {
> - if (filter->filter == 1) {
> + if (filter->action == PERF_ADDR_FILTER_ACTION_START) {
> etm_filter->start_addr = start;
> etm_filter->type = ETM_ADDR_TYPE_START;
> } else {
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 214c81588e..fcb37c81ca 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 = PERF_ADDR_FILTER_ACTION_START,
If we are to embark on a renaming exercise I think
PERF_ADDR_FILTER_ACTION_FILTER should be renamed PERF_ADDR_FILTER_ACTION_RANGE.
In the end that's exactly what it is.
> +};
> +
> /**
> * 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 458ff624d6..b422b5feee 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -8141,7 +8141,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.
> */
> enum {
> IF_ACT_NONE = -1,
> @@ -8207,7 +8208,7 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
> switch (token) {
> case IF_ACT_FILTER:
> case IF_ACT_START:
> - filter->filter = 1;
> + filter->action = PERF_ADDR_FILTER_ACTION_FILTER;
To fix the problem mentioned above:
1) Reorder the "if_tokens" match table to list stop, start and filter, in that
order.
2) Then for this case statement filter->action = token;
>
> case IF_ACT_STOP:
> if (state != IF_STATE_ACTION)
> @@ -8225,15 +8226,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) {
if (filter->action == PERF_ADDR_FILTER_ACTION_RANGE)
> *args[1].to = 0;
> ret = kstrtoul(args[1].from, 0, &filter->size);
> if (ret)
> @@ -8241,7 +8239,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) {
> --
> 2.11.0
>
Powered by blists - more mailing lists