linux-kernel - Re: [PATCH bpf-next v5 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] [day] [month] [year] [list]

Date:   Fri, 21 Jul 2023 14:03:29 -0600
From:   Daniel Xu <dxu@...uu.xyz>
To:     Florian Westphal <fw@...len.de>
Cc:     daniel@...earbox.net, kadlec@...filter.org, ast@...nel.org,
        pablo@...filter.org, kuba@...nel.org, davem@...emloft.net,
        andrii@...nel.org, edumazet@...gle.com, pabeni@...hat.com,
        alexei.starovoitov@...il.com, 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 v5 2/5] netfilter: bpf: Support
 BPF_F_NETFILTER_IP_DEFRAG in netfilter link

On Fri, Jul 21, 2023 at 01:19:04AM +0200, Florian Westphal wrote:
> Daniel Xu <dxu@...uu.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