Re: [PATCH bpf v3] bpf: Fix netns reference imbalance in conntrack kfuncs

--===============7093018806644806532==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit

> diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
> --- a/net/netfilter/nf_conntrack_bpf.c
> +++ b/net/netfilter/nf_conntrack_bpf.c
> @@ -122,42 +122,54 @@ __bpf_nf_ct_alloc_entry(struct net *net, struct bpf_sock_tuple *bpf_tuple,
>  	struct nf_conntrack_tuple otuple, rtuple;
>  	struct nf_conntrack_zone ct_zone;
>  	struct nf_conn *ct;
> +	u8 ct_zone_dir = 0;
> +	u16 ct_zone_id;
> +	s32 netns_id;
> +	u8 l4proto;
>  	int err;
>
>  	if (!(opts_len == NF_BPF_CT_OPTS_SZ || opts_len == 12))
>  		return ERR_PTR(-EINVAL);
> +
> +	netns_id = READ_ONCE(opts->netns_id);
> +	l4proto = READ_ONCE(opts->l4proto);
> +	ct_zone_id = READ_ONCE(opts->ct_zone_id);
>  	if (opts_len == NF_BPF_CT_OPTS_SZ) {
> -		if (opts->reserved[0] || opts->reserved[1] || opts->reserved[2])
> +		ct_zone_dir = READ_ONCE(opts->ct_zone_dir);
> +		if (READ_ONCE(opts->reserved[0]) ||
> +		    READ_ONCE(opts->reserved[1]) ||
> +		    READ_ONCE(opts->reserved[2]))
>  			return ERR_PTR(-EINVAL);
>  	} else {
> -		if (opts->ct_zone_id)
> +		if (ct_zone_id)
>  			return ERR_PTR(-EINVAL);
>  	}
>
> -	if (unlikely(opts->netns_id < BPF_F_CURRENT_NETNS))
> +	if (unlikely(netns_id < BPF_F_CURRENT_NETNS))
>  		return ERR_PTR(-EINVAL);

[ ... ]

>  	if (opts_len == NF_BPF_CT_OPTS_SZ) {
> -		if (opts->ct_zone_dir == 0)
> -			opts->ct_zone_dir = NF_CT_DEFAULT_ZONE_DIR;
> -		nf_ct_zone_init(&ct_zone,
> -				opts->ct_zone_id, opts->ct_zone_dir, 0);
> +		if (ct_zone_dir == 0) {
> +			ct_zone_dir = NF_CT_DEFAULT_ZONE_DIR;
> +			opts->ct_zone_dir = ct_zone_dir;
> +		}
> +		nf_ct_zone_init(&ct_zone, ct_zone_id, ct_zone_dir, 0);
>  	} else {
>  		ct_zone = nf_ct_zone_dflt;
>  	}

[ ... ]

> @@ -186,46 +198,58 @@ static struct nf_conn *__bpf_nf_ct_lookup(struct net *net,
>  	struct nf_conntrack_tuple tuple;
>  	struct nf_conntrack_zone ct_zone;
>  	struct nf_conn *ct;
> +	u8 ct_zone_dir = 0;
> +	u16 ct_zone_id;
> +	s32 netns_id;
> +	u8 l4proto;
>  	int err;
>
>  	if (!opts || !bpf_tuple)
>  		return ERR_PTR(-EINVAL);
>  	if (!(opts_len == NF_BPF_CT_OPTS_SZ || opts_len == 12))
>  		return ERR_PTR(-EINVAL);
> +
> +	netns_id = READ_ONCE(opts->netns_id);
> +	l4proto = READ_ONCE(opts->l4proto);
> +	ct_zone_id = READ_ONCE(opts->ct_zone_id);
>  	if (opts_len == NF_BPF_CT_OPTS_SZ) {
> -		if (opts->reserved[0] || opts->reserved[1] || opts->reserved[2])
> +		ct_zone_dir = READ_ONCE(opts->ct_zone_dir);
> +		if (READ_ONCE(opts->reserved[0]) ||
> +		    READ_ONCE(opts->reserved[1]) ||
> +		    READ_ONCE(opts->reserved[2]))
>  			return ERR_PTR(-EINVAL);
>  	} else {
> -		if (opts->ct_zone_id)
> +		if (ct_zone_id)
>  			return ERR_PTR(-EINVAL);
>  	}
> -	if (unlikely(opts->l4proto != IPPROTO_TCP && opts->l4proto != IPPROTO_UDP))
> +	if (unlikely(l4proto != IPPROTO_TCP && l4proto != IPPROTO_UDP))
>  		return ERR_PTR(-EPROTO);
> -	if (unlikely(opts->netns_id < BPF_F_CURRENT_NETNS))
> +	if (unlikely(netns_id < BPF_F_CURRENT_NETNS))
>  		return ERR_PTR(-EINVAL);

[ ... ]

>  	if (opts_len == NF_BPF_CT_OPTS_SZ) {
> -		if (opts->ct_zone_dir == 0)
> -			opts->ct_zone_dir = NF_CT_DEFAULT_ZONE_DIR;
> -		nf_ct_zone_init(&ct_zone,
> -				opts->ct_zone_id, opts->ct_zone_dir, 0);
> +		if (ct_zone_dir == 0) {
> +			ct_zone_dir = NF_CT_DEFAULT_ZONE_DIR;
> +			opts->ct_zone_dir = ct_zone_dir;
> +		}
> +		nf_ct_zone_init(&ct_zone, ct_zone_id, ct_zone_dir, 0);
>  	} else {
>  		ct_zone = nf_ct_zone_dflt;
>  	}

This isn't a bug, but both __bpf_nf_ct_alloc_entry() and
__bpf_nf_ct_lookup() now snapshot and validate the same five opts fields
in the same order with the same declarations (u8 ct_zone_dir = 0; u16
ct_zone_id; s32 netns_id; u8 l4proto;), and both carry the identical
zone-init block. The two functions now share about 25 lines of duplicated
code, and the patch grows each copy from roughly 8 lines to roughly 14.

Would it be worth factoring the snapshot and validation into a shared
helper so a future opts field only has to be handled once?  One of the
Fixes targets, aed8ee7feb44, was itself a de-duplication of similar code
in this file, so the direction of travel here has been toward sharing
rather than copying.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/30647492237
--===============7093018806644806532==--