Messages in this thread | | | From | Hawkins Jiawei <> | Subject | Re: [PATCH v2] net/smc: fix refcount bug in sk_psock_get (2) | Date | Tue, 2 Aug 2022 22:32:14 +0800 |
| |
Thanks for your suggestion!
On Mon, 1 Aug 2022 at 17:16, Jakub Sitnicki <jakub@cloudflare.com> wrote: > This way we would also avoid some confusion. With the change below, the > SK_USER_DATA_NOTPSOCK is not *always* set when sk_user_data holds a > non-psock pointer. Only when SMC sets it. > > If we go with the current approach, the rest of sites, execpt for psock, > that assign to sk_user_data should be updated to set > SK_USER_DATA_NOTPSOCK as well, IMO. Yes, as you point out, in this patch, this flag's name should be *SK_USER_DATA_NEEDCHECK_NOTPSOCK*, which is more clearly.
To be more specific, we don't need to set this flag for every sk_user_data who holds non-psock pointer. Only set the flag for the site that has been reported involved with the type-mismatch bug like this bug. > > During SMC fallback process in connect syscall, kernel will > > replaces TCP with SMC. In order to forward wakeup > > smc socket waitqueue after fallback, kernel will sets > > clcsk->sk_user_data to origin smc socket in > > smc_fback_replace_callbacks(). > > > > Later, in shutdown syscall, kernel will calls > > sk_psock_get(), which treats the clcsk->sk_user_data > > as sk_psock type, triggering the refcnt warning.
For other sites, this patch is actually transparent to them, because the *SK_USER_DATA_NEEDCHECK_NOTPSOCK* flag is always unset. So this patch won't affect them, which won't introduce any extra potential bugs. > +/** > + * rcu_dereference_sk_user_data_psock - return psock if sk_user_data points > + * to the psock > + * @sk: socket > + */ > +static inline > +struct sk_psock *rcu_dereference_sk_user_data_psock(const struct sock *sk) > +{ > + uintptr_t __tmp = (uintptr_t)rcu_dereference(__sk_user_data((sk))); > + > + if (__tmp & SK_USER_DATA_NOTPSOCK) > + return NULL; > + return (struct sk_psock *)(__tmp & SK_USER_DATA_PTRMASK); > +}
> > Hi, > Since using psock is not the common case, I'm wondering if it makes more > sense to have an inverse flag - SK_USER_DATA_PSOCK. Flag would be set by > the psock code on assignment to sk_user_data. However, your suggestion seems more elegant. For my patch, as you point out, when anyone reports a new type-mismatch bug, the relative assign to sk_user_data should be updated to set *SK_USER_DATA_NEEDCHECK_NOTPSOCK* flag.
For your suggestion, you seems avoid above situation. What's more, as I use git grep to search, there seems no direct access to sk_user_data, all via a small amount macros and wrapper functions. So we can keep transparent by only update those macros and wrapper functions, which also won't introduce any extra potential bugs.
I will patch as you suggest in v3 patch.
| |