Received: by 2002:a05:6a10:a0d1:0:0:0:0 with SMTP id j17csp1181144pxa; Thu, 20 Aug 2020 04:59:37 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwIl20hmZ0lHTChpq51RVo3G24mIMr5QhqyhAzeH3kaUfVXJSy3JjympJ6mD7ESJaze/nCM X-Received: by 2002:aa7:dcd2:: with SMTP id w18mr2587385edu.307.1597924777206; Thu, 20 Aug 2020 04:59:37 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1597924777; cv=none; d=google.com; s=arc-20160816; b=p/N52kBcdm7CdA0gI1r7UBrc5vbzV0scH5pmgghOZz9O+PZ68vF8sQeKFDFRQnHFJD 05XZY73PFNWAVQnpNkFseqvAfRdqcu/5OLRSPOMHEyQu64N/zz/C32/gdkaO+Pc48kxC iFNj4Ust81zTwd2xAx33/eWwzcz7jKhbPEp9k5qctwGD8RRNjp2WdNAon8cU00ScD7Ua WdgqoPaz5ZEjlTRtYzDtvfmaaDMF5phutgFucDJ/QHv59xJuootz/goxPa4hsfYlR4AW W7Cz1KgXepSgfpGdxkH6197K7GStI26BfNMJdBP7542z5T5a35OJ6ziRde2FJvuerPAr 2ujA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=WYTZ5oggGW4vnzG+aCQEqmegKWwKj3jml6L7NHXjNYI=; b=XXJCf7yXIDw2DectEjBulsoYAE9QlHc408WsD3SfVxtLW11RdFdDrhvvPOCKdBv4Kn KekD8+zesOM7nAbsgoGGJZtMhDUiK/IC0sp7jYl1ZPG1uZn/dA7vWLfA46l1z+YSI3Gx EXlSm6CTxKpT49rbqGdub0rQi62PWGzW+TxUV861OQcohi261pRceVmUUW5DaWzAjY46 SkcCrhCvnZlKlREQC57neEWA9OJDUw2ur/1I7la80qd8DVBkKNPbZYicmWXCS0zpuDJo l8FG+vYEjkoiKhArSPfo9P6933yDzyEL6prbKPalK/v7JALQXEaTWxcwoINx7t0LnIfs PH/g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@cloudflare.com header.s=google header.b=PbHlW3SV; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=cloudflare.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id u27si903078ejj.716.2020.08.20.04.59.13; Thu, 20 Aug 2020 04:59:37 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@cloudflare.com header.s=google header.b=PbHlW3SV; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=cloudflare.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730457AbgHTL6j (ORCPT + 99 others); Thu, 20 Aug 2020 07:58:39 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51630 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730320AbgHTL6U (ORCPT ); Thu, 20 Aug 2020 07:58:20 -0400 Received: from mail-oi1-x244.google.com (mail-oi1-x244.google.com [IPv6:2607:f8b0:4864:20::244]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id F35B1C061385 for ; Thu, 20 Aug 2020 04:58:19 -0700 (PDT) Received: by mail-oi1-x244.google.com with SMTP id b22so1595967oic.8 for ; Thu, 20 Aug 2020 04:58:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cloudflare.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=WYTZ5oggGW4vnzG+aCQEqmegKWwKj3jml6L7NHXjNYI=; b=PbHlW3SVBwPWT6+JRJN2hRCld4WOZY3LufJi+ZUjHgO7SHb8ZadSRR5VI/+sG/8kq3 Vs/Djz3hkKLDwPtYDjJ1Ldm2wll4UIRbjwalRzWoSB/3cGRWrxBmYSmhZGDLy1Lh2UgJ oSB7vNuDglyUp4UxSkWaTOPZAX8HVOnuueN1Y= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=WYTZ5oggGW4vnzG+aCQEqmegKWwKj3jml6L7NHXjNYI=; b=tuEvH5sMW3vgdCR89hnZ1zIFo9F37a54n1cbKgln34lQwuWrMOvZa9gvYXzQaNAXr1 JPoVMbWcNjcl95sNWJmInMM6p+pRa85JFif3IEGFF4o8cD0Y69IV37VJKrcjg70QTR4Z U7iyfQTIG9dHCam3ELVXOkjhzsscWuThpIPdK7pS7gatjDl3FPYwOMQFDaXbeiRKRMZr zJWnSlyC88jiAK3XS+ZgaFVWNcv+S6IdllGpmmVRCe4UlRaDam7kTxZsNy5DG9UlhU5p Bq7A+GSoOMp2Ha0SGxJFAqdaVlCYRv6P+pmEue6M/G14+8O0yY8FLw87Ve4iTRdNOF0/ fRdA== X-Gm-Message-State: AOAM532FXuCHdsFKoaRs4Q3ApaewEhiUQ2I8dLy8vVehMRgZqwfSJmxr tYkPiBmYeDNHgOAnlNfv6AQ9EnLDrCQcn2OZNlkJ5A== X-Received: by 2002:aca:a88e:: with SMTP id r136mr1516559oie.110.1597924699316; Thu, 20 Aug 2020 04:58:19 -0700 (PDT) MIME-Version: 1.0 References: <20200819092436.58232-1-lmb@cloudflare.com> <20200819092436.58232-7-lmb@cloudflare.com> <1ad29823-1925-01ee-f042-20b422a62a73@fb.com> In-Reply-To: <1ad29823-1925-01ee-f042-20b422a62a73@fb.com> From: Lorenz Bauer Date: Thu, 20 Aug 2020 12:58:07 +0100 Message-ID: Subject: Re: [PATCH bpf-next 6/6] selftests: bpf: test sockmap update from BPF To: Yonghong Song Cc: Jakub Sitnicki , John Fastabend , Shuah Khan , Alexei Starovoitov , Daniel Borkmann , kernel-team , LKML , linux-kselftest@vger.kernel.org, Networking , bpf Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 19 Aug 2020 at 21:46, Yonghong Song wrote: > > > > On 8/19/20 2:24 AM, Lorenz Bauer wrote: > > Add a test which copies a socket from a sockmap into another sockmap > > or sockhash. This excercises bpf_map_update_elem support from BPF > > context. Compare the socket cookies from source and destination to > > ensure that the copy succeeded. > > > > Signed-off-by: Lorenz Bauer > > --- > > .../selftests/bpf/prog_tests/sockmap_basic.c | 76 +++++++++++++++++++ > > .../selftests/bpf/progs/test_sockmap_copy.c | 48 ++++++++++++ > > 2 files changed, 124 insertions(+) > > create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_copy.c > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c > > index 96e7b7f84c65..d30cabc00e9e 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c > > +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c > > @@ -4,6 +4,7 @@ > > > > #include "test_progs.h" > > #include "test_skmsg_load_helpers.skel.h" > > +#include "test_sockmap_copy.skel.h" > > > > #define TCP_REPAIR 19 /* TCP sock is under repair right now */ > > > > @@ -101,6 +102,77 @@ static void test_skmsg_helpers(enum bpf_map_type map_type) > > test_skmsg_load_helpers__destroy(skel); > > } > > > > +static void test_sockmap_copy(enum bpf_map_type map_type) > > +{ > > + struct bpf_prog_test_run_attr attr; > > + struct test_sockmap_copy *skel; > > + __u64 src_cookie, dst_cookie; > > + int err, prog, s, src, dst; > > + const __u32 zero = 0; > > + char dummy[14] = {0}; > > + > > + s = connected_socket_v4(); > > Maybe change variable name to "sk" for better clarity? Yup! > > > + if (CHECK_FAIL(s == -1)) > > + return; > > + > > + skel = test_sockmap_copy__open_and_load(); > > + if (CHECK_FAIL(!skel)) { > > + close(s); > > + perror("test_sockmap_copy__open_and_load"); > > + return; > > + } > > Could you use CHECK instead of CHECK_FAIL? > With CHECK, you can print additional information without perror. I avoid CHECK because it requires `duration`, which doesn't make sense for most things that I call CHECK_FAIL on here. So either it outputs 0 nsec (which is bogus) or it outputs the value from the last bpf_prog_test_run call (which is also bogus). How do other tests handle this? Just ignore it? > > > > + > > + prog = bpf_program__fd(skel->progs.copy_sock_map); > > + src = bpf_map__fd(skel->maps.src); > > + if (map_type == BPF_MAP_TYPE_SOCKMAP) > > + dst = bpf_map__fd(skel->maps.dst_sock_map); > > + else > > + dst = bpf_map__fd(skel->maps.dst_sock_hash); > > + > > + err = bpf_map_update_elem(src, &zero, &s, BPF_NOEXIST); > > The map defined in bpf program is __u64 and here "s" is int. > Any potential issues? Hm, good point. This is a quirk of the sockmap API, I need to dig into this a bit. > > > + if (CHECK_FAIL(err)) { > > + perror("bpf_map_update"); > > + goto out; > > + } > > + > > + err = bpf_map_lookup_elem(src, &zero, &src_cookie); > > + if (CHECK_FAIL(err)) { > > + perror("bpf_map_lookup_elem(src)"); > > + goto out; > > + } > > + > > + attr = (struct bpf_prog_test_run_attr){ > > + .prog_fd = prog, > > + .repeat = 1, > > + .data_in = dummy, > > + .data_size_in = sizeof(dummy), > > + }; > > + > > + err = bpf_prog_test_run_xattr(&attr); > > + if (err) { > > You can use CHECK macro here. > > > + test__fail(); > > + perror("bpf_prog_test_run"); > > + goto out; > > + } else if (!attr.retval) { > > + PRINT_FAIL("bpf_prog_test_run: program returned %u\n", > > + attr.retval); > > + goto out; > > + } > > + > > + err = bpf_map_lookup_elem(dst, &zero, &dst_cookie); > > + if (CHECK_FAIL(err)) { > > + perror("bpf_map_lookup_elem(dst)"); > > + goto out; > > + } > > + > > + if (dst_cookie != src_cookie) > > + PRINT_FAIL("cookie %llu != %llu\n", dst_cookie, src_cookie); > > Just replace the whole if statement with a CHECK macro. See above, re duration. > > > + > > +out: > > + close(s); > > + test_sockmap_copy__destroy(skel); > > +} > > + > > void test_sockmap_basic(void) > > { > > if (test__start_subtest("sockmap create_update_free")) > > @@ -111,4 +183,8 @@ void test_sockmap_basic(void) > > test_skmsg_helpers(BPF_MAP_TYPE_SOCKMAP); > > if (test__start_subtest("sockhash sk_msg load helpers")) > > test_skmsg_helpers(BPF_MAP_TYPE_SOCKHASH); > > + if (test__start_subtest("sockmap copy")) > > + test_sockmap_copy(BPF_MAP_TYPE_SOCKMAP); > > + if (test__start_subtest("sockhash copy")) > > + test_sockmap_copy(BPF_MAP_TYPE_SOCKHASH); > > } > > diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_copy.c b/tools/testing/selftests/bpf/progs/test_sockmap_copy.c > > new file mode 100644 > > index 000000000000..9d0c9f28cab2 > > --- /dev/null > > +++ b/tools/testing/selftests/bpf/progs/test_sockmap_copy.c > > @@ -0,0 +1,48 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +// Copyright (c) 2020 Cloudflare > > +#include "vmlinux.h" > > +#include > > + > > +struct { > > + __uint(type, BPF_MAP_TYPE_SOCKMAP); > > + __uint(max_entries, 1); > > + __type(key, __u32); > > + __type(value, __u64); > > +} src SEC(".maps"); > > + > > +struct { > > + __uint(type, BPF_MAP_TYPE_SOCKMAP); > > + __uint(max_entries, 1); > > + __type(key, __u32); > > + __type(value, __u64); > > +} dst_sock_map SEC(".maps"); > > + > > +struct { > > + __uint(type, BPF_MAP_TYPE_SOCKHASH); > > + __uint(max_entries, 1); > > + __type(key, __u32); > > + __type(value, __u64); > > +} dst_sock_hash SEC(".maps"); > > + > > +SEC("classifier/copy_sock_map") > > +int copy_sock_map(void *ctx) > > +{ > > + struct bpf_sock *sk; > > + bool failed = false; > > + __u32 key = 0; > > + > > + sk = bpf_map_lookup_elem(&src, &key); > > + if (!sk) > > + return SK_DROP; > > + > > + if (bpf_map_update_elem(&dst_sock_map, &key, sk, 0)) > > + failed = true; > > + > > + if (bpf_map_update_elem(&dst_sock_hash, &key, sk, 0)) > > + failed = true; > > + > > + bpf_sk_release(sk); > > + return failed ? SK_DROP : SK_PASS; > > +} > > + > > +char _license[] SEC("license") = "GPL"; > > -- Lorenz Bauer | Systems Engineer 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK www.cloudflare.com