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

Commit Message

Matthieu Baerts (NGI0) Oct. 22, 2024, 8:49 p.m. UTC

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) <[e-mail]>
---
 net/mptcp/pm_netlink.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)


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

Best regards,

Comments

MPTCP CI Oct. 22, 2024, 10:02 p.m. UTC | #1

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/11468538757

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/4c879c6d556f
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=901964


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Mat Martineau Oct. 23, 2024, 12:31 a.m. UTC | #2

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) <[e-mail]>
> ---
> 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) <[e-mail]>
>
>
>

Matthieu Baerts (NGI0) Oct. 23, 2024, 10:52 a.m. UTC | #3

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

Patch

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;