Hello,

On Thu, Jan 29, 2026 at 06:40:01AM +1000, David Gwynne wrote:
</snip>
> 
> this chunk makes more sense if you can see more context.
> 
>   1296	int
>   1297	pf_state_insert(struct pfi_kif *kif, struct pf_state_key **skwp,
>   1298	    struct pf_state_key **sksp, struct pf_state *st)
>   1299	{
>   1300		struct pf_state_key *skw = *skwp;
>   1301		struct pf_state_key *sks = *sksp;
>   1302		int same = (skw == sks);
>   1303	
>   1304		PF_ASSERT_LOCKED();
>   1305	
>   1306		st->kif = kif;
>   1307		PF_STATE_ENTER_WRITE();
>   1308	
>   1309		skw = pf_state_key_attach(skw, st, PF_SK_WIRE);
>   1310		if (skw == NULL) {
>   1311			pf_state_key_unref(sks);
>   1312			PF_STATE_EXIT_WRITE();
>   1313			return (-1);
>   1314		}
>   1315	
>   1316		if (same) {
>   1317			/* pf_state_key_attach might have swapped skw */
>   1318			if (skw != sks) {
>   1319				pf_state_key_unref(sks);
>   1320				sks = pf_state_key_ref(skw);
>   1321			}
>   1322			st->key[PF_SK_STACK] = sks;
>   1323		} else if (pf_state_key_attach(sks, st, PF_SK_STACK) == NULL) {
>   1324			pf_state_key_detach(st, PF_SK_WIRE);
>   1325			pf_state_key_unref(skw);
>   1326			PF_STATE_EXIT_WRITE();
>   1327			return (-1);
>   1328		}
> 
> when creating and inserting a state, the code assumes that it has
> to allocate the pf_state_key structs for that new state.
> 
> if there's no nat, the stack and wire addresses are identical, which is
> represented by using the same pf_state_key struct for both. this is
> detected on line 1302.
> 
> however, the same addresses can be used by multiple states.
> pf_state_key_attach() on line 1309 attempts to insert the newly
> created skw pf_state_key into the tree, but if there's a collision
> with an equivalent pf_state_key it will replace the new one with
> the existing one.
> 
> the chunk starting at 1316 basically assumes that pf_state_key_attach()
> did replace skw, so it replaces sks to restore the semantic that the
> same state key pointers for sks and skw represents the no NAT situation.
> 
> my change here actually checks if skw was replaced and avoids unecessary
> unref and ref operations if it can.
> 
> i'm happy to omit this chunk from the bigger diff and commit it
> separately though. it works (with or) without the rest of the changes
> in this diff, so it can and probably should be done independently.

    thanks for clarification. I either missed comment at line 1317 or comment
    did not ring the bell...

let me take one more look at your diff. I'll one/two more days.

thanks and
regards
sashan