Re: [PATCH bpf] bpf: handle return value of BPF_PROG_TYPE_STRUCT_OPS prog

[Hou Tao <houtao1@huawei.com>]

Hi,

On 9/8/2021 2:06 PM, Martin KaFai Lau wrote:
> On Wed, Sep 01, 2021 at 04:53:44PM +0800, Hou Tao wrote:
>> Currently if a function ptr in struct_ops has a return value, its
>> caller will get a random return value from it, because the return
>> value of related BPF_PROG_TYPE_STRUCT_OPS prog is just dropped.
>>
>> So adding a new flag BPF_TRAMP_F_RET_FENTRY_RET to tell bpf trampoline
>> to save and return the return value of struct_ops prog if ret_size of
>> the function ptr is greater than 0. Also restricting the flag to be
>> used alone.
> Thanks for the report and fix!  Sorry for the late reply.
>
> This bug is missed because the tcp-cc func is not always called.
> A better test needs to be created to force exercising these funcs
> in bpf_test_run(), which can be a follow-up patch in the bpf-next.
> Could you help to create this test as a follow up?

Yes, will do. The first thought comes into my mind is implementing .get_info hook

in a bpf tcp_congestion_ops and checking its return value in userspace by

getsockopt(fd, TCP_CC_INFO).  I also consider to add a new BPF struct_ops

for testing purpose, but it may be a little overkill.

> The patch lgtm.  However, it does not apply cleanly on bpf,
> so please rebase and repost.  I applied it manually and
> tested it by hard coding to call the ->ssthresh() and
> observes the return value.

I just check that it can be applied both on bpf and bpf-next, do you

have other commits in your tree ?

> .