Message-ID: <20200914194304.4ccb6n5sdcfkzxcp@kafai-mbp>
Date:   Mon, 14 Sep 2020 12:43:04 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     Lorenz Bauer <lmb@...udflare.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, Sep 14, 2020 at 10:26:18AM +0100, Lorenz Bauer wrote:
> On Sat, 12 Sep 2020 at 05:59, Martin KaFai Lau <kafai@...com> wrote:
> >
> > There is a constant need to add more fields into the bpf_tcp_sock
> > for the bpf programs running at tc, sock_ops...etc.
> >
> > A current workaround could be to use bpf_probe_read_kernel().  However,
> > other than making another helper call for reading each field and missing
> > CO-RE, it is also not as intuitive to use as directly reading
> > "tp->lsndtime" for example.  While already having perfmon cap to do
> > bpf_probe_read_kernel(), it will be much easier if the bpf prog can
> > directly read from the tcp_sock.
> >
> > This patch tries to do that by using the existing casting-helpers
> > bpf_skc_to_*() whose func_proto returns a btf_id.  For example, the
> > func_proto of bpf_skc_to_tcp_sock returns the btf_id of the
> > kernel "struct tcp_sock".
> >
> > [ One approach is to make a separate copy of the bpf_skc_to_*
> >   func_proto and use ARG_PTR_TO_SOCK_COMMON instead of ARG_PTR_TO_BTF_ID.
> >   More on this later (1). ]
> >
> > This patch modifies the existing bpf_skc_to_* func_proto to take
> > ARG_PTR_TO_SOCK_COMMON instead of taking
> > "ARG_PTR_TO_BTF_ID + &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON]".
> > That will allow tc, sock_ops,...etc to call these casting helpers
> > because they already hold the PTR_TO_SOCK_COMMON (or its
> > equivalent).  For example:
> >
> >         sk = sock_ops->sk;
> >         if (!sk)
> >                 return;
> >         tp = bpf_skc_to_tcp_sock(sk);
> >         if (!tp)
> >                 return;
> >         /* Read tp as a PTR_TO_BTF_ID */
> >         lsndtime = tp->lsndtime;
> >
> > To ensure the current bpf prog passing a PTR_TO_BTF_ID to
> > bpf_skc_to_*() still works as is, the verifier is modified such that
> > ARG_PTR_TO_SOCK_COMMON can accept a reg with reg->type == PTR_TO_BTF_ID
> > and reg->btf_id is btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON]
> >
> > To do that, an idea is borrowed from one of the Lorenz's patch:
> > https://lore.kernel.org/bpf/20200904112401.667645-12-lmb@cloudflare.com/ .
> > It adds PTR_TO_BTF_ID as one of the acceptable reg->type for
> > ARG_PTR_TO_SOCK_COMMON and also specifies what btf_id it can take.
> > By doing this, the bpf_skc_to_* will work as before and can still
> > take PTR_TO_BTF_ID as the arg.  e.g. The bpf tcp iter will work
> > as is.
> >
> > This will also make other existing helper taking ARG_PTR_TO_SOCK_COMMON
> > works with the pointer obtained from bpf_skc_to_*(). For example:
> 
> Unfortunately, I think that we need to introduce a new
> ARG_PTR_TO_SOCK_COMMON_OR_NULL for this to work. This is because
> dereferencing a "tracing" pointer can yield NULL at runtime due to a
> page fault, so the helper has to deal with this. Other than that I
> think this is a really nice approach: we can gradually move helpers to
> PTR_TO_SOCK_COMMON_OR_NULL and in doing so make them compatible with
> BTF pointers.
Good point on the sk could be NULL.  I think the existing
bpf_skc_to_*() helper is missing the "!sk" check regardless of
this patch.

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.

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

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.


> 
> [...]
> 
> > @@ -4014,7 +4022,17 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 arg,
> >
> >  found:
> >         if (type == PTR_TO_BTF_ID) {
> > -               u32 *expected_btf_id = fn->arg_btf_id[arg];
> > +               u32 *expected_btf_id;
> > +
> > +               if (arg_type == ARG_PTR_TO_BTF_ID) {
> > +                       expected_btf_id = fn->arg_btf_id[arg];
> 
> Personal preference, but what do you think about moving this to after
> the assignment of compatible?
> 
>     btf_id = compatible->btf_id;
>     if (arg_type == ARG_PTR_TO_BTF_ID)
>        btf_id = fn->fn->arg_btf_id[arg];
> 
> That makes it clearer that we have to special case ARG_PTR_TO_BTF_ID since
> it doesn't make sense to use compatible->btf_id in that case.
Sure.  I don't mind moving this assignment out of the else.
However, I think resorting to "compatible" btf_id is the exception
instead.

Powered by blists - more mailing lists