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=-4.1 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no 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 208BCC433E0 for ; Fri, 26 Jun 2020 09:45:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id E2CD621556 for ; Fri, 26 Jun 2020 09:45:52 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=cloudflare.com header.i=@cloudflare.com header.b="mJW7dH1l" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726838AbgFZJpw (ORCPT ); Fri, 26 Jun 2020 05:45:52 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56090 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725275AbgFZJpw (ORCPT ); Fri, 26 Jun 2020 05:45:52 -0400 Received: from mail-lj1-x241.google.com (mail-lj1-x241.google.com [IPv6:2a00:1450:4864:20::241]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BBF60C08C5C1 for ; Fri, 26 Jun 2020 02:45:51 -0700 (PDT) Received: by mail-lj1-x241.google.com with SMTP id i3so9666126ljg.3 for ; Fri, 26 Jun 2020 02:45:51 -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=5XWtFWgwZnD66tBDM/3i+lmk/MTgyC/Rg07cXurUlV8=; b=mJW7dH1liKL64hBpVLiWMMacAp5RmxEToSnRzbALmrc8CTk7EhcIDsLbXtzD7HKjoG tFgSic4SueaSPjTyvSSc921loJilw6ile5F8SqNObI6Wqtk8pfyYZOKfQXD9ZmuZI6P6 yowUcEJcL85BNQ60LXU8ymxE9NMnD6ySR9qGA= 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=5XWtFWgwZnD66tBDM/3i+lmk/MTgyC/Rg07cXurUlV8=; b=At1D62iy5ReXo5a5Tm6C6xGf3St8OPLyiZi7Z5m7cIEjZZ3E1Vw/Z3IGC88ieedIVL G0yUVenxmZaDJld6kfBbxmkdo9HshNlnl9sSPqJV4ngm0xl/u8k37iepl/g5mZwubk6S 5vZjEg5wIxukeny/nC51XtawJjRUYH6acVW4Yi5lL4JJ0Qg6T7+7n/CpU+YYsmNnqNQk fOSJaRKBknXKyclevhjTralkvefkZ+suyWMN9H8rq93yShs8qCgULeN2w5rL/+R1t+ID QzLheln+Bjr5MWNbSaaxW9UWAH3/w+euBKezdHPzqDNJ3xTu0XEa0iE7rIDeaib9WuT2 ZAiQ== X-Gm-Message-State: AOAM533sWSbc09LqcALiaqBEDS/iE5CePbWoglZw2UXBFjxVlt35dBbW lqb4Xqt8QE8cnCBX6ZaHHLrKRk8rxYdYsA== X-Google-Smtp-Source: ABdhPJyQhsH57f44jLnzS4ztkkIc8i1c/lOljaceK1e1Z14DG2IMs8pEpfRegX8FeRB7md053E3jLg== X-Received: by 2002:a2e:991:: with SMTP id 139mr1004349ljj.314.1593164750141; Fri, 26 Jun 2020 02:45:50 -0700 (PDT) Received: from cloudflare.com ([2a02:a310:c262:aa00:b35e:8938:2c2a:ba8b]) by smtp.gmail.com with ESMTPSA id n8sm6292176lji.126.2020.06.26.02.45.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 26 Jun 2020 02:45:49 -0700 (PDT) References: <20200625141357.910330-1-jakub@cloudflare.com> <20200625141357.910330-3-jakub@cloudflare.com> User-agent: mu4e 1.1.0; emacs 26.3 From: Jakub Sitnicki To: Andrii Nakryiko Cc: bpf , Networking , kernel-team Subject: Re: [PATCH bpf-next v3 2/4] bpf, netns: Keep attached programs in bpf_prog_array In-reply-to: Date: Fri, 26 Jun 2020 11:45:48 +0200 Message-ID: <87imfema7n.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 Thu, Jun 25, 2020 at 10:50 PM CEST, Andrii Nakryiko wrote: > On Thu, Jun 25, 2020 at 7:17 AM Jakub Sitnicki wrote: >> >> Prepare for having multi-prog attachments for new netns attach types by >> storing programs to run in a bpf_prog_array, which is well suited for >> iterating over programs and running them in sequence. >> >> After this change bpf(PROG_QUERY) may block to allocate memory in >> bpf_prog_array_copy_to_user() for collected program IDs. This forces a >> change in how we protect access to the attached program in the query >> callback. Because bpf_prog_array_copy_to_user() can sleep, we switch from >> an RCU read lock to holding a mutex that serializes updaters. >> >> Because we allow only one BPF flow_dissector program to be attached to >> netns at all times, the bpf_prog_array pointed by net->bpf.run_array is >> always either detached (null) or one element long. >> >> No functional changes intended. >> >> Signed-off-by: Jakub Sitnicki >> --- > > I wonder if instead of NULL prog_array, it's better to just use a > dummy empty (but allocated) array. Might help eliminate some of the > IFs, maybe even in the hot path. That was my initial approach, which I abandoned seeing that it leads to replacing NULL prog_array checks in flow_dissector with bpf_prog_array_is_empty() checks to determine which netns has a BPF program attached. So no IFs gone there. While on the hot path, where we run the program, we probably would still be left with an IF checking for empty prog_array to avoid building the context if no progs will RUN. The checks I'm referring to are on attach path, in flow_dissector_bpf_prog_attach_check(), and hot-path, __skb_flow_dissect(). > > >> include/net/netns/bpf.h | 5 +- >> kernel/bpf/net_namespace.c | 120 +++++++++++++++++++++++++------------ >> net/core/flow_dissector.c | 19 +++--- >> 3 files changed, 96 insertions(+), 48 deletions(-) >> > > [...] > > >> >> +/* Must be called with netns_bpf_mutex held. */ >> +static int __netns_bpf_prog_query(const union bpf_attr *attr, >> + union bpf_attr __user *uattr, >> + struct net *net, >> + enum netns_bpf_attach_type type) >> +{ >> + __u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids); >> + struct bpf_prog_array *run_array; >> + u32 prog_cnt = 0, flags = 0; >> + >> + run_array = rcu_dereference_protected(net->bpf.run_array[type], >> + lockdep_is_held(&netns_bpf_mutex)); >> + if (run_array) >> + prog_cnt = bpf_prog_array_length(run_array); >> + >> + if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags))) >> + return -EFAULT; >> + if (copy_to_user(&uattr->query.prog_cnt, &prog_cnt, sizeof(prog_cnt))) >> + return -EFAULT; >> + if (!attr->query.prog_cnt || !prog_ids || !prog_cnt) >> + return 0; >> + >> + return bpf_prog_array_copy_to_user(run_array, prog_ids, >> + attr->query.prog_cnt); > > It doesn't seem like bpf_prog_array_copy_to_user can handle NULL run_array Correct. And we never invoke it when run_array is NULL because then prog_cnt == 0. > >> +} >> + >> int netns_bpf_prog_query(const union bpf_attr *attr, >> union bpf_attr __user *uattr) >> { >> - __u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids); >> - u32 prog_id, prog_cnt = 0, flags = 0; >> enum netns_bpf_attach_type type; >> - struct bpf_prog *attached; >> struct net *net; >> + int ret; >> >> if (attr->query.query_flags) >> return -EINVAL; > > [...]