linux-kernel - Re: [PATCH v6 1/6] perf/cgroup: Reorder perf_cgroup

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]

Date:   Fri, 14 Feb 2020 09:11:24 -0700
From:   Shuah Khan <skhan@...uxfoundation.org>
To:     Ian Rogers <irogers@...gle.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Marco Elver <elver@...gle.com>,
        Kent Overstreet <kent.overstreet@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Ard Biesheuvel <ardb@...nel.org>,
        Gary Hook <Gary.Hook@....com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        linux-kernel@...r.kernel.org
Cc:     Stephane Eranian <eranian@...gle.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH v6 1/6] perf/cgroup: Reorder perf_cgroup_connect()

On 2/14/20 12:51 AM, Ian Rogers wrote:
> From: Peter Zijlstra <peterz@...radead.org>
> 
> Move perf_cgroup_connect() after perf_event_alloc(), such that we can
> find/use the PMU's cpu context.

Can you elaborate on this usage? It will helpful to know how
this is used and what do we get from it. What were we missing
with the way it was done before?


> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> Signed-off-by: Ian Rogers <irogers@...gle.com>
> ---
>   kernel/events/core.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 3f1f77de7247..9bd2af954c54 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -10804,12 +10804,6 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
>   	if (!has_branch_stack(event))
>   		event->attr.branch_sample_type = 0;
>   
> -	if (cgroup_fd != -1) {
> -		err = perf_cgroup_connect(cgroup_fd, event, attr, group_leader);
> -		if (err)
> -			goto err_ns;
> -	}
> -
>   	pmu = perf_init_event(event);
>   	if (IS_ERR(pmu)) {
>   		err = PTR_ERR(pmu);
> @@ -10831,6 +10825,12 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
>   		goto err_pmu;

Is this patch based on linux-next or linux 5.6-rc1. I am finding code
path to be different in those. Also in 
https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git

goto err_ns makes more sense since if perf_init_event() doesn't return
valid pmu, especially since err_pmu tries to do a put pmu->module.

Something doesn't look right.


>   	}


>   
> +	if (cgroup_fd != -1) {
> +		err = perf_cgroup_connect(cgroup_fd, event, attr, group_leader);
> +		if (err)
> +			goto err_pmu;
> +	}
> +
>   	err = exclusive_event_init(event);
>   	if (err)
>   		goto err_pmu;
> @@ -10891,12 +10891,12 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
>   	exclusive_event_destroy(event);
>   
>   err_pmu:
> +	if (is_cgroup_event(event))
> +		perf_detach_cgroup(event);
>   	if (event->destroy)
>   		event->destroy(event);
>   	module_put(pmu->module);
>   err_ns:
> -	if (is_cgroup_event(event))
> -		perf_detach_cgroup(event);
>   	if (event->ns)
>   		put_pid_ns(event->ns);
>   	if (event->hw.target)
> 

thanks,
-- Shuah