linux-kernel - Re: [PATCH 03/24] perf: Add build id data in mmap2 event

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20201113042935.GA167797@google.com>
Date:   Fri, 13 Nov 2020 13:29:35 +0900
From:   Namhyung Kim <namhyung@...nel.org>
To:     Jiri Olsa <jolsa@...nel.org>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Ingo Molnar <mingo@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Michael Petlan <mpetlan@...hat.com>,
        Song Liu <songliubraving@...com>,
        Ian Rogers <irogers@...gle.com>,
        Stephane Eranian <eranian@...gle.com>,
        Alexey Budankov <alexey.budankov@...ux.intel.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Adrian Hunter <adrian.hunter@...el.com>
Subject: Re: [PATCH 03/24] perf: Add build id data in mmap2 event

Hi Jiri,

On Mon, Nov 09, 2020 at 10:53:54PM +0100, Jiri Olsa wrote:
> Adding support to carry build id data in mmap2 event.
> 
> The build id data replaces maj/min/ino/ino_generation
> fields, whichc are also used to identify map's binary,
> so it's ok to replace them with build id data:
> 
>   union {
>           struct {
>                   u32       maj;
>                   u32       min;
>                   u64       ino;
>                   u64       ino_generation;
>           };
>           struct {
>                   u8        build_id[20];
>                   u8        build_id_size;
>                   u8        __reserved_1;
>                   u16       __reserved_2;
>           };
>   };
> 
> Replaced maj/min/ino/ino_generation fields give us size
> of 24 bytes. We use 20 bytes for build id data, 1 byte
> for size and rest is unused.
> 
> There's new misc bit for mmap2 to signal there's build
> id data in it:
> 
>   #define PERF_RECORD_MISC_BUILD_ID              (1 << 14)
> 
> Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> ---
[SNIP]
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index da467e1dd49a..808473b6ce85 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -51,6 +51,7 @@
>  #include <linux/proc_ns.h>
>  #include <linux/mount.h>
>  #include <linux/min_heap.h>
> +#include <linux/buildid.h>
>  
>  #include "internal.h"
>  
> @@ -395,6 +396,7 @@ static atomic_t nr_ksymbol_events __read_mostly;
>  static atomic_t nr_bpf_events __read_mostly;
>  static atomic_t nr_cgroup_events __read_mostly;
>  static atomic_t nr_text_poke_events __read_mostly;
> +static atomic_t nr_build_id_events __read_mostly;
>  
>  static LIST_HEAD(pmus);
>  static DEFINE_MUTEX(pmus_lock);
> @@ -4672,6 +4674,8 @@ static void unaccount_event(struct perf_event *event)
>  		dec = true;
>  	if (event->attr.mmap || event->attr.mmap_data)
>  		atomic_dec(&nr_mmap_events);
> +	if (event->attr.build_id)
> +		atomic_dec(&nr_build_id_events);
>  	if (event->attr.comm)
>  		atomic_dec(&nr_comm_events);
>  	if (event->attr.namespaces)
> @@ -7942,6 +7946,8 @@ struct perf_mmap_event {
>  	u64			ino;
>  	u64			ino_generation;
>  	u32			prot, flags;
> +	u8			build_id[BUILD_ID_SIZE];
> +	u32			build_id_size;
>  
>  	struct {
>  		struct perf_event_header	header;
> @@ -7997,13 +8003,23 @@ static void perf_event_mmap_output(struct perf_event *event,
>  	mmap_event->event_id.pid = perf_event_pid(event, current);
>  	mmap_event->event_id.tid = perf_event_tid(event, current);
>  
> +	if (event->attr.mmap2 && event->attr.build_id)
> +		mmap_event->event_id.header.misc |= PERF_RECORD_MISC_BUILD_ID;
> +
>  	perf_output_put(&handle, mmap_event->event_id);
>  
>  	if (event->attr.mmap2) {
> -		perf_output_put(&handle, mmap_event->maj);
> -		perf_output_put(&handle, mmap_event->min);
> -		perf_output_put(&handle, mmap_event->ino);
> -		perf_output_put(&handle, mmap_event->ino_generation);
> +		if (event->attr.build_id) {
> +			u8 size[4] = { (u8) mmap_event->build_id_size, 0, 0, 0 };
> +
> +			__output_copy(&handle, mmap_event->build_id, BUILD_ID_SIZE);
> +			__output_copy(&handle, size, 4);
> +		} else {
> +			perf_output_put(&handle, mmap_event->maj);
> +			perf_output_put(&handle, mmap_event->min);
> +			perf_output_put(&handle, mmap_event->ino);
> +			perf_output_put(&handle, mmap_event->ino_generation);
> +		}
>  		perf_output_put(&handle, mmap_event->prot);
>  		perf_output_put(&handle, mmap_event->flags);
>  	}
> @@ -8132,6 +8148,11 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
>  
>  	mmap_event->event_id.header.size = sizeof(mmap_event->event_id) + size;
>  
> +	if (atomic_read(&nr_build_id_events)) {
> +		build_id_parse_size(vma, mmap_event->build_id,
> +				    &mmap_event->build_id_size);

This can fail, right?  Then we need to fallback to use dev/ino..

Thanks,
Namhyung


> +	}
> +
>  	perf_iterate_sb(perf_event_mmap_output,
>  		       mmap_event,
>  		       NULL);
> @@ -11069,6 +11090,8 @@ static void account_event(struct perf_event *event)
>  		inc = true;
>  	if (event->attr.mmap || event->attr.mmap_data)
>  		atomic_inc(&nr_mmap_events);
> +	if (event->attr.build_id)
> +		atomic_inc(&nr_build_id_events);
>  	if (event->attr.comm)
>  		atomic_inc(&nr_comm_events);
>  	if (event->attr.namespaces)
> -- 
> 2.26.2
>