Re: [PATCH v4] net: fix refcount bug in sk_psock_get (2)

[Hawkins Jiawei <yin31149@gmail.com>]

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