Query on reads being flagged as direct writes...

Query on reads being flagged as direct writes...

[Maciej Żenczykowski]

From kernel/bpf/verifier.c with some simplifications (removed some of
the cases to make this shorter):

static bool may_access_direct_pkt_data(struct bpf_verifier_env *env,
const struct bpf_call_arg_meta *meta, enum bpf_access_type t)
{
  enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
  switch (prog_type) {
    /* Program types only with direct read access go here! */
    case BPF_PROG_TYPE_CGROUP_SKB: (and some others)
      if (t == BPF_WRITE) return false;
      fallthrough;
    /* Program types with direct read + write access go here! */
    case BPF_PROG_TYPE_SCHED_CLS: (and some others)
      if (meta) return meta->pkt_access;
      env->seen_direct_write = true;
      return true;
    case BPF_PROG_TYPE_CGROUP_SOCKOPT:
      if (t == BPF_WRITE) env->seen_direct_write = true;
      return true;
  }
}

why does the above set env->seen_direct_write to true even when t !=
BPF_WRITE, even for programs that only allow (per the comment) direct
read access.

Is this working correctly?  Is there some gotcha this is papering over?

Should 'env->seen_direct_write = true; return true;' be changed into
'fallthrough' so that write is only set if t == BPF_WRITE?

This matters because 'env->seen_direct_write = true' then triggers an
unconditional unclone in the bpf prologue, which I'd like to avoid
unless I actually need to modify the packet (with
bpf_skb_store_bytes)...

may_access_direct_pkt_data() has two call sites, in one it only gets
called with BPF_WRITE so it's ok, but the other one is in
check_func_arg():

