From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <netdev-owner@vger.kernel.org>
Received: from mail-pl0-f53.google.com ([209.85.160.53]:46893 "EHLO
        mail-pl0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751714AbeCOV76 (ORCPT
        <rfc822;netdev@vger.kernel.org>); Thu, 15 Mar 2018 17:59:58 -0400
Received: by mail-pl0-f53.google.com with SMTP id f5-v6so4610158plj.13
        for <netdev@vger.kernel.org>; Thu, 15 Mar 2018 14:59:58 -0700 (PDT)
Date: Thu, 15 Mar 2018 14:59:55 -0700
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: John Fastabend <john.fastabend@gmail.com>
Cc: davem@davemloft.net, ast@kernel.org, daniel@iogearbox.net,
        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: <20180315215954.wufvwdhcjpntdxbb@ast-mbp>
References: <20180312192034.8039.70022.stgit@john-Precision-Tower-5810>
 <20180312192329.8039.75277.stgit@john-Precision-Tower-5810>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180312192329.8039.75277.stgit@john-Precision-Tower-5810>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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.

> +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.