From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from www62.your-server.de ([213.133.104.62]:33375 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751497AbeCOWRR (ORCPT ); Thu, 15 Mar 2018 18:17:17 -0400 Subject: Re: [bpf-next PATCH v2 05/18] bpf: create tcp_bpf_ulp allowing BPF to monitor socket TX/RX data To: Alexei Starovoitov , John Fastabend Cc: davem@davemloft.net, ast@kernel.org, davejwatson@fb.com, netdev@vger.kernel.org References: <20180312192034.8039.70022.stgit@john-Precision-Tower-5810> <20180312192329.8039.75277.stgit@john-Precision-Tower-5810> <20180315215954.wufvwdhcjpntdxbb@ast-mbp> From: Daniel Borkmann Message-ID: <31bf7017-7456-1637-3a99-9630fc4d7009@iogearbox.net> Date: Thu, 15 Mar 2018 23:17:12 +0100 MIME-Version: 1.0 In-Reply-To: <20180315215954.wufvwdhcjpntdxbb@ast-mbp> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org List-ID: On 03/15/2018 10:59 PM, Alexei Starovoitov wrote: > On Mon, Mar 12, 2018 at 12:23:29PM -0700, John Fastabend wrote: >> >> +/* User return codes for SK_MSG prog type. */ >> +enum sk_msg_action { >> + SK_MSG_DROP = 0, >> + SK_MSG_PASS, >> +}; > > do we really need new enum here? > It's the same as 'enum sk_action' and SK_DROP == SK_MSG_DROP > and there will be only drop/pass in both enums. > Also I don't see where these two new SK_MSG_* are used... > >> + >> +/* user accessible metadata for SK_MSG packet hook, new fields must >> + * be added to the end of this structure >> + */ >> +struct sk_msg_md { >> + __u32 data; >> + __u32 data_end; >> +}; > > I think it's time for me to ask for forgiveness :) :-) > I used __u32 for data and data_end only because all other fields > in __sk_buff were __u32 at the time and I couldn't easily figure out > how to teach verifier to recognize 8-byte rewrites. > Unfortunately my mistake stuck and was copied over into xdp. > Since this is new struct let's do it right and add > 'void *data, *data_end' here, > since bpf prog will use them as 'void *' pointers. > There are no compat issues here, since bpf is always 64-bit. But at least offset-wise when you do the ctx rewrite this would then be a bit more tricky when you have 64 bit kernel with 32 bit user space since void * members are in each cases at different offset. So unless I'm missing something, this still should either be __u32 or __u64 instead of void *, no? >> +static int bpf_map_msg_verdict(int _rc, struct sk_msg_buff *md) >> +{ >> + return ((_rc == SK_PASS) ? >> + (md->map ? __SK_REDIRECT : __SK_PASS) : >> + __SK_DROP); > > you're using old SK_PASS here too ;) > that's to my point of not adding SK_MSG_PASS... > > Overall the patch set looks absolutely great. > Thank you for working on it. +1