lkml.org 
[lkml]   [2022]   [Aug]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v2] net/smc: fix refcount bug in sk_psock_get (2)
    Date
    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.

    \
     
     \ /
      Last update: 2022-08-02 16:32    [W:4.769 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site