Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 27 Jul 2023 18:16:20 -0700
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
To:     Daniel Xu <dxu@dxuuu.xyz>
Cc:     daniel@iogearbox.net, kadlec@netfilter.org, edumazet@google.com,
        ast@kernel.org, fw@strlen.de, kuba@kernel.org, pabeni@redhat.com,
        pablo@netfilter.org, andrii@kernel.org, davem@davemloft.net,
        martin.lau@linux.dev, song@kernel.org, yhs@fb.com,
        john.fastabend@gmail.com, kpsingh@kernel.org, sdf@google.com,
        haoluo@google.com, jolsa@kernel.org, bpf@vger.kernel.org,
        linux-kernel@vger.kernel.org, netfilter-devel@vger.kernel.org,
        coreteam@netfilter.org, netdev@vger.kernel.org, dsahern@kernel.org
Subject: Re: [PATCH bpf-next v6 2/5] netfilter: bpf: Support
 BPF_F_NETFILTER_IP_DEFRAG in netfilter link
Message-ID: <20230728011620.psvselzqdm7ku5e4@macbook-pro-8.dhcp.thefacebook.com>
References: <cover.1689970773.git.dxu@dxuuu.xyz>
 <5cff26f97e55161b7d56b09ddcf5f8888a5add1d.1689970773.git.dxu@dxuuu.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5cff26f97e55161b7d56b09ddcf5f8888a5add1d.1689970773.git.dxu@dxuuu.xyz>
Precedence: bulk

On Fri, Jul 21, 2023 at 02:22:46PM -0600, Daniel Xu wrote:
> This commit adds support for enabling IP defrag using pre-existing
> netfilter defrag support. Basically all the flag does is bump a refcnt
> while the link the active. Checks are also added to ensure the prog
> requesting defrag support is run _after_ netfilter defrag hooks.
> 
> We also take care to avoid any issues w.r.t. module unloading -- while
> defrag is active on a link, the module is prevented from unloading.
> 
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> ---
>  include/uapi/linux/bpf.h       |   5 ++
>  net/netfilter/nf_bpf_link.c    | 123 +++++++++++++++++++++++++++++----
>  tools/include/uapi/linux/bpf.h |   5 ++
>  3 files changed, 118 insertions(+), 15 deletions(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 739c15906a65..12a5480314a2 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1187,6 +1187,11 @@ enum bpf_perf_event_type {
>   */
>  #define BPF_F_KPROBE_MULTI_RETURN	(1U << 0)
>  
> +/* link_create.netfilter.flags used in LINK_CREATE command for
> + * BPF_PROG_TYPE_NETFILTER to enable IP packet defragmentation.
> + */
> +#define BPF_F_NETFILTER_IP_DEFRAG (1U << 0)
> +
>  /* When BPF ldimm64's insn[0].src_reg != 0 then this can have
>   * the following extensions:
>   *
> diff --git a/net/netfilter/nf_bpf_link.c b/net/netfilter/nf_bpf_link.c
> index c36da56d756f..8fe594bbc7e2 100644
> --- a/net/netfilter/nf_bpf_link.c
> +++ b/net/netfilter/nf_bpf_link.c
> @@ -1,6 +1,8 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <linux/bpf.h>
>  #include <linux/filter.h>
> +#include <linux/kmod.h>
> +#include <linux/module.h>
>  #include <linux/netfilter.h>
>  
>  #include <net/netfilter/nf_bpf_link.h>
> @@ -23,8 +25,88 @@ struct bpf_nf_link {
>  	struct nf_hook_ops hook_ops;
>  	struct net *net;
>  	u32 dead;
> +	const struct nf_defrag_hook *defrag_hook;
>  };
>  
> +static const struct nf_defrag_hook *
> +get_proto_defrag_hook(struct bpf_nf_link *link,
> +		      const struct nf_defrag_hook __rcu *global_hook,
> +		      const char *mod)
> +{
> +	const struct nf_defrag_hook *hook;
> +	int err;
> +
> +	/* RCU protects us from races against module unloading */
> +	rcu_read_lock();
> +	hook = rcu_dereference(global_hook);
> +	if (!hook) {
> +		rcu_read_unlock();
> +		err = request_module(mod);
> +		if (err)
> +			return ERR_PTR(err < 0 ? err : -EINVAL);
> +
> +		rcu_read_lock();
> +		hook = rcu_dereference(global_hook);
> +	}
> +
> +	if (hook && try_module_get(hook->owner)) {
> +		/* Once we have a refcnt on the module, we no longer need RCU */
> +		hook = rcu_pointer_handoff(hook);
> +	} else {
> +		WARN_ONCE(!hook, "%s has bad registration", mod);
> +		hook = ERR_PTR(-ENOENT);
> +	}
> +	rcu_read_unlock();
> +
> +	if (!IS_ERR(hook)) {
> +		err = hook->enable(link->net);
> +		if (err) {
> +			module_put(hook->owner);
> +			hook = ERR_PTR(err);
> +		}
> +	}
> +
> +	return hook;

The rcu + module_get logic looks correct to me, but you've dropped all Florian's acks.
What's going on?

We need explicit acks to merge this through bpf-next.