netdev - Re: [PATCH v5 perf, bpf-next 1/7] perf, bpf: Introduce PERF_RECORD

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:   Tue, 8 Jan 2019 18:56:19 +0000
From:   Song Liu <songliubraving@...com>
To:     Peter Zijlstra <peterz@...radead.org>
CC:     lkml <linux-kernel@...r.kernel.org>,
        Netdev <netdev@...r.kernel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH v5 perf, bpf-next 1/7] perf, bpf: Introduce
 PERF_RECORD_KSYMBOL

Thanks Peter!

> On Jan 8, 2019, at 10:31 AM, Peter Zijlstra <peterz@...radead.org> wrote:
> 
> On Thu, Dec 20, 2018 at 10:28:58AM -0800, Song Liu wrote:
> 
>> @@ -648,11 +649,18 @@ struct perf_event_mmap_page {
>>  *   PERF_RECORD_MISC_COMM_EXEC  - PERF_RECORD_COMM event
>>  *   PERF_RECORD_MISC_FORK_EXEC  - PERF_RECORD_FORK event (perf internal)
>>  *   PERF_RECORD_MISC_SWITCH_OUT - PERF_RECORD_SWITCH* events
>> + *   PERF_RECORD_MISC_KSYMBOL_*  - PERF_RECORD_KSYMBOL event
>>  */
>> #define PERF_RECORD_MISC_MMAP_DATA		(1 << 13)
>> #define PERF_RECORD_MISC_COMM_EXEC		(1 << 13)
>> #define PERF_RECORD_MISC_FORK_EXEC		(1 << 13)
>> #define PERF_RECORD_MISC_SWITCH_OUT		(1 << 13)
>> +
>> +#define PERF_RECORD_MISC_KSYMBOL_UNREGISTER	(1 << 3)
>> +#define PERF_RECORD_MISC_KSYMBOL_TYPE_MASK	(7 << 4)
>> +#define PERF_RECORD_MISC_KSYMBOL_TYPE_UNKNOWN	(0 << 4)
>> +#define PERF_RECORD_MISC_KSYMBOL_TYPE_BPF	(1 << 4)
> 
> So this gives us 8 possible types, of which 2 are claimed.
> 
> I suppose we already know of FTRACE_TRAMPOLINE and MODULE, which
> accounts for half the space.
> 
>> +
>> /*
>>  * These PERF_RECORD_MISC_* flags below are safely reused
>>  * for the following events:
>> @@ -965,6 +973,19 @@ enum perf_event_type {
>> 	 */
>> 	PERF_RECORD_NAMESPACES			= 16,
>> 
>> +	/*
>> +	 * Record ksymbol register/unregister events:
>> +	 *
>> +	 * struct {
>> +	 *	struct perf_event_header	header;
>> +	 *	u64				addr;
>> +	 *	u64				len;
>> +	 *	char				name[];
>> +	 *	struct sample_id		sample_id;
>> +	 * };
>> +	 */
>> +	PERF_RECORD_KSYMBOL			= 17,
> 
> Do we really need u64 length? That is, are we willing to consider
> symbols larger than 4G ?
> 
> Could we not reclaim the top 32 or 16 bits thereof for that type thing
> instead of using a few misc bits?

I don't think we need u64 length. 32 bit should be more than enough. 

How about we revise it as:

+       /*
+        * Record ksymbol register/unregister events:
+        *
+        * struct {
+        *      struct perf_event_header        header;
+        *      u64                             addr;
+        *      u32                             len;
+        *      u16                             type;
+        *      u16                             reserved;
+        *      char                            name[];
+        *      struct sample_id                sample_id;
+        * };
+        */
+       PERF_RECORD_KSYMBOL                     = 17,

[...]

+#define PERF_RECORD_MISC_KSYMBOL_TYPE_UNKNOWN  0
+#define PERF_RECORD_MISC_KSYMBOL_TYPE_BPF      1
+#define PERF_RECORD_MISC_KSYMBOL_TYPE_MODULE   2
+#define PERF_RECORD_MISC_KSYMBOL_TYPE_FTRACE_TRAMPOLINE      3

Thanks again,
Song