Re: [MPTCP][PATCH v7 mptcp-next 1/6] mptcp: drop flags and ifindex arguments

[Paolo Abeni <pabeni@redhat.com>]

On Thu, 2021-07-29 at 15:20 +0800, Geliang Tang wrote:
> From: Geliang Tang <geliangtang@xiaomi.com>
> 
> This patch added a new helper mptcp_pm_get_flags_and_ifindex_by_id(),
> and used it in __mptcp_subflow_connect() to get the flags and ifindex
> values.
> 
> Then the two arguments flags and ifindex of __mptcp_subflow_connect()
> can be dropped.
> 
> Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
> ---
>  net/mptcp/pm_netlink.c | 25 ++++++++++++++++++++++---
>  net/mptcp/protocol.h   |  5 +++--
>  net/mptcp/subflow.c    |  7 +++++--
>  3 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index ba0e1d71504d..94c68d6093de 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -462,8 +462,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
>  			check_work_pending(msk);
>  			remote_address((struct sock_common *)sk, &remote);
>  			spin_unlock_bh(&msk->pm.lock);
> -			__mptcp_subflow_connect(sk, &local->addr, &remote,
> -						local->flags, local->ifindex);
> +			__mptcp_subflow_connect(sk, &local->addr, &remote);
>  			spin_lock_bh(&msk->pm.lock);
>  			return;
>  		}
> @@ -518,7 +517,7 @@ static void mptcp_pm_nl_add_addr_received(struct mptcp_sock *msk)
>  	local.family = remote.family;
>  
>  	spin_unlock_bh(&msk->pm.lock);
> -	__mptcp_subflow_connect(sk, &local, &remote, 0, 0);
> +	__mptcp_subflow_connect(sk, &local, &remote);
>  	spin_lock_bh(&msk->pm.lock);
>  
>  add_addr_echo:
> @@ -1103,6 +1102,26 @@ __lookup_addr_by_id(struct pm_nl_pernet *pernet, unsigned int id)
>  	return NULL;
>  }
>  
> +int mptcp_pm_get_flags_and_ifindex_by_id(struct net *net, unsigned int id,
> +					 u8 *flags, int *ifindex)
> +{
> +	struct mptcp_pm_addr_entry *entry;
> +
> +	rcu_read_lock();
> +	entry = __lookup_addr_by_id(net_generic(net, pm_nl_pernet_id), id);
> +	rcu_read_unlock();
> +
> +	if (entry) {
> +		*flags = entry->flags;
> +		*ifindex = entry->ifindex;
> +	} else {
> +		*flags = 0;
> +		*ifindex = 0;
> +	}

You need to extend the RCU section after the entry reference above, or
on preempt-enabled build, the entry could be deleted in between the rcu
 unlock and the later access.

I'm wondering if there are issues with the 0 id ?!?

/P