[PATCH mptcp-next] Squash to "selftests/bpf: Add getsockopt to inspect mptcp subflow"

[PATCH mptcp-next] Squash to "selftests/bpf: Add getsockopt to inspect mptcp subflow"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Update error messages:

 run_subflow:FAIL:mark unexpected mark: actual 3 != expected 0
 run_subflow:FAIL:cc unexpected cc: actual 'reno' != expected 'cubic'

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/bpf/progs/mptcp_subflow.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/mptcp_subflow.c b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
index 70302477e326..a5e42bfddbbf 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_subflow.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
@@ -63,6 +63,7 @@ int mptcp_subflow(struct bpf_sock_ops *skops)
 static int _check_getsockopt_subflow_mark(struct mptcp_sock *msk, struct bpf_sockopt *ctx)
 {
 	struct mptcp_subflow_context *subflow;
+	int *optval = ctx->optval;
 	int i = 0;
 
 	mptcp_for_each_subflow(msk, subflow) {
@@ -72,7 +73,10 @@ static int _check_getsockopt_subflow_mark(struct mptcp_sock *msk, struct bpf_soc
 							   struct mptcp_subflow_context));
 
 		if (ssk->sk_mark != ++i) {
-			ctx->retval = -2;
+			if (ctx->optval + sizeof(int) <= ctx->optval_end) {
+				*optval = ssk->sk_mark;
+				ctx->retval = 0;
+			}
 			break;
 		}
 	}
