From: Joe Stringer <joe@wand.net.nz>
To: Yonghong Song <yhs@fb.com>
Cc: Joe Stringer <joe@wand.net.nz>, bpf <bpf@vger.kernel.org>,
Lorenz Bauer <lmb@cloudflare.com>,
netdev <netdev@vger.kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Alexei Starovoitov <ast@kernel.org>,
Eric Dumazet <eric.dumazet@gmail.com>,
Martin KaFai Lau <kafai@fb.com>
Subject: Re: [PATCHv2 bpf-next 5/5] selftests: bpf: add test for sk_assign
Date: Wed, 25 Mar 2020 16:07:32 -0700 [thread overview]
Message-ID: <CAOftzPjnXNM387OP4jN+EzKG05u_5Es2OH7PDc9cbTodBrMjgg@mail.gmail.com> (raw)
In-Reply-To: <6d92317a-5274-5718-c78e-fb7b309cdee7@fb.com>
On Wed, Mar 25, 2020 at 3:01 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 3/25/20 2:20 PM, Joe Stringer wrote:
> > On Wed, Mar 25, 2020 at 11:18 AM Yonghong Song <yhs@fb.com> wrote:
> >>
> >>
> >>
> >> On 3/24/20 10:57 PM, Joe Stringer wrote:
> >>> From: Lorenz Bauer <lmb@cloudflare.com>
> >>>
> >>> Attach a tc direct-action classifier to lo in a fresh network
> >>> namespace, and rewrite all connection attempts to localhost:4321
> >>> to localhost:1234 (for port tests) and connections to unreachable
> >>> IPv4/IPv6 IPs to the local socket (for address tests).
> >>>
> >>> Keep in mind that both client to server and server to client traffic
> >>> passes the classifier.
> >>>
> >>> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> >>> Co-authored-by: Joe Stringer <joe@wand.net.nz>
> >>> Signed-off-by: Joe Stringer <joe@wand.net.nz>
> >>> ---
> >>> v2: Rebase onto test_progs infrastructure
> >>> v1: Initial commit
> >>> ---
> >>> tools/testing/selftests/bpf/Makefile | 2 +-
> >>> .../selftests/bpf/prog_tests/sk_assign.c | 244 ++++++++++++++++++
> >>> .../selftests/bpf/progs/test_sk_assign.c | 127 +++++++++
> >>> 3 files changed, 372 insertions(+), 1 deletion(-)
> >>> create mode 100644 tools/testing/selftests/bpf/prog_tests/sk_assign.c
> >>> create mode 100644 tools/testing/selftests/bpf/progs/test_sk_assign.c
> >>>
> >>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> >>> index 7729892e0b04..4f7f83d059ca 100644
> >>> --- a/tools/testing/selftests/bpf/Makefile
> >>> +++ b/tools/testing/selftests/bpf/Makefile
> >>> @@ -76,7 +76,7 @@ TEST_PROGS_EXTENDED := with_addr.sh \
> >>> # Compile but not part of 'make run_tests'
> >>> TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
> >>> flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \
> >>> - test_lirc_mode2_user xdping test_cpp runqslower
> >>> + test_lirc_mode2_user xdping test_cpp runqslower test_sk_assign
> >>
> >> No test_sk_assign any more as the test is integrated into test_progs, right?
> >
> > I'll fix it up.
> >
> >>> +static __u32 duration;
> >>> +
> >>> +static bool configure_stack(int self_net)
> >>
> >> self_net parameter is not used.
> >
> > Hrm, why didn't the compiler tell me this..? Will fix.
> >
> >>> +{
> >>> + /* Move to a new networking namespace */
> >>> + if (CHECK_FAIL(unshare(CLONE_NEWNET)))
> >>> + return false;
> >>
> >> You can use CHECK to encode better error messages. Thhis is what
> >> most test_progs tests are using.
> >
> > I was going back and forth on this when I was writing this bit.
> > CHECK_FAIL() already prints the line that fails, so when debugging
> > it's pretty clear what call went wrong if you dig into the code.
> > Combine with perror() and you actually get a readable string of the
> > error, whereas the common form for CHECK() seems to be just printing
> > the error code which the developer then has to do symbol lookup to
> > interpret..
> >
> > if (CHECK(efd < 0, "open", "err %d errno %d\n", efd, errno))
> >
> > Example output with CHECK_FAIL / perror approach:
> >
> > # ./test_progs -t assign
> > ...
> > Timed out while connecting to server
> > connect_to_server:FAIL:90
> > Cannot connect to server: Interrupted system call
> > #46/1 ipv4 port redir:FAIL
> > #46 sk_assign:FAIL
> > Summary: 0/0 PASSED, 0 SKIPPED, 2 FAILED
>
> I won't insist since CHECK_FAIL should roughly provide enough
> information for failure. CHECK might be more useful if you want
> to provide more context, esp. if the same routine is called
> in multiple places and you can have a marker to differentiate
> which call site caused the problem.
Good point, maybe for extra context the subtests can use CHECK() in
addition to the CHECK_FAIL.
> But again, just a suggestion. CHECK_FAIL is okay to me.
<snip>
> >>> + /* We can't do a single skc_lookup_tcp here, because then the compiler
> >>> + * will likely spill tuple_len to the stack. This makes it lose all
> >>> + * bounds information in the verifier, which then rejects the call as
> >>> + * unsafe.
> >>> + */
> >>
> >> This is a known issue. For scalars, only constant is restored properly
> >> in verifier at this moment. I did some hacking before to enable any
> >> scalars. The fear is this will make pruning performs worse. More
> >> study is needed here.
> >
> > Thanks for the background. Do you want me to refer to any specific
> > release version or date or commit for this comment or it's fine to
> > leave as-is?
>
> Maybe add a "workaround:" marker in the comments so later we can search
> and find these examples if we have compiler/verifier improvements.
>
> -bash-4.4$ egrep -ri workaround
> test_get_stack_rawtp.c: * This is an acceptable workaround since there
> is one entry here.
> test_seg6_loop.c: // workaround: define induction variable "i" as
> "long" instead
> test_sysctl_loop1.c: /* a workaround to prevent compiler from generating
> -bash-4.4$
SGTM, Will roll that in thanks.
next prev parent reply other threads:[~2020-03-25 23:08 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-25 5:57 [PATCHv2 bpf-next 0/5] Add bpf_sk_assign eBPF helper Joe Stringer
2020-03-25 5:57 ` [PATCHv2 bpf-next 1/5] bpf: Add socket assign support Joe Stringer
2020-03-26 6:23 ` Martin KaFai Lau
2020-03-26 6:31 ` Joe Stringer
2020-03-26 10:24 ` Lorenz Bauer
2020-03-26 22:52 ` Joe Stringer
2020-03-27 2:40 ` Joe Stringer
2020-03-25 5:57 ` [PATCHv2 bpf-next 2/5] bpf: Prefetch established socket destinations Joe Stringer
2020-03-26 21:11 ` Alexei Starovoitov
2020-03-26 21:45 ` Joe Stringer
2020-03-25 5:57 ` [PATCHv2 bpf-next 3/5] net: Track socket refcounts in skb_steal_sock() Joe Stringer
2020-03-25 5:57 ` [PATCHv2 bpf-next 4/5] bpf: Don't refcount LISTEN sockets in sk_assign() Joe Stringer
2020-03-25 10:29 ` Lorenz Bauer
2020-03-25 20:46 ` Joe Stringer
2020-03-26 10:20 ` Lorenz Bauer
2020-03-26 21:37 ` Joe Stringer
2020-03-25 5:57 ` [PATCHv2 bpf-next 5/5] selftests: bpf: add test for sk_assign Joe Stringer
2020-03-25 10:35 ` Lorenz Bauer
2020-03-25 20:55 ` Joe Stringer
2020-03-26 6:25 ` Martin KaFai Lau
2020-03-26 6:38 ` Joe Stringer
2020-03-26 23:39 ` Joe Stringer
2020-03-25 18:17 ` Yonghong Song
2020-03-25 21:20 ` Joe Stringer
2020-03-25 22:00 ` Yonghong Song
2020-03-25 23:07 ` Joe Stringer [this message]
2020-03-26 10:13 ` Lorenz Bauer
2020-03-26 21:07 ` call for bpf progs. " Alexei Starovoitov
2020-03-26 23:14 ` Yonghong Song
2020-03-27 10:02 ` Lorenz Bauer
2020-03-27 16:08 ` Alexei Starovoitov
2020-03-27 19:06 ` Joe Stringer
2020-03-27 20:16 ` Daniel Borkmann
2020-03-27 22:24 ` Alexei Starovoitov
2020-03-28 0:17 ` Andrii Nakryiko
2020-03-26 2:04 ` Andrii Nakryiko
2020-03-26 2:16 ` Andrii Nakryiko
2020-03-26 5:28 ` Joe Stringer
2020-03-26 6:31 ` Martin KaFai Lau
2020-03-26 19:36 ` Andrii Nakryiko
2020-03-26 21:38 ` Joe Stringer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOftzPjnXNM387OP4jN+EzKG05u_5Es2OH7PDc9cbTodBrMjgg@mail.gmail.com \
--to=joe@wand.net.nz \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=eric.dumazet@gmail.com \
--cc=kafai@fb.com \
--cc=lmb@cloudflare.com \
--cc=netdev@vger.kernel.org \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).