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

[Matthieu Baerts <matttbe@kernel.org>]

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.