On Wed, 3 Aug 2022 at 23:37, Martin KaFai Lau <kafai@xxxxxx> 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
