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

[Geliang Tang <geliang@kernel.org>]

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