Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Hawkins Jiawei <yin31149@gmail.com>
To:     kafai@fb.com
Cc:     18801353760@163.com, andrii@kernel.org, ast@kernel.org,
        borisp@nvidia.com, bpf@vger.kernel.org, daniel@iogearbox.net,
        davem@davemloft.net, edumazet@google.com, guwen@linux.alibaba.com,
        jakub@cloudflare.com, john.fastabend@gmail.com,
        kgraul@linux.ibm.com, kpsingh@kernel.org, kuba@kernel.org,
        linux-kernel-mentees@lists.linuxfoundation.org,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        pabeni@redhat.com, paskripkin@gmail.com, skhan@linuxfoundation.org,
        songliubraving@fb.com,
        syzbot+5f26f85569bd179c18ce@syzkaller.appspotmail.com,
        syzkaller-bugs@googlegroups.com, yhs@fb.com, yin31149@gmail.com
Subject: Re: [PATCH v4] net: fix refcount bug in sk_psock_get (2)
Date:   Thu,  4 Aug 2022 11:05:15 +0800
Message-Id: <20220804030514.7118-1-yin31149@gmail.com>
In-Reply-To: <20220803153706.oo47lv3kvkpb7yem@kafai-mbp.dhcp.thefacebook.com>
References: <20220803153706.oo47lv3kvkpb7yem@kafai-mbp.dhcp.thefacebook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Wed, 3 Aug 2022 at 23:37, Martin KaFai Lau <kafai@fb.com> wrote:
> > > + * SK_USER_DATA_BPF    - Managed by BPF
> >
> > I'd use this opportunity to add more info here, BPF is too general.
> > Maybe "Pointer is used by a BPF reuseport array"? Martin, WDYT?
> SGTM.  Thanks.
OK. It seems that this flag is introduced from
c9a368f1c0fb ("bpf: net: Avoid incorrect bpf_sk_reuseport_detach call").
I will search for more detailed description in this commit.

> >
> > > +/**
> > > + * rcu_dereference_sk_user_data_psock - return psock if sk_user_data
> > > + * points to the psock type(SK_USER_DATA_PSOCK flag is set), otherwise
> > > + * return NULL
> > > + *
> > > + * @sk: socket
> > > + */
> > > +static inline
> > > +struct sk_psock *rcu_dereference_sk_user_data_psock(const struct sock *sk)
> >
> > nit: the return type more commonly goes on the same line as "static
> > inline"
Ok. I will correct it.

> >
> > > +{
> > > +     uintptr_t __tmp = (uintptr_t)rcu_dereference(__sk_user_data((sk)));
> > > +
> > > +     if (__tmp & SK_USER_DATA_PSOCK)
> > > +             return (struct sk_psock *)(__tmp & SK_USER_DATA_PTRMASK);
> > > +
> > > +     return NULL;
> > > +}
> >
> > As a follow up we can probably generalize this into
> >  __rcu_dereference_sk_user_data_cond(sk, bit)
> >
> > and make the psock just call that:
> >
> > static inline struct sk_psock *
> > rcu_dereference_sk_user_data_psock(const struct sock *sk)
> > {
> >         return __rcu_dereference_sk_user_data_cond(sk, SK_USER_DATA_PSOCK);
> > }
Yes. I will refactor it in this way.

> >
> > diff --git a/kernel/bpf/reuseport_array.c b/kernel/bpf/reuseport_array.c
> > index e2618fb5870e..ad5c447a690c 100644
> > --- a/kernel/bpf/reuseport_array.c
> > +++ b/kernel/bpf/reuseport_array.c
> > @@ -21,14 +21,11 @@ static struct reuseport_array *reuseport_array(struct bpf_map *map)
> >  /* The caller must hold the reuseport_lock */
> >  void bpf_sk_reuseport_detach(struct sock *sk)
> >  {
> > -       uintptr_t sk_user_data;
> > +       struct sock __rcu **socks;
> >
> >         write_lock_bh(&sk->sk_callback_lock);
> > -       sk_user_data = (uintptr_t)sk->sk_user_data;
> > -       if (sk_user_data & SK_USER_DATA_BPF) {
> > -               struct sock __rcu **socks;
> > -
> > -               socks = (void *)(sk_user_data & SK_USER_DATA_PTRMASK);
> > +       socks = __rcu_dereference_sk_user_data_cond(sk, SK_USER_DATA_BPF);
> > +       if (socks) {
> >                 WRITE_ONCE(sk->sk_user_data, NULL);
> >                 /*
> >                  * Do not move this NULL assignment outside of
> >
> >
> > But that must be a separate patch, not part of this fix.
I wonder if it is proper to gather these together in a patchset, for
they are all about flags in sk_user_data, maybe:

[PATCH v5 0/2] net: enhancement to flags in sk_user_data field
	- introduce the patchset

[PATCH v5 1/2] net: clean up code for flags in sk_user_data field
	- refactor the things in include/linux/skmsg.h and
include/net/sock.h
	- refactor the flags's usage by other code, such as
net/core/skmsg.c and kernel/bpf/reuseport_array.c

[PATCH v5 2/2] net: fix refcount bug in sk_psock_get (2)
	- add SK_USER_DATA_PSOCK flag in sk_user_data field