Re: [PATCH v2 bpf-next 3/3] selftests/bpf: Add tests for bpf_sock_destroy

From: Aditi Ghag <aditi.ghag@isovalent.com>
To: Martin KaFai Lau <martin.lau@linux.dev>
Cc: kafai@fb.com, Stanislav Fomichev <sdf@google.com>,
	edumazet@google.com, bpf@vger.kernel.org
Subject: Re: [PATCH v2 bpf-next 3/3] selftests/bpf: Add tests for bpf_sock_destroy
Date: Tue, 28 Feb 2023 18:17:59 -0800	[thread overview]
Message-ID: <F6E6FEAD-5003-44BE-AA76-6CDAE40A0A71@isovalent.com> (raw)
In-Reply-To: <2552f727-57f3-0d76-c0da-f6543a93a45f@linux.dev>

> On Feb 28, 2023, at 3:08 PM, Martin KaFai Lau <martin.lau@linux.dev> wrote:
> 
> On 2/23/23 1:53 PM, Aditi Ghag wrote:
>> The test cases for TCP and UDP iterators mirror the intended usages of the
>> helper.
>> The destroy helpers set `ECONNABORTED` error code that we can validate in the
>> test code with client sockets. But UDP sockets have an overriding error code
>> from the disconnect called during abort, so the error code the validation is
>> only done for TCP sockets.
>> The `struct sock` is redefined as vmlinux.h forward declares the struct, and the
>> loader fails to load the program as it finds the BTF FWD type for the struct
>> incompatible with the BTF STRUCT type.
>> Here are the snippets of the verifier error, and corresponding BTF output:
>> ```
>> verifier error: extern (func ksym) ...: func_proto ... incompatible with kernel
>> BTF for selftest prog binary:
>> [104] FWD 'sock' fwd_kind=struct
>> [70] PTR '(anon)' type_id=104
>> [84] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1
>> 	'(anon)' type_id=70
>> [85] FUNC 'bpf_sock_destroy' type_id=84 linkage=extern
>> --
>> [96] DATASEC '.ksyms' size=0 vlen=1
>> 	type_id=85 offset=0 size=0 (FUNC 'bpf_sock_destroy')
>> BTF for selftest vmlinux:
>> [74923] FUNC 'bpf_sock_destroy' type_id=48965 linkage=static
>> [48965] FUNC_PROTO '(anon)' ret_type_id=9 vlen=1
>> 	'sk' type_id=1340
>> [1340] PTR '(anon)' type_id=2363
>> [2363] STRUCT 'sock' size=1280 vlen=93
>> ```
> 
>> +int bpf_sock_destroy(struct sock_common *sk) __ksym;
> 
> This does not match the bpf prog's BTF dump above which has pointer [70] pointing to FWD 'sock' [104] as the argument. It should be at least FWD 'sock_common' if not STRUCT 'sock_common'. I tried to change the func signature to 'struct sock *sk' but cannot reproduce the issue in my environment also.
> 
> Could you confirm the BTF paste and 'incompatible with kernel" error in the commit message do match the bpf_sock_destroy declaration? If not, can you re-paste the BTF output and libbpf error message that matches the bpf_sock_destroy signature.

I don't think you'll be able to reproduce the issue with `sock_common`, as `struct sock_common` isn't forward declared in vmlinux.h. But I find it odd that you weren't able to reproduce it with `struct sock`. Just to confirm, did you remove the minimal `struct sock` definition from the program? Per the commit description, I added that because libbpf was throwing this error -
`libbpf: extern (func ksym) 'bpf_sock_destroy': func_proto [83] incompatible with kernel [75285]`

Sending the BTF snippet again (full BTF - https://pastebin.com/etkFyuJk)

```
85] FUNC 'bpf_sock_destroy' type_id=84 linkage=extern
	type_id=85 offset=0 size=0 (FUNC 'bpf_sock_destroy')
[84] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1
	'(anon)' type_id=70
[70] PTR '(anon)' type_id=104
[104] FWD 'sock' fwd_kind=struct
```

Compare this to the BTF snippet once I undef and define the struct in the test prog:

```
[87] FUNC 'bpf_sock_destroy' type_id=84 linkage=extern
	type_id=87 offset=0 size=0 (FUNC 'bpf_sock_destroy')
[84] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1
	'(anon)' type_id=85
[85] PTR '(anon)' type_id=86
[86] STRUCT 'sock' size=136 vlen=1
	'__sk_common' type_id=34 bits_offset=0
```

(Anyway looks like I needed to define the struct in the test prog only when bpf_sock_destory had `struct sock` as the argument.)