On 10/10, Jakub Sitnicki wrote:
> On Wed, Oct 09, 2019 at 06:33 PM CEST, Stanislav Fomichev wrote:
> > On 10/09, Jakub Sitnicki wrote:
> >> Make sure a new flow dissector program can be attached to replace the old
> >> one with a single syscall. Also check that attaching the same program twice
> >> is prohibited.
> > Overall the series looks good, left a bunch of nits/questions below.
>
> Thanks for the comments.
>
> >
> >> Signed-off-by: Jakub Sitnicki <ja...@cloudflare.com>
> >> ---
> >> .../bpf/prog_tests/flow_dissector_reattach.c | 93 +++++++++++++++++++
> >> 1 file changed, 93 insertions(+)
> >> create mode 100644
> >> tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
> >>
> >> diff --git
> >> a/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
> >> b/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
> >> new file mode 100644
> >> index 000000000000..0f0006c93956
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
> >> @@ -0,0 +1,93 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Test that the flow_dissector program can be updated with a single
> >> + * syscall by attaching a new program that replaces the existing one.
> >> + *
> >> + * Corner case - the same program cannot be attached twice.
> >> + */
> >> +#include <errno.h>
> >> +#include <fcntl.h>
> >> +#include <stdbool.h>
> >> +#include <unistd.h>
> >> +
> >> +#include <linux/bpf.h>
> >> +#include <bpf/bpf.h>
> >> +
> >> +#include "test_progs.h"
> >> +
> > [..]
> >> +/* Not used here. For CHECK macro sake only. */
> >> +static int duration;
> > nit: you can use CHECK_FAIL macro instead which doesn't require this.
> >
> > if (CHECK_FAIL(expr)) {
> > printf("something bad has happened\n");
> > return/goto;
> > }
> >
> > It may be more verbose than doing CHECK() with its embedded error
> > message, so I leave it up to you to decide on whether you want to switch
> > to CHECK_FAIL or stick to CHECK.
> >
>
> I wouldn't mind switching to CHECK_FAIL. It reads better than CHECK with
> error message stuck in the if expression. (There is a side-issue with
> printf(). Will explain at the end [*].)
>
> Another thing to consider is that with CHECK the message indicating a
> failure ("<test>:FAIL:<lineno>") and the actual explanation message are
> on the same line. This makes the error log easier to reason.
>
> I'm torn here, and considering another alternative to address at least
> the readability issue:
>
> if (fail_expr) {
> CHECK(1, "action", "explanation");
> return;
> }
Can we use perror for the error reporting?
if (CHECK(fail_expr)) {
perror("failed to do something"); // will print errno as well
}
This should give all the info needed to grep for this message and debug
the problem.
Alternatively, we can copy/move log_err() from the cgroup_helpers.h,
and use it in test_progs; it prints file:line:errno <msg>.
> It doesn't address the extra variable problem. Maybe we need another
> CHECK variant.
>
> >> +static bool is_attached(void)
> >> +{
> >> + bool attached = true;
> >> + int err, net_fd = -1;
> > nit: maybe don't need to initialize net_fd to -1 here as well.
>
> Will fix.
>
> >
> >> + __u32 cnt;
> >> +
> >> + net_fd = open("/proc/self/ns/net", O_RDONLY);
> >> + if (net_fd < 0)
> >> + goto out;
> >> +
> >> + err = bpf_prog_query(net_fd, BPF_FLOW_DISSECTOR, 0, NULL, NULL, &cnt);
> >> + if (CHECK(err, "bpf_prog_query", "ret %d errno %d\n", err, errno))
> >> + goto out;
> >> +
> >> + attached = (cnt > 0);
> >> +out:
> >> + close(net_fd);
> >> + return attached;
> >> +}
> >> +
> >> +static int load_prog(void)
> >> +{
> >> + struct bpf_insn prog[] = {
> >> + BPF_MOV64_IMM(BPF_REG_0, BPF_OK),
> >> + BPF_EXIT_INSN(),
> >> + };
> >> + int fd;
> >> +
> >> + fd = bpf_load_program(BPF_PROG_TYPE_FLOW_DISSECTOR, prog,
> >> + ARRAY_SIZE(prog), "GPL", 0, NULL, 0);
> >> + CHECK(fd < 0, "bpf_load_program", "ret %d errno %d\n", fd, errno);
> >> +
> >> + return fd;
> >> +}
> >> +
> >> +void test_flow_dissector_reattach(void)
> >> +{
> >> + int prog_fd[2] = { -1, -1 };
> >> + int err;
> >> +
> >> + if (is_attached())
> >> + return;
> > Should we call test__skip() here to indicate that the test has been
> > skipped?
> > Also, do we need to run this test against non-root namespace as well?
>
> Makes sense. Skip the test if anything is attached to root
> namespace. Otherwise test twice, once in non-root and once in root
> namespace.
>
> [*] The printf() issue.
>
> I've noticed that stdio hijacking that test_progs runner applies doesn't
> quite work. printf() seems to skip the FILE stream buffer and write
> whole lines directly to stdout. This results in reordered messages on
> output.
>
> Here's a distilled reproducer for what test_progs does:
>
> int main(void)
> {
> FILE *stream;
> char *buf;
> size_t cnt;
>
> stream = stdout;
> stdout = open_memstream(&buf, &cnt);
> if (!stdout)
> error(1, errno, "open_memstream");
>
> printf("foo");
> printf("bar\n");
> printf("baz");
> printf("qux\n");
>
> fflush(stdout);
> fclose(stdout);
>
> buf[cnt] = '\0';
> fprintf(stream, "<<%s>>", buf);
> if (buf[cnt-1] != '\n')
> fprintf(stream, "\n");
>
> free(buf);
> return 0;
> }
>
> On output we get:
>
> $ ./hijack_stdout
> bar
> qux
> <<foobaz>>
> $
What glibc do you have? I don't see any issues with your reproducer
on my setup:
$ ./a.out
<<foobar
bazqux
>>$
$ ldd --version
ldd (Debian GLIBC 2.28-10) 2.28
>
> Not sure what's a good fix.
>
> Ideally - dup2(STDOUT_FILENO, ...). But memstream doesn't have an FD.
> We can switch to fprintf(stdout, ...) which works for some reason.
>
> -Jakub
>
> >
> >> + prog_fd[0] = load_prog();
> >> + if (prog_fd[0] < 0)
> >> + return;
> >> +
> >> + prog_fd[1] = load_prog();
> >> + if (prog_fd[1] < 0)
> >> + goto out_close;
> >> +
> >> + err = bpf_prog_attach(prog_fd[0], 0, BPF_FLOW_DISSECTOR, 0);
> >> + if (CHECK(err, "bpf_prog_attach-0", "ret %d errno %d\n", err, errno))
> >> + goto out_close;
> >> +
> >> + /* Expect success when attaching a different program */
> >> + err = bpf_prog_attach(prog_fd[1], 0, BPF_FLOW_DISSECTOR, 0);
> >> + if (CHECK(err, "bpf_prog_attach-1", "ret %d errno %d\n", err, errno))
> >> + goto out_detach;
> >> +
> >> + /* Expect failure when attaching the same program twice */
> >> + err = bpf_prog_attach(prog_fd[1], 0, BPF_FLOW_DISSECTOR, 0);
> >> + CHECK(!err || errno != EINVAL, "bpf_prog_attach-2",
> >> + "ret %d errno %d\n", err, errno);
> >> +
> >> +out_detach:
> >> + err = bpf_prog_detach(0, BPF_FLOW_DISSECTOR);
> >> + CHECK(err, "bpf_prog_detach", "ret %d errno %d\n", err, errno);
> >> +
> >> +out_close:
> >> + close(prog_fd[1]);
> >> + close(prog_fd[0]);
> >> +}
> >> --
> >> 2.20.1
> >>
