linux-kernel - Re: [PATCH RESEND v5] perf/core: Fix installing arbitrary cgroup event into cpu

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:   Mon, 12 Mar 2018 19:46:52 +0800
From:   Lin Xiulei <linxiulei@...il.com>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Jiri Olsa <jolsa@...hat.com>, mingo@...hat.com,
        acme@...nel.org, alexander.shishkin@...ux.intel.com,
        linux-kernel@...r.kernel.org, tglx@...utronix.de,
        Stephane Eranian <eranian@...il.com>,
        torvalds@...ux-foundation.org, linux-perf-users@...r.kernel.org,
        Brendan Gregg <brendan.d.gregg@...il.com>,
        yang_oliver@...mail.com, "leilei.lin" <leilei.lin@...baba-inc.com>
Subject: Re: [PATCH RESEND v5] perf/core: Fix installing arbitrary cgroup
 event into cpu

2018-03-12 15:53 GMT+08:00 Ingo Molnar <mingo@...nel.org>:
>
> * linxiulei@...il.com <linxiulei@...il.com> wrote:
>
>>       /*
>>        * Because cgroup events are always per-cpu events,
>>        * this will always be called from the right CPU.
>>        */
>
>> +     /*
>> +      * if only the cgroup is running on this cpu
>> +      * and cpuctx->cgrp == NULL (otherwise it would've
>> +      * been set with running cgroup), we put this cgroup
>> +      * into cpu context. Or it would case mismatch in
>> +      * following cgroup events at event_filter_match()
>> +      */
>
> Beyond making sure that what you comment on makes sense, please also follow
> existing comment style, which I quoted above.
>
> There's 3 separate mistakes in that paragraph alone.
>
> Thanks,
>
>         Ingo

Thank Ingo for pointing it out. though I'd try to follow it as
possible as I could. I wish I have somewhere to check style with
specific documents : )

+       /*
+        * We will set cpuctx->cgrp only if:
+        *   1) cpuctx->cgrp is NULL. Because cpuctx->cgrp was set
+        *      correctly by sched_in(). We don't need to set it again.
+        *   2) cgroup related to this event is running on this CPU.
+        *      Otherwise it will fail in event_filter_match()
+        */

Peter, did I explain that logic clearly and prove it right?

Regards