Skip to content

Commit 16dc75e

Browse files
kuba-mookuba-moo
committed
Merge branch 'mptcp-fix-endpoints-with-signal-and-subflow-flags'
Matthieu Baerts says: ==================== mptcp: fix endpoints with 'signal' and 'subflow' flags When looking at improving the user experience around the MPTCP endpoints setup, I noticed that setting an endpoint with both the 'signal' and the 'subflow' flags -- as it has been done in the past by users according to bug reports we got -- was resulting on only announcing the endpoint, but not using it to create subflows: the 'subflow' flag was then ignored. My initial thought was to modify IPRoute2 to warn the user when the two flags were set, but it doesn't sound normal to ignore one of them. I then looked at modifying the kernel not to allow having the two flags set, but when discussing about that with Mat, we thought it was maybe not ideal to do that, as there might be use-cases, we might break some configs. Then I saw it was working before v5.17. So instead, I fixed the support on the kernel side (patch 5) using Paolo's suggestion. This also includes a fix on the options side (patch 1: for v5.11+), an explicit deny of some options combinations (patch 2: for v5.18+), and some refactoring (patches 3 and 4) to ease the inclusion of the patch 5. While at it, I added a new selftest (patch 7) to validate this case -- including a modification of the chk_add_nr helper to inverse the sides were the counters are checked (patch 6) -- and allowed ADD_ADDR echo just after the MP_JOIN 3WHS. The selftests modification have the same Fixes tag as the previous commit, but no 'Cc: Stable': if the backport can work, that's good -- but it still need to be verified by running the selftests -- if not, no need to worry, many CIs will use the selftests from the last stable version to validate previous stable releases. ==================== Link: https://patch.msgid.link/20240731-upstream-net-20240731-mptcp-endp-subflow-signal-v1-0-c8a9b036493b@kernel.org Signed-off-by: Jakub Kicinski <kuba@kernel.org>
2 parents 183d46f + 4d2868b commit 16dc75e

3 files changed

Lines changed: 73 additions & 32 deletions

File tree

‎net/mptcp/options.c‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -958,7 +958,8 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
958958

