From mboxrd@z Thu Jan 1 00:00:00 1970 From: Petar Penkov Subject: Re: [bpf-next 1/3] flow_dissector: implements flow dissector BPF hook Date: Mon, 3 Sep 2018 13:54:18 -0700 Message-ID: References: <20180830182301.89435-1-peterpenkov96@gmail.com> <20180830182301.89435-2-peterpenkov96@gmail.com> <7eed9392-f260-942a-b32b-1cacd4a1f9f4@iogearbox.net> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: Petar Penkov , Networking , "David S . Miller" , Alexei Starovoitov , simon.horman@netronome.com, ecree@solarflare.com, songliubraving@fb.com, tom@herbertland.com, Willem de Bruijn To: Daniel Borkmann Return-path: Received: from mail-qk1-f195.google.com ([209.85.222.195]:41164 "EHLO mail-qk1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727199AbeIDBQQ (ORCPT ); Mon, 3 Sep 2018 21:16:16 -0400 Received: by mail-qk1-f195.google.com with SMTP id h138-v6so1078644qke.8 for ; Mon, 03 Sep 2018 13:54:19 -0700 (PDT) In-Reply-To: <7eed9392-f260-942a-b32b-1cacd4a1f9f4@iogearbox.net> Sender: netdev-owner@vger.kernel.org List-ID: On Sun, Sep 2, 2018 at 2:03 PM, Daniel Borkmann wrote: > On 08/30/2018 08:22 PM, Petar Penkov wrote: >> From: Petar Penkov >> >> Adds a hook for programs of type BPF_PROG_TYPE_FLOW_DISSECTOR and >> attach type BPF_FLOW_DISSECTOR that is executed in the flow dissector >> path. The BPF program is per-network namespace. >> >> Signed-off-by: Petar Penkov >> Signed-off-by: Willem de Bruijn > [...] >> + err = check_flow_keys_access(env, off, size); >> + if (!err && t == BPF_READ && value_regno >= 0) >> + mark_reg_unknown(env, regs, value_regno); >> } else { >> verbose(env, "R%d invalid mem access '%s'\n", regno, >> reg_type_str[reg->type]); >> @@ -1925,6 +1954,8 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno, >> case PTR_TO_PACKET_META: >> return check_packet_access(env, regno, reg->off, access_size, >> zero_size_allowed); >> + case PTR_TO_FLOW_KEYS: >> + return check_flow_keys_access(env, reg->off, access_size); >> case PTR_TO_MAP_VALUE: >> return check_map_access(env, regno, reg->off, access_size, >> zero_size_allowed); >> @@ -3976,6 +4007,7 @@ static bool may_access_skb(enum bpf_prog_type type) >> case BPF_PROG_TYPE_SOCKET_FILTER: >> case BPF_PROG_TYPE_SCHED_CLS: >> case BPF_PROG_TYPE_SCHED_ACT: >> + case BPF_PROG_TYPE_FLOW_DISSECTOR: >> return true; > > This one should not be added here. It would allow for LD_ABS to be used, but > you already have direct packet access as well as bpf_skb_load_bytes() helper > enabled. Downside on LD_ABS is that error path will exit the BPF prog with > return 0 for historical reasons w/o user realizing (here: to BPF_OK mapping). > So we should not encourage use of LD_ABS/IND anymore in eBPF context and > avoid surprises. > >> default: >> return false; >> @@ -4451,6 +4483,7 @@ static bool regsafe(struct bpf_reg_state *rold, struct bpf_reg_state *rcur, >> case PTR_TO_CTX: >> case CONST_PTR_TO_MAP: >> case PTR_TO_PACKET_END: >> + case PTR_TO_FLOW_KEYS: >> /* Only valid matches are exact, which memcmp() above >> * would have accepted >> */ >> diff --git a/net/core/filter.c b/net/core/filter.c >> index c25eb36f1320..0143b9c0c67e 100644 >> --- a/net/core/filter.c >> +++ b/net/core/filter.c >> @@ -5092,6 +5092,17 @@ sk_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) >> } >> } >> >> +static const struct bpf_func_proto * >> +flow_dissector_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) >> +{ >> + switch (func_id) { >> + case BPF_FUNC_skb_load_bytes: >> + return &bpf_skb_load_bytes_proto; > > Probably makes sense to also enable bpf_skb_pull_data helper for direct packet > access use to fetch non-linear data from here once. > >> + default: >> + return bpf_base_func_proto(func_id); >> + } >> +} >> + >> static const struct bpf_func_proto * >> lwt_out_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) >> { >> @@ -5207,6 +5218,7 @@ static bool bpf_skb_is_valid_access(int off, int size, enum bpf_access_type type >> case bpf_ctx_range(struct __sk_buff, data): >> case bpf_ctx_range(struct __sk_buff, data_meta): >> case bpf_ctx_range(struct __sk_buff, data_end): >> + case bpf_ctx_range(struct __sk_buff, flow_keys): >> if (size != size_default) >> return false; >> break; >> @@ -5235,6 +5247,7 @@ static bool sk_filter_is_valid_access(int off, int size, >> case bpf_ctx_range(struct __sk_buff, data): >> case bpf_ctx_range(struct __sk_buff, data_meta): >> case bpf_ctx_range(struct __sk_buff, data_end): >> + case bpf_ctx_range(struct __sk_buff, flow_keys): >> case bpf_ctx_range_till(struct __sk_buff, family, local_port): > [...] > Thanks, > Daniel Thank you for your feedback, Daniel! I'll make these changes and submit a v2. Petar