[PATCH mptcp-net v2 0/3] mptcp: pm: use _rcu variant under rcu_read_lock

[PATCH mptcp-net v2 0/3] mptcp: pm: use _rcu variant under rcu_read_lock

[Matthieu Baerts (NGI0)]

When looking at something else, I noticed that the local endpoint
entries list was iterated under rcu_read_lock, but using
list_for_each_entry() instead of the _rcu variant. That's what patch 1
is fixing.

At the previous meeting, Mat and Christoph mentioned we should also use
this _rcu variant in mptcp_pm_nl_set_flags(). But then the modifications
look too important for -net, because __lookup_addr_by_id() also needs to
be modified, and similar parts in the code as well to harmonise how
entries are read from the list. That's what is done in patch 2, for
-next then.

Patch 3 is a simple change to remove duplicated code.

Note: I see that we are using spin_lock_bh(), but the RCU read "locks"
are always used without the _bh() variant. Is that OK here, or did we
miss something?

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Changes in v2:
- Add patch 2 and 3
- Patch 1: avoid > 80 chars per line in __lookup_addr_rcu() + update
  commit message.
- Link to v1: https://lore.kernel.org/r/20241022-mptcp-pm-lookup_addr_rcu-v1-1-19d45f26c872@kernel.org

---
Geliang Tang (1):
      mptcp: pm: avoid code duplication to lookup endp

Matthieu Baerts (NGI0) (2):
      mptcp: pm: use _rcu variant under rcu_read_lock
      mptcp: pm: lockless list traversal

 net/mptcp/pm_netlink.c | 69 +++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 31 deletions(-)
---
base-commit: c2990db510fcdde78645791def80551c0f77ff76
change-id: 20241022-mptcp-pm-lookup_addr_rcu-01833ea95155

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>

[PATCH mptcp-net v2 1/3] mptcp: pm: use _rcu variant under rcu_read_lock

[Matthieu Baerts (NGI0)]

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, the _rcu variant cannot be used in all
cases. A new helper is then created.

Note that this new helper can be reused later to reduce some duplicated
code elsewhere in this file, and some sections could be used lockless,
also using this new helper then. But all of these extra modifications
should probably be better considered as new improvements, and not as
fixes.

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 | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 618289aac0ab7f558d55d8b2ebb00dc62fc72f88..a93b9b7776b48781a883673fe5fd521a978487ff 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -531,6 +531,19 @@ __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 +569,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;

-- 
2.45.2

[PATCH mptcp-net v2 2/3] mptcp: pm: lockless list traversal

[Matthieu Baerts (NGI0)]

In a few places -- to get an endpoint, dump all of them, and change
their flags -- the list is iterated while holding the pernet->lock, but
only to read the content of the list. In these cases, we can replace the
spin locks, by RCU read ones, and use the _rcu variants to iterate over
the entries list in a lockless way.

To make it clear, the lookup helpers using the _rcu variant are renamed
with a _rcu suffix. The previous __lookup_addr() helper can then be
removed, but __lookup_addr_by_id() is still needed.

While at it, the IDs bitmap is copied before iterating the list to dump
the different addresses, to avoid any consistencies.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Notes:
 - This is not a fix, a small improvement for -next.
---
 net/mptcp/pm_netlink.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index a93b9b7776b48781a883673fe5fd521a978487ff..f38e1ccd34e95cd88b179a8b50e6965731542871 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -520,12 +520,12 @@ __lookup_addr_by_id(struct pm_nl_pernet *pernet, unsigned int id)
 }
 
 static struct mptcp_pm_addr_entry *
-__lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info)
+__lookup_addr_by_id_rcu(struct pm_nl_pernet *pernet, unsigned int id)
 {
 	struct mptcp_pm_addr_entry *entry;
 
-	list_for_each_entry(entry, &pernet->local_addr_list, list) {
-		if (mptcp_addresses_equal(&entry->addr, info, entry->addr.port))
+	list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
+		if (entry->addr.id == id)
 			return entry;
 	}
 	return NULL;
@@ -1836,8 +1836,8 @@ int mptcp_pm_nl_get_addr(struct sk_buff *skb, struct genl_info *info)
 		goto fail;
 	}
 
