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.
