On Mon, Dec 9, 2024 at 9:57 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Mon, 2024-12-09 at 08:53 -0800, Alexei Starovoitov wrote:
>
> [...]
>
> > >
> > > // tc_bpf2bpf.c
> > > __noinline freplace
> > > int subprog_tc(struct __sk_buff *skb) <--------.
> > > { |
> > > int ret = 1; |
> > > |
> > > __sink(skb); |
> > > __sink(ret); |
> > > return ret; |
> > > } |
> > > |
> > > SEC("tc") |
> > > int entry_tc(struct __sk_buff *skb) |
> > > { |
> > > return subprog_tc(skb); |
> > > } |
> > > |
> > > // tailcall_freplace.c |
> > > struct { |
> > > __uint(type, BPF_MAP_TYPE_PROG_ARRAY); |
> > > __uint(max_entries, 1); |
> > > __uint(key_size, sizeof(__u32)); |
> > > __uint(value_size, sizeof(__u32)); |
> > > } jmp_table SEC(".maps"); |
> > > |
> > > int count = 0; |
> > > |
> > > SEC("freplace") |
> > > int entry_freplace(struct __sk_buff *skb) -----'
> > > {
> > > count++;
> > > bpf_tail_call_static(skb, &jmp_table, 0);
> > > return count;
> > > }
> >
> > hmm. none of the above changes pkt_data, so it should be allowed.
> > The prog doesn't read skb->data either.
> > So I don't quite see the problem.
>
> The problem is when I use simplified rule: "every tail call changes packet data",
> as a substitute for proper map content effects tracking.
>
> If map content effects are tracked, there should be no problems
> verifying this program. However, that can't be done in check_cfg(),
> as it does not track register values, and register value is needed to
> identify the map.
We don't need data flow analysis and R1 tracking.
We could do the following rule:
if prog has a tail call and any prog array map in prog->aux->used_maps
calls into prog with adjust_pkt_data assume that this prog
also adjusts pkt_data.
> Hence, mechanics with "in-line" global sub-program
> traversal is needed (as described by Andrii):
> - during a regular verification pass get to a global sub-program call:
> - if sub-program had not been visited yet, verify it completely
> and compute changes_pkt_data effect;
> - continue from the call-site using the computed effect;
> - during a regular verification pass get to a tail call:
> - check the map pointed to by R1 to see whether it has
> changes_pkt_data effect.
I don't think we should be adding all that logic when a much simpler
rule (as described above) is good enough.
Also either inline global sub-prog traversal or the simple rule above
both suffer from the following issue:
in case prog_array is empty map->changes_pkt_data will be false.
It will be set to true only when the first
prog_fd_array_get_ptr()->bpf_prog_map_compatible()
will record changes_pkt_data from the first prog inserted in prog_array.
So main prog reading skb->data after calling subprog that tail_calls
somewhere should assume that skb->data is invalidated.
That's pretty much your rule "every tail call changes packet data".
I think we can go with this simplest approach as well.
The test you mentioned have to be adjusted. Not a big deal.
Or we can do:
"if prog_array empty assume adjusts_pkt_data == true,
otherwise adj_pkt_data | = for each map in used_maps {
map->owner.adj_pkt_data }"
The fancy inline global subprog traversal would have to have the same
"simple" (or call it dumb) rule.
So at the end both inline or check_cfg are not accurate at all,
but check_cfg approach is so much simpler.
