Re: [PATCH 2/2] add selftest for TCP_ULP in bpf_setsockopt

[Martin KaFai Lau <martin.lau@xxxxxxxxx>]

On 1/27/25 1:07 AM, zhangmingyi wrote:
> This case invokes bpf_setsockopt and bpf_getsockopt to set ulp.
> The existing smc_ulp_ops of the kernel is used as a test case to test
> whether the setting and get operations can be performed normally.
>
> Signed-off-by: zhangmingyi <zhangmingyi5@xxxxxxxxxx>
> ---
>   .../selftests/bpf/progs/setget_sockopt.c      | 21 ++++++++++++++++---
>   1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/progs/setget_sockopt.c b/tools/testing/selftests/bpf/progs/setget_sockopt.c
> index 6dd4318debbf..dcdf26ef41c4 100644
> --- a/tools/testing/selftests/bpf/progs/setget_sockopt.c
> +++ b/tools/testing/selftests/bpf/progs/setget_sockopt.c
> @@ -327,6 +327,18 @@ static int test_tcp_maxseg(void *ctx, struct sock *sk)
>   	return 0;
>   }
>   
> +static int test_tcp_ulp(void *ctx, struct sock *sk)
> +{
> +	__u8 saved_syn[20];
> +
> +	if (sk->sk_state == TCP_SYN_SENT)
> +		return bpf_setsockopt(ctx, IPPROTO_TCP, TCP_ULP,
> +						"smc", sizeof("smc"));

The test_progs/setget_sockopt.c is using "tls" in a setsockopt(TCP_ULP) call. I 
would rather not to introduce another ulp in this selftest. Let stay with "tls".

btw, the indentation is off...

> +
> +	return bpf_getsockopt(ctx, IPPROTO_TCP, TCP_ULP,
> +			    saved_syn, sizeof(saved_syn));

same here on indentation.

Also, the getsockopt test should ensure it gets the same ulp name back (i.e. 
"tls"). Take a look at bpf_strncmp.

> +}
> +
>   static int test_tcp_saved_syn(void *ctx, struct sock *sk)
>   {
>   	__u8 saved_syn[20];
> @@ -395,16 +407,19 @@ int skops_sockopt(struct bpf_sock_ops *skops)
>   		break;
>   	case BPF_SOCK_OPS_TCP_CONNECT_CB:
>   		nr_connect += !(bpf_test_sockopt(skops, sk) ||
> -				test_tcp_maxseg(skops, sk));
> +				test_tcp_maxseg(skops, sk) ||
> +				test_tcp_ulp(skops, sk));

For other optnames, it makes sense to reuse the existing "skops_sockopt" BPF 
program. For ulp, it could change the sendmsg, recvmsg, and a few other 
behaviors. I would prefer to separate it out into its own BPF program to avoid 
future surprises on the existing tests in prog_tests/setget_sockopt.c. Keep the 
new BPF program simple, e.g. implement a new BPF program for 
"lsm_cgroup/socket_post_create" and only check for bpf_set/getsockopt(TCP_ULP).

Please tag the set for bpf-next. The "ipv4" in the patch 1's subject is not 
accurate also. afaik, ulp is not specific to ipv4.

Also, the bpf CI complains that the test cannot compile.

pw-bot: cr

>   		break;
>   	case BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB:
>   		nr_active += !(bpf_test_sockopt(skops, sk) ||
> -			       test_tcp_maxseg(skops, sk));
> +			       test_tcp_maxseg(skops, sk) ||
> +				   test_tcp_ulp(skops, sk));
>   		break;
>   	case BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB:
>   		nr_passive += !(bpf_test_sockopt(skops, sk) ||
>   				test_tcp_maxseg(skops, sk) ||
> -				test_tcp_saved_syn(skops, sk));
> +				test_tcp_saved_syn(skops, sk) ||
> +				test_tcp_ulp(skops, sk));
>   		flags = skops->bpf_sock_ops_cb_flags | BPF_SOCK_OPS_STATE_CB_FLAG;
>   		bpf_setsockopt(skops, SOL_TCP, TCP_BPF_SOCK_OPS_CB_FLAGS, &flags, sizeof(flags));
>   		break;