Date:   Wed, 8 Sep 2021 13:36:58 +0200
From:   Johan Almbladh <johan.almbladh@...finetworks.com>
To:     Tiezhu Yang <yangtiezhu@...ngson.cn>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Shubham Bansal <illusionist.neo@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Zi Shen Lim <zlim.lnx@...il.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Paul Burton <paulburton@...nel.org>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        naveen.n.rao@...ux.ibm.com, Michael Ellerman <mpe@...erman.id.au>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Luke Nelson <luke.r.nels@...il.com>,
        Xi Wang <xi.wang@...il.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>, bjorn@...nel.org,
        "David S. Miller" <davem@...emloft.net>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Paul Chaignon <paul@...ium.io>
Subject: Re: [RFC PATCH bpf-next] bpf: Make actual max tail call count as MAX_TAIL_CALL_CNT

On Wed, Sep 8, 2021 at 12:56 PM Tiezhu Yang <yangtiezhu@...ngson.cn> wrote:
>
> On 09/08/2021 04:47 PM, Daniel Borkmann wrote:
> > [ You have a huge Cc list, but forgot to add Paul and Johan who recently
> >   looked into this. Added here. ]
> >
> > On 9/8/21 10:20 AM, Tiezhu Yang wrote:
> >> In the current code, the actual max tail call count is 33 which is
> >> greater
> >> than MAX_TAIL_CALL_CNT, this is not consistent with the intended meaning
> >> in the commit 04fd61ab36ec ("bpf: allow bpf programs to tail-call other
> >> bpf programs"):
> >>
> >> "The chain of tail calls can form unpredictable dynamic loops therefore
> >> tail_call_cnt is used to limit the number of calls and currently is set
> >> to 32."
> >>
> >> Additionally, after commit 874be05f525e ("bpf, tests: Add tail call test
> >> suite"), we can see there exists failed testcase.
> >>
> >> On all archs when CONFIG_BPF_JIT_ALWAYS_ON is not set:
> >>   # echo 0 > /proc/sys/net/core/bpf_jit_enable
> >>   # modprobe test_bpf
> >>   # dmesg | grep -w FAIL
> >>   Tail call error path, max count reached jited:0 ret 34 != 33 FAIL
> >>
> >> On some archs:
> >>   # echo 1 > /proc/sys/net/core/bpf_jit_enable
> >>   # modprobe test_bpf
> >>   # dmesg | grep -w FAIL
> >>   Tail call error path, max count reached jited:1 ret 34 != 33 FAIL
> >>
> >> with this patch, make the actual max tail call count as
> >> MAX_TAIL_CALL_CNT,
> >> at the same time, the above failed testcase can be fixed.
> >>
> >> Signed-off-by: Tiezhu Yang <yangtiezhu@...ngson.cn>
> >> ---
> >>
> >> Hi all,
> >>
> >> This is a RFC patch, if I am wrong or I missed something,
> >> please let me know, thank you!
> >
> > Yes, the original commit from 04fd61ab36ec ("bpf: allow bpf programs
> > to tail-call
> > other bpf programs") got the counting wrong, but please also check
> > f9dabe016b63
> > ("bpf: Undo off-by-one in interpreter tail call count limit") where we
> > agreed to
> > align everything to 33 in order to avoid changing existing behavior,
> > and if we
> > intend to ever change the count, then only in terms of increasing but
> > not decreasing
> > since that ship has sailed.
>
> Thank you, understood.
>
> But I still think there is some confusion about the macro MAX_TAIL_CALL_CNT
> which is 32 and the actual value 33, I spent some time to understand it
> at the first glance.
>
> Is it impossible to keep the actual max tail call count consistent with
> the value 32 of MAX_TAIL_CALL_CNT now?

Yes. If the limit is 32 or 33 does not really matter, but there has to
be a limit. Since the actual limit has been 33, we don't want to break
any user space program relying on this value.

> At least, maybe we need to modify the testcase?

Before making any changes in the test or the BPF code, we need to
understand what the current behaviour actually is. Then we can make
the necessary changes to make everything consistent with 33.

> > Tiezhu, do you still see any arch that is not on 33
> > from your testing?
>
> If the testcase "Tail call error path, max count reached" in test_bpf is
> right,
> it seems that the tail call count limit is 32 on x86, because the testcase
> passed on x86 jited.

When I run the test_bpf.ko suite I get the following limits.

Interpreter: 33
JIT for arm{32,64}, mips, s390x, powerpc{32,64}, sparc: 33
JIT for x86-{32,64}: 32

However, there are also tail call tests in the selftests suite.
According to Paul those tests show that everything is consistent with
33 now. So, there seem to be a discrepancy between the test_bpf.ko
tests and the selftests.

I am trying to investigate this matter further, but so far I have only
been able to run the test_bpf.ko module for various archs in QEMU.
Link: https://github.com/almbladh/test-linux

I am currently working on getting the selftests to run in QEMU too,
using Buildroot. If you have a working setup for this, it would be
great if you could share that.

Thanks,
Johan

>
> > Last time Paul fixed the remaining ones in 96bc4432f5ad ("bpf,
> > riscv: Limit to 33 tail calls") and e49e6f6db04e ("bpf, mips: Limit to
> > 33 tail calls").
> >
> > Thanks,
> > Daniel
>

Powered by blists - more mailing lists