linux-kernel - Re: [PATCH v2] perf inject --jit: Remove //anon mmap events

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] [day] [month] [year] [list]

Message-ID: <20191031175902.GB15593@kernel.org>
Date:   Thu, 31 Oct 2019 14:59:02 -0300
From:   Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
To:     Steve MacLean <steve.maclean@...ux.microsoft.com>
Cc:     Steve MacLean <Steve.MacLean@...rosoft.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Stephane Eranian <eranian@...gle.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] perf inject --jit: Remove //anon mmap events

Em Wed, Oct 30, 2019 at 06:05:12PM -0700, Steve MacLean escreveu:
> From: Steve MacLean <Steve.MacLean@...rosoft.com>
> @@ -749,6 +750,34 @@ static int jit_repipe_debug_info(struct jit_buf_desc *jd, union jr_entry *jr)
>  	return 0;
>  }
>  
> +static void jit_add_pid(struct machine *machine, pid_t pid)
> +{
> +	struct thread *thread = machine__findnew_thread(machine, pid, pid);
> +
> +	if (!thread)
> +	{
> +		pr_err("jit_add_pid() thread not found\n");
> +
> +		return;
> +	}


Can you please follow the coding style used in tools/? Its mostly the
same as for the kernel sources.

I.e. above you need to have it as:

	if (!thread) {
		pr_err("jit_add_pid() thread not found\n");
		return;
	}

There also consider using __funct__ and showing the pid that wasn't
found.

> +
> +	thread->priv = (void *) 1;

No space after the cast.

> +}
> +
> +static bool jit_has_pid(struct machine *machine, pid_t pid)
> +{
> +	struct thread *thread = machine__findnew_thread(machine, pid, pid);
> +
> +	if (!thread)
> +	{
> +		pr_err("jit_has_pid() thread not found\n");
> +
> +		return 0;
> +	}
> +
> +	return (bool) thread->priv;

Same style problems and the only way for machine__findnew_thread() to
fail is if the system is under severe memory exhaustion, what you
probably want to use here is machine__find_thread(), that will fail if
the pid isn't found, the findnew methods, in the machine class or
elsewhere will return an existing thread _or_ create and insert it, then
return the newly added/inserted object.

> +}
> +
>  int
>  jit_process(struct perf_session *session,
>  	    struct perf_data *output,
> @@ -765,7 +794,15 @@ static int jit_repipe_debug_info(struct jit_buf_desc *jd, union jr_entry *jr)
>  	 * first, detect marker mmap (i.e., the jitdump mmap)
>  	 */
>  	if (jit_detect(filename, pid))
> +	{

if () {

> +		/*
> +		 * Strip //anon* mmaps if we processed a jitdump for this pid
> +		 */

// can be used, not strictly required tho, the way you used is
acceptable.

> +		if (jit_has_pid(machine, pid) && (strncmp(filename, "//anon", 6) == 0))
> +			return 1;
> +
>  		return 0;
> +	}
>  
>  	memset(&jd, 0, sizeof(jd));
>  
> @@ -784,6 +821,7 @@ static int jit_repipe_debug_info(struct jit_buf_desc *jd, union jr_entry *jr)
>  
>  	ret = jit_inject(&jd, filename);
>  	if (!ret) {
> +		jit_add_pid(machine, pid);
>  		*nbytes = jd.bytes_written;
>  		ret = 1;
>  	}