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;
Feedback-ID: i6a694271:Fastmail
Date:   Fri, 21 Jul 2023 14:03:29 -0600
From:   Daniel Xu <dxu@dxuuu.xyz>
To:     Florian Westphal <fw@strlen.de>
Cc:     daniel@iogearbox.net, kadlec@netfilter.org, ast@kernel.org,
        pablo@netfilter.org, kuba@kernel.org, davem@davemloft.net,
        andrii@kernel.org, edumazet@google.com, pabeni@redhat.com,
        alexei.starovoitov@gmail.com, 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 v5 2/5] netfilter: bpf: Support
 BPF_F_NETFILTER_IP_DEFRAG in netfilter link
Message-ID: <waoejzg2unjytkjflwvcff4z6wu2vlsji5neybrt4p5a3bn3ev@nznwevyfywvh>
References: <cover.1689884827.git.dxu@dxuuu.xyz>
 <690a1b09db84547b0f0c73654df3f4950f1262b7.1689884827.git.dxu@dxuuu.xyz>
 <20230720231904.GA31372@breakpoint.cc>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230720231904.GA31372@breakpoint.cc>
Precedence: bulk

On Fri, Jul 21, 2023 at 01:19:04AM +0200, Florian Westphal wrote:
> Daniel Xu <dxu@dxuuu.xyz> wrote:
> > +	const struct nf_defrag_hook __maybe_unused *hook;
> > +
> > +	switch (link->hook_ops.pf) {
> > +#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV4)
> > +	case NFPROTO_IPV4:
> > +		hook = get_proto_defrag_hook(link, nf_defrag_v4_hook, "nf_defrag_ipv4");
> > +		if (IS_ERR(hook))
> > +			return PTR_ERR(hook);
> > +
> > +		link->defrag_hook = hook;
> > +		return 0;
> > +#endif
> > +#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV6)
> > +	case NFPROTO_IPV6:
> > +		hook = get_proto_defrag_hook(link, nf_defrag_v6_hook, "nf_defrag_ipv6");
> > +		if (IS_ERR(hook))
> > +			return PTR_ERR(hook);
> > +
> > +		link->defrag_hook = hook;
> > +		return 0;
> > +#endif
> > +	default:
> > +		return -EAFNOSUPPORT;
> > +	}
> > +}
> > +
> > +static void bpf_nf_disable_defrag(struct bpf_nf_link *link)
> > +{
> > +	const struct nf_defrag_hook *hook = link->defrag_hook;
> > +
> > +	if (!hook)
> > +		return;
> > +	hook->disable(link->net);
> > +	module_put(hook->owner);
> > +}
> > +
> >  static void bpf_nf_link_release(struct bpf_link *link)
> >  {
> >  	struct bpf_nf_link *nf_link = container_of(link, struct bpf_nf_link, link);
> > @@ -37,6 +119,8 @@ static void bpf_nf_link_release(struct bpf_link *link)
> >  	 */
> >  	if (!cmpxchg(&nf_link->dead, 0, 1))
> >  		nf_unregister_net_hook(nf_link->net, &nf_link->hook_ops);
> > +
> > +	bpf_nf_disable_defrag(nf_link);
> >  }
> 
> I suspect this needs to be within the cmpxchg() branch to avoid
> multiple ->disable() calls.

Ah, good catch.

> 
> > +	if (attr->link_create.netfilter.flags & BPF_F_NETFILTER_IP_DEFRAG) {
> > +		err = bpf_nf_enable_defrag(link);
> > +		if (err) {
> > +			bpf_link_cleanup(&link_primer);
> > +			return err;
> > +		}
> > +	}
> > +
> >  	err = nf_register_net_hook(net, &link->hook_ops);
> >  	if (err) {
> 		bpf_nf_disable_defrag(link);

Ack. Did not see that bpf_link_cleanup() sets link->prog to NULL which
disables bpf_link_ops:release() on the release codepath.

> 
> Other than those nits this lgtm, thanks!
> 

Thanks for reviewing!

Daniel