-	spin_lock_bh(&pernet->lock);
-	entry = __lookup_addr_by_id(pernet, addr.addr.id);
+	rcu_read_lock();
+	entry = __lookup_addr_by_id_rcu(pernet, addr.addr.id);
 	if (!entry) {
 		GENL_SET_ERR_MSG(info, "address not found");
 		ret = -EINVAL;
@@ -1850,11 +1850,11 @@ int mptcp_pm_nl_get_addr(struct sk_buff *skb, struct genl_info *info)
 
 	genlmsg_end(msg, reply);
 	ret = genlmsg_reply(msg, info);
-	spin_unlock_bh(&pernet->lock);
+	rcu_read_unlock();
 	return ret;
 
 unlock_fail:
-	spin_unlock_bh(&pernet->lock);
+	rcu_read_unlock();
 
 fail:
 	nlmsg_free(msg);
@@ -1872,16 +1872,18 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
 	struct net *net = sock_net(msg->sk);
 	struct mptcp_pm_addr_entry *entry;
 	struct pm_nl_pernet *pernet;
+	unsigned long id_bitmap[4];
 	int id = cb->args[0];
 	void *hdr;
 	int i;
 
 	pernet = pm_nl_get_pernet(net);
+	bitmap_copy(id_bitmap, pernet->id_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
 
-	spin_lock_bh(&pernet->lock);
+	rcu_read_lock();
 	for (i = id; i < MPTCP_PM_MAX_ADDR_ID + 1; i++) {
-		if (test_bit(i, pernet->id_bitmap)) {
-			entry = __lookup_addr_by_id(pernet, i);
+		if (test_bit(i, id_bitmap)) {
+			entry = __lookup_addr_by_id_rcu(pernet, i);
 			if (!entry)
 				break;
 
@@ -1903,7 +1905,7 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
 			genlmsg_end(msg, hdr);
 		}
 	}
-	spin_unlock_bh(&pernet->lock);
+	rcu_read_unlock();
 
 	cb->args[0] = id;
 	return msg->len;
@@ -2060,17 +2062,17 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
 	if (addr.flags & MPTCP_PM_ADDR_FLAG_BACKUP)
 		bkup = 1;
 
-	spin_lock_bh(&pernet->lock);
-	entry = lookup_by_id ? __lookup_addr_by_id(pernet, addr.addr.id) :
-			       __lookup_addr(pernet, &addr.addr);
+	rcu_read_lock();
+	entry = lookup_by_id ? __lookup_addr_by_id_rcu(pernet, addr.addr.id) :
+			       __lookup_addr_rcu(pernet, &addr.addr);
 	if (!entry) {
-		spin_unlock_bh(&pernet->lock);
+		rcu_read_unlock();
 		GENL_SET_ERR_MSG(info, "address not found");
 		return -EINVAL;
 	}
 	if ((addr.flags & MPTCP_PM_ADDR_FLAG_FULLMESH) &&
 	    (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
-		spin_unlock_bh(&pernet->lock);
+		rcu_read_unlock();
 		GENL_SET_ERR_MSG(info, "invalid addr flags");
 		return -EINVAL;
 	}
@@ -2078,7 +2080,7 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
 	changed = (addr.flags ^ entry->flags) & mask;
 	entry->flags = (entry->flags & ~mask) | (addr.flags & mask);
 	addr = *entry;
-	spin_unlock_bh(&pernet->lock);
+	rcu_read_unlock();
 
 	mptcp_nl_set_flags(net, &addr.addr, bkup, changed);
 	return 0;

-- 
2.45.2

[PATCH mptcp-net v2 3/3] mptcp: pm: avoid code duplication to lookup endp

[Matthieu Baerts (NGI0)]

From: Geliang Tang <tanggeliang@kylinos.cn>

The helper __lookup_addr_rcu() can be used in mptcp_pm_nl_get_local_id()
and mptcp_pm_nl_is_backup() to simplify the code, and avoid code
duplication.

Co-developed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
Notes:
 - This is also for -next.
---
 net/mptcp/pm_netlink.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index f38e1ccd34e95cd88b179a8b50e6965731542871..7c6e664b236d1659a554d003c78c72ec91895ba5 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1154,17 +1154,13 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc
 {
 	struct mptcp_pm_addr_entry *entry;
 	struct pm_nl_pernet *pernet;
-	int ret = -1;
+	int ret;
 
 	pernet = pm_nl_get_pernet_from_msk(msk);
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
-		if (mptcp_addresses_equal(&entry->addr, skc, entry->addr.port)) {
-			ret = entry->addr.id;
-			break;
-		}
-	}
+	entry = __lookup_addr_rcu(pernet, skc);
+	ret = entry ? entry->addr.id : -1;
 	rcu_read_unlock();
 	if (ret >= 0)
 		return ret;
@@ -1191,15 +1187,11 @@ bool mptcp_pm_nl_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc)
 {
 	struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
 	struct mptcp_pm_addr_entry *entry;
-	bool backup = false;
+	bool backup;
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
-		if (mptcp_addresses_equal(&entry->addr, skc, entry->addr.port)) {
-			backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
-			break;
-		}
-	}
+	entry = __lookup_addr_rcu(pernet, skc);
+	backup = entry && !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
 	rcu_read_unlock();
 
 	return backup;

-- 
2.45.2

Re: [PATCH mptcp-net v2 3/3] mptcp: pm: avoid code duplication to lookup endp

[Geliang Tang]

Hi Matt,

Thanks for this patch.

On Fri, 2024-10-25 at 11:32 +0200, Matthieu Baerts (NGI0) wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The helper __lookup_addr_rcu() can be used in
> mptcp_pm_nl_get_local_id()
> and mptcp_pm_nl_is_backup() to simplify the code, and avoid code
> duplication.
> 
> Co-developed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> Notes:
>  - This is also for -next.
> ---
>  net/mptcp/pm_netlink.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index
> f38e1ccd34e95cd88b179a8b50e6965731542871..7c6e664b236d1659a554d003c78
> c72ec91895ba5 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1154,17 +1154,13 @@ int mptcp_pm_nl_get_local_id(struct
> mptcp_sock *msk, struct mptcp_addr_info *skc
>  {
>  	struct mptcp_pm_addr_entry *entry;
>  	struct pm_nl_pernet *pernet;
> -	int ret = -1;
> +	int ret;
>  
>  	pernet = pm_nl_get_pernet_from_msk(msk);
>  
>  	rcu_read_lock();
> -	list_for_each_entry_rcu(entry, &pernet->local_addr_list,
> list) {
> -		if (mptcp_addresses_equal(&entry->addr, skc, entry-
> >addr.port)) {
> -			ret = entry->addr.id;
> -			break;
> -		}
> -	}
> +	entry = __lookup_addr_rcu(pernet, skc);
> +	ret = entry ? entry->addr.id : -1;
>  	rcu_read_unlock();
>  	if (ret >= 0)
>  		return ret;
> @@ -1191,15 +1187,11 @@ bool mptcp_pm_nl_is_backup(struct mptcp_sock
> *msk, struct mptcp_addr_info *skc)
>  {
>  	struct pm_nl_pernet *pernet =
> pm_nl_get_pernet_from_msk(msk);
>  	struct mptcp_pm_addr_entry *entry;
> -	bool backup = false;
> +	bool backup;
>  
>  	rcu_read_lock();
> -	list_for_each_entry_rcu(entry, &pernet->local_addr_list,
> list) {
> -		if (mptcp_addresses_equal(&entry->addr, skc, entry-
> >addr.port)) {
> -			backup = !!(entry->flags &
> MPTCP_PM_ADDR_FLAG_BACKUP);
> -			break;
> -		}
> -	}
> +	entry = __lookup_addr_rcu(pernet, skc);
> +	backup = entry && !!(entry->flags &
> MPTCP_PM_ADDR_FLAG_BACKUP);

I think we should check whether entry is NULL here too. No?

-Geliang

>  	rcu_read_unlock();
>  
>  	return backup;
> 

Re: [PATCH mptcp-net v2 3/3] mptcp: pm: avoid code duplication to lookup endp

[Matthieu Baerts]

Hi Geliang,

Thank you for the review!

On 25/10/2024 12:37, Geliang Tang wrote:
> On Fri, 2024-10-25 at 11:32 +0200, Matthieu Baerts (NGI0) wrote:
>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>
>> The helper __lookup_addr_rcu() can be used in
>> mptcp_pm_nl_get_local_id()
>> and mptcp_pm_nl_is_backup() to simplify the code, and avoid code
>> duplication.

(...)

>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>> index
>> f38e1ccd34e95cd88b179a8b50e6965731542871..7c6e664b236d1659a554d003c78
>> c72ec91895ba5 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c

(...)

>> @@ -1191,15 +1187,11 @@ bool mptcp_pm_nl_is_backup(struct mptcp_sock
>> *msk, struct mptcp_addr_info *skc)
>>  {
>>  	struct pm_nl_pernet *pernet =
>> pm_nl_get_pernet_from_msk(msk);
>>  	struct mptcp_pm_addr_entry *entry;
>> -	bool backup = false;
>> +	bool backup;
>>  
>>  	rcu_read_lock();
>> -	list_for_each_entry_rcu(entry, &pernet->local_addr_list,
>> list) {
>> -		if (mptcp_addresses_equal(&entry->addr, skc, entry-
>>> addr.port)) {
>> -			backup = !!(entry->flags &
>> MPTCP_PM_ADDR_FLAG_BACKUP);
>> -			break;
>> -		}
>> -	}
>> +	entry = __lookup_addr_rcu(pernet, skc);
>> +	backup = entry && !!(entry->flags &
>> MPTCP_PM_ADDR_FLAG_BACKUP);
> 
> I think we should check whether entry is NULL here too. No?

Yes, but that's what I did, no?

  backup = entry && (entry->flags & BACKUP)

"backup" is set to "true" if entry is not NULL and the backup flag is set.

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

Re: [PATCH mptcp-net v2 3/3] mptcp: pm: avoid code duplication to lookup endp

[Geliang Tang]

On Fri, 2024-10-25 at 18:37 +0800, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for this patch.
> 
> On Fri, 2024-10-25 at 11:32 +0200, Matthieu Baerts (NGI0) wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > The helper __lookup_addr_rcu() can be used in
> > mptcp_pm_nl_get_local_id()
> > and mptcp_pm_nl_is_backup() to simplify the code, and avoid code
> > duplication.
> > 
> > Co-developed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> > Notes:
> >  - This is also for -next.
> > ---
> >  net/mptcp/pm_netlink.c | 20 ++++++--------------
> >  1 file changed, 6 insertions(+), 14 deletions(-)
> > 
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index
> > f38e1ccd34e95cd88b179a8b50e6965731542871..7c6e664b236d1659a554d003c
> > 78
> > c72ec91895ba5 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -1154,17 +1154,13 @@ int mptcp_pm_nl_get_local_id(struct
> > mptcp_sock *msk, struct mptcp_addr_info *skc
> >  {
> >  	struct mptcp_pm_addr_entry *entry;
> >  	struct pm_nl_pernet *pernet;
> > -	int ret = -1;
> > +	int ret;
> >  
> >  	pernet = pm_nl_get_pernet_from_msk(msk);
> >  
> >  	rcu_read_lock();
> > -	list_for_each_entry_rcu(entry, &pernet->local_addr_list,
> > list) {
> > -		if (mptcp_addresses_equal(&entry->addr, skc,
> > entry-
> > > addr.port)) {
> > -			ret = entry->addr.id;
> > -			break;
> > -		}
> > -	}
> > +	entry = __lookup_addr_rcu(pernet, skc);
> > +	ret = entry ? entry->addr.id : -1;
> >  	rcu_read_unlock();
> >  	if (ret >= 0)
> >  		return ret;
> > @@ -1191,15 +1187,11 @@ bool mptcp_pm_nl_is_backup(struct
> > mptcp_sock
> > *msk, struct mptcp_addr_info *skc)
> >  {
> >  	struct pm_nl_pernet *pernet =
> > pm_nl_get_pernet_from_msk(msk);
> >  	struct mptcp_pm_addr_entry *entry;
> > -	bool backup = false;
> > +	bool backup;
> >  
> >  	rcu_read_lock();
> > -	list_for_each_entry_rcu(entry, &pernet->local_addr_list,
> > list) {
> > -		if (mptcp_addresses_equal(&entry->addr, skc,
> > entry-
> > > addr.port)) {
> > -			backup = !!(entry->flags &
> > MPTCP_PM_ADDR_FLAG_BACKUP);
> > -			break;
> > -		}
> > -	}
> > +	entry = __lookup_addr_rcu(pernet, skc);
> > +	backup = entry && !!(entry->flags &
> > MPTCP_PM_ADDR_FLAG_BACKUP);
> 
> I think we should check whether entry is NULL here too. No?

Sorry, ignore my comment, your code is correct. :)

-Geliang

> 
> -Geliang
> 
> >  	rcu_read_unlock();
> >  
> >  	return backup;
> > 
> 
> 

Re: [PATCH mptcp-net v2 0/3] mptcp: pm: use _rcu variant under rcu_read_lock

[MPTCP CI]

Hi Geliang,

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/11515979216

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


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)

Re: [PATCH mptcp-net v2 2/3] mptcp: pm: lockless list traversal

[Geliang Tang]

Hi Matt,

Thanks for this patch.

On Fri, 2024-10-25 at 11:32 +0200, Matthieu Baerts (NGI0) wrote:
> In a few places -- to get an endpoint, dump all of them, and change
> their flags -- the list is iterated while holding the pernet->lock,
> but
> only to read the content of the list. In these cases, we can replace
> the
> spin locks, by RCU read ones, and use the _rcu variants to iterate
> over
> the entries list in a lockless way.
> 
> To make it clear, the lookup helpers using the _rcu variant are
> renamed
> with a _rcu suffix. The previous __lookup_addr() helper can then be
> removed, but __lookup_addr_by_id() is still needed.
> 
> While at it, the IDs bitmap is copied before iterating the list to
> dump
> the different addresses, to avoid any consistencies.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Notes:
>  - This is not a fix, a small improvement for -next.
> ---
>  net/mptcp/pm_netlink.c | 36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index
> a93b9b7776b48781a883673fe5fd521a978487ff..f38e1ccd34e95cd88b179a8b50e
> 6965731542871 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -520,12 +520,12 @@ __lookup_addr_by_id(struct pm_nl_pernet
> *pernet, unsigned int id)
>  }
>  
>  static struct mptcp_pm_addr_entry *
> -__lookup_addr(struct pm_nl_pernet *pernet, const struct
> mptcp_addr_info *info)
> +__lookup_addr_by_id_rcu(struct pm_nl_pernet *pernet, unsigned int
> id)
>  {
>  	struct mptcp_pm_addr_entry *entry;
>  
> -	list_for_each_entry(entry, &pernet->local_addr_list, list) {
> -		if (mptcp_addresses_equal(&entry->addr, info, entry-
> >addr.port))
> +	list_for_each_entry_rcu(entry, &pernet->local_addr_list,
> list) {
> +		if (entry->addr.id == id)
>  			return entry;
>  	}
>  	return NULL;
> @@ -1836,8 +1836,8 @@ int mptcp_pm_nl_get_addr(struct sk_buff *skb,
> struct genl_info *info)
>  		goto fail;
>  	}
>  
> -	spin_lock_bh(&pernet->lock);
> -	entry = __lookup_addr_by_id(pernet, addr.addr.id);
> +	rcu_read_lock();
> +	entry = __lookup_addr_by_id_rcu(pernet, addr.addr.id);
>  	if (!entry) {
>  		GENL_SET_ERR_MSG(info, "address not found");
>  		ret = -EINVAL;
> @@ -1850,11 +1850,11 @@ int mptcp_pm_nl_get_addr(struct sk_buff *skb,
> struct genl_info *info)
>  
>  	genlmsg_end(msg, reply);
>  	ret = genlmsg_reply(msg, info);
> -	spin_unlock_bh(&pernet->lock);
> +	rcu_read_unlock();
>  	return ret;
>  
>  unlock_fail:
> -	spin_unlock_bh(&pernet->lock);
> +	rcu_read_unlock();
>  
>  fail:
>  	nlmsg_free(msg);
> @@ -1872,16 +1872,18 @@ int mptcp_pm_nl_dump_addr(struct sk_buff
> *msg,
>  	struct net *net = sock_net(msg->sk);
>  	struct mptcp_pm_addr_entry *entry;
>  	struct pm_nl_pernet *pernet;
> +	unsigned long id_bitmap[4];
>  	int id = cb->args[0];
>  	void *hdr;
>  	int i;
>  
>  	pernet = pm_nl_get_pernet(net);
> +	bitmap_copy(id_bitmap, pernet->id_bitmap,
> MPTCP_PM_MAX_ADDR_ID + 1);

I think the id bitmap should only be copied when id is 0:

if (!id)
  bitmap_copy(id_bitmap, pernet->id_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);

Since this mptcp_pm_nl_dump_addr() may be called repeatedly when the
dump information is very long. We only copy it the first time it is
called, and subsequent calls cannot copy it again. WDYT?

Thanks,
-Geliang

>  
> -	spin_lock_bh(&pernet->lock);
> +	rcu_read_lock();
>  	for (i = id; i < MPTCP_PM_MAX_ADDR_ID + 1; i++) {
> -		if (test_bit(i, pernet->id_bitmap)) {
> -			entry = __lookup_addr_by_id(pernet, i);
> +		if (test_bit(i, id_bitmap)) {
> +			entry = __lookup_addr_by_id_rcu(pernet, i);
>  			if (!entry)
>  				break;
>  
> @@ -1903,7 +1905,7 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
>  			genlmsg_end(msg, hdr);
>  		}
>  	}
> -	spin_unlock_bh(&pernet->lock);
> +	rcu_read_unlock();
>  
>  	cb->args[0] = id;
>  	return msg->len;
> @@ -2060,17 +2062,17 @@ int mptcp_pm_nl_set_flags(struct sk_buff
> *skb, struct genl_info *info)
>  	if (addr.flags & MPTCP_PM_ADDR_FLAG_BACKUP)
>  		bkup = 1;
>  
> -	spin_lock_bh(&pernet->lock);
> -	entry = lookup_by_id ? __lookup_addr_by_id(pernet,
> addr.addr.id) :
> -			       __lookup_addr(pernet, &addr.addr);
> +	rcu_read_lock();
> +	entry = lookup_by_id ? __lookup_addr_by_id_rcu(pernet,
> addr.addr.id) :
> +			       __lookup_addr_rcu(pernet,
> &addr.addr);
>  	if (!entry) {
> -		spin_unlock_bh(&pernet->lock);
> +		rcu_read_unlock();
>  		GENL_SET_ERR_MSG(info, "address not found");
>  		return -EINVAL;
>  	}
>  	if ((addr.flags & MPTCP_PM_ADDR_FLAG_FULLMESH) &&
>  	    (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
> -		spin_unlock_bh(&pernet->lock);
> +		rcu_read_unlock();
>  		GENL_SET_ERR_MSG(info, "invalid addr flags");
>  		return -EINVAL;
>  	}
> @@ -2078,7 +2080,7 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb,
> struct genl_info *info)
>  	changed = (addr.flags ^ entry->flags) & mask;
>  	entry->flags = (entry->flags & ~mask) | (addr.flags & mask);
>  	addr = *entry;
> -	spin_unlock_bh(&pernet->lock);
> +	rcu_read_unlock();
>  
>  	mptcp_nl_set_flags(net, &addr.addr, bkup, changed);
>  	return 0;
> 

Re: [PATCH mptcp-net v2 2/3] mptcp: pm: lockless list traversal

[Matthieu Baerts]

On 25/10/2024 11:32, Matthieu Baerts (NGI0) wrote:
> In a few places -- to get an endpoint, dump all of them, and change
> their flags -- the list is iterated while holding the pernet->lock, but
> only to read the content of the list. In these cases, we can replace the
> spin locks, by RCU read ones, and use the _rcu variants to iterate over
> the entries list in a lockless way.
> 
> To make it clear, the lookup helpers using the _rcu variant are renamed
> with a _rcu suffix. The previous __lookup_addr() helper can then be
> removed, but __lookup_addr_by_id() is still needed.
> 
> While at it, the IDs bitmap is copied before iterating the list to dump
> the different addresses, to avoid any consistencies.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Notes:
>  - This is not a fix, a small improvement for -next.
> ---
>  net/mptcp/pm_netlink.c | 36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index a93b9b7776b48781a883673fe5fd521a978487ff..f38e1ccd34e95cd88b179a8b50e6965731542871 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -520,12 +520,12 @@ __lookup_addr_by_id(struct pm_nl_pernet *pernet, unsigned int id)
>  }
>  
>  static struct mptcp_pm_addr_entry *
> -__lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info)
> +__lookup_addr_by_id_rcu(struct pm_nl_pernet *pernet, unsigned int id)
>  {
>  	struct mptcp_pm_addr_entry *entry;
>  
> -	list_for_each_entry(entry, &pernet->local_addr_list, list) {
> -		if (mptcp_addresses_equal(&entry->addr, info, entry->addr.port))
> +	list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {

I didn't see the following in the RCU doc -- but it is quite big, so I
probably missed it -- but I suppose we don't need to keep the two
helpers here with and without the _rcu variant (__lookup_addr_by_id())
if here we add an extra lockdep condition:

  list_for_each_entry_rcu(entry, &pernet->local_addr_list, list,
                          lockdep_is_held(&pernet->lock)) {

WDYT?

> +		if (entry->addr.id == id)
>  			return entry;
>  	}
>  	return NULL;

(...)

> @@ -1872,16 +1872,18 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
>  	struct net *net = sock_net(msg->sk);
>  	struct mptcp_pm_addr_entry *entry;
>  	struct pm_nl_pernet *pernet;
> +	unsigned long id_bitmap[4];

Oops, I left my temp declaration, it should be the following line, which
will result in the same thing anyway on x86_64, no need to re-run the
tests → I can fix that when applying the patches, or in a future version:

  DECLARE_BITMAP(id_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);

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

Re: [PATCH mptcp-net v2 2/3] mptcp: pm: lockless list traversal

[Matthieu Baerts]

Hi Geliang,

Thank you for the review!

On 25/10/2024 16:25, Geliang Tang wrote:
> On Fri, 2024-10-25 at 11:32 +0200, Matthieu Baerts (NGI0) wrote:
>> In a few places -- to get an endpoint, dump all of them, and change
>> their flags -- the list is iterated while holding the pernet->lock,
>> but
>> only to read the content of the list. In these cases, we can replace
>> the
>> spin locks, by RCU read ones, and use the _rcu variants to iterate
>> over
>> the entries list in a lockless way.
>>
>> To make it clear, the lookup helpers using the _rcu variant are
>> renamed
>> with a _rcu suffix. The previous __lookup_addr() helper can then be
>> removed, but __lookup_addr_by_id() is still needed.
>>
>> While at it, the IDs bitmap is copied before iterating the list to
>> dump
>> the different addresses, to avoid any consistencies.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> Notes:
>>  - This is not a fix, a small improvement for -next.
>> ---
>>  net/mptcp/pm_netlink.c | 36 +++++++++++++++++++-----------------
>>  1 file changed, 19 insertions(+), 17 deletions(-)
>>
>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>> index
>> a93b9b7776b48781a883673fe5fd521a978487ff..f38e1ccd34e95cd88b179a8b50e
>> 6965731542871 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c

(...)

>> @@ -1872,16 +1872,18 @@ int mptcp_pm_nl_dump_addr(struct sk_buff
>> *msg,
>>  	struct net *net = sock_net(msg->sk);
>>  	struct mptcp_pm_addr_entry *entry;
>>  	struct pm_nl_pernet *pernet;
>> +	unsigned long id_bitmap[4];
>>  	int id = cb->args[0];
>>  	void *hdr;
>>  	int i;
>>  
>>  	pernet = pm_nl_get_pernet(net);
>> +	bitmap_copy(id_bitmap, pernet->id_bitmap,
>> MPTCP_PM_MAX_ADDR_ID + 1);
> 
> I think the id bitmap should only be copied when id is 0:
> 
> if (!id)
>   bitmap_copy(id_bitmap, pernet->id_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
> 
> Since this mptcp_pm_nl_dump_addr() may be called repeatedly when the
> dump information is very long. We only copy it the first time it is
> called, and subsequent calls cannot copy it again. WDYT?

Sorry, I'm not sure to understand. Here, I did a local copy of
'id_bitmap' just to keep a certain consistency if the bitmap is changed
during the RCU read section below: not to have this bitmap being changed
during the for loop here below. Before, we had this protection because
pernet->lock was held.

Are you suggesting here to keep a copy in the cb, not a local one, to do
the copy only once? Are you sure it is worth it (and safe)?

>>  
>> -	spin_lock_bh(&pernet->lock);
>> +	rcu_read_lock();
>>  	for (i = id; i < MPTCP_PM_MAX_ADDR_ID + 1; i++) {
>> -		if (test_bit(i, pernet->id_bitmap)) {
>> -			entry = __lookup_addr_by_id(pernet, i);
>> +		if (test_bit(i, id_bitmap)) {
>> +			entry = __lookup_addr_by_id_rcu(pernet, i);
>>  			if (!entry)
>>  				break;
>>  
Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net v2 2/3] mptcp: pm: lockless list traversal

[Geliang Tang]

Hi Matt,

On Fri, 2024-10-25 at 17:26 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for the review!
> 
> On 25/10/2024 16:25, Geliang Tang wrote:
> > On Fri, 2024-10-25 at 11:32 +0200, Matthieu Baerts (NGI0) wrote:
> > > In a few places -- to get an endpoint, dump all of them, and
> > > change
> > > their flags -- the list is iterated while holding the pernet-
> > > >lock,
> > > but
> > > only to read the content of the list. In these cases, we can
> > > replace
> > > the
> > > spin locks, by RCU read ones, and use the _rcu variants to
> > > iterate
> > > over
> > > the entries list in a lockless way.
> > > 
> > > To make it clear, the lookup helpers using the _rcu variant are
> > > renamed
> > > with a _rcu suffix. The previous __lookup_addr() helper can then
> > > be
> > > removed, but __lookup_addr_by_id() is still needed.
> > > 
> > > While at it, the IDs bitmap is copied before iterating the list
> > > to
> > > dump
> > > the different addresses, to avoid any consistencies.
> > > 
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > > Notes:
> > >  - This is not a fix, a small improvement for -next.
> > > ---
> > >  net/mptcp/pm_netlink.c | 36 +++++++++++++++++++-----------------
> > >  1 file changed, 19 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > > index
> > > a93b9b7776b48781a883673fe5fd521a978487ff..f38e1ccd34e95cd88b179a8
> > > b50e
> > > 6965731542871 100644
> > > --- a/net/mptcp/pm_netlink.c
> > > +++ b/net/mptcp/pm_netlink.c
> 
> (...)
> 
> > > @@ -1872,16 +1872,18 @@ int mptcp_pm_nl_dump_addr(struct sk_buff
> > > *msg,
> > >  	struct net *net = sock_net(msg->sk);
> > >  	struct mptcp_pm_addr_entry *entry;
> > >  	struct pm_nl_pernet *pernet;
> > > +	unsigned long id_bitmap[4];
> > >  	int id = cb->args[0];
> > >  	void *hdr;
> > >  	int i;
> > >  
> > >  	pernet = pm_nl_get_pernet(net);
> > > +	bitmap_copy(id_bitmap, pernet->id_bitmap,
> > > MPTCP_PM_MAX_ADDR_ID + 1);
> > 
> > I think the id bitmap should only be copied when id is 0:
> > 
> > if (!id)
> >   bitmap_copy(id_bitmap, pernet->id_bitmap, MPTCP_PM_MAX_ADDR_ID +
> > 1);
> > 
> > Since this mptcp_pm_nl_dump_addr() may be called repeatedly when
> > the
> > dump information is very long. We only copy it the first time it is

A correction is needed here. Regardless of whether the dump information
is large or small, the dumpit function will be called repeatedly when
dumpit returns non-zero. The loop stops when dumpit returns 0.

> > called, and subsequent calls cannot copy it again. WDYT?
> 
> Sorry, I'm not sure to understand. Here, I did a local copy of
> 'id_bitmap' just to keep a certain consistency if the bitmap is
> changed
> during the RCU read section below: not to have this bitmap being
> changed
> during the for loop here below. Before, we had this protection
> because
> pernet->lock was held.

It is precisely because we need to maintain this consistency that we
need to copy the bitmap only once. If we copy the bitmap when dumpit is
called a second time, the bitmap obtained at this time is a new bitmap,
which destroys this consistency, right?

> 
> Are you suggesting here to keep a copy in the cb, not a local one, to
> do
> the copy only once? Are you sure it is worth it (and safe)?

Because dumpit will be called repeatedly, we need to ensure that the
same bitmap is accessed each time, so we cannot use a local one, but
need to keep a copy in the cb just like what we do in
mptcp_userspace_pm_dump_addr.


In addition, when doing bitmap_copy, we need to hold pernet->lock:

bitmap = (struct mptcp_id_bitmap *)cb->ctx;

if (!id) {
  spin_lock_bh(&pernet->lock);
  bitmap_copy(id_bitmap, pernet->id_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
  spin_unlock_bh(&pernet->lock);
}

Also, we can put this bitmap copy code into a separate patch, which is
applied before this one. In this patch we only need to replace
spin_lock_bh with rcu_read_lock.

WDYT?

Thanks,
-Geliang

> 
> > >  
> > > -	spin_lock_bh(&pernet->lock);
> > > +	rcu_read_lock();
> > >  	for (i = id; i < MPTCP_PM_MAX_ADDR_ID + 1; i++) {
> > > -		if (test_bit(i, pernet->id_bitmap)) {
> > > -			entry = __lookup_addr_by_id(pernet, i);
> > > +		if (test_bit(i, id_bitmap)) {
> > > +			entry = __lookup_addr_by_id_rcu(pernet,
> > > i);
> > >  			if (!entry)
> > >  				break;
> > >  
> Cheers,
> Matt

Re: [PATCH mptcp-net v2 2/3] mptcp: pm: lockless list traversal

[Matthieu Baerts]

Hi Geliang,

On 28/10/2024 03:08, Geliang Tang wrote:
> Hi Matt,
> 
> On Fri, 2024-10-25 at 17:26 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> Thank you for the review!
>>
>> On 25/10/2024 16:25, Geliang Tang wrote:
>>> On Fri, 2024-10-25 at 11:32 +0200, Matthieu Baerts (NGI0) wrote:
>>>> In a few places -- to get an endpoint, dump all of them, and
>>>> change
>>>> their flags -- the list is iterated while holding the pernet-
>>>>> lock,
>>>> but
>>>> only to read the content of the list. In these cases, we can
>>>> replace
>>>> the
>>>> spin locks, by RCU read ones, and use the _rcu variants to
>>>> iterate
>>>> over
>>>> the entries list in a lockless way.
>>>>
>>>> To make it clear, the lookup helpers using the _rcu variant are
>>>> renamed
>>>> with a _rcu suffix. The previous __lookup_addr() helper can then
>>>> be
>>>> removed, but __lookup_addr_by_id() is still needed.
>>>>
>>>> While at it, the IDs bitmap is copied before iterating the list
>>>> to
>>>> dump
>>>> the different addresses, to avoid any consistencies.
>>>>
>>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>>> ---
>>>> Notes:
>>>>  - This is not a fix, a small improvement for -next.
>>>> ---
>>>>  net/mptcp/pm_netlink.c | 36 +++++++++++++++++++-----------------
>>>>  1 file changed, 19 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>>> index
>>>> a93b9b7776b48781a883673fe5fd521a978487ff..f38e1ccd34e95cd88b179a8
>>>> b50e
>>>> 6965731542871 100644
>>>> --- a/net/mptcp/pm_netlink.c
>>>> +++ b/net/mptcp/pm_netlink.c
>>
>> (...)
>>
>>>> @@ -1872,16 +1872,18 @@ int mptcp_pm_nl_dump_addr(struct sk_buff
>>>> *msg,
>>>>  	struct net *net = sock_net(msg->sk);
>>>>  	struct mptcp_pm_addr_entry *entry;
>>>>  	struct pm_nl_pernet *pernet;
>>>> +	unsigned long id_bitmap[4];
>>>>  	int id = cb->args[0];
>>>>  	void *hdr;
>>>>  	int i;
>>>>  
>>>>  	pernet = pm_nl_get_pernet(net);
>>>> +	bitmap_copy(id_bitmap, pernet->id_bitmap,
>>>> MPTCP_PM_MAX_ADDR_ID + 1);
>>>
>>> I think the id bitmap should only be copied when id is 0:
>>>
>>> if (!id)
>>>   bitmap_copy(id_bitmap, pernet->id_bitmap, MPTCP_PM_MAX_ADDR_ID +
>>> 1);
>>>
>>> Since this mptcp_pm_nl_dump_addr() may be called repeatedly when
>>> the
>>> dump information is very long. We only copy it the first time it is
> 
> A correction is needed here. Regardless of whether the dump information
> is large or small, the dumpit function will be called repeatedly when
> dumpit returns non-zero. The loop stops when dumpit returns 0.
> 
>>> called, and subsequent calls cannot copy it again. WDYT?
>>
>> Sorry, I'm not sure to understand. Here, I did a local copy of
>> 'id_bitmap' just to keep a certain consistency if the bitmap is
>> changed
>> during the RCU read section below: not to have this bitmap being
>> changed
>> during the for loop here below. Before, we had this protection
>> because
>> pernet->lock was held.
> 
> It is precisely because we need to maintain this consistency that we
> need to copy the bitmap only once. If we copy the bitmap when dumpit is
> called a second time, the bitmap obtained at this time is a new bitmap,
> which destroys this consistency, right?

OK, but then I have a few questions:

- Was the code before my patch here prevented such consistency issues?
Or said differently: is there a regression introduced by this patch?

- Is it really an issue that can lead to a crash or reading freed info?

- Where would you store the copy of the bitmap? In cb-args?

- If we store this copy somewhere, could you not have situations where
the bitmap would no longer be in sync with the addresses that are stored
in 'pernet->local_addr_list', and then displaying wrong/freed info? It
looks like more would be needed to cope with that. And maybe that's not
worth it?

>> Are you suggesting here to keep a copy in the cb, not a local one, to
>> do
>> the copy only once? Are you sure it is worth it (and safe)?
> 
> Because dumpit will be called repeatedly, we need to ensure that the
> same bitmap is accessed each time, so we cannot use a local one, but
> need to keep a copy in the cb just like what we do in
> mptcp_userspace_pm_dump_addr.
> 
> 
> In addition, when doing bitmap_copy, we need to hold pernet->lock:
> 
> bitmap = (struct mptcp_id_bitmap *)cb->ctx;
> 
> if (!id) {
>   spin_lock_bh(&pernet->lock);
>   bitmap_copy(id_bitmap, pernet->id_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
>   spin_unlock_bh(&pernet->lock);
> }

Ah yes, maybe, I need to check: I thought the modification of the bitmap
was done atomically, and it was fine to read without the lock. But maybe
not because it is stored in multiple bytes.

> Also, we can put this bitmap copy code into a separate patch, which is
> applied before this one. In this patch we only need to replace
> spin_lock_bh with rcu_read_lock.

OK so we are talking about separate fix that is not directly linked to
my change, apart from the fact I modified code around, right?

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

Re: [PATCH mptcp-net v2 2/3] mptcp: pm: lockless list traversal

[Geliang Tang]

Hi Matt,

On Mon, 2024-10-28 at 12:48 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 28/10/2024 03:08, Geliang Tang wrote:
> > Hi Matt,
> > 
> > On Fri, 2024-10-25 at 17:26 +0200, Matthieu Baerts wrote:
> > > Hi Geliang,
> > > 
> > > Thank you for the review!
> > > 
> > > On 25/10/2024 16:25, Geliang Tang wrote:
> > > > On Fri, 2024-10-25 at 11:32 +0200, Matthieu Baerts (NGI0)
> > > > wrote:
> > > > > In a few places -- to get an endpoint, dump all of them, and
> > > > > change
> > > > > their flags -- the list is iterated while holding the pernet-
> > > > > > lock,
> > > > > but
> > > > > only to read the content of the list. In these cases, we can
> > > > > replace
> > > > > the
> > > > > spin locks, by RCU read ones, and use the _rcu variants to
> > > > > iterate
> > > > > over
> > > > > the entries list in a lockless way.
> > > > > 
> > > > > To make it clear, the lookup helpers using the _rcu variant
> > > > > are
> > > > > renamed
> > > > > with a _rcu suffix. The previous __lookup_addr() helper can
> > > > > then
> > > > > be
> > > > > removed, but __lookup_addr_by_id() is still needed.
> > > > > 
> > > > > While at it, the IDs bitmap is copied before iterating the
> > > > > list
> > > > > to
> > > > > dump
> > > > > the different addresses, to avoid any consistencies.
> > > > > 
> > > > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > > > ---
> > > > > Notes:
> > > > >  - This is not a fix, a small improvement for -next.
> > > > > ---
> > > > >  net/mptcp/pm_netlink.c | 36 +++++++++++++++++++-------------
> > > > > ----
> > > > >  1 file changed, 19 insertions(+), 17 deletions(-)
> > > > > 
> > > > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > > > > index
> > > > > a93b9b7776b48781a883673fe5fd521a978487ff..f38e1ccd34e95cd88b1
> > > > > 79a8
> > > > > b50e
> > > > > 6965731542871 100644
> > > > > --- a/net/mptcp/pm_netlink.c
> > > > > +++ b/net/mptcp/pm_netlink.c
> > > 
> > > (...)
> > > 
> > > > > @@ -1872,16 +1872,18 @@ int mptcp_pm_nl_dump_addr(struct
> > > > > sk_buff
> > > > > *msg,
> > > > >   struct net *net = sock_net(msg->sk);
> > > > >   struct mptcp_pm_addr_entry *entry;
> > > > >   struct pm_nl_pernet *pernet;
> > > > > + unsigned long id_bitmap[4];
> > > > >   int id = cb->args[0];
> > > > >   void *hdr;
> > > > >   int i;
> > > > >  
> > > > >   pernet = pm_nl_get_pernet(net);
> > > > > + bitmap_copy(id_bitmap, pernet->id_bitmap,
> > > > > MPTCP_PM_MAX_ADDR_ID + 1);
> > > > 
> > > > I think the id bitmap should only be copied when id is 0:
> > > > 
> > > > if (!id)
> > > >   bitmap_copy(id_bitmap, pernet->id_bitmap,
> > > > MPTCP_PM_MAX_ADDR_ID +
> > > > 1);
> > > > 
> > > > Since this mptcp_pm_nl_dump_addr() may be called repeatedly
> > > > when
> > > > the
> > > > dump information is very long. We only copy it the first time
> > > > it is
> > 
> > A correction is needed here. Regardless of whether the dump
> > information
> > is large or small, the dumpit function will be called repeatedly
> > when
> > dumpit returns non-zero. The loop stops when dumpit returns 0.
> > 
> > > > called, and subsequent calls cannot copy it again. WDYT?
> > > 
> > > Sorry, I'm not sure to understand. Here, I did a local copy of
> > > 'id_bitmap' just to keep a certain consistency if the bitmap is
> > > changed
> > > during the RCU read section below: not to have this bitmap being
> > > changed
> > > during the for loop here below. Before, we had this protection
> > > because
> > > pernet->lock was held.
> > 
> > It is precisely because we need to maintain this consistency that
> > we
> > need to copy the bitmap only once. If we copy the bitmap when
> > dumpit is
> > called a second time, the bitmap obtained at this time is a new
> > bitmap,
> > which destroys this consistency, right?
> 
> OK, but then I have a few questions:
> 
> - Was the code before my patch here prevented such consistency
> issues?
> Or said differently: is there a regression introduced by this patch?
> 
> - Is it really an issue that can lead to a crash or reading freed
> info?
> 
> - Where would you store the copy of the bitmap? In cb-args?
> 
> - If we store this copy somewhere, could you not have situations
> where
> the bitmap would no longer be in sync with the addresses that are
> stored
> in 'pernet->local_addr_list', and then displaying wrong/freed info?
> It
> looks like more would be needed to cope with that. And maybe that's
> not
> worth it?

Your doubts are very reasonable. It seems that I have not considered it
carefully enough. So we do need to define a local bitmap and copy it
every time. It's fine to me and it's OK to implement BPF path manager
based on it.

Thanks,
-Geliang

> 
> > > Are you suggesting here to keep a copy in the cb, not a local
> > > one, to
> > > do
> > > the copy only once? Are you sure it is worth it (and safe)?
> > 
> > Because dumpit will be called repeatedly, we need to ensure that
> > the
> > same bitmap is accessed each time, so we cannot use a local one,
> > but
> > need to keep a copy in the cb just like what we do in
> > mptcp_userspace_pm_dump_addr.
> > 
> > 
> > In addition, when doing bitmap_copy, we need to hold pernet->lock:
> > 
> > bitmap = (struct mptcp_id_bitmap *)cb->ctx;
> > 
> > if (!id) {
> >   spin_lock_bh(&pernet->lock);
> >   bitmap_copy(id_bitmap, pernet->id_bitmap, MPTCP_PM_MAX_ADDR_ID +
> > 1);
> >   spin_unlock_bh(&pernet->lock);
> > }
> 
> Ah yes, maybe, I need to check: I thought the modification of the
> bitmap
> was done atomically, and it was fine to read without the lock. But
> maybe
> not because it is stored in multiple bytes.
> 
> > Also, we can put this bitmap copy code into a separate patch, which
> > is
> > applied before this one. In this patch we only need to replace
> > spin_lock_bh with rcu_read_lock.
> 
> OK so we are talking about separate fix that is not directly linked
> to
> my change, apart from the fact I modified code around, right?
> 
> Cheers,
> Matt

Re: [PATCH mptcp-net v2 2/3] mptcp: pm: lockless list traversal

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 2998 bytes --]

On Fri, 25 Oct 2024, Matthieu Baerts wrote:

> On 25/10/2024 11:32, Matthieu Baerts (NGI0) wrote:
>> In a few places -- to get an endpoint, dump all of them, and change
>> their flags -- the list is iterated while holding the pernet->lock, but
>> only to read the content of the list. In these cases, we can replace the
>> spin locks, by RCU read ones, and use the _rcu variants to iterate over
>> the entries list in a lockless way.
>>
>> To make it clear, the lookup helpers using the _rcu variant are renamed
>> with a _rcu suffix. The previous __lookup_addr() helper can then be
>> removed, but __lookup_addr_by_id() is still needed.
>>
>> While at it, the IDs bitmap is copied before iterating the list to dump
>> the different addresses, to avoid any consistencies.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> Notes:
>>  - This is not a fix, a small improvement for -next.
>> ---
>>  net/mptcp/pm_netlink.c | 36 +++++++++++++++++++-----------------
>>  1 file changed, 19 insertions(+), 17 deletions(-)
>>
>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>> index a93b9b7776b48781a883673fe5fd521a978487ff..f38e1ccd34e95cd88b179a8b50e6965731542871 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c
>> @@ -520,12 +520,12 @@ __lookup_addr_by_id(struct pm_nl_pernet *pernet, unsigned int id)
>>  }
>>
>>  static struct mptcp_pm_addr_entry *
>> -__lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info)
>> +__lookup_addr_by_id_rcu(struct pm_nl_pernet *pernet, unsigned int id)
>>  {
>>  	struct mptcp_pm_addr_entry *entry;
>>
>> -	list_for_each_entry(entry, &pernet->local_addr_list, list) {
>> -		if (mptcp_addresses_equal(&entry->addr, info, entry->addr.port))
>> +	list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) {
>
> I didn't see the following in the RCU doc -- but it is quite big, so I
> probably missed it -- but I suppose we don't need to keep the two
> helpers here with and without the _rcu variant (__lookup_addr_by_id())
> if here we add an extra lockdep condition:
>
>  list_for_each_entry_rcu(entry, &pernet->local_addr_list, list,
>                          lockdep_is_held(&pernet->lock)) {
>
> WDYT?

I like this idea, would clean things up.

>
>> +		if (entry->addr.id == id)
>>  			return entry;
>>  	}
>>  	return NULL;
>
> (...)
>
>> @@ -1872,16 +1872,18 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
>>  	struct net *net = sock_net(msg->sk);
>>  	struct mptcp_pm_addr_entry *entry;
>>  	struct pm_nl_pernet *pernet;
>> +	unsigned long id_bitmap[4];
>
> Oops, I left my temp declaration, it should be the following line, which
> will result in the same thing anyway on x86_64, no need to re-run the
> tests → I can fix that when applying the patches, or in a future version:
>
>  DECLARE_BITMAP(id_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
>

Thanks for catching that. I think it would be best to have a v3 with the 
lockdep_is_held() approach you mentioned above.


- Mat

Re: [PATCH mptcp-net v2 2/3] mptcp: pm: lockless list traversal

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 7470 bytes --]

On Tue, 29 Oct 2024, Geliang Tang wrote:

> Hi Matt,
>
> On Mon, 2024-10-28 at 12:48 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 28/10/2024 03:08, Geliang Tang wrote:
>>> Hi Matt,
>>>
>>> On Fri, 2024-10-25 at 17:26 +0200, Matthieu Baerts wrote:
>>>> Hi Geliang,
>>>>
>>>> Thank you for the review!
>>>>
>>>> On 25/10/2024 16:25, Geliang Tang wrote:
>>>>> On Fri, 2024-10-25 at 11:32 +0200, Matthieu Baerts (NGI0)
>>>>> wrote:
>>>>>> In a few places -- to get an endpoint, dump all of them, and
>>>>>> change
>>>>>> their flags -- the list is iterated while holding the pernet-
>>>>>>> lock,
>>>>>> but
>>>>>> only to read the content of the list. In these cases, we can
>>>>>> replace
>>>>>> the
>>>>>> spin locks, by RCU read ones, and use the _rcu variants to
>>>>>> iterate
>>>>>> over
>>>>>> the entries list in a lockless way.
>>>>>>
>>>>>> To make it clear, the lookup helpers using the _rcu variant
>>>>>> are
>>>>>> renamed
>>>>>> with a _rcu suffix. The previous __lookup_addr() helper can
>>>>>> then
>>>>>> be
>>>>>> removed, but __lookup_addr_by_id() is still needed.
>>>>>>
>>>>>> While at it, the IDs bitmap is copied before iterating the
>>>>>> list
>>>>>> to
>>>>>> dump
>>>>>> the different addresses, to avoid any consistencies.
>>>>>>
>>>>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>>>>> ---
>>>>>> Notes:
>>>>>>  - This is not a fix, a small improvement for -next.
>>>>>> ---
>>>>>>  net/mptcp/pm_netlink.c | 36 +++++++++++++++++++-------------
>>>>>> ----
>>>>>>  1 file changed, 19 insertions(+), 17 deletions(-)
>>>>>>
>>>>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>>>>> index
>>>>>> a93b9b7776b48781a883673fe5fd521a978487ff..f38e1ccd34e95cd88b1
>>>>>> 79a8
>>>>>> b50e
>>>>>> 6965731542871 100644
>>>>>> --- a/net/mptcp/pm_netlink.c
>>>>>> +++ b/net/mptcp/pm_netlink.c
>>>>
>>>> (...)
>>>>
>>>>>> @@ -1872,16 +1872,18 @@ int mptcp_pm_nl_dump_addr(struct
>>>>>> sk_buff
>>>>>> *msg,
>>>>>>   struct net *net = sock_net(msg->sk);
>>>>>>   struct mptcp_pm_addr_entry *entry;
>>>>>>   struct pm_nl_pernet *pernet;
>>>>>> + unsigned long id_bitmap[4];
>>>>>>   int id = cb->args[0];
>>>>>>   void *hdr;
>>>>>>   int i;
>>>>>>
>>>>>   pernet = pm_nl_get_pernet(net);
>>>>>> + bitmap_copy(id_bitmap, pernet->id_bitmap,
>>>>>> MPTCP_PM_MAX_ADDR_ID + 1);
>>>>>
>>>>> I think the id bitmap should only be copied when id is 0:
>>>>>
>>>>> if (!id)
>>>>>   bitmap_copy(id_bitmap, pernet->id_bitmap,
>>>>> MPTCP_PM_MAX_ADDR_ID +
>>>>> 1);
>>>>>
>>>>> Since this mptcp_pm_nl_dump_addr() may be called repeatedly
>>>>> when
>>>>> the
>>>>> dump information is very long. We only copy it the first time
>>>>> it is
>>>
>>> A correction is needed here. Regardless of whether the dump
>>> information
>>> is large or small, the dumpit function will be called repeatedly
>>> when
>>> dumpit returns non-zero. The loop stops when dumpit returns 0.
>>>
>>>>> called, and subsequent calls cannot copy it again. WDYT?
>>>>
>>>> Sorry, I'm not sure to understand. Here, I did a local copy of
>>>> 'id_bitmap' just to keep a certain consistency if the bitmap is
>>>> changed
>>>> during the RCU read section below: not to have this bitmap being
>>>> changed
>>>> during the for loop here below. Before, we had this protection
>>>> because
>>>> pernet->lock was held.
>>>
>>> It is precisely because we need to maintain this consistency that
>>> we
>>> need to copy the bitmap only once. If we copy the bitmap when
>>> dumpit is
>>> called a second time, the bitmap obtained at this time is a new
>>> bitmap,
>>> which destroys this consistency, right?
>>
>> OK, but then I have a few questions:
>>
>> - Was the code before my patch here prevented such consistency
>> issues?
>> Or said differently: is there a regression introduced by this patch?
>>
>> - Is it really an issue that can lead to a crash or reading freed
>> info?
>>
>> - Where would you store the copy of the bitmap? In cb-args?
>>
>> - If we store this copy somewhere, could you not have situations
>> where
>> the bitmap would no longer be in sync with the addresses that are
>> stored
>> in 'pernet->local_addr_list', and then displaying wrong/freed info?
>> It
>> looks like more would be needed to cope with that. And maybe that's
>> not
>> worth it?
>
> Your doubts are very reasonable. It seems that I have not considered it
> carefully enough. So we do need to define a local bitmap and copy it
> every time. It's fine to me and it's OK to implement BPF path manager
> based on it.
>

Matthieu and Geliang -

I think the only way to guarantee strict consistency would be to acquire 
the lock at the beginning of the dump and make a complete copy of both the 
bitmap and the address list at that moment. Even with all the work to 
achieve this "strictly consistent" snapshot, the userspace caller doesn't 
have any guarantee that the dumped data is accurate at the moment the 
netlink response completes.


The good news is that I don't think this strict consistency is needed for 
the dump function. If the list is being modified concurrently, it's not a 
big problem to miss newly-added entries or show recently-deleted ones.


Since bitmap_copy() is non-atomic, my suggestion is to not copy the 
bitmap. Instead use test_bit() (which is atomic) directly on 
pernet->id_bitmap without locking.

Then, make sure that inconsistencies between the bitmap and the 
local_addr_list are handled cleanly. The two cases are:

1. Bitmap has a 0 even though there's a new entry in local_addr_list. In 
this case it gets skipped. No problem here.

2. Bitmap has a 1 even though there's no entry in local_addr_list (ignore 
it!), or there's an entry that is in the process of getting freed. For the 
latter, RCU will ensure there's no risk of an invalid dereference. If we 
want to detect a deletion-in-progress, we would have to add an atomic flag 
to 'struct mptcp_pm_addr_entry' that is set before the deletion code calls 
list_del_rcu(). Even if the address is in this state, I would argue that 
the just-barely-stale data is fine to show.


Sound reasonable? Do you see a reason that bitmap/addr_list 
inconsistencies would cause issues?

Thanks

- Mat


>>
>>>> Are you suggesting here to keep a copy in the cb, not a local
>>>> one, to
>>>> do
>>>> the copy only once? Are you sure it is worth it (and safe)?
>>>
>>> Because dumpit will be called repeatedly, we need to ensure that
>>> the
>>> same bitmap is accessed each time, so we cannot use a local one,
>>> but
>>> need to keep a copy in the cb just like what we do in
>>> mptcp_userspace_pm_dump_addr.
>>>
>>>
>>> In addition, when doing bitmap_copy, we need to hold pernet->lock:
>>>
>>> bitmap = (struct mptcp_id_bitmap *)cb->ctx;
>>>
>>> if (!id) {
>>>   spin_lock_bh(&pernet->lock);
>>>   bitmap_copy(id_bitmap, pernet->id_bitmap, MPTCP_PM_MAX_ADDR_ID +
>>> 1);
>>>   spin_unlock_bh(&pernet->lock);
>>> }
>>
>> Ah yes, maybe, I need to check: I thought the modification of the
>> bitmap
>> was done atomically, and it was fine to read without the lock. But
>> maybe
>> not because it is stored in multiple bytes.



>>
>>> Also, we can put this bitmap copy code into a separate patch, which
>>> is
>>> applied before this one. In this patch we only need to replace
>>> spin_lock_bh with rcu_read_lock.
>>
>> OK so we are talking about separate fix that is not directly linked
>> to
>> my change, apart from the fact I modified code around, right?
>>
>> Cheers,
>> Matt
>
>
>

Re: [PATCH mptcp-net v2 2/3] mptcp: pm: lockless list traversal

[Paolo Abeni]

On 10/25/24 11:32, Matthieu Baerts (NGI0) wrote:
> @@ -2060,17 +2062,17 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
>  	if (addr.flags & MPTCP_PM_ADDR_FLAG_BACKUP)
>  		bkup = 1;
>  
> -	spin_lock_bh(&pernet->lock);
> -	entry = lookup_by_id ? __lookup_addr_by_id(pernet, addr.addr.id) :
> -			       __lookup_addr(pernet, &addr.addr);
> +	rcu_read_lock();
> +	entry = lookup_by_id ? __lookup_addr_by_id_rcu(pernet, addr.addr.id) :
> +			       __lookup_addr_rcu(pernet, &addr.addr);
>  	if (!entry) {
> -		spin_unlock_bh(&pernet->lock);
> +		rcu_read_unlock();
>  		GENL_SET_ERR_MSG(info, "address not found");
>  		return -EINVAL;
>  	}
>  	if ((addr.flags & MPTCP_PM_ADDR_FLAG_FULLMESH) &&
>  	    (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
> -		spin_unlock_bh(&pernet->lock);
> +		rcu_read_unlock();
>  		GENL_SET_ERR_MSG(info, "invalid addr flags");
>  		return -EINVAL;
>  	}
> @@ -2078,7 +2080,7 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
>  	changed = (addr.flags ^ entry->flags) & mask;
>  	entry->flags = (entry->flags & ~mask) | (addr.flags & mask);
>  	addr = *entry;
> -	spin_unlock_bh(&pernet->lock);
> +	rcu_read_unlock();
>  
>  	mptcp_nl_set_flags(net, &addr.addr, bkup, changed);
>  	return 0;
> 

I think we must retain the lock in this function, otherwise we could
end-up with an unexpected flag combination.

i.e. set_flag(MPTCP_PM_ADDR_FLAG_FULLMESH) and
set_flag(MPTCP_PM_ADDR_FLAG_SIGNAL) run concurrently on CPU1 and CPU2.
entry->flags could end-up having a single bit set instead of both, if
both CPUs read the entry before the other would store the new value.

I don't think _ONCE() annotations will be enough to avoid such thing
without a lock.

Cheers,

Paolo

Re: [PATCH mptcp-net v2 2/3] mptcp: pm: lockless list traversal

[Matthieu Baerts]

Hi Mat,

On 01/11/2024 00:24, Mat Martineau wrote:
> On Tue, 29 Oct 2024, Geliang Tang wrote:
> 
>> Hi Matt,
>>
>> On Mon, 2024-10-28 at 12:48 +0100, Matthieu Baerts wrote:
>>> Hi Geliang,
>>>
>>> On 28/10/2024 03:08, Geliang Tang wrote:
>>>> Hi Matt,
>>>>
>>>> On Fri, 2024-10-25 at 17:26 +0200, Matthieu Baerts wrote:
>>>>> Hi Geliang,
>>>>>
>>>>> Thank you for the review!
>>>>>
>>>>> On 25/10/2024 16:25, Geliang Tang wrote:
>>>>>> On Fri, 2024-10-25 at 11:32 +0200, Matthieu Baerts (NGI0)
>>>>>> wrote:
>>>>>>> In a few places -- to get an endpoint, dump all of them, and
>>>>>>> change
>>>>>>> their flags -- the list is iterated while holding the pernet-
>>>>>>>> lock,
>>>>>>> but
>>>>>>> only to read the content of the list. In these cases, we can
>>>>>>> replace
>>>>>>> the
>>>>>>> spin locks, by RCU read ones, and use the _rcu variants to
>>>>>>> iterate
>>>>>>> over
>>>>>>> the entries list in a lockless way.
>>>>>>>
>>>>>>> To make it clear, the lookup helpers using the _rcu variant
>>>>>>> are
>>>>>>> renamed
>>>>>>> with a _rcu suffix. The previous __lookup_addr() helper can
>>>>>>> then
>>>>>>> be
>>>>>>> removed, but __lookup_addr_by_id() is still needed.
>>>>>>>
>>>>>>> While at it, the IDs bitmap is copied before iterating the
>>>>>>> list
>>>>>>> to
>>>>>>> dump
>>>>>>> the different addresses, to avoid any consistencies.
>>>>>>>
>>>>>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>>>>>> ---
>>>>>>> Notes:
>>>>>>>  - This is not a fix, a small improvement for -next.
>>>>>>> ---
>>>>>>>  net/mptcp/pm_netlink.c | 36 +++++++++++++++++++-------------
>>>>>>> ----
>>>>>>>  1 file changed, 19 insertions(+), 17 deletions(-)
>>>>>>>
>>>>>>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>>>>>>> index
>>>>>>> a93b9b7776b48781a883673fe5fd521a978487ff..f38e1ccd34e95cd88b1
>>>>>>> 79a8
>>>>>>> b50e
>>>>>>> 6965731542871 100644
>>>>>>> --- a/net/mptcp/pm_netlink.c
>>>>>>> +++ b/net/mptcp/pm_netlink.c
>>>>>
>>>>> (...)
>>>>>
>>>>>>> @@ -1872,16 +1872,18 @@ int mptcp_pm_nl_dump_addr(struct
>>>>>>> sk_buff
>>>>>>> *msg,
>>>>>>>   struct net *net = sock_net(msg->sk);
>>>>>>>   struct mptcp_pm_addr_entry *entry;
>>>>>>>   struct pm_nl_pernet *pernet;
>>>>>>> + unsigned long id_bitmap[4];
>>>>>>>   int id = cb->args[0];
>>>>>>>   void *hdr;
>>>>>>>   int i;
>>>>>>>
>>>>>>   pernet = pm_nl_get_pernet(net);
>>>>>>> + bitmap_copy(id_bitmap, pernet->id_bitmap,
>>>>>>> MPTCP_PM_MAX_ADDR_ID + 1);
>>>>>>
>>>>>> I think the id bitmap should only be copied when id is 0:
>>>>>>
>>>>>> if (!id)
>>>>>>   bitmap_copy(id_bitmap, pernet->id_bitmap,
>>>>>> MPTCP_PM_MAX_ADDR_ID +
>>>>>> 1);
>>>>>>
>>>>>> Since this mptcp_pm_nl_dump_addr() may be called repeatedly
>>>>>> when
>>>>>> the
>>>>>> dump information is very long. We only copy it the first time
>>>>>> it is
>>>>
>>>> A correction is needed here. Regardless of whether the dump
>>>> information
>>>> is large or small, the dumpit function will be called repeatedly
>>>> when
>>>> dumpit returns non-zero. The loop stops when dumpit returns 0.
>>>>
>>>>>> called, and subsequent calls cannot copy it again. WDYT?
>>>>>
>>>>> Sorry, I'm not sure to understand. Here, I did a local copy of
>>>>> 'id_bitmap' just to keep a certain consistency if the bitmap is
>>>>> changed
>>>>> during the RCU read section below: not to have this bitmap being
>>>>> changed
>>>>> during the for loop here below. Before, we had this protection
>>>>> because
>>>>> pernet->lock was held.
>>>>
>>>> It is precisely because we need to maintain this consistency that
>>>> we
>>>> need to copy the bitmap only once. If we copy the bitmap when
>>>> dumpit is
>>>> called a second time, the bitmap obtained at this time is a new
>>>> bitmap,
>>>> which destroys this consistency, right?
>>>
>>> OK, but then I have a few questions:
>>>
>>> - Was the code before my patch here prevented such consistency
>>> issues?
>>> Or said differently: is there a regression introduced by this patch?
>>>
>>> - Is it really an issue that can lead to a crash or reading freed
>>> info?
>>>
>>> - Where would you store the copy of the bitmap? In cb-args?
>>>
>>> - If we store this copy somewhere, could you not have situations
>>> where
>>> the bitmap would no longer be in sync with the addresses that are
>>> stored
>>> in 'pernet->local_addr_list', and then displaying wrong/freed info?
>>> It
>>> looks like more would be needed to cope with that. And maybe that's
>>> not
>>> worth it?
>>
>> Your doubts are very reasonable. It seems that I have not considered it
>> carefully enough. So we do need to define a local bitmap and copy it
>> every time. It's fine to me and it's OK to implement BPF path manager
>> based on it.
>>
> 
> Matthieu and Geliang -
> 
> I think the only way to guarantee strict consistency would be to acquire
> the lock at the beginning of the dump and make a complete copy of both
> the bitmap and the address list at that moment. Even with all the work
> to achieve this "strictly consistent" snapshot, the userspace caller
> doesn't have any guarantee that the dumped data is accurate at the
> moment the netlink response completes.
> 
> 
> The good news is that I don't think this strict consistency is needed
> for the dump function. If the list is being modified concurrently, it's
> not a big problem to miss newly-added entries or show recently-deleted
> ones.
> 
> 
> Since bitmap_copy() is non-atomic, my suggestion is to not copy the
> bitmap. Instead use test_bit() (which is atomic) directly on pernet-
>>id_bitmap without locking.
> 
> Then, make sure that inconsistencies between the bitmap and the
> local_addr_list are handled cleanly. The two cases are:
> 
> 1. Bitmap has a 0 even though there's a new entry in local_addr_list. In
> this case it gets skipped. No problem here.
> 
> 2. Bitmap has a 1 even though there's no entry in local_addr_list
> (ignore it!), or there's an entry that is in the process of getting
> freed. For the latter, RCU will ensure there's no risk of an invalid
> dereference. If we want to detect a deletion-in-progress, we would have
> to add an atomic flag to 'struct mptcp_pm_addr_entry' that is set before
> the deletion code calls list_del_rcu(). Even if the address is in this
> state, I would argue that the just-barely-stale data is fine to show.

Thank you for your review!

> Sound reasonable? Do you see a reason that bitmap/addr_list
> inconsistencies would cause issues?
Yes, that's sounds reasonable, as long as we don't try to access freed
resources, I don't think we need to worry about inconsistencies if there
are ongoing changes while the userspace is requesting the list.

When I did the copy of the bitmap, it was mainly "as a matter of
principle", but if it is not needed, that's even simpler.

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

Re: [PATCH mptcp-net v2 2/3] mptcp: pm: lockless list traversal

[Matthieu Baerts]

Hi Paolo,

Thank you for the review!

On 05/11/2024 19:21, Paolo Abeni wrote:
> On 10/25/24 11:32, Matthieu Baerts (NGI0) wrote:
>> @@ -2060,17 +2062,17 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
>>  	if (addr.flags & MPTCP_PM_ADDR_FLAG_BACKUP)
>>  		bkup = 1;
>>  
>> -	spin_lock_bh(&pernet->lock);
>> -	entry = lookup_by_id ? __lookup_addr_by_id(pernet, addr.addr.id) :
>> -			       __lookup_addr(pernet, &addr.addr);
>> +	rcu_read_lock();
>> +	entry = lookup_by_id ? __lookup_addr_by_id_rcu(pernet, addr.addr.id) :
>> +			       __lookup_addr_rcu(pernet, &addr.addr);
>>  	if (!entry) {
>> -		spin_unlock_bh(&pernet->lock);
>> +		rcu_read_unlock();
>>  		GENL_SET_ERR_MSG(info, "address not found");
>>  		return -EINVAL;
>>  	}
>>  	if ((addr.flags & MPTCP_PM_ADDR_FLAG_FULLMESH) &&
>>  	    (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
>> -		spin_unlock_bh(&pernet->lock);
>> +		rcu_read_unlock();
>>  		GENL_SET_ERR_MSG(info, "invalid addr flags");
>>  		return -EINVAL;
>>  	}
>> @@ -2078,7 +2080,7 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
>>  	changed = (addr.flags ^ entry->flags) & mask;
>>  	entry->flags = (entry->flags & ~mask) | (addr.flags & mask);
>>  	addr = *entry;
>> -	spin_unlock_bh(&pernet->lock);
>> +	rcu_read_unlock();
>>  
>>  	mptcp_nl_set_flags(net, &addr.addr, bkup, changed);
>>  	return 0;
>>
> 
> I think we must retain the lock in this function, otherwise we could
> end-up with an unexpected flag combination.
> 
> i.e. set_flag(MPTCP_PM_ADDR_FLAG_FULLMESH) and
> set_flag(MPTCP_PM_ADDR_FLAG_SIGNAL) run concurrently on CPU1 and CPU2.
> entry->flags could end-up having a single bit set instead of both, if
> both CPUs read the entry before the other would store the new value.

Good point, I didn't think about this case where two commands would be
done in parallel from the userspace. But can we get this case? By
default, are the commands not serialised? (linked to parallel_ops member
in struct genl_family?)

In this case, is it OK to switch to rcu_read_lock? Or is it maybe not
worth it?

> I don't think _ONCE() annotations will be enough to avoid such thing
> without a lock.

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