Re: [PATCH mptcp-net 1/3] mptcp: Avoid acquiring PM lock for subflow priority changes

[Paolo Abeni <pabeni@redhat.com>]

On Tue, 2022-06-28 at 12:03 -0700, Mat Martineau wrote:
> The in-kernel path manager code for changing subflow flags acquired both
> the msk socket lock and the PM lock when possibly changing the "backup"
> and "fullmesh" flags. mptcp_pm_nl_mp_prio_send_ack() does not access
> anything protected by the PM lock, and it must release and reacquire
> the PM lock.
> 
> By pushing the PM lock to where it is needed in mptcp_pm_nl_fullmesh(),
> the lock is only acquired when the fullmesh flag is changed and the
> backup flag code no longer has to release and reacquire the PM lock.
> 
> This change also makes it possible to call
> mptcp_pm_nl_mp_prio_send_ack() for the userspace PM commands without
> manipulating the in-kernel PM lock.
> 
> Fixes: 0f9f696a502e ("mptcp: add set_flags command in PM netlink")
> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> ---
>  net/mptcp/pm_netlink.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index e099f2a12504..d04b34fc9a8e 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -741,10 +741,8 @@ static int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
>  		subflow->request_bkup = bkup;
>  		__MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPPRIOTX);
>  
> -		spin_unlock_bh(&msk->pm.lock);
>  		pr_debug("send ack for mp_prio");
>  		mptcp_subflow_send_ack(ssk);
> -		spin_lock_bh(&msk->pm.lock);
>  
>  		return 0;
>  	}
> @@ -1816,8 +1814,10 @@ static void mptcp_pm_nl_fullmesh(struct mptcp_sock *msk,
>  
>  	list.ids[list.nr++] = addr->id;
>  
> +	spin_lock_bh(&msk->pm.lock);
>  	mptcp_pm_nl_rm_subflow_received(msk, &list);
>  	mptcp_pm_create_subflow_or_signal_addr(msk);
> +	spin_unlock_bh(&msk->pm.lock);
>  }
>  
>  static int mptcp_nl_set_flags(struct net *net,
> @@ -1835,12 +1835,10 @@ static int mptcp_nl_set_flags(struct net *net,
>  			goto next;
>  
>  		lock_sock(sk);
> -		spin_lock_bh(&msk->pm.lock);
>  		if (changed & MPTCP_PM_ADDR_FLAG_BACKUP)
>  			ret = mptcp_pm_nl_mp_prio_send_ack(msk, addr, bkup);

This causes a

BUG: using __this_cpu_add() in preemptible code

as
		__MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPPRIOTX);

in mptcp_pm_nl_mp_prio_send_ack is no longer in atomic context. Either
use MPTCP_INC_STATS() and/or move the increment in write_options (I
think the latter is better)

>  		if (changed & MPTCP_PM_ADDR_FLAG_FULLMESH)
>  			mptcp_pm_nl_fullmesh(msk, addr);
> -		spin_unlock_bh(&msk->pm.lock);
>  		release_sock(sk);
>  
>  next: