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

[bot+bpf-ci]

> diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
> index f98d1d4b42c3..c3395cb98c00 100644
> --- a/net/netfilter/nf_conntrack_bpf.c
> +++ b/net/netfilter/nf_conntrack_bpf.c
> @@ -122,42 +122,56 @@ __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;
> +	u16 ct_zone_id;
> +	s32 netns_id;
> +	u8 ct_zone_dir = 0;
> +	u8 reserved[3] = {};
> +	u8 l4proto;

This isn't a bug, but would it be worth reordering these declarations
longest-to-shortest to match the netdev reverse-xmas-tree convention
before this goes out again? The block goes from 'struct nf_conn *ct;'
(21 chars) down to 's32 netns_id;' (15), then back up to 'u8 reserved[3]
= {};' (22). The same ordering appears in __bpf_nf_ct_lookup() further
down.

>  	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);
> +		reserved[0] = READ_ONCE(opts->reserved[0]);
> +		reserved[1] = READ_ONCE(opts->reserved[1]);
> +		reserved[2] = READ_ONCE(opts->reserved[2]);
> +		if (reserved[0] || reserved[1] || reserved[2])

This isn't a bug, but since reserved[] is only used by the check on the
next line, could the three READ_ONCE() calls be inlined into the condition
instead of going through a local array? The same block appears in
__bpf_nf_ct_lookup() at the corresponding location.

>  			return ERR_PTR(-EINVAL);
>  	} else {
> -		if (opts->ct_zone_id)
> +		if (ct_zone_id)
>  			return ERR_PTR(-EINVAL);
>  	}

[ ... ]

> @@ -186,46 +200,60 @@ 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;
> +	u16 ct_zone_id;
> +	s32 netns_id;
> +	u8 ct_zone_dir = 0;
> +	u8 reserved[3] = {};
> +	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);
> +		reserved[0] = READ_ONCE(opts->reserved[0]);
> +		reserved[1] = READ_ONCE(opts->reserved[1]);
> +		reserved[2] = READ_ONCE(opts->reserved[2]);
> +		if (reserved[0] || reserved[1] || reserved[2])
>  			return ERR_PTR(-EINVAL);

[ ... ]


---
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/30527661826