linux-kernel - Re: [PATCH] Perf: bug fix: distinguish between rename and exec

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: <CABPqkBQkbWWyaEKt-Oyqetyd11jm9mL86oi5TJgYaqXLOo7rpQ@mail.gmail.com>
Date:	Fri, 2 Mar 2012 14:44:37 +0100
From:	Stephane Eranian <eranian@...gle.com>
To:	Arnaldo Carvalho de Melo <acme@...stprotocols.net>
Cc:	Luigi Semenzato <semenzato@...omium.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Paul Mackerras <paulus@...ba.org>, Ingo Molnar <mingo@...e.hu>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Vasiliy Kulikov <segoon@...nwall.com>,
	Stephen Wilson <wilsons@...rt.ca>,
	Oleg Nesterov <oleg@...hat.com>, Tejun Heo <tj@...nel.org>,
	Paul Gortmaker <paul.gortmaker@...driver.com>,
	Andi Kleen <ak@...ux.intel.com>,
	Lucas De Marchi <lucas.demarchi@...fusion.mobi>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	Frederic Weisbecker <fweisbec@...il.com>,
	David Ahern <dsahern@...il.com>,
	Namhyung Kim <namhyung@...il.com>,
	Robert Richter <robert.richter@....com>,
	linux-kernel@...r.kernel.org, sonnyrao@...omium.org,
	olofj@...omium.org
Subject: Re: [PATCH] Perf: bug fix: distinguish between rename and exec

Did we come to an agreement on this problem?

Seems like we do want to distinguish exec from renames, as perf
David's example with top.
In Luigi's patch, the distinction is made via the header->misc
bitmask. Now, I see people have
proposed a new record type: PERF_RECORD_COMM vs. PERF_RECORD_EXEC. This would
also work but it would require a lot more changes in the tool, i.e.,
you have to process a new
record type. What's wrong with the header->misc approach?


On Wed, Feb 15, 2012 at 6:47 PM, Arnaldo Carvalho de Melo
<acme@...stprotocols.net> wrote:
> Em Wed, Feb 15, 2012 at 09:07:47AM -0800, Luigi Semenzato escreveu:
>> On Wed, Feb 15, 2012 at 5:47 AM, Arnaldo Carvalho de Melo
>> <acme@...stprotocols.net> wrote:
>> > Em Wed, Feb 15, 2012 at 01:48:33PM +0100, Peter Zijlstra escreveu:
>> >> I really dislike changing generic code purely for the purpose of
>> >> instrumentation like this. Better to pull perf_event_comm() out of here
>> >> if you want to change semantics.
>> >>
>> >> Personally I couldn't care less about renames, I think they're a waste
>> >> of time, so I'm ok with the simple patch moving the perf_event_comm()
>> >> into setup_new_exec() and possibly renaming it to perf_event_exec().
>> >>
>> >> Acme, do you care about renames?
>> >
>> > I like your idea of keeping the semantics of PERF_RECORD_COMM and
>> > introducing a PERF_RECORD_EXEC, just have to think about how to handle
>> > that in a way that the tools detect that we have PERF_RECORD_EXEC...
>>
>> I considered this but I don't know how important it is to be backward
>> compatible.  Adding a new record type makes old "perf report" fail to
>> parse new perf.data files.  (Unless we pad the new record to a
>
> Hey, old perf record would still see the PERF_RECORD_COMM, i.e. we would
> continue asking for PERF_RECORD_COMM in new versions. Together with
> PERF_RECORD_EXEC.
>
>> multiple of 8 bytes, but I don't think we want to go down that path).
>>
>> If looking forward is more important, I agree a new new record type is
>> best.  We might want to consider adding a PERF_RECORD_RENAME for
>
> PERF_RECORD_COMM is good enough, well, it always was confusing for most
> people that asked "hey, that means an EXEC, right?"
>
> First thing pople think is "hey, this is when it sets the thread COMM,
> right?"
>
>> renames, and leaving the COMM record to its historical meaning (exec),
>> possibly renaming it to PERF_RECORD_EXEC for clarity.  And yes, the
>> perf instrumentation should not be in set_task_comm(), that's why the
>> bug exists in the first place.
>>
>> We might also want to change the parsing of perf.data so that in the
>> future it is more tolerant of new record types.
>
> Yes, what is the behaviour now? Lemme see... Well, difficult, I'm barely
> reading this, just after magnifying it, dilated pupils two hours ago,
> grrr
>
> Wiĺl check later, but IIRC it just warns and skips the record, right?
>
>> > Humm, will be yet another fallback for setting an perf_event_attr bit,
>> > just like with .sample_id_all and .exclude_{guest,host}...
>> >
>> > That together with the per class errnos + __strerror() method will allow
>> > to move all the event creation finally to perf_evlist__open() where all
>> > this gets nicely hidden away from poor tools.
>> >
>> > We can then even have an ui__evlist_perror() method that does all the
>> > ui__warning calls, etc.
>> >
>> > So, yes, from a tooling perspective, I want to be notified of renames
>> > and being able to stop relying on PERF_RECORD_COMM to call
>> > map_groups__flush and instead do it at PERF_RECORD_EXEC seems a
>> > bonus.
>> >
>> > - Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/