From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f195.google.com ([209.85.192.195]:33394 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932539AbeCOXGL (ORCPT ); Thu, 15 Mar 2018 19:06:11 -0400 Received: by mail-pf0-f195.google.com with SMTP id q13so3437391pff.0 for ; Thu, 15 Mar 2018 16:06:11 -0700 (PDT) Date: Thu, 15 Mar 2018 16:06:07 -0700 From: Alexei Starovoitov To: Daniel Borkmann Cc: John Fastabend , davem@davemloft.net, ast@kernel.org, davejwatson@fb.com, netdev@vger.kernel.org Subject: Re: [bpf-next PATCH v2 05/18] bpf: create tcp_bpf_ulp allowing BPF to monitor socket TX/RX data Message-ID: <20180315230605.vndmzwxso57puskx@ast-mbp> References: <20180312192034.8039.70022.stgit@john-Precision-Tower-5810> <20180312192329.8039.75277.stgit@john-Precision-Tower-5810> <20180315215954.wufvwdhcjpntdxbb@ast-mbp> <31bf7017-7456-1637-3a99-9630fc4d7009@iogearbox.net> <20180315222010.kq5hcjsl33d3smho@ast-mbp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Thu, Mar 15, 2018 at 11:55:39PM +0100, Daniel Borkmann wrote: > On 03/15/2018 11:20 PM, Alexei Starovoitov wrote: > > On Thu, Mar 15, 2018 at 11:17:12PM +0100, Daniel Borkmann wrote: > >> 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? > > > > there is no 32-bit user space. these structs are seen by bpf progs only > > and bpf is 64-bit only too. > > unless I'm missing your point. > > Ok, so lets say you have 32 bit LLVM binary and compile the prog where > you access md->data_end. Given the void * in the struct will that access > end up being BPF_W at ctx offset 4 or BPF_DW at ctx offset 8 from clang > perspective (iow, is the back end treating this special and always use > fixed BPF_DW in such case)? If not and it would be the first case with > offset 4, then we could have the case that underlying 64 bit kernel is > expecting ctx offset 8 for doing the md ctx conversion. i'm still not quite following. Whether llvm itself is 32-bit binary or it's arm32 or sprac32 binary doesn't matter. It will produce the same 64-bit bpf code. It will see 'void *' deref from this struct and will emit DW. May be confusion is from newly added -mattr=+alu32 flag? That option doesn't change that sizeof(void*)==8. It only allows backend to emit 32-bit alu insns.