On Sat, Mar 02, 2019 at 10:03:03AM -0800, Alexei Starovoitov wrote: > On Sat, Mar 02, 2019 at 08:10:10AM -0800, Martin KaFai Lau wrote: > > Lorenz Bauer [thanks!] reported that a ptr returned by bpf_tcp_sock(sk) > > can still be accessed after bpf_sk_release(sk). > > Both bpf_tcp_sock() and bpf_sk_fullsock() have the same issue. > > This patch addresses them together. > > > > A simple reproducer looks like this: > > > > sk = bpf_sk_lookup_tcp(); > > /* if (!sk) ... */ > > tp = bpf_tcp_sock(sk); > > /* if (!tp) ... */ > > bpf_sk_release(sk); > > snd_cwnd = tp->snd_cwnd; /* oops! The verifier does not complain. */ > > > > The problem is the verifier did not scrub the register's states of > > the tcp_sock ptr (tp) after bpf_sk_release(sk). > > > > [ Note that when calling bpf_tcp_sock(sk), the sk is not always > > refcount-acquired. e.g. bpf_tcp_sock(skb->sk). The verifier works > > fine for this case. ] > > > > Currently, the verifier does not track if a helper's return ptr (in REG_0) > > is "carry"-ing one of its argument's refcount status. To carry this info, > > the reg1->id needs to be stored in reg0. The reg0->id has already > > been used for NULL checking purpose. Hence, a new "refcount_id" > > is needed in "struct bpf_reg_state". > > > > With refcount_id, when bpf_sk_release(sk) is called, the verifier can scrub > > all reg states which has a refcount_id match. It is done with the changes > > in release_reg_references(). > > > > When acquiring and releasing a refcount, the reg->id is still used. > > Hence, we cannot do "bpf_sk_release(tp)" in the above reproducer > > example. > > I think the choice of returning listener full sock from req sock > in sk_to_full_sk() was a wrong one. > It seems better to make semantics of bpf_tcp_sock() and bpf_sk_fullsock() as > always type cast or null. > And have a separate helper for req socket that returns > inet_reqsk(sk)->rsk_listener. > > Then it will be ok to call bpf_sk_release(tp) when tp came from > bpf_sk_lookup_tcp. > The verifier will know that it's the case because its ID will be in > acquired_refs. > > The additional refcount_id won't be necessary. > bpf_sk_fullsock() and bpf_tcp_sock() will not call sk_to_full_sk > and the verifier will be copying reg1->id into reg0->id. > > In release_reference() the verifier will do > if (regs[i].id == id) > mark_reg_unknown(env, regs, i); > for all socket types. > > release_reference_state() will stay as-is. > > imo such logic will be easier to follow. > > This implicit sk_to_full_sk() makes the whole thing much harder for the > verifier > and for the bpf program writers. > > The new bpf_get_listener_sock(sk) doesn't have to copy ID from reg1 to reg0 > since req socket will not be returned from bpf_sk_lookup_tcp and its ID > will not be stored in acuired_refs. > > Does it make sense ? I like this idea. Many thanks for thinking it through!
Allowing bpf_sk_release(tp), no need to call bpf_sk_release() on ptr returned from bpf_get_listener_sock(sk) and keep one reg->id. I think it should work. I will rework the patches.
