From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <netdev-owner@vger.kernel.org>
Received: from mail-pg0-f68.google.com ([74.125.83.68]:45715 "EHLO
        mail-pg0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751466AbeCOWUR (ORCPT
        <rfc822;netdev@vger.kernel.org>); Thu, 15 Mar 2018 18:20:17 -0400
Received: by mail-pg0-f68.google.com with SMTP id s13so3315262pgn.12
        for <netdev@vger.kernel.org>; Thu, 15 Mar 2018 15:20:16 -0700 (PDT)
Date: Thu, 15 Mar 2018 15:20:12 -0700
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: John Fastabend <john.fastabend@gmail.com>, 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: <20180315222010.kq5hcjsl33d3smho@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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <31bf7017-7456-1637-3a99-9630fc4d7009@iogearbox.net>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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.