From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932415AbcH3APZ (ORCPT ); Mon, 29 Aug 2016 20:15:25 -0400 Received: from www62.your-server.de ([213.133.104.62]:35807 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755695AbcH3APX (ORCPT ); Mon, 29 Aug 2016 20:15:23 -0400 Message-ID: <57C4CFFC.7010006@iogearbox.net> Date: Tue, 30 Aug 2016 02:14:52 +0200 From: Daniel Borkmann User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Alexei Starovoitov , "David S . Miller" CC: Peter Zijlstra , Brendan Gregg , Arnaldo Carvalho de Melo , Wang Nan , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-team@fb.com Subject: Re: [PATCH net-next 2/6] bpf: introduce BPF_PROG_TYPE_PERF_EVENT program type References: <1472265084-1767670-1-git-send-email-ast@fb.com> <1472265084-1767670-3-git-send-email-ast@fb.com> In-Reply-To: <1472265084-1767670-3-git-send-email-ast@fb.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Authenticated-Sender: daniel@iogearbox.net Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/27/2016 04:31 AM, Alexei Starovoitov wrote: > Introduce BPF_PROG_TYPE_PERF_EVENT programs that can be attached to > HW and SW perf events (PERF_TYPE_HARDWARE and PERF_TYPE_SOFTWARE > correspondingly in uapi/linux/perf_event.h) > > The program visible context meta structure is > struct bpf_perf_event_data { > struct pt_regs regs; > __u64 sample_period; > }; > which is accessible directly from the program: > int bpf_prog(struct bpf_perf_event_data *ctx) > { > ... ctx->sample_period ... > ... ctx->regs.ip ... > } > > The bpf verifier rewrites the accesses into kernel internal > struct bpf_perf_event_data_kern which allows changing > struct perf_sample_data without affecting bpf programs. > New fields can be added to the end of struct bpf_perf_event_data > in the future. > > Signed-off-by: Alexei Starovoitov Two things I noticed below, otherwise for BPF bits: Acked-by: Daniel Borkmann [...] > > +static bool pe_prog_is_valid_access(int off, int size, enum bpf_access_type type, > + enum bpf_reg_type *reg_type) > +{ > + if (off < 0 || off >= sizeof(struct bpf_perf_event_data)) > + return false; > + if (type != BPF_READ) > + return false; > + if (off % size != 0) > + return false; > + if (off == offsetof(struct bpf_perf_event_data, sample_period) && > + size != sizeof(u64)) > + return false; > + if (size != sizeof(long)) > + return false; Wouldn't this one rather need to be: if (off == offsetof(struct bpf_perf_event_data, sample_period) { if (size != sizeof(u64)) return false; } else { if (size != sizeof(long)) return false; } Otherwise on 32bit accessing sample_period might fail? > + return true; > +} > + > +static u32 pe_prog_convert_ctx_access(enum bpf_access_type type, int dst_reg, > + int src_reg, int ctx_off, > + struct bpf_insn *insn_buf, > + struct bpf_prog *prog) > +{ > + struct bpf_insn *insn = insn_buf; > + > + switch (ctx_off) { > + case offsetof(struct bpf_perf_event_data, sample_period): Would be good to add a test as we usually have done: BUILD_BUG_ON(FIELD_SIZEOF(struct perf_sample_data, period) != 8); > + *insn++ = BPF_LDX_MEM(bytes_to_bpf_size(FIELD_SIZEOF(struct bpf_perf_event_data_kern, data)), > + dst_reg, src_reg, > + offsetof(struct bpf_perf_event_data_kern, data)); > + *insn++ = BPF_LDX_MEM(BPF_DW, dst_reg, dst_reg, > + offsetof(struct perf_sample_data, period)); > + break; > + default: > + *insn++ = BPF_LDX_MEM(bytes_to_bpf_size(FIELD_SIZEOF(struct bpf_perf_event_data_kern, regs)), > + dst_reg, src_reg, > + offsetof(struct bpf_perf_event_data_kern, regs)); > + *insn++ = BPF_LDX_MEM(bytes_to_bpf_size(sizeof(long)), > + dst_reg, dst_reg, ctx_off); > + break; > + } > + return insn - insn_buf; > +} > +