Re: [PATCH bpf-next v3 1/7] bpf, sockmap: add BPF_F_PERMANENT flag for skmsg redirect

[Jakub Sitnicki <jakub@cloudflare.com>]

On Fri, Aug 25, 2023 at 06:32 PM -07, John Fastabend wrote:
> Jakub Sitnicki wrote:

[...]

>> But as I wrote earlier, I don't think it's a good idea to ignore the
>> flag. We can detect this conflict at the time the bpf_msg_sk_redirect_*
>> helper is called and return an error.
>> 
>> Naturally that means that that bpf_msg_{cork,apply}_bytes helpers need
>> to be adjusted to return an error if BPF_F_PERMANENT has been set.
>
> So far we've not really done much to protect a user from doing
> rather silly things. The following will all do something without
> errors,
>
>   bpf_msg_apply_bytes()
>   bpf_msg_apply_bytes() <- reset apply bytes
>
>   bpf_msg_cork_bytes()
>   bpf_msg_cork_bytes() <- resets cork byte
>
> also,
>
>   bpf_msg_redirect(..., BPF_F_INGRESS);
>   bpf_msg_redirect(..., 0); <- resets sk_redir and flags
>
> maybe there is some valid reason to even do above if further parsing
> identifies some reason to redirect to a alert socket or something.
>
> My original thinking was in the interest of not having a bunch of
> extra checks for performance reasons we shouldn't add guard rails
> unless something really unexpected might happen like a kernel
> panic or what not.
>
> This does feel a bit different though because before we
> didn't have calls that could impact other calls. My best idea
> is to just create a precedence and follow it. I would propose,
>
> 'If BPF_F_PERMANENT is set apply_bytes and cork_bytes are
>  ignored.'
>
> The other direction (what is above?) has a bit of an inconsistency
> where these two flows are different?
>
>   bpf_apply_bytes()
>   bpf_msg_redirect(..., BPF_F_PERMANENT)
>
> and
>
>   bpf_msg_redirect(..., BPF_F_PERMANENT)
>   bpf_apply_bytes()
>
> It would be best if order of operations doesn't change the
> outcome because that starts to get really hard to reason about.
>
> This avoids having to add checks all over the place and then
> if users want we could give some mechanisms to read apply
> and cork bytes so people could write macros over those if
> they really want the hard error.
>
> WDYT?

These semantics sound sane to me. Easy to explain:

BPF_F_PERMANENT takes precedence over apply/cork_bytes.

Good point about order of operations.