diff mbox series

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

Message ID 11a224e1bade27689271af77cd47c6a49852a2cf.1725543947.git.tanggeliang@kylinos.cn (mailing list archive)
State Rejected, archived
Delegated to: Matthieu Baerts
Headers show
Series [mptcp-next] Squash to "selftests/bpf: Add getsockopt to inspect mptcp subflow" | expand

Checks

Context Check Description
matttbe/checkpatch success total: 0 errors, 0 warnings, 0 checks, 36 lines checked
matttbe/shellcheck success MPTCP selftests files have not been modified
matttbe/build success Build and static analysis OK
matttbe/KVM_Validation__normal success Success! ✅
matttbe/KVM_Validation__debug success Success! ✅
matttbe/KVM_Validation__btf__only_bpftest_all_ success Success! ✅

Commit Message

Geliang Tang Sept. 5, 2024, 1:46 p.m. UTC 
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(-)

Comments

Matthieu Baerts Sept. 5, 2024, 2:03 p.m. UTC | #1 
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
MPTCP CI Sept. 5, 2024, 2:43 p.m. UTC | #2 
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)
Geliang Tang Sept. 6, 2024, 9:45 a.m. UTC | #3 
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
Geliang Tang Sept. 7, 2024, 3:48 a.m. UTC | #4 
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
>
diff mbox series

Patch

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;
 		}
 	}