netdev - Re: [PATCH net-next v2 1/2] bpf/tracing: allow user space to query prog array on the same tp

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: <6f064613-0b18-0b70-0061-fa012040b7b9@fb.com>
Date:   Wed, 6 Dec 2017 10:37:57 -0800
From:   Yonghong Song <yhs@...com>
To:     Peter Zijlstra <peterz@...radead.org>
CC:     <rostedt@...dmis.org>, <ast@...com>, <daniel@...earbox.net>,
        <kafai@...com>, <netdev@...r.kernel.org>, <kernel-team@...com>
Subject: Re: [PATCH net-next v2 1/2] bpf/tracing: allow user space to query
 prog array on the same tp



On 12/6/17 5:16 AM, Peter Zijlstra wrote:
> On Wed, Dec 06, 2017 at 12:56:36PM +0100, Peter Zijlstra wrote:
>> On Tue, Dec 05, 2017 at 10:31:28PM -0800, Yonghong Song wrote:
>>> Commit e87c6bc3852b ("bpf: permit multiple bpf attachments
>>> for a single perf event") added support to attach multiple
>>> bpf programs to a single perf event.
>>> Commit 2541517c32be ("tracing, perf: Implement BPF programs
>>> attached to kprobes") utilized the existing perf ioctl
>>> interface and added the command PERF_EVENT_IOC_SET_BPF
>>> to attach a bpf program to a tracepoint.
>>>
>>> This patch adds a new ioctl
>>> command, given a perf event fd, to query the bpf program array
>>> attached to the same perf tracepoint event.
>>>
>>> The new uapi ioctl command:
>>>    PERF_EVENT_IOC_QUERY_BPF
>>>
>>> The new uapi/linux/perf_event.h structure:
>>>    struct perf_event_query_bpf {
>>>         __u64	prog_ids;
>>>         __u32	prog_cnt;
>>>    };
>>>
>>> The usage:
>>>    struct perf_event_query_bpf query;
>>>    query.prog_ids = (__u64)usr_prog_ids_buf;
>>>    query.prog_cnt = usr_prog_ids_buf_len;
>>>    err = ioctl(pmu_efd, PERF_EVENT_IOC_QUERY_BPF, &query);
>>>
>>> Signed-off-by: Yonghong Song <yhs@...com>
>>> Acked-by: Alexei Starovoitov <ast@...nel.org>
>>
>> Can you please fix that example to make it clear that prog_ids is in
>> fact a pointer to an array of size prog_cnt. Ideally also describing
>> what the type of array is.

Right. Will address this with more descriptions in the commit message
and also add some comments in the perf_event.h.

>>
>> In fact, would not something like:
>>
>> 	struct perf_event_query_bpf {
>> 		__u32 len;
>> 		__u32 __reserved;
> 
> I suppose we could use this field to store the number of entries
> returned, retaining the len to indicate how large the structure is.
> 
>> 		__u64 ids[0];
>> 	};
>>
>> be a much clearer interface?

Yes, this is clearer and may be consistent with perf interface.

FYI, my old interface is similar to the BPF query interface below:
         struct { /* anonymous struct used by BPF_PROG_QUERY command */
                 __u32           target_fd;      /* container object to 
query */
                 __u32           attach_type;
                 __u32           query_flags;
                 __u32           attach_flags;
                 __aligned_u64   prog_ids;
                 __u32           prog_cnt;
         } query;


>>
>> Also, you forgot to tell us why we need this interface at all.

Right. Will add some descriptions for this.