From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3BE05ECE58E for ; Thu, 10 Oct 2019 11:37:39 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 0CA232053B for ; Thu, 10 Oct 2019 11:37:39 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=cloudflare.com header.i=@cloudflare.com header.b="w+PO4u8W" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725901AbfJJLhi (ORCPT ); Thu, 10 Oct 2019 07:37:38 -0400 Received: from mail-lf1-f66.google.com ([209.85.167.66]:34039 "EHLO mail-lf1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726523AbfJJLhi (ORCPT ); Thu, 10 Oct 2019 07:37:38 -0400 Received: by mail-lf1-f66.google.com with SMTP id r22so4132719lfm.1 for ; Thu, 10 Oct 2019 04:37:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cloudflare.com; s=google; h=references:user-agent:from:to:cc:subject:in-reply-to:date :message-id:mime-version; bh=hiy5AVBqTzFPdMykXPXdntwcxs9Lxc3BOJl02HMVZEc=; b=w+PO4u8W5CvO0zUt4uJmoXKN24jUmFfBbtlAgNdTtbPj9hyYVrBUhE47HIdKcGGTxC K3uupyLZKrn94WtS1bqBP/hnecvKPNGl92GjhlVSfxTc+L+anjO9/khHaUZdVxBqL7C/ CG+E942yLOpUijkDea+X5T90VI0H8Iih9VvYU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:references:user-agent:from:to:cc:subject :in-reply-to:date:message-id:mime-version; bh=hiy5AVBqTzFPdMykXPXdntwcxs9Lxc3BOJl02HMVZEc=; b=MRWx1Qrf7ziglTHbs9cze5Sg2x5K0U2CYLpmwSZPntw1a97IDdbdHLmXqLyvYD0lIr O+8MU7K/0Klgl7LDNVB4nkE51u5FCxlLG0ZyJetqxlhPf+CUPGriW/o7JYf1C2C4wZzh ny+zjP98RgRLIm9RX3k3MflQ5Df54rPLOan19IGjvL7GIylMLGD5oqhPXl5QDO8NcK/4 Kxld+g5OHs/P6dH7fiSB4WxV7XQtWCPqMTY8tqqeJz5E9aFeltpHSvZVj2RzY04lfyIa FMwOOFTGPvt6kduVIsZ4jSkrFolqCvwMygPRr9TMJ0iGMf52wiiBxpyhuO8TsWhKFrMB uCMw== X-Gm-Message-State: APjAAAWndE9jL4iqOUSVZkRGOniPB18KDNMhjLJu7wqh2lTgswOzRyJk 2DBpPl7ukhZ3z9Ikrs/fGi9ovw== X-Google-Smtp-Source: APXvYqxPUNwbQL9yrPB2N10+CEb8trzM9kyMSKcXTsalGadl9YhtEvvuXMfvfAWgjHwuCkzpXMpEYw== X-Received: by 2002:a19:ad4a:: with SMTP id s10mr5667108lfd.159.1570707454386; Thu, 10 Oct 2019 04:37:34 -0700 (PDT) Received: from cloudflare.com ([176.221.114.230]) by smtp.gmail.com with ESMTPSA id j17sm1183166lfb.11.2019.10.10.04.37.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 10 Oct 2019 04:37:33 -0700 (PDT) References: <20191009094312.15284-1-jakub@cloudflare.com> <20191009094312.15284-2-jakub@cloudflare.com> <20191009163341.GE2096@mini-arch> User-agent: mu4e 1.1.0; emacs 26.1 From: Jakub Sitnicki To: Stanislav Fomichev Cc: bpf@vger.kernel.org, netdev@vger.kernel.org, kernel-team@cloudflare.com, Stanislav Fomichev Subject: Re: [PATH bpf-next 2/2] selftests/bpf: Check that flow dissector can be re-attached In-reply-to: <20191009163341.GE2096@mini-arch> Date: Thu, 10 Oct 2019 13:37:33 +0200 Message-ID: <87lfts25mq.fsf@cloudflare.com> MIME-Version: 1.0 Content-Type: text/plain Sender: bpf-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org 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 >> --- >> .../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 >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> + >> +#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 (":FAIL:") 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; } 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 <> $ 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 >>