@@ -83,6 +87,7 @@ static int _check_getsockopt_subflow_mark(struct mptcp_sock *msk, struct bpf_soc
 static int _check_getsockopt_subflow_cc(struct mptcp_sock *msk, struct bpf_sockopt *ctx)
 {
 	struct mptcp_subflow_context *subflow;
+	char *optval = ctx->optval;
 
 	mptcp_for_each_subflow(msk, subflow) {
 		struct inet_connection_sock *icsk;
@@ -94,7 +99,10 @@ static int _check_getsockopt_subflow_cc(struct mptcp_sock *msk, struct bpf_socko
 
 		if (ssk->sk_mark == 2 &&
 		    __builtin_memcmp(icsk->icsk_ca_ops->name, cc, TCP_CA_NAME_MAX)) {
-			ctx->retval = -2;
+			if (ctx->optval + TCP_CA_NAME_MAX <= ctx->optval_end) {
+				__builtin_memcpy(optval, icsk->icsk_ca_ops->name, TCP_CA_NAME_MAX);
+				ctx->retval = 0;
+			}
 			break;
 		}
 	}
-- 
2.43.0

Re: [PATCH mptcp-next] Squash to "selftests/bpf: Add getsockopt to inspect mptcp subflow"

[Matthieu Baerts]

Hi Geliang,

On 05/09/2024 15:46, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Update error messages:
> 
>  run_subflow:FAIL:mark unexpected mark: actual 3 != expected 0
>  run_subflow:FAIL:cc unexpected cc: actual 'reno' != expected 'cubic'

That's a good idea to improve the error message in case of error, but I
think this technique or changing the optval is dangerous:

(...)

> diff --git a/tools/testing/selftests/bpf/progs/mptcp_subflow.c b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> index 70302477e326..a5e42bfddbbf 100644
> --- a/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> +++ b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> @@ -63,6 +63,7 @@ int mptcp_subflow(struct bpf_sock_ops *skops)
>  static int _check_getsockopt_subflow_mark(struct mptcp_sock *msk, struct bpf_sockopt *ctx)
>  {
>  	struct mptcp_subflow_context *subflow;
> +	int *optval = ctx->optval;
>  	int i = 0;
>  
>  	mptcp_for_each_subflow(msk, subflow) {
> @@ -72,7 +73,10 @@ static int _check_getsockopt_subflow_mark(struct mptcp_sock *msk, struct bpf_soc
>  							   struct mptcp_subflow_context));
>  
>  		if (ssk->sk_mark != ++i) {
> -			ctx->retval = -2;
> +			if (ctx->optval + sizeof(int) <= ctx->optval_end) {
> +				*optval = ssk->sk_mark;

I guess a likely error to have is that the mark (or the cc) has not been
changed on the subflow, and it then has its default value. In this case
here with the mark, you will not change optval: it will be 0 before, and
you will set it to 0. That means your test will not fail here (retval =
0) or in the verifications done by the userspace (mark = 0).

I think it is safer not to change 'optval' here in the verification
step. If you want to pass info to the userspace, can you not write a
string somewhere? e.g. skel->bss->error_msg, or using a storage map?

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

Re: [PATCH mptcp-next] Squash to "selftests/bpf: Add getsockopt to inspect mptcp subflow"

[Geliang Tang]

Hi Matt,

On Thu, 2024-09-05 at 16:03 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 05/09/2024 15:46, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Update error messages:
> > 
> >  run_subflow:FAIL:mark unexpected mark: actual 3 != expected 0
> >  run_subflow:FAIL:cc unexpected cc: actual 'reno' != expected
> > 'cubic'
> 
> That's a good idea to improve the error message in case of error, but
> I
> think this technique or changing the optval is dangerous:
> 
> (...)
> 
> > diff --git a/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > index 70302477e326..a5e42bfddbbf 100644
> > --- a/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > +++ b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > @@ -63,6 +63,7 @@ int mptcp_subflow(struct bpf_sock_ops *skops)
> >  static int _check_getsockopt_subflow_mark(struct mptcp_sock *msk,
> > struct bpf_sockopt *ctx)
> >  {
> >  	struct mptcp_subflow_context *subflow;
> > +	int *optval = ctx->optval;
> >  	int i = 0;
> >  
> >  	mptcp_for_each_subflow(msk, subflow) {
> > @@ -72,7 +73,10 @@ static int _check_getsockopt_subflow_mark(struct
> > mptcp_sock *msk, struct bpf_soc
> >  							   struct
> > mptcp_subflow_context));
> >  
> >  		if (ssk->sk_mark != ++i) {
> > -			ctx->retval = -2;
> > +			if (ctx->optval + sizeof(int) <= ctx-
> > >optval_end) {
> > +				*optval = ssk->sk_mark;
> 
> I guess a likely error to have is that the mark (or the cc) has not
> been
> changed on the subflow, and it then has its default value. In this
> case
> here with the mark, you will not change optval: it will be 0 before,
> and
> you will set it to 0. That means your test will not fail here (retval
> =
> 0) or in the verifications done by the userspace (mark = 0).
> 
> I think it is safer not to change 'optval' here in the verification
> step. If you want to pass info to the userspace, can you not write a
> string somewhere? e.g. skel->bss->error_msg, or using a storage map?

Let's drop this patch then.

Please send v5 of "new MPTCP subflow subtest" set to bpf-next for me.

Thanks,
-Geliang

> 
> Cheers,
> Matt

Re: [PATCH mptcp-next] Squash to "selftests/bpf: Add getsockopt to inspect mptcp subflow"

[Geliang Tang]

Hi Matt,

On Fri, 2024-09-06 at 17:45 +0800, Geliang Tang wrote:
> Hi Matt,
> 
> On Thu, 2024-09-05 at 16:03 +0200, Matthieu Baerts wrote:
> > Hi Geliang,
> > 
> > On 05/09/2024 15:46, Geliang Tang wrote:
> > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > 
> > > Update error messages:
> > > 
> > >  run_subflow:FAIL:mark unexpected mark: actual 3 != expected 0
> > >  run_subflow:FAIL:cc unexpected cc: actual 'reno' != expected
> > > 'cubic'
> > 
> > That's a good idea to improve the error message in case of error,
> > but
> > I
> > think this technique or changing the optval is dangerous:
> > 
> > (...)
> > 
> > > diff --git a/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > > b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > > index 70302477e326..a5e42bfddbbf 100644
> > > --- a/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > > +++ b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
> > > @@ -63,6 +63,7 @@ int mptcp_subflow(struct bpf_sock_ops *skops)
> > >  static int _check_getsockopt_subflow_mark(struct mptcp_sock
> > > *msk,
> > > struct bpf_sockopt *ctx)
> > >  {
> > >  	struct mptcp_subflow_context *subflow;
> > > +	int *optval = ctx->optval;
> > >  	int i = 0;
> > >  
> > >  	mptcp_for_each_subflow(msk, subflow) {
> > > @@ -72,7 +73,10 @@ static int
> > > _check_getsockopt_subflow_mark(struct
> > > mptcp_sock *msk, struct bpf_soc
> > >  							  
> > > struct
> > > mptcp_subflow_context));
> > >  
> > >  		if (ssk->sk_mark != ++i) {
> > > -			ctx->retval = -2;
> > > +			if (ctx->optval + sizeof(int) <= ctx-
> > > > optval_end) {
> > > +				*optval = ssk->sk_mark;
> > 
> > I guess a likely error to have is that the mark (or the cc) has not
> > been
> > changed on the subflow, and it then has its default value. In this
> > case
> > here with the mark, you will not change optval: it will be 0
> > before,
> > and
> > you will set it to 0. That means your test will not fail here
> > (retval
> > =
> > 0) or in the verifications done by the userspace (mark = 0).
> > 
> > I think it is safer not to change 'optval' here in the verification
> > step. If you want to pass info to the userspace, can you not write
> > a
> > string somewhere? e.g. skel->bss->error_msg, or using a storage
> > map?
> 
> Let's drop this patch then.
> 
> Please send v5 of "new MPTCP subflow subtest" set to bpf-next for me.

One more squash-to patch for this set, use ASSERT_OK_FD to check the
fds.

Thanks,
-Geliang 

> 
> Thanks,
> -Geliang
> 
> > 
> > Cheers,
> > Matt
> 

Re: [PATCH mptcp-next] Squash to "selftests/bpf: Add getsockopt to inspect mptcp subflow"

[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 (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10721992952

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


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)