On Tue, Mar 2, 2021 at 10:23 AM Cong Wang <xiyou.wangcong@xxxxxxxxx> wrote:
>
> On Tue, Mar 2, 2021 at 8:22 AM Lorenz Bauer <lmb@xxxxxxxxxxxxxx> wrote:
> >
> > On Tue, 2 Mar 2021 at 02:37, Cong Wang <xiyou.wangcong@xxxxxxxxx> 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?
>
> 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@xxxxxxxxx>
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.