959959
if (subflow->remote_key_valid &&
960960
(((mp_opt->suboptions & OPTION_MPTCP_DSS) && mp_opt->use_ack) ||
961-
((mp_opt->suboptions & OPTION_MPTCP_ADD_ADDR) && !mp_opt->echo))) {
961+
((mp_opt->suboptions & OPTION_MPTCP_ADD_ADDR) &&
962+
(!mp_opt->echo || subflow->mp_join)))) {
962963
/* subflows are fully established as soon as we get any
963964
* additional ack, including ADD_ADDR.
964965
*/

‎net/mptcp/pm_netlink.c‎

Lines changed: 30 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,7 @@ bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
348348
add_entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
349349

350350
if (add_entry) {
351-
if (mptcp_pm_is_kernel(msk))
351+
if (WARN_ON_ONCE(mptcp_pm_is_kernel(msk)))
352352
return false;
353353

354354
sk_reset_timer(sk, &add_entry->add_timer,
@@ -512,8 +512,8 @@ __lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info)
512512

513513
static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
514514
{
515+
struct mptcp_pm_addr_entry *local, *signal_and_subflow = NULL;
515516
struct sock *sk = (struct sock *)msk;
516-
struct mptcp_pm_addr_entry *local;
517517
unsigned int add_addr_signal_max;
518518
unsigned int local_addr_max;
519519
struct pm_nl_pernet *pernet;
@@ -555,8 +555,6 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
555555

556556
/* check first for announce */
557557
if (msk->pm.add_addr_signaled < add_addr_signal_max) {
558-
local = select_signal_address(pernet, msk);
559-
560558
/* due to racing events on both ends we can reach here while
561559
* previous add address is still running: if we invoke now
562560
* mptcp_pm_announce_addr(), that will fail and the
@@ -567,26 +565,41 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
567565
if (msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL))
568566
return;
569567

570-
if (local) {
571-
if (mptcp_pm_alloc_anno_list(msk, &local->addr)) {
572-
__clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
573-
msk->pm.add_addr_signaled++;
574-
mptcp_pm_announce_addr(msk, &local->addr, false);
575-
mptcp_pm_nl_addr_send_ack(msk);
576-
}
577-
}
568+
local = select_signal_address(pernet, msk);
569+
if (!local)
570+
goto subflow;
571+
572+
/* If the alloc fails, we are on memory pressure, not worth
573+
* continuing, and trying to create subflows.
574+
*/
575+
if (!mptcp_pm_alloc_anno_list(msk, &local->addr))
576+
return;
577+
578+
__clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
579+
msk->pm.add_addr_signaled++;
580+
mptcp_pm_announce_addr(msk, &local->addr, false);
581+
mptcp_pm_nl_addr_send_ack(msk);
582+
583+
if (local->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW)
584+
signal_and_subflow = local;
578585
}
579586

587+
subflow:
580588
/* check if should create a new subflow */
581589
while (msk->pm.local_addr_used < local_addr_max &&
582590
msk->pm.subflows < subflows_max) {
583591
struct mptcp_addr_info addrs[MPTCP_PM_ADDR_MAX];
584592
bool fullmesh;
585593
int i, nr;
586594

587-
local = select_local_address(pernet, msk);
588-
if (!local)
589-
break;
595+
if (signal_and_subflow) {
596+
local = signal_and_subflow;
597+
signal_and_subflow = NULL;
598+
} else {
599+
local = select_local_address(pernet, msk);
600+
if (!local)
601+
break;
602+
}
590603

591604
fullmesh = !!(local->flags & MPTCP_PM_ADDR_FLAG_FULLMESH);
592605

@@ -1328,8 +1341,8 @@ int mptcp_pm_nl_add_addr_doit(struct sk_buff *skb, struct genl_info *info)
13281341
if (ret < 0)
13291342
return ret;
13301343

1331-
if (addr.addr.port && !(addr.flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
1332-
GENL_SET_ERR_MSG(info, "flags must have signal when using port");
1344+
if (addr.addr.port && !address_use_port(&addr)) {
1345+
GENL_SET_ERR_MSG(info, "flags must have signal and not subflow when using port");
13331346
return -EINVAL;
13341347
}
13351348

‎tools/testing/selftests/net/mptcp/mptcp_join.sh‎

Lines changed: 41 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1415,18 +1415,28 @@ chk_add_nr()
14151415
local add_nr=$1
14161416
local echo_nr=$2
14171417
local port_nr=${3:-0}
1418-
local syn_nr=${4:-$port_nr}
1419-
local syn_ack_nr=${5:-$port_nr}
1420-
local ack_nr=${6:-$port_nr}
1421-
local mis_syn_nr=${7:-0}
1422-
local mis_ack_nr=${8:-0}
1418+
local ns_invert=${4:-""}
1419+
local syn_nr=$port_nr
1420+
local syn_ack_nr=$port_nr
1421+
local ack_nr=$port_nr
1422+
local mis_syn_nr=0
1423+
local mis_ack_nr=0
1424+
local ns_tx=$ns1
1425+
local ns_rx=$ns2
1426+
local extra_msg=""
14231427
local count
14241428
local timeout
14251429

1426-
timeout=$(ip netns exec $ns1 sysctl -n net.mptcp.add_addr_timeout)
1430+
if [[ $ns_invert = "invert" ]]; then
1431+
ns_tx=$ns2
1432+
ns_rx=$ns1
1433+
extra_msg="invert"
1434+
fi
1435+
1436+
timeout=$(ip netns exec ${ns_tx} sysctl -n net.mptcp.add_addr_timeout)
14271437

14281438
print_check "add"
1429-
count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtAddAddr")
1439+
count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtAddAddr")
14301440
if [ -z "$count" ]; then
14311441
print_skip
14321442
# if the test configured a short timeout tolerate greater then expected
@@ -1438,7 +1448,7 @@ chk_add_nr()
14381448
fi
14391449

14401450
print_check "echo"
1441-
count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtEchoAdd")
1451+
count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtEchoAdd")
14421452
if [ -z "$count" ]; then
14431453
print_skip
14441454
elif [ "$count" != "$echo_nr" ]; then
@@ -1449,7 +1459,7 @@ chk_add_nr()
14491459

14501460
if [ $port_nr -gt 0 ]; then
14511461
print_check "pt"
1452-
count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtPortAdd")
1462+
count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtPortAdd")
14531463
if [ -z "$count" ]; then
14541464
print_skip
14551465
elif [ "$count" != "$port_nr" ]; then
@@ -1459,7 +1469,7 @@ chk_add_nr()
14591469
fi
14601470

14611471
print_check "syn"
1462-
count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinPortSynRx")
1472+
count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPJoinPortSynRx")
14631473
if [ -z "$count" ]; then
14641474
print_skip
14651475
elif [ "$count" != "$syn_nr" ]; then
@@ -1470,7 +1480,7 @@ chk_add_nr()
14701480
fi
14711481

14721482
print_check "synack"
1473-
count=$(mptcp_lib_get_counter ${ns2} "MPTcpExtMPJoinPortSynAckRx")
1483+
count=$(mptcp_lib_get_counter ${ns_rx} "MPTcpExtMPJoinPortSynAckRx")
14741484
if [ -z "$count" ]; then
14751485
print_skip
14761486
elif [ "$count" != "$syn_ack_nr" ]; then
@@ -1481,7 +1491,7 @@ chk_add_nr()
14811491
fi
14821492

14831493
print_check "ack"
1484-
count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMPJoinPortAckRx")
1494+
count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMPJoinPortAckRx")
14851495
if [ -z "$count" ]; then
14861496
print_skip
14871497
elif [ "$count" != "$ack_nr" ]; then
@@ -1492,7 +1502,7 @@ chk_add_nr()
14921502
fi
14931503

14941504
print_check "syn"
1495-
count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMismatchPortSynRx")
1505+
count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMismatchPortSynRx")
14961506
if [ -z "$count" ]; then
14971507
print_skip
14981508
elif [ "$count" != "$mis_syn_nr" ]; then
@@ -1503,7 +1513,7 @@ chk_add_nr()
15031513
fi
15041514

15051515
print_check "ack"
1506-
count=$(mptcp_lib_get_counter ${ns1} "MPTcpExtMismatchPortAckRx")
1516+
count=$(mptcp_lib_get_counter ${ns_tx} "MPTcpExtMismatchPortAckRx")
15071517
if [ -z "$count" ]; then
15081518
print_skip
15091519
elif [ "$count" != "$mis_ack_nr" ]; then
@@ -1513,6 +1523,8 @@ chk_add_nr()
15131523
print_ok
15141524
fi
15151525
fi
1526+
1527+
print_info "$extra_msg"
15161528
}
15171529

15181530
chk_add_tx_nr()
@@ -1977,6 +1989,21 @@ signal_address_tests()
19771989
chk_add_nr 1 1
19781990
fi
19791991

1992+
# uncommon: subflow and signal flags on the same endpoint
1993+
# or because the user wrongly picked both, but still expects the client
1994+
# to create additional subflows
1995+
if reset "subflow and signal together"; then
1996+
pm_nl_set_limits $ns1 0 2
1997+
pm_nl_set_limits $ns2 0 2
1998+
pm_nl_add_endpoint $ns2 10.0.3.2 flags signal,subflow
1999+
run_tests $ns1 $ns2 10.0.1.1
2000+
chk_join_nr 1 1 1
2001+
chk_add_nr 1 1 0 invert # only initiated by ns2
2002+
chk_add_nr 0 0 0 # none initiated by ns1
2003+
chk_rst_nr 0 0 invert # no RST sent by the client
2004+
chk_rst_nr 0 0 # no RST sent by the server
2005+
fi
2006+
19802007
# accept and use add_addr with additional subflows
19812008
if reset "multiple subflows and signal"; then
19822009
pm_nl_set_limits $ns1 0 3

0 commit comments

Comments
 (0)