Adding the following warning ...
WARN_ON_ONCE(msk->pm.local_addr_used == 0)
... before decrementing the local_addr_used counter helped to find a bug
when running the "remove single address" subtest from the mptcp_join.sh
selftests.
Removing a 'signal' endpoint will trigger the removal of all subflows
linked to this endpoint via mptcp_pm_nl_rm_addr_or_subflow() with
rm_type == MPTCP_MIB_RMSUBFLOW. This will decrement the local_addr_used
counter, which is wrong in this case because this counter is linked to
'subflow' endpoints, and here it is a 'signal' endpoint that is being
removed.
Now, the counter is decremented, only if the ID is being used outside
of mptcp_pm_nl_rm_addr_or_subflow(), only for 'subflow' endpoints, and
if the ID is not 0 -- local_addr_used is not taking into account these
ones. This marking of the ID as being available, and the decrement is
done no matter if a subflow using this ID is currently available,
because the subflow could have been closed before.
Fixes: 06faa2271034 ("mptcp: remove multi addresses and subflows in PM")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
net/mptcp/pm_netlink.c | 26 +++++++++++++++++---------
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 8a28fdaf3bb6..3ea417b52ff4 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -833,10 +833,10 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk,
if (rm_type == MPTCP_MIB_RMSUBFLOW)
__MPTCP_INC_STATS(sock_net(sk), rm_type);
}
- if (rm_type == MPTCP_MIB_RMSUBFLOW)
- __set_bit(rm_id ? rm_id : msk->mpc_endpoint_id, msk->pm.id_avail_bitmap);
- else if (rm_type == MPTCP_MIB_RMADDR)
+
+ if (rm_type == MPTCP_MIB_RMADDR)
__MPTCP_INC_STATS(sock_net(sk), rm_type);
+
if (!removed)
continue;
@@ -846,8 +846,6 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk,
if (rm_type == MPTCP_MIB_RMADDR) {
msk->pm.add_addr_accepted--;
WRITE_ONCE(msk->pm.accept_addr, true);
- } else if (rm_type == MPTCP_MIB_RMSUBFLOW) {
- msk->pm.local_addr_used--;
}
}
}
@@ -1443,6 +1441,14 @@ static bool mptcp_pm_remove_anno_addr(struct mptcp_sock *msk,
return ret;
}
+static void __mark_subflow_endp_available(struct mptcp_sock *msk, u8 id)
+{
+ /* If it was marked as used, and not ID 0, decrement local_addr_used */
+ if (!__test_and_set_bit(id ? : msk->mpc_endpoint_id, msk->pm.id_avail_bitmap) &&
+ id && !WARN_ON_ONCE(msk->pm.local_addr_used == 0))
+ msk->pm.local_addr_used--;
+}
+
static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
const struct mptcp_pm_addr_entry *entry)
{
@@ -1476,11 +1482,11 @@ static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
spin_lock_bh(&msk->pm.lock);
mptcp_pm_nl_rm_subflow_received(msk, &list);
spin_unlock_bh(&msk->pm.lock);
- } else if (entry->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW) {
- /* If the subflow has been used, but now closed */
+ }
+
+ if (entry->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW) {
spin_lock_bh(&msk->pm.lock);
- if (!__test_and_set_bit(entry->addr.id, msk->pm.id_avail_bitmap))
- msk->pm.local_addr_used--;
+ __mark_subflow_endp_available(msk, entry->addr.id);
spin_unlock_bh(&msk->pm.lock);
}
@@ -1518,6 +1524,7 @@ static int mptcp_nl_remove_id_zero_address(struct net *net,
spin_lock_bh(&msk->pm.lock);
mptcp_pm_remove_addr(msk, &list);
mptcp_pm_nl_rm_subflow_received(msk, &list);
+ __mark_subflow_endp_available(msk, 0);
spin_unlock_bh(&msk->pm.lock);
release_sock(sk);
@@ -1919,6 +1926,7 @@ static void mptcp_pm_nl_fullmesh(struct mptcp_sock *msk,
spin_lock_bh(&msk->pm.lock);
mptcp_pm_nl_rm_subflow_received(msk, &list);
+ __mark_subflow_endp_available(msk, addr->id);
mptcp_pm_create_subflow_or_signal_addr(msk);
spin_unlock_bh(&msk->pm.lock);
}
--
2.45.2Hi Matt,
On Fri, 2024-07-26 at 16:28 +0200, Matthieu Baerts (NGI0) wrote:
> Adding the following warning ...
>
> WARN_ON_ONCE(msk->pm.local_addr_used == 0)
>
> ... before decrementing the local_addr_used counter helped to find a
> bug
> when running the "remove single address" subtest from the
> mptcp_join.sh
> selftests.
>
> Removing a 'signal' endpoint will trigger the removal of all subflows
> linked to this endpoint via mptcp_pm_nl_rm_addr_or_subflow() with
> rm_type == MPTCP_MIB_RMSUBFLOW. This will decrement the
> local_addr_used
> counter, which is wrong in this case because this counter is linked
> to
> 'subflow' endpoints, and here it is a 'signal' endpoint that is being
> removed.
>
> Now, the counter is decremented, only if the ID is being used outside
> of mptcp_pm_nl_rm_addr_or_subflow(), only for 'subflow' endpoints,
> and
> if the ID is not 0 -- local_addr_used is not taking into account
> these
> ones. This marking of the ID as being available, and the decrement is
> done no matter if a subflow using this ID is currently available,
> because the subflow could have been closed before.
>
> Fixes: 06faa2271034 ("mptcp: remove multi addresses and subflows in
> PM")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> net/mptcp/pm_netlink.c | 26 +++++++++++++++++---------
> 1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 8a28fdaf3bb6..3ea417b52ff4 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -833,10 +833,10 @@ static void
> mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk,
> if (rm_type == MPTCP_MIB_RMSUBFLOW)
> __MPTCP_INC_STATS(sock_net(sk),
> rm_type);
> }
> - if (rm_type == MPTCP_MIB_RMSUBFLOW)
> - __set_bit(rm_id ? rm_id : msk-
> >mpc_endpoint_id, msk->pm.id_avail_bitmap);
> - else if (rm_type == MPTCP_MIB_RMADDR)
> +
> + if (rm_type == MPTCP_MIB_RMADDR)
> __MPTCP_INC_STATS(sock_net(sk), rm_type);
> +
> if (!removed)
> continue;
>
> @@ -846,8 +846,6 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct
> mptcp_sock *msk,
> if (rm_type == MPTCP_MIB_RMADDR) {
> msk->pm.add_addr_accepted--;
> WRITE_ONCE(msk->pm.accept_addr, true);
> - } else if (rm_type == MPTCP_MIB_RMSUBFLOW) {
> - msk->pm.local_addr_used--;
Is it necessary to call __mark_subflow_endp_available() here? I'm not
sure.
> }
> }
> }
> @@ -1443,6 +1441,14 @@ static bool mptcp_pm_remove_anno_addr(struct
> mptcp_sock *msk,
> return ret;
> }
>
> +static void __mark_subflow_endp_available(struct mptcp_sock *msk, u8
> id)
> +{
> + /* If it was marked as used, and not ID 0, decrement
> local_addr_used */
> + if (!__test_and_set_bit(id ? : msk->mpc_endpoint_id, msk-
> >pm.id_avail_bitmap) &&
> + id && !WARN_ON_ONCE(msk->pm.local_addr_used == 0))
> + msk->pm.local_addr_used--;
> +}
> +
> static int mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
> const struct
> mptcp_pm_addr_entry *entry)
> {
> @@ -1476,11 +1482,11 @@ static int
> mptcp_nl_remove_subflow_and_signal_addr(struct net *net,
> spin_lock_bh(&msk->pm.lock);
> mptcp_pm_nl_rm_subflow_received(msk, &list);
> spin_unlock_bh(&msk->pm.lock);
> - } else if (entry->flags &
> MPTCP_PM_ADDR_FLAG_SUBFLOW) {
> - /* If the subflow has been used, but now
> closed */
> + }
> +
> + if (entry->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW) {
> spin_lock_bh(&msk->pm.lock);
> - if (!__test_and_set_bit(entry->addr.id, msk-
> >pm.id_avail_bitmap))
> - msk->pm.local_addr_used--;
> + __mark_subflow_endp_available(msk, entry-
> >addr.id);
> spin_unlock_bh(&msk->pm.lock);
> }
>
> @@ -1518,6 +1524,7 @@ static int
> mptcp_nl_remove_id_zero_address(struct net *net,
> spin_lock_bh(&msk->pm.lock);
> mptcp_pm_remove_addr(msk, &list);
> mptcp_pm_nl_rm_subflow_received(msk, &list);
> + __mark_subflow_endp_available(msk, 0);
> spin_unlock_bh(&msk->pm.lock);
> release_sock(sk);
>
> @@ -1919,6 +1926,7 @@ static void mptcp_pm_nl_fullmesh(struct
> mptcp_sock *msk,
>
> spin_lock_bh(&msk->pm.lock);
> mptcp_pm_nl_rm_subflow_received(msk, &list);
> + __mark_subflow_endp_available(msk, addr->id);
> mptcp_pm_create_subflow_or_signal_addr(msk);
> spin_unlock_bh(&msk->pm.lock);
> }
>
Hi Geliang,
Thank you for the review.
On 29/07/2024 03:09, Geliang Tang wrote:
> Hi Matt,
>
> On Fri, 2024-07-26 at 16:28 +0200, Matthieu Baerts (NGI0) wrote:
>> Adding the following warning ...
>>
>> WARN_ON_ONCE(msk->pm.local_addr_used == 0)
>>
>> ... before decrementing the local_addr_used counter helped to find a
>> bug
>> when running the "remove single address" subtest from the
>> mptcp_join.sh
>> selftests.
>>
>> Removing a 'signal' endpoint will trigger the removal of all subflows
>> linked to this endpoint via mptcp_pm_nl_rm_addr_or_subflow() with
>> rm_type == MPTCP_MIB_RMSUBFLOW. This will decrement the
>> local_addr_used
>> counter, which is wrong in this case because this counter is linked
>> to
>> 'subflow' endpoints, and here it is a 'signal' endpoint that is being
>> removed.
>>
>> Now, the counter is decremented, only if the ID is being used outside
>> of mptcp_pm_nl_rm_addr_or_subflow(), only for 'subflow' endpoints,
>> and
>> if the ID is not 0 -- local_addr_used is not taking into account
>> these
>> ones. This marking of the ID as being available, and the decrement is
>> done no matter if a subflow using this ID is currently available,
>> because the subflow could have been closed before.
>>
>> Fixes: 06faa2271034 ("mptcp: remove multi addresses and subflows in
>> PM")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> net/mptcp/pm_netlink.c | 26 +++++++++++++++++---------
>> 1 file changed, 17 insertions(+), 9 deletions(-)
>>
>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>> index 8a28fdaf3bb6..3ea417b52ff4 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c
>> @@ -833,10 +833,10 @@ static void
>> mptcp_pm_nl_rm_addr_or_subflow(struct mptcp_sock *msk,
>> if (rm_type == MPTCP_MIB_RMSUBFLOW)
>> __MPTCP_INC_STATS(sock_net(sk),
>> rm_type);
>> }
>> - if (rm_type == MPTCP_MIB_RMSUBFLOW)
>> - __set_bit(rm_id ? rm_id : msk-
>>> mpc_endpoint_id, msk->pm.id_avail_bitmap);
>> - else if (rm_type == MPTCP_MIB_RMADDR)
>> +
>> + if (rm_type == MPTCP_MIB_RMADDR)
>> __MPTCP_INC_STATS(sock_net(sk), rm_type);
>> +
>> if (!removed)
>> continue;
>>
>> @@ -846,8 +846,6 @@ static void mptcp_pm_nl_rm_addr_or_subflow(struct
>> mptcp_sock *msk,
>> if (rm_type == MPTCP_MIB_RMADDR) {
>> msk->pm.add_addr_accepted--;
>> WRITE_ONCE(msk->pm.accept_addr, true);
>> - } else if (rm_type == MPTCP_MIB_RMSUBFLOW) {
>> - msk->pm.local_addr_used--;
>
>
> Is it necessary to call __mark_subflow_endp_available() here? I'm not
> sure.
No, because mptcp_pm_nl_rm_addr_or_subflow(MPTCP_MIB_RMSUBFLOW) will be
called when removing an endpoint, to delete all linked subflows. That
can be a 'signal' endpoint as well, while 'local_addr_used' is only
linked to 'subflow' endpoint. There are more details about that in the
commit message, but in short, that why I moved this to functions calling
this helper, where there are more info about the endpoint type.
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
© 2016 - 2024 Red Hat, Inc.