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. 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.
> > Here 'entry_freplace' is assumed to invalidate packet data because of
> > the bpf_tail_call_static(), and thus it can't replace 'subprog_tc'.
> > There is an option to add a dummy call to bpf_skb_pull_data(),
> > but this operation is not a noop, as far as I can tell.
>
> skb_pull is not, but there are plenty that are practically nop helpers.
> bpf_helper_changes_pkt_data() lists them all.
> Like bpf_xdp_adjust_meta(xdp, 0)
>
> > Same situation was discussed in the sub-thread regarding use of tags.
> > (Note: because of the tail calls, some form of changes_pkt_data effect
> > propagation similar to one done in check_cfg() would be needed with
> > tags as well. That, or tags would be needed not only for global
> > sub-programs but also for BPF_MAP_TYPE_PROG_ARRAY maps).
>
> nack to tags approach.
Understood.
