[PATCH 0/3] perf/core: miscellaneous fix for address filtering

[PATCH 0/3] perf/core: miscellaneous fix for address filtering

[Mathieu Poirier]

Good day Alex and friends,

I'm sending you a few patches that address bugs I've encounter while
implementing address filtering on CoreSight.  I especially draw your
attention to patch 2/3 - I am pretty sure the same problem can be
found in the x86 world.

The set is based on 4.7-rc7.

Thanks,
Mathieu

Mathieu Poirier (3):
  perf/core: fixing filename for start/stop filters
  perf/core: update filter only on executable mmap
  perf/core: enabling mapping of the stop filters

 kernel/events/core.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH 1/3] perf/core: fixing filename for start/stop filters

[Mathieu Poirier]

Binary file names have to be supplied for both range and start/stop
filters but the current code only process the filename if an
address range filter is specified.  This code adds processing of
the filename for start/stop filters.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 kernel/events/core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 912f10dfbfe5..df21611585d7 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7852,8 +7852,13 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 					goto fail;
 			}
 
-			if (token == IF_SRC_FILE) {
-				filename = match_strdup(&args[2]);
+			if (token == IF_SRC_FILE || token == IF_SRC_FILEADDR) {
+				substring_t *fargs;
+
+				fargs = (token == IF_SRC_FILEADDR ?
+					 &args[1] : &args[2]);
+
+				filename = match_strdup(fargs);
 				if (!filename) {
 					ret = -ENOMEM;
 					goto fail;
-- 
2.7.4

Re: [PATCH 1/3] perf/core: fixing filename for start/stop filters

[Alexander Shishkin]

Mathieu Poirier <mathieu.poirier@linaro.org> writes:

> Binary file names have to be supplied for both range and start/stop
> filters but the current code only process the filename if an
> address range filter is specified.  This code adds processing of
> the filename for start/stop filters.
>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  kernel/events/core.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 912f10dfbfe5..df21611585d7 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -7852,8 +7852,13 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
>  					goto fail;
>  			}
>  
> -			if (token == IF_SRC_FILE) {
> -				filename = match_strdup(&args[2]);
> +			if (token == IF_SRC_FILE || token == IF_SRC_FILEADDR) {
> +				substring_t *fargs;
> +
> +				fargs = (token == IF_SRC_FILEADDR ?
> +					 &args[1] : &args[2]);
> +
> +				filename = match_strdup(fargs);
>  				if (!filename) {
>  					ret = -ENOMEM;
>  					goto fail;

How about a bit shorter version:

diff --git a/kernel/events/core.c b/kernel/events/core.c
index e6a78ccc07..f05d89b605 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -8035,8 +8035,10 @@ perf_event_parse_addr_filter(struct perf_event *event, char *fstr,
 					goto fail;
 			}
 
-			if (token == IF_SRC_FILE) {
-				filename = match_strdup(&args[2]);
+			if (token == IF_SRC_FILE || token == IF_SRC_FILEADDR) {
+				int fpos = filter->range ? 2 : 1;
+
+				filename = match_strdup(&args[fpos]);
 				if (!filename) {
 					ret = -ENOMEM;
 					goto fail;

Thanks,
--
Alex

[PATCH RFC 2/3] perf/core: update filter only on executable mmap

[Mathieu Poirier]

Function perf_event_mmap() is called by the MM subsystem each time
part of a binary is loaded in memory.  There can be several mapping
for a binary, many times unrelated to the code section.

Each time a section of a binary is mapped address filters are
updated, event when the map doesn't pertain to the code section.
The end result is that filters are configured based on the last map
event that was received rather than the last mapping of the code
segment.

For example if we have an executable 'main' that calls library
'libcstest.so.1.0', and that we want to collect traces on code
that is in that library.  The perf cmd line for this scenario
would be:

perf record -e cs_etm// --filter 'filter 0x72c/0x40@/opt/lib/libcstest.so.1.0' --per-thread ./main

Resulting in binaries being mapped this way:

root@linaro-nano:~# cat /proc/1950/maps
00400000-00401000 r-xp 00000000 08:02 33169     /home/linaro/main
00410000-00411000 r--p 00000000 08:02 33169     /home/linaro/main
00411000-00412000 rw-p 00001000 08:02 33169     /home/linaro/main
7fa2464000-7fa2474000 rw-p 00000000 00:00 0
7fa2474000-7fa25a4000 r-xp 00000000 08:02 543   /lib/aarch64-linux-gnu/libc-2.21.so
7fa25a4000-7fa25b3000 ---p 00130000 08:02 543   /lib/aarch64-linux-gnu/libc-2.21.so
7fa25b3000-7fa25b7000 r--p 0012f000 08:02 543   /lib/aarch64-linux-gnu/libc-2.21.so
7fa25b7000-7fa25b9000 rw-p 00133000 08:02 543   /lib/aarch64-linux-gnu/libc-2.21.so
7fa25b9000-7fa25bd000 rw-p 00000000 00:00 0
7fa25bd000-7fa25be000 r-xp 00000000 08:02 38308 /opt/lib/libcstest.so.1.0
7fa25be000-7fa25cd000 ---p 00001000 08:02 38308 /opt/lib/libcstest.so.1.0
7fa25cd000-7fa25ce000 r--p 00000000 08:02 38308 /opt/lib/libcstest.so.1.0
7fa25ce000-7fa25cf000 rw-p 00001000 08:02 38308 /opt/lib/libcstest.so.1.0
7fa25cf000-7fa25eb000 r-xp 00000000 08:02 574   /lib/aarch64-linux-gnu/ld-2.21.so
7fa25ef000-7fa25f2000 rw-p 00000000 00:00 0
7fa25f7000-7fa25f9000 rw-p 00000000 00:00 0
7fa25f9000-7fa25fa000 r--p 00000000 00:00 0     [vvar]
7fa25fa000-7fa25fb000 r-xp 00000000 00:00 0     [vdso]
7fa25fb000-7fa25fc000 r--p 0001c000 08:02 574   /lib/aarch64-linux-gnu/ld-2.21.so
7fa25fc000-7fa25fe000 rw-p 0001d000 08:02 574   /lib/aarch64-linux-gnu/ld-2.21.so
7ff2ea8000-7ff2ec9000 rw-p 00000000 00:00 0     [stack]
root@linaro-nano:~#

Before 'main' can execute 'libcstest.so.1.0' has to be loaded in
memory.  Once that has been done perf_event_mmap() has been called
4 times, with the last map starting at address 0x7fa25ce000 and
the address filter configured to start filtering when the
IP has passed over address 0x0x7fa25ce72c (0x7fa25ce000 + 0x72c).

But that is wrong since the code segment for library 'libcstest.so.1.0'
as been mapped at 0x7fa25bd000, resulting in traces not being
collected.

This patch corrects the situation by requesting that address
filters be updated only if the mapped event is for a code
segment.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 kernel/events/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index df21611585d7..b9aa8f0ff070 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6604,7 +6604,8 @@ void perf_event_mmap(struct vm_area_struct *vma)
 		/* .flags (attr_mmap2 only) */
 	};
 
-	perf_addr_filters_adjust(vma);
+	if ((vma->vm_flags & VM_EXEC) && (vma->vm_pgoff == 0))
+		perf_addr_filters_adjust(vma);
 	perf_event_mmap_event(&mmap_event);
 }
 
-- 
2.7.4

Re: [PATCH RFC 2/3] perf/core: update filter only on executable mmap

[Alexander Shishkin]

Mathieu Poirier <mathieu.poirier@linaro.org> writes:

> Function perf_event_mmap() is called by the MM subsystem each time
> part of a binary is loaded in memory.  There can be several mapping
> for a binary, many times unrelated to the code section.
>
> Each time a section of a binary is mapped address filters are
> updated, event when the map doesn't pertain to the code section.
> The end result is that filters are configured based on the last map
> event that was received rather than the last mapping of the code
> segment.

Good catch! I'd like to fix it in 4.7-stable as well; I think it's too
late for 4.7 already.

> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  kernel/events/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index df21611585d7..b9aa8f0ff070 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6604,7 +6604,8 @@ void perf_event_mmap(struct vm_area_struct *vma)
>  		/* .flags (attr_mmap2 only) */
>  	};
>  
> -	perf_addr_filters_adjust(vma);
> +	if ((vma->vm_flags & VM_EXEC) && (vma->vm_pgoff == 0))
> +		perf_addr_filters_adjust(vma);

You shouldn't need the vm_pgoff check; the range comparison logic in
__perf_addr_filters_adjust() should already take it into account.

Also, I'd put the check to perf_addr_filters_adjust() instead, with a
comment that we don't do data-based filters yet or something along those
lines.

Thanks,
--
Alex

[PATCH 3/3] perf/core: enabling mapping of the stop filters

[Mathieu Poirier]

At this time function perf_addr_filter_needs_mmap() will _not_
return true on a user space 'stop' filter.  But stop filters
needs exactly the same kind of mapping that range and start
filters get.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 kernel/events/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index b9aa8f0ff070..b0bf00c728c2 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6498,7 +6498,7 @@ got_name:
  */
 static bool perf_addr_filter_needs_mmap(struct perf_addr_filter *filter)
 {
-	return filter->filter && filter->inode;
+	return filter->inode;
 }
 
 /*
-- 
2.7.4

Re: [PATCH 3/3] perf/core: enabling mapping of the stop filters

[Alexander Shishkin]

Mathieu Poirier <mathieu.poirier@linaro.org> writes:

> At this time function perf_addr_filter_needs_mmap() will _not_
> return true on a user space 'stop' filter.  But stop filters
> needs exactly the same kind of mapping that range and start
> filters get.

Indeed.

>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  kernel/events/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index b9aa8f0ff070..b0bf00c728c2 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6498,7 +6498,7 @@ got_name:
>   */
>  static bool perf_addr_filter_needs_mmap(struct perf_addr_filter *filter)
>  {
> -	return filter->filter && filter->inode;
> +	return filter->inode;
>  }

Maybe kill the function altogether and instead:

diff --git a/kernel/events/core.c b/kernel/events/core.c
index a001be5d4b..bc8e1ad170 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7905,7 +7905,8 @@ static void perf_event_addr_filters_apply(struct perf_event *event)
 	list_for_each_entry(filter, &ifh->list, entry) {
 		event->addr_filters_offs[count] = 0;
 
-		if (perf_addr_filter_needs_mmap(filter))
+		/* file-based filter */
+		if (filter->inode)
 			event->addr_filters_offs[count] =
 				perf_addr_filter_apply(filter, mm);

Thanks,
--
Alex

Re: [PATCH 0/3] perf/core: miscellaneous fix for address filtering

[Alexander Shishkin]

Mathieu Poirier <mathieu.poirier@linaro.org> writes:

> Good day Alex and friends,
>
> I'm sending you a few patches that address bugs I've encounter while
> implementing address filtering on CoreSight.  I especially draw your
> attention to patch 2/3 - I am pretty sure the same problem can be
> found in the x86 world.

Yes, all three are valid good catches. Some minor things could be
amended, though. Also, don't forget to start with a capital letter after
the colon in the subjects. :)

Regards,
--
Alex