netdev - Re: [PATCH bpf-next v6 2/5] netfilter: bpf: Support BPF_F_NETFILTER_IP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 27 Jul 2023 22:37:47 -0600
From: Daniel Xu <dxu@...uu.xyz>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: daniel@...earbox.net, kadlec@...filter.org, edumazet@...gle.com, 
	ast@...nel.org, fw@...len.de, kuba@...nel.org, pabeni@...hat.com, 
	pablo@...filter.org, andrii@...nel.org, davem@...emloft.net, martin.lau@...ux.dev, 
	song@...nel.org, yhs@...com, john.fastabend@...il.com, kpsingh@...nel.org, 
	sdf@...gle.com, haoluo@...gle.com, jolsa@...nel.org, bpf@...r.kernel.org, 
	linux-kernel@...r.kernel.org, netfilter-devel@...r.kernel.org, coreteam@...filter.org, 
	netdev@...r.kernel.org, dsahern@...nel.org
Subject: Re: [PATCH bpf-next v6 2/5] netfilter: bpf: Support
 BPF_F_NETFILTER_IP_DEFRAG in netfilter link

Hi Alexei,

On Thu, Jul 27, 2023 at 06:16:20PM -0700, Alexei Starovoitov wrote:
> 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@...uu.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.

I understood acked-by tags to be a lighter form of reviewed-by tag. So
b/c the patches changed so much I dropped the tag. It sounds like maybe
I misunderstand -- I'll keep it in mind the next time around.

Thanks,
Daniel