Re: [PATCH mptcp-next] Revert "mptcp: add data lock for sk timers"

[Paolo Abeni <pabeni@redhat.com>]

On Thu, 2022-05-05 at 16:40 -0700, Mat Martineau wrote:
> On Thu, 5 May 2022, Paolo Abeni wrote:
> 
> > This reverts commit 4293248c6704b854bf816aa1967e433402bee11c.
> > 
> > Additional locks are not needed, all the touched sections
> > are already under mptcp socket lock protection.
> > 
> 
> I agree that this needs to be reverted:
> 
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> 
> 
> But msk->sk_timer is *also* accessed in two places without the mptcp 
> socket lock (but with the data lock):
>   * mptcp_pm_mp_fail_received() (stop timer when mp_fail received)
> 
>   * subflow_check_data_avail() (start timer on infinite mapping rx)

I'm reasonably no additional lock is required to call sk_stop_timer()
and/or sk_reset_timer(): they boil down to the timer_{del,mod}
primitives which in turns are irq safe.

The mptcp wrappers *could* require additional locking because they
additionally touch mptcp_sk(sk)->timer_ival. I *think* we could avoid
the lock even there with some additional barrier, but it looks every
caller is already under the lock.

I think we don't need to defer touching the timer. Eventully we could
remove the check on the msk socket status, which again looks not
needed.


Cheers,

Paolo