if (type_is_pkt_pointer(type) && !may_access_direct_pkt_data(env,
meta, BPF_READ)) { verbose(env, "helper access to the packet is not
allowed\n"); return -EACCES; }

and I'm not really following what this does, but it seems like bpf
helper read access to the packet triggers unclone?

(side note: all packets ingressing from the rndis gadget driver are
clones due to how it deals with usb packet deaggregation [not to be
mistaken with lro/tso])

Confused...

Re: Query on reads being flagged as direct writes...

[Stanislav Fomichev]

On Fri, Aug 12, 2022 at 5:06 AM Maciej Żenczykowski
<zenczykowski@gmail.com> wrote:
>
> From kernel/bpf/verifier.c with some simplifications (removed some of
> the cases to make this shorter):
>
> static bool may_access_direct_pkt_data(struct bpf_verifier_env *env,
> const struct bpf_call_arg_meta *meta, enum bpf_access_type t)
> {
>   enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
>   switch (prog_type) {
>     /* Program types only with direct read access go here! */
>     case BPF_PROG_TYPE_CGROUP_SKB: (and some others)
>       if (t == BPF_WRITE) return false;
>       fallthrough;
>     /* Program types with direct read + write access go here! */
>     case BPF_PROG_TYPE_SCHED_CLS: (and some others)
>       if (meta) return meta->pkt_access;
>       env->seen_direct_write = true;
>       return true;
>     case BPF_PROG_TYPE_CGROUP_SOCKOPT:
>       if (t == BPF_WRITE) env->seen_direct_write = true;
>       return true;
>   }
> }
>
> why does the above set env->seen_direct_write to true even when t !=
> BPF_WRITE, even for programs that only allow (per the comment) direct
> read access.
>
> Is this working correctly?  Is there some gotcha this is papering over?
>
> Should 'env->seen_direct_write = true; return true;' be changed into
> 'fallthrough' so that write is only set if t == BPF_WRITE?
>
> This matters because 'env->seen_direct_write = true' then triggers an
> unconditional unclone in the bpf prologue, which I'd like to avoid
> unless I actually need to modify the packet (with
> bpf_skb_store_bytes)...
>
> may_access_direct_pkt_data() has two call sites, in one it only gets
> called with BPF_WRITE so it's ok, but the other one is in
> check_func_arg():
>
> if (type_is_pkt_pointer(type) && !may_access_direct_pkt_data(env,
> meta, BPF_READ)) { verbose(env, "helper access to the packet is not
> allowed\n"); return -EACCES; }
>
> and I'm not really following what this does, but it seems like bpf
> helper read access to the packet triggers unclone?

There seems to be a set of helpers (pkt_access=true) which accept
direct packet pointers and are known to be doing only reads of the skb
data (safe without clone).
You seem to be hitting the case where you're passing that packet
pointer to one of the "unsafe" (pkt_acces=false) helpers which
triggers that seen_direct_write=true condition.
So it seems like it's by design? Which helper are you calling? Maybe
that one should also have pkt_access=true?

Tangential: I wish there was an explicit BPF_F_MAY_ATTEMPT_TO_CLONE
flag that gates this auto-clone. I think at some point we also
accidentally hit it :-(

> (side note: all packets ingressing from the rndis gadget driver are
> clones due to how it deals with usb packet deaggregation [not to be
> mistaken with lro/tso])
>
> Confused...

Re: Query on reads being flagged as direct writes...

[Maciej Żenczykowski]

I haven't tried figuring it out yet (via printks)... as I can't
currently trigger this myself, so I'm basically stuck with code
spelunking.
(I do know we currently legitimately do at least one dpa write... and
converting that one line to use bpf_skb_store_bytes results in the
program not even loading...
https://android-review.googlesource.com/c/platform/packages/modules/Connectivity/+/2181376
- but I haven't yet had the opportunity to figure out what, likely
obvious, mistake I made)

We do make use of at least the following helpers:

bpf_map_lookup_elem
bpf_map_update_elem

which AFAICT are all marked as pkt_access == true, even though we
don't use them to read nor write to the packet.

Having said that, and having dug deeper into the code I think only
may_access_direct_pkt_data(env, meta, BPF_READ) is the problematic
call site, and AFAICT it always has meta != NULL since it is called
via check_func_arg(env, i, &meta, fn).
So maybe this does just work? even if it is super confusing... and
should probably be documented better.

ie. right now we have two callers of may_access_direct_pkt_data():
  may_access_direct_pkt_data(env, NULL, BPF_WRITE)
  may_access_direct_pkt_data(env, non-NULL, BPF_READ)
so meta != NULL implies t == BPF_WRITE, and using fallthrough would be
a no-op (with current callers)

Maybe this is just a single function that does two very different
things in the two call sites...

On Fri, Aug 12, 2022 at 9:58 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> On Fri, Aug 12, 2022 at 5:06 AM Maciej Żenczykowski
> <zenczykowski@gmail.com> wrote:
> >
> > From kernel/bpf/verifier.c with some simplifications (removed some of
> > the cases to make this shorter):
> >
> > static bool may_access_direct_pkt_data(struct bpf_verifier_env *env,
> > const struct bpf_call_arg_meta *meta, enum bpf_access_type t)
> > {
> >   enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
> >   switch (prog_type) {
> >     /* Program types only with direct read access go here! */
> >     case BPF_PROG_TYPE_CGROUP_SKB: (and some others)
> >       if (t == BPF_WRITE) return false;
> >       fallthrough;
> >     /* Program types with direct read + write access go here! */
> >     case BPF_PROG_TYPE_SCHED_CLS: (and some others)
> >       if (meta) return meta->pkt_access;
> >       env->seen_direct_write = true;
> >       return true;
> >     case BPF_PROG_TYPE_CGROUP_SOCKOPT:
> >       if (t == BPF_WRITE) env->seen_direct_write = true;
> >       return true;
> >   }
> > }
> >
> > why does the above set env->seen_direct_write to true even when t !=
> > BPF_WRITE, even for programs that only allow (per the comment) direct
> > read access.
> >
> > Is this working correctly?  Is there some gotcha this is papering over?
> >
> > Should 'env->seen_direct_write = true; return true;' be changed into
> > 'fallthrough' so that write is only set if t == BPF_WRITE?
> >
> > This matters because 'env->seen_direct_write = true' then triggers an
> > unconditional unclone in the bpf prologue, which I'd like to avoid
> > unless I actually need to modify the packet (with
> > bpf_skb_store_bytes)...
> >
> > may_access_direct_pkt_data() has two call sites, in one it only gets
> > called with BPF_WRITE so it's ok, but the other one is in
> > check_func_arg():
> >
> > if (type_is_pkt_pointer(type) && !may_access_direct_pkt_data(env,
> > meta, BPF_READ)) { verbose(env, "helper access to the packet is not
> > allowed\n"); return -EACCES; }
> >
> > and I'm not really following what this does, but it seems like bpf
> > helper read access to the packet triggers unclone?
>
> There seems to be a set of helpers (pkt_access=true) which accept
> direct packet pointers and are known to be doing only reads of the skb
> data (safe without clone).
> You seem to be hitting the case where you're passing that packet
> pointer to one of the "unsafe" (pkt_acces=false) helpers which
> triggers that seen_direct_write=true condition.
> So it seems like it's by design? Which helper are you calling? Maybe
> that one should also have pkt_access=true?
>
> Tangential: I wish there was an explicit BPF_F_MAY_ATTEMPT_TO_CLONE
> flag that gates this auto-clone. I think at some point we also
> accidentally hit it :-(
>
> > (side note: all packets ingressing from the rndis gadget driver are
> > clones due to how it deals with usb packet deaggregation [not to be
> > mistaken with lro/tso])
> >
> > Confused...