On Wed, Sep 30, 2020 at 08:23:47AM -0700, sdf@xxxxxxxxxx wrote:
> On 08/20, Martin KaFai Lau wrote:
> > [..]
> > +static inline void bpf_skops_init_child(const struct sock *sk,
> > + struct sock *child)
> > +{
> > + tcp_sk(child)->bpf_sock_ops_cb_flags =
> > + tcp_sk(sk)->bpf_sock_ops_cb_flags &
> > + (BPF_SOCK_OPS_PARSE_ALL_HDR_OPT_CB_FLAG |
> > + BPF_SOCK_OPS_PARSE_UNKNOWN_HDR_OPT_CB_FLAG |
> > + BPF_SOCK_OPS_WRITE_HDR_OPT_CB_FLAG);
> > +}
> It looks like it breaks test_tcpbpf_user test in an interesting way, can
> you verify on your side?
>
> Awhile ago, I've added retries to this test to make it less flaky.
> The test is waiting for 3 BPF_TCP_CLOSE events and now it
> only gets 2 BPF_TCP_CLOSE events.
>
> IIUC, we used to copy/inherit parent bpf_sock_ops_cb_flags and now
> we are doing only a small subset (bpf tcp header) with the code above.
>
> I'm still trying to understand whether that's working as intended
> and we need to fix the test or it's a user-visible breakage.
> Thoughts?
Thanks for the report.
Agree. bpf_skops_init_child() is unnecessary and it will break
existing assumption that the passive established socket
will inherit all cb_flags from the listen socket. It should just
allow sock_copy() to do its job.
I will post a fix.
