On Wed, Mar 25, 2020 at 3:01 PM Yonghong Song <yhs@xxxxxx> wrote:
>
>
>
> On 3/25/20 2:20 PM, Joe Stringer wrote:
> > On Wed, Mar 25, 2020 at 11:18 AM Yonghong Song <yhs@xxxxxx> wrote:
> >>
> >>
> >>
> >> On 3/24/20 10:57 PM, Joe Stringer wrote:
> >>> From: Lorenz Bauer <lmb@xxxxxxxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxxxxxxx>
> >>> Co-authored-by: Joe Stringer <joe@xxxxxxxxxxx>
> >>> Signed-off-by: Joe Stringer <joe@xxxxxxxxxxx>
> >>> ---
> >>> 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.
