Re: [Patch bpf-next v2 2/9] sock: introduce sk_prot->update_proto()

[John Fastabend <john.fastabend@gmail.com>]

Cong Wang wrote:
> On Tue, Mar 2, 2021 at 10:23 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Tue, Mar 2, 2021 at 8:22 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
> > >
> > > On Tue, 2 Mar 2021 at 02:37, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> > > ...
> > > >  static inline void sk_psock_restore_proto(struct sock *sk,
> > > >                                           struct sk_psock *psock)
> > > >  {
> > > >         sk->sk_prot->unhash = psock->saved_unhash;
> > >
> > > Not related to your patch set, but why do an extra restore of
> > > sk_prot->unhash here? At this point sk->sk_prot is one of our tcp_bpf
> > > / udp_bpf protos, so overwriting that seems wrong?

"extra"? restore_proto should only be called when the psock ref count
is zero and we need to transition back to the original socks proto
handlers. To trigger this we can simply delete a sock from the map.
In the case where we are deleting the psock overwriting the tcp_bpf
protos is exactly what we want.?

> >
> > Good catch. It seems you are right, but I need a double check. And
> > yes, it is completely unrelated to my patch, as the current code has
> > the same problem.
> 
> Looking at this again. I noticed
> 
> commit 4da6a196f93b1af7612340e8c1ad8ce71e18f955
> Author: John Fastabend <john.fastabend@gmail.com>
> Date:   Sat Jan 11 06:11:59 2020 +0000
> 
>     bpf: Sockmap/tls, during free we may call tcp_bpf_unhash() in loop
> 
> intentionally fixed a bug in kTLS with overwriting this ->unhash.
> 
> I agree with you that it should not be updated for sockmap case,
> however I don't know what to do with kTLS case, it seems the bug the
> above commit fixed still exists if we just revert it.
> 
> Anyway, this should be targeted for -bpf as a bug fix, so it does not
> belong to this patchset.
> 
> Thanks.

Hi,

I'm missing the error case here. The restore logic happens when the refcnt
hits 0 on the psock, indicating its time to garbage collect the psock. 

 sk_psock_put
   if (refcount_dec_and_test(&psock->refcnt))
    sk_psock_drop(sk, psock);
      sk_psock_restore_proto(sk, psock)
         sk->sk_prot->unhash = psock->saved_unhash

When sockets are initialized via sk_psock_init() we opulate the unhash field

 psock->saved_unhash = prot->unhash;

So we need to unwind this otherwise a future unhash() call would not call
the original protos unhash handler.

Care to give me some more context on what the bug is?

Thanks,
John