[PATCH mptcp-net] mptcp: pm: use _rcu variant under rcu_read_lock

Matthieu Baerts (NGI0) posted 1 patch 3 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/20241022-mptcp-pm-lookup._5Faddr._5Frcu-v1-1-19d45f26c872@kernel.org
There is a newer version of this series
net/mptcp/pm_netlink.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
[PATCH mptcp-net] mptcp: pm: use _rcu variant under rcu_read_lock
Posted by Matthieu Baerts (NGI0) 3 months, 3 weeks ago
In mptcp_pm_create_subflow_or_signal_addr(), rcu_read_(un)lock() are
used as expected to iterate over the list of local addresses, but
list_for_each_entry() was used instead of list_for_each_entry_rcu() in
__lookup_addr() (and lookup_id_by_addr() before). It is important to use
this variant which adds the required READ_ONCE() (and diagnostic checks
if enabled).

Because __lookup_addr() is also used in mptcp_pm_nl_set_flags() where it
is called under the pernet->lock because the returned entry might be
modified, the _rcu variant cannot be used in all cases. It is then
required to create a new helper. Note that this new helper can be reused
later to reduce some duplicated code elsewhere in this file.

Fixes: 86e39e04482b ("mptcp: keep track of local endpoint still available for each msk")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_netlink.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 618289aac0ab7f558d55d8b2ebb00dc62fc72f88..fe84a11cfbab1afa34ee3586ecc29658931e9a4c 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -531,6 +531,18 @@ __lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info)
 	return NULL;
 }
 
+static struct mptcp_pm_addr_entry *
+__lookup_addr_rcu(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info)
+{
+	struct mptcp_pm_addr_entry *entry;
+
+	list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
+		if (mptcp_addresses_equal(&entry->addr, info, entry->addr.port))
+			return entry;
+	}
+	return NULL;
+}
+
 static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 {
 	struct sock *sk = (struct sock *)msk;
@@ -556,7 +568,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 
 		mptcp_local_address((struct sock_common *)msk->first, &mpc_addr);
 		rcu_read_lock();
-		entry = __lookup_addr(pernet, &mpc_addr);
+		entry = __lookup_addr_rcu(pernet, &mpc_addr);
 		if (entry) {
 			__clear_bit(entry->addr.id, msk->pm.id_avail_bitmap);
 			msk->mpc_endpoint_id = entry->addr.id;

---
base-commit: 66bee427d4db76bd1afbf0184a2fd1f238a4ee27
change-id: 20241022-mptcp-pm-lookup_addr_rcu-01833ea95155

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>
Re: [PATCH mptcp-net] mptcp: pm: use _rcu variant under rcu_read_lock
Posted by Mat Martineau 3 months, 3 weeks ago
On Tue, 22 Oct 2024, Matthieu Baerts (NGI0) wrote:

> In mptcp_pm_create_subflow_or_signal_addr(), rcu_read_(un)lock() are
> used as expected to iterate over the list of local addresses, but
> list_for_each_entry() was used instead of list_for_each_entry_rcu() in
> __lookup_addr() (and lookup_id_by_addr() before). It is important to use
> this variant which adds the required READ_ONCE() (and diagnostic checks
> if enabled).
>
> Because __lookup_addr() is also used in mptcp_pm_nl_set_flags() where it
> is called under the pernet->lock because the returned entry might be
> modified, the _rcu variant cannot be used in all cases. It is then
> required to create a new helper. Note that this new helper can be reused
> later to reduce some duplicated code elsewhere in this file.

Hi Matthieu -

I'm not sure this is the case, if you add rcu read lock/unlocks to 
mptcp_pm_nl_set_flags() then the _rcu variant could be used there (and 
then only one __lookup_addr() would be needed).

- Mat

>
> Fixes: 86e39e04482b ("mptcp: keep track of local endpoint still available for each msk")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> net/mptcp/pm_netlink.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 618289aac0ab7f558d55d8b2ebb00dc62fc72f88..fe84a11cfbab1afa34ee3586ecc29658931e9a4c 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -531,6 +531,18 @@ __lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info)
> 	return NULL;
> }
>
> +static struct mptcp_pm_addr_entry *
> +__lookup_addr_rcu(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info)
> +{
> +	struct mptcp_pm_addr_entry *entry;
> +
> +	list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
> +		if (mptcp_addresses_equal(&entry->addr, info, entry->addr.port))
> +			return entry;
> +	}
> +	return NULL;
> +}
> +
> static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
> {
> 	struct sock *sk = (struct sock *)msk;
> @@ -556,7 +568,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
>
> 		mptcp_local_address((struct sock_common *)msk->first, &mpc_addr);
> 		rcu_read_lock();
> -		entry = __lookup_addr(pernet, &mpc_addr);
> +		entry = __lookup_addr_rcu(pernet, &mpc_addr);
> 		if (entry) {
> 			__clear_bit(entry->addr.id, msk->pm.id_avail_bitmap);
> 			msk->mpc_endpoint_id = entry->addr.id;
>
> ---
> base-commit: 66bee427d4db76bd1afbf0184a2fd1f238a4ee27
> change-id: 20241022-mptcp-pm-lookup_addr_rcu-01833ea95155
>
> Best regards,
> -- 
> Matthieu Baerts (NGI0) <matttbe@kernel.org>
>
>
>
Re: [PATCH mptcp-net] mptcp: pm: use _rcu variant under rcu_read_lock
Posted by Matthieu Baerts 3 months, 3 weeks ago
Hi Mat,

Thank you for the review!

On 23/10/2024 02:31, Mat Martineau wrote:
> On Tue, 22 Oct 2024, Matthieu Baerts (NGI0) wrote:
> 
>> In mptcp_pm_create_subflow_or_signal_addr(), rcu_read_(un)lock() are
>> used as expected to iterate over the list of local addresses, but
>> list_for_each_entry() was used instead of list_for_each_entry_rcu() in
>> __lookup_addr() (and lookup_id_by_addr() before). It is important to use
>> this variant which adds the required READ_ONCE() (and diagnostic checks
>> if enabled).
>>
>> Because __lookup_addr() is also used in mptcp_pm_nl_set_flags() where it
>> is called under the pernet->lock because the returned entry might be
>> modified, the _rcu variant cannot be used in all cases. It is then
>> required to create a new helper. Note that this new helper can be reused
>> later to reduce some duplicated code elsewhere in this file.
> 
> I'm not sure this is the case, if you add rcu read lock/unlocks to
> mptcp_pm_nl_set_flags() then the _rcu variant could be used there (and
> then only one __lookup_addr() would be needed).

If I'm not mistaken, I don't think we can add rcu_read_(un)lock() to
mptcp_pm_nl_set_flags(), because it is not only doing a read of the
data, right?

On the other hand, when re-reading the code, in some conditions,
entry->flags will be modified directly. It is done under the pernet
lock, but it is not done by creating a new entry, and use
list_replace_rcu(). Is it not what we are supposed to do, otherwise
there is still a risk of data race with readers of entry->flags, no?

(Or maybe this data race is not an issue here? But even if it is not an
issue, can we really use rcu_read_lock() here with the pernet lock to
update an item of the list?)

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.