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@xxxxxxxxxx> wrote: > > On Fri, Aug 12, 2022 at 5:06 AM Maciej Żenczykowski > <zenczykowski@xxxxxxxxx> 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...
