Message-ID: <CACAyw98xzFf-j8dy45U+90Y5FiPEvb9w7GS5UrQCnxWaZGAZUw@mail.gmail.com>
Date:   Tue, 15 Sep 2020 10:03:41 +0100
From:   Lorenz Bauer <lmb@...udflare.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>,
        Networking <netdev@...r.kernel.org>, Yonghong Song <yhs@...com>
Subject: Re: [PATCH RFC bpf-next 2/2] bpf: Enable bpf_skc_to_* sock casting
 helper to networking prog type

On Mon, 14 Sep 2020 at 20:43, Martin KaFai Lau <kafai@...com> wrote:
[...]
>
> For other ARG_PTR_TO_SOCK_COMMON helpers, they are not available to
> the tracing prog type.  Hence, they are fine to accept PTR_TO_BTF_ID
> as ARG_PTR_TO_SOCK_COMMON since the only way for non tracing prog to
> get a PTR_TO_BTF_ID is from casting helpers bpf_skc_to_* and
> the NULL check on return value must be done first.  If these
> ARG_PTR_TO_* helpers were ever made available to tracing prog,
> it might be better off to have another func_proto taking
> ARG_PTR_TO_BTF_ID instead.

I think such special cases increase the maintenance burden going
forward, I'd prefer it if we had one set of rules that applies to all
program types.

> For the verifier, I think the PTR_TO_BTF_ID should only be accepted
> as ARG_TO_* for non tracing program. That means the bpf_skc_to_*
> proto has to be duplicated to take ARG_PTR_TO_SOCK_COMMON.  I think
> that may be cleaner going forward.  Then the verifier does not need
> to worry about how to deal with what btf_id can be taken as fullsock
> ARG_PTR_TO_SOCKET.  The helper taking ARG_PTR_TO_BTF_ID will decide
> where it could be called from and see how it wants to treat
> "struct sock *sk".

So basically, we allow function prototypes to be specialised according
to context type?

> For example, the sk_storage_get_btf_proto
> is taking &btf_sock_ids[BTF_SOCK_TYPE_SOCK] and is only used from
> the LSM context that is holding a fullsock.

That is a tempting simplification, but it makes future extensions of
LSM context harder. Also, how could we enforce that we only have
fullsocks in LSM? By looking at the list of helpers? I worry that this
is very brittle.

>
> The same goes for the sock_map iter, how about the map_update
> and map_lookup use a ARG_PTR_TO_BTF_ID and PTR_TO_BTF_ID instead?
> For other prog types, they can keep using ARG_PTR_TO_SOCKET and
> PTR_TO_SOCKET.

Yeah, I've thought about that approach as well. The upside is that
it's a much more limited change, and therefore I know that it is
fairly safe to do. It relies on maps being able to override the
"common" map_lookup_elem function proto, which is something we already
do for sockmap and which could use some cleaning up. So if
PTR_TO_BTF_ID aliasing with ARG_PTR_TO_SOCK_COMMON doesn't work out
I'll propose this.

The downside of specialised function protos is that we'll have to do
it for every helper, context type, etc. To me it sounds like we want
to use BTF to define context objects going forward as much as
possible, so having a general solution to unify socket helpers across
old-style and BTF contexts seems really useful to me. If we introduce
ARG_PTR_TO_SOCK_COMMON_OR_NULL we can do this in a gradual way, by
changing helpers that we want to be cross-compatible to take the new
arg type and adding a few NULL and fullsock checks here and there.

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Powered by blists - more mailing lists