| Message ID | 20241210041100.1898468-8-eddyz87@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1a4607ffba35bf2a630aab299e34dd3f6e658d70 |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: track changes_pkt_data property for global functions | expand |
> Tail-called programs could execute any of the helpers that invalidate > packet pointers. Hence, conservatively assume that each tail call > invalidates packet pointers. Tail calls look like a clear limitation of "auto-infer packet invalidation effect" approach. Correct solution requires propagating effects in the dynamic callee-caller graph, unlikely to ever happen. I'm curious if assuming that every call to a global sub program invalidates packet pointers might be an option. Does it break too many programs in the wild? From an end-user perspective, the presented solution makes debugging verifier errors harder. An error message doesn't tell which call invalidated pointers. Whether verifier considers a particular sub program as pointer-invalidating is not revealed. I foresee exciting debugging sessions. It probably doesn't matter, but I don't like bpf_xdp_adjust_meta(xdp, 0) hack to mark a program as pointer-invalidating either. I would've preferred a simple static rule "calls to global sub programs invalidate packet pointers" with an optional decl tag to mark a sub program as non-invalidating, in line with "arg:nonnull". > Making the change in bpf_helper_changes_pkt_data() automatically makes > use of check_cfg() logic that computes 'changes_pkt_data' effect for > global sub-programs, such that the following program could be > rejected: > > int tail_call(struct __sk_buff *sk) > { > bpf_tail_call_static(sk, &jmp_table, 0); > return 0; > } > > SEC("tc") > int not_safe(struct __sk_buff *sk) > { > int *p = (void *)(long)sk->data; > ... make p valid ... > tail_call(sk); > *p = 42; /* this is unsafe */ > ... > } > > The tc_bpf2bpf.c:subprog_tc() needs change: mark it as a function that > can invalidate packet pointers. Otherwise, it can't be freplaced with > tailcall_freplace.c:entry_freplace() that does a tail call. > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> > --- > net/core/filter.c | 2 ++ > tools/testing/selftests/bpf/progs/tc_bpf2bpf.c | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git a/net/core/filter.c b/net/core/filter.c > index efb75eed2e35..21131ec25f24 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -7924,6 +7924,8 @@ bool bpf_helper_changes_pkt_data(enum bpf_func_id func_id) > case BPF_FUNC_xdp_adjust_head: > case BPF_FUNC_xdp_adjust_meta: > case BPF_FUNC_xdp_adjust_tail: > + /* tail-called program could call any of the above */ > + case BPF_FUNC_tail_call: > return true; > default: > return false; > diff --git a/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c > index d1a57f7d09bd..fe6249d99b31 100644 > --- a/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c > +++ b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c > @@ -11,6 +11,8 @@ int subprog_tc(struct __sk_buff *skb) > > __sink(skb); > __sink(ret); > + /* let verifier know that 'subprog_tc' can change pointers to skb->data */ > + bpf_skb_change_proto(skb, 0, 0); > return ret; > } > > -- > 2.47.0 >
On Tue, Dec 10, 2024 at 2:35 AM Nick Zavaritsky <mejedi@gmail.com> wrote: > > > > Tail-called programs could execute any of the helpers that invalidate > > packet pointers. Hence, conservatively assume that each tail call > > invalidates packet pointers. > > Tail calls look like a clear limitation of "auto-infer packet > invalidation effect" approach. Correct solution requires propagating > effects in the dynamic callee-caller graph, unlikely to ever happen. > > I'm curious if assuming that every call to a global sub program > invalidates packet pointers might be an option. Does it break too many > programs in the wild? It might. Assuming every global prog changes pkt data is too risky, also it would diverge global vs static verification even further, which is a bad user experience. > From an end-user perspective, the presented solution makes debugging > verifier errors harder. An error message doesn't tell which call > invalidated pointers. Whether verifier considers a particular sub > program as pointer-invalidating is not revealed. I foresee exciting > debugging sessions. There is such a risk. > It probably doesn't matter, but I don't like bpf_xdp_adjust_meta(xdp, 0) > hack to mark a program as pointer-invalidating either. > > I would've preferred a simple static rule "calls to global sub programs > invalidate packet pointers" with an optional decl tag to mark a sub > program as non-invalidating, in line with "arg:nonnull". That's not an option.
On Tue, 2024-12-10 at 10:23 -0800, Alexei Starovoitov wrote: > On Tue, Dec 10, 2024 at 2:35 AM Nick Zavaritsky <mejedi@gmail.com> wrote: > > > > > > > Tail-called programs could execute any of the helpers that invalidate > > > packet pointers. Hence, conservatively assume that each tail call > > > invalidates packet pointers. > > > > Tail calls look like a clear limitation of "auto-infer packet > > invalidation effect" approach. Correct solution requires propagating > > effects in the dynamic callee-caller graph, unlikely to ever happen. > > > > I'm curious if assuming that every call to a global sub program > > invalidates packet pointers might be an option. Does it break too many > > programs in the wild? > > It might. Assuming every global prog changes pkt data is too risky, > also it would diverge global vs static verification even further, > which is a bad user experience. I assume that freplace and tail calls are used much less often than global calls. If so, I think that some degree of inference, even with limitations, would be convenient more often than not. > > From an end-user perspective, the presented solution makes debugging > > verifier errors harder. An error message doesn't tell which call > > invalidated pointers. Whether verifier considers a particular sub > > program as pointer-invalidating is not revealed. I foresee exciting > > debugging sessions. > > There is such a risk. I can do a v4 and add a line in the log each time the packet pointers are invalidated. Such lines would be presented in verification failure logs. (Can also print every register/stack slot where packet pointer is invalidated, but this may be too verbose). [...]
On Tue, Dec 10, 2024 at 10:29 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Tue, 2024-12-10 at 10:23 -0800, Alexei Starovoitov wrote: > > On Tue, Dec 10, 2024 at 2:35 AM Nick Zavaritsky <mejedi@gmail.com> wrote: > > > > > > > > > > Tail-called programs could execute any of the helpers that invalidate > > > > packet pointers. Hence, conservatively assume that each tail call > > > > invalidates packet pointers. > > > > > > Tail calls look like a clear limitation of "auto-infer packet > > > invalidation effect" approach. Correct solution requires propagating > > > effects in the dynamic callee-caller graph, unlikely to ever happen. > > > > > > I'm curious if assuming that every call to a global sub program > > > invalidates packet pointers might be an option. Does it break too many > > > programs in the wild? > > > > It might. Assuming every global prog changes pkt data is too risky, > > also it would diverge global vs static verification even further, > > which is a bad user experience. > > I assume that freplace and tail calls are used much less often than > global calls. If so, I think that some degree of inference, even with > limitations, would be convenient more often than not. > > > > From an end-user perspective, the presented solution makes debugging > > > verifier errors harder. An error message doesn't tell which call > > > invalidated pointers. Whether verifier considers a particular sub > > > program as pointer-invalidating is not revealed. I foresee exciting > > > debugging sessions. > > > > There is such a risk. > > I can do a v4 and add a line in the log each time the packet pointers > are invalidated. Such lines would be presented in verification failure > logs. (Can also print every register/stack slot where packet pointer > is invalidated, but this may be too verbose). This is something to consider for bpf-next. For bpf we need a minimal fix. So I applied as-is.
On Tue, 2024-12-10 at 10:31 -0800, Alexei Starovoitov wrote: [...] > > > > From an end-user perspective, the presented solution makes debugging > > > > verifier errors harder. An error message doesn't tell which call > > > > invalidated pointers. Whether verifier considers a particular sub > > > > program as pointer-invalidating is not revealed. I foresee exciting > > > > debugging sessions. > > > > > > There is such a risk. > > > > I can do a v4 and add a line in the log each time the packet pointers > > are invalidated. Such lines would be presented in verification failure > > logs. (Can also print every register/stack slot where packet pointer > > is invalidated, but this may be too verbose). > > This is something to consider for bpf-next. > For bpf we need a minimal fix. So I applied as-is. I must admit, I'm not familiar with the way bpf/bpf-next interact. Should I wait for certain merges to happen before posting a patch to bpf-next?
On Tue, Dec 10, 2024 at 10:52 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Tue, 2024-12-10 at 10:31 -0800, Alexei Starovoitov wrote: > > [...] > > > > > > From an end-user perspective, the presented solution makes debugging > > > > > verifier errors harder. An error message doesn't tell which call > > > > > invalidated pointers. Whether verifier considers a particular sub > > > > > program as pointer-invalidating is not revealed. I foresee exciting > > > > > debugging sessions. > > > > > > > > There is such a risk. > > > > > > I can do a v4 and add a line in the log each time the packet pointers > > > are invalidated. Such lines would be presented in verification failure > > > logs. (Can also print every register/stack slot where packet pointer > > > is invalidated, but this may be too verbose). > > > > This is something to consider for bpf-next. > > For bpf we need a minimal fix. So I applied as-is. > > I must admit, I'm not familiar with the way bpf/bpf-next interact. > Should I wait for certain merges to happen before posting a patch > to bpf-next? bpf tree is for fixes only. We typically send PR every week. Once it lands in Linus's tree we merge the fixes into bpf-next. At that time follows up can be send targeting bpf-next.
On Tue, 2024-12-10 at 11:00 -0800, Alexei Starovoitov wrote: [...] > bpf tree is for fixes only. > We typically send PR every week. > Once it lands in Linus's tree we merge the fixes into bpf-next. > At that time follows up can be send targeting bpf-next. Will send next week, then. Thank you.
diff --git a/net/core/filter.c b/net/core/filter.c index efb75eed2e35..21131ec25f24 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -7924,6 +7924,8 @@ bool bpf_helper_changes_pkt_data(enum bpf_func_id func_id) case BPF_FUNC_xdp_adjust_head: case BPF_FUNC_xdp_adjust_meta: case BPF_FUNC_xdp_adjust_tail: + /* tail-called program could call any of the above */ + case BPF_FUNC_tail_call: return true; default: return false; diff --git a/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c index d1a57f7d09bd..fe6249d99b31 100644 --- a/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c +++ b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c @@ -11,6 +11,8 @@ int subprog_tc(struct __sk_buff *skb) __sink(skb); __sink(ret); + /* let verifier know that 'subprog_tc' can change pointers to skb->data */ + bpf_skb_change_proto(skb, 0, 0); return ret; }
Tail-called programs could execute any of the helpers that invalidate packet pointers. Hence, conservatively assume that each tail call invalidates packet pointers. Making the change in bpf_helper_changes_pkt_data() automatically makes use of check_cfg() logic that computes 'changes_pkt_data' effect for global sub-programs, such that the following program could be rejected: int tail_call(struct __sk_buff *sk) { bpf_tail_call_static(sk, &jmp_table, 0); return 0; } SEC("tc") int not_safe(struct __sk_buff *sk) { int *p = (void *)(long)sk->data; ... make p valid ... tail_call(sk); *p = 42; /* this is unsafe */ ... } The tc_bpf2bpf.c:subprog_tc() needs change: mark it as a function that can invalidate packet pointers. Otherwise, it can't be freplaced with tailcall_freplace.c:entry_freplace() that does a tail call. Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> --- net/core/filter.c | 2 ++ tools/testing/selftests/bpf/progs/tc_bpf2bpf.c | 2 ++ 2 files changed, 4 insertions(+)