[PATCH mptcp-net] mptcp: do not fill info not used by the PM in used

[PATCH mptcp-net] mptcp: do not fill info not used by the PM in used

[Matthieu Baerts]

Only the in-kernel PM uses the number of address and subflow limits
allowed per connection.

It then makes more sense not to display such info when other PMs are
used not to confuse the userspace by showing limits not being used.

While at it, we can get rid of the "val" variable and add indentations
instead.

Fixes: 3fd4c2a2d672 ("mptcp: bypass in-kernel PM restrictions for non-kernel PMs")
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/sockopt.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index f7842a0b6536..fa1e72235f28 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -889,7 +889,6 @@ static int mptcp_getsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
 void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
 {
 	u32 flags = 0;
-	u8 val;
 
 	memset(info, 0, sizeof(*info));
 
@@ -897,12 +896,19 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
 	info->mptcpi_add_addr_signal = READ_ONCE(msk->pm.add_addr_signaled);
 	info->mptcpi_add_addr_accepted = READ_ONCE(msk->pm.add_addr_accepted);
 	info->mptcpi_local_addr_used = READ_ONCE(msk->pm.local_addr_used);
-	info->mptcpi_subflows_max = mptcp_pm_get_subflows_max(msk);
-	val = mptcp_pm_get_add_addr_signal_max(msk);
-	info->mptcpi_add_addr_signal_max = val;
-	val = mptcp_pm_get_add_addr_accept_max(msk);
-	info->mptcpi_add_addr_accepted_max = val;
-	info->mptcpi_local_addr_max = mptcp_pm_get_local_addr_max(msk);
+
+	/* The following limits only make sense for the in-kernel PM */
+	if (mptcp_pm_is_kernel(msk)) {
+		info->mptcpi_subflows_max =
+			mptcp_pm_get_subflows_max(msk);
+		info->mptcpi_add_addr_signal_max =
+			mptcp_pm_get_add_addr_signal_max(msk);
+		info->mptcpi_add_addr_accepted_max =
+			mptcp_pm_get_add_addr_accept_max(msk);
+		info->mptcpi_local_addr_max =
+			mptcp_pm_get_local_addr_max(msk);
+	}
+
 	if (test_bit(MPTCP_FALLBACK_DONE, &msk->flags))
 		flags |= MPTCP_INFO_FLAG_FALLBACK;
 	if (READ_ONCE(msk->can_ack))

---
base-commit: 7264bacd354401e60be232ea28eb4a8a249937e4
change-id: 20230223-mptcp-diag-info-limits-in-kernel-pm-65bf3b67132d

Best regards,
-- 
Matthieu Baerts <matthieu.baerts@tessares.net>

Re: mptcp: do not fill info not used by the PM in used: Tests Results

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6249986447376384
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6249986447376384/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/4842611563823104
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4842611563823104/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5405561517244416
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5405561517244416/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5968511470665728
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5968511470665728/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/48c8d60ff9f4


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-debug

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 (Tessares)

Re: [PATCH mptcp-net] mptcp: do not fill info not used by the PM in used

[Matthieu Baerts]

Hi Paolo,

On 23/02/2023 12:20, Matthieu Baerts wrote:
> Only the in-kernel PM uses the number of address and subflow limits
> allowed per connection.
> 
> It then makes more sense not to display such info when other PMs are
> used not to confuse the userspace by showing limits not being used.
> 
> While at it, we can get rid of the "val" variable and add indentations
> instead.
> 
> Fixes: 3fd4c2a2d672 ("mptcp: bypass in-kernel PM restrictions for non-kernel PMs")
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>

Thank you for the review (on IRC).

Now in our tree (feat. for next) without the Fixes tag but with your ACK:

New patches for t/upstream:
- 5435c97679c1: mptcp: do not fill info not used by the PM in used
- Results: 6ce53727880d..fc5dcb18c972 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230309T093419

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-net] mptcp: do not fill info not used by the PM in used

[Geliang Tang]

Hi Matt, Paolo,

Sorry for the late reply. I think these limits are still useful for
userspace PM too, although we haven't used them yet. Since we also do
not allow userspace PM to use addresses without limits.

If you also think it's necessary to use these limits, let's add a
ticket for this on Github. And I'll look at it.

If so, I think this patch should be reverted except the var 'val'
should be dropped indeed. How about only dropping 'val' in this patch?
WDYT?

Thanks,
-Geliang

Matthieu Baerts <matthieu.baerts@tessares.net> 于2023年3月9日周四 17:35写道：
>
> Hi Paolo,
>
> On 23/02/2023 12:20, Matthieu Baerts wrote:
> > Only the in-kernel PM uses the number of address and subflow limits
> > allowed per connection.
> >
> > It then makes more sense not to display such info when other PMs are
> > used not to confuse the userspace by showing limits not being used.
> >
> > While at it, we can get rid of the "val" variable and add indentations
> > instead.
> >
> > Fixes: 3fd4c2a2d672 ("mptcp: bypass in-kernel PM restrictions for non-kernel PMs")
> > Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>
> Thank you for the review (on IRC).
>
> Now in our tree (feat. for next) without the Fixes tag but with your ACK:
>
> New patches for t/upstream:
> - 5435c97679c1: mptcp: do not fill info not used by the PM in used
> - Results: 6ce53727880d..fc5dcb18c972 (export)
>
> Tests are now in progress:
>
> https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230309T093419
>
> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
>

Re: [PATCH mptcp-net] mptcp: do not fill info not used by the PM in used

[Matthieu Baerts]

Hi Geliang,

On 15/03/2023 06:09, Geliang Tang wrote:
> Sorry for the late reply. I think these limits are still useful for
> userspace PM too, although we haven't used them yet. Since we also do
> not allow userspace PM to use addresses without limits.

I think it is better to let the userspace PM daemon managing these
limits itself. I guess that's why the limits have been explicitly
bypassed for this PM, see commit 3fd4c2a2d672 ("mptcp: bypass in-kernel
PM restrictions for non-kernel PMs").

> If you also think it's necessary to use these limits, let's add a
> ticket for this on Github. And I'll look at it.
> 
> If so, I think this patch should be reverted except the var 'val'
> should be dropped indeed. How about only dropping 'val' in this patch?
> WDYT?

I think we should keep this patch and have these limits only used by the
in-kernel PM. Except if you see a case where these limits cannot be
properly handled by the userspace PM daemon?

Also, we might want to avoid the complexity of tracking in the kernel
which ADD_ADDR have been received and how they are being used by the
userspace, etc. But that's a technical detail.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net