Re: fentry/fexit attach to EXT type XDP program does not work

["Eelco Chaudron" <echaudro@redhat.com>]

On 27 Jul 2020, at 16:53, Jiri Olsa wrote:

> On Mon, Jul 27, 2020 at 09:59:14AM +0200, Eelco Chaudron wrote:
>>
>>
>> On 26 Jul 2020, at 14:24, Jiri Olsa wrote:
>>
>>> On Tue, Jun 09, 2020 at 10:52:34AM +0200, Eelco Chaudron wrote:
>>>
>>> SNIP
>>>
>>>>>>    libbpf: failed to load object 'test_xdp_bpf2bpf'
>>>>>>    libbpf: failed to load BPF skeleton 'test_xdp_bpf2bpf': -4007
>>>>>>    test_xdp_fentry_ext:FAIL:__load ftrace skeleton failed
>>>>>>    #91 xdp_fentry_ext:FAIL
>>>>>>    Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED
>>>>>>
>>>>>> Any idea what could be the case here? The same fentry/fexit 
>>>>>> attach
>>>>>> code works fine in the xdp_bpf2bpf.c tests case.
>>>>
>>>> <SNIP>
>>>>>
>>>>> I think this is not supported now. That is, you cannot attach a
>>>>> fentry
>>>>> trace
>>>>> to the EXT program. The current implementation for fentry
>>>>> program simply
>>>>> trying to find and match the signature of freplace program which 
>>>>> by
>>>>> default
>>>>> is a pointer to void.
>>>>>
>>>>> It is doable in that in kernel we could recognize to-be-attached
>>>>> program
>>>>> is
>>>>> a freplace and further trace down to find the real signature. The
>>>>> related
>>>>> kernel function is btf_get_prog_ctx_type(). You can try to
>>>>> implement by
>>>>> yourself
>>>>> or I can have a patch for this once bpf-next opens.
>>>>
>>>> I’m not familiar with this area of the code, so if you could 
>>>> prepare
>>>> a patch
>>>> that would nice.
>>>> You can also send it to me before bpf-next opens and I can verify
>>>> it, and
>>>> clean up the self-test so it can be included as well.
>>>>
>>>
>>> hi,
>>> it seems that you cannot exten fentry/fexit programs,
>>> but it's possible to attach fentry/fexit to ext program.
>>>
>>>    /* Program extensions can extend all program types
>>>     * except fentry/fexit. The reason is the following.
>>>     * The fentry/fexit programs are used for performance
>>>     * analysis, stats and can be attached to any program
>>>     * type except themselves. When extension program is
>>>     * replacing XDP function it is necessary to allow
>>>     * performance analysis of all functions. Both original
>>>     * XDP program and its program extension. Hence
>>>     * attaching fentry/fexit to BPF_PROG_TYPE_EXT is
>>>     * allowed. If extending of fentry/fexit was allowed it
>>>     * would be possible to create long call chain
>>>     * fentry->extension->fentry->extension beyond
>>>     * reasonable stack size. Hence extending fentry is not
>>>     * allowed.
>>>     */
>>>
>>> I changed fexit_bpf2bpf.c test just to do a quick check
>>> and it seems to work:
>>
>> Hi Jiri this is exactly what I’m trying, however when you do this 
>> where the
>> first argument is a pointer to some context data which you are 
>> accessing
>> it’s failing in the verifier.
>> This is a link to the original email, which has a test patch attached 
>> that
>> will show the failure when trying to load/attach the fentry function 
>> and
>> access the context:
>>
>> https://lore.kernel.org/bpf/159162546868.10791.12432342618156330247.stgit@ebuild/
>
> ok, I tried to trace ext program with __sk_buff argument and I can see
> the issue as well.. can't acess the skb argument
>
> patch below fixes it for me, I can access the skb pointer and its data
> via probe read, like:
>
> 	SEC("fexit/new_get_skb_ifindex")
> 	int BPF_PROG(fexit_new_get_skb_ifindex, int val, struct __sk_buff 
> *skb, int var, int ret)
> 	{
> 		__u32 data;
> 		int err;
>
> 		bpf_printk("EXIT skb %p", skb);
> 		bpf_probe_read_kernel(&data, sizeof(data), &skb->data);
> 		bpf_printk("EXIT ret %d, data %p", err, data);
> 		return 0;
> 	}
>
> I think it should fix the xdp_md acess as well

Excellent patch ;) It works with xdp_md as well, and even better it does 
not require the bpf_probe_read_kernel(), so the test_xdp_bpf2bpf.c code 
just works.

Are you planning to send the patch upstream?

> jirka
>
>
> ---
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index ee36b7f60936..2145329f7b1b 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3828,6 +3828,10 @@ bool btf_ctx_access(int off, int size, enum 
> bpf_access_type type,
>  	}
>
>  	info->reg_type = PTR_TO_BTF_ID;
> +
> +	if (tgt_prog && tgt_prog->type == BPF_PROG_TYPE_EXT)
> +		tgt_prog = tgt_prog->aux->linked_prog;
> +
>  	if (tgt_prog) {
>  		ret = btf_translate_to_vmlinux(log, btf, t, tgt_prog->type, arg);
>  		if (ret > 0) {