Squash to "selftests/bpf: Add getsockopt to inspect mptcp subflow"

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

Posted by Geliang Tang 2 months, 4 weeks ago

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"

Posted by Matthieu Baerts 2 months, 4 weeks ago

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"

Posted by Geliang Tang 2 months, 4 weeks ago

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"

Posted by Geliang Tang 2 months, 4 weeks ago

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
>