pf: improve pf_state_key_attach() error handling

[kp]

Summary

If we fail to attach the stack key that means we've already attached the wire
key. That means the state could be found by other cores, and given that we then
free it, be used after free.
Fix this by not releasing the ID hashrow lock and key locks, ensuring the state
cannot be found by other cores.

Reported by: markj
Submitted by: glebius
MFC after: 3 weeks
Sponsored by: Rubicon Communications, LLC ("Netgate")

[glebius]

sys/netpfil/pf/pf.c

1544–1547

Both clauses have the same epilogue:

if (skw != sks)
        uma_zfree(V_pf_state_key_z, sks);

It can be moved out.

[markj]

That means the state could be found by other cores

Nitpicking, it could also happen on the same core if the current thread is preempted at the wrong moment.

sys/netpfil/pf/pf.c

1544–1547

I'd go even further. When cleaning up resources, I usually try to avoid holding unnecessary locks, even in infrequently executed code. (Or otherwise perhaps add a comment explaining the relevant locking protocol.) To that end:

• we can PF_HASHROW_UNLOCK before the state key cleanup,
• we can free state keys without any locks.

So:

PF_HASHROW_UNLOCK(ih);
s->timeout = PFTM_UNLINKED;
if (idx == PF_SK_STACK)
    /* Remove the wire key from the hash. Other threads can't be referencing it because we still hold the hash lock. */
    pf_state_key_detach(skw);
KEYS_UNLOCK();
if (idx == PF_SK_WIRE)
    uma_zfree(skw);
if (sks != skw)
    uma_zfree(sks);
return (EEXIST);

This makes the locking a bit clearer, at least to my taste.

[glebius]

sys/netpfil/pf/pf.c

1544–1547

Yep, that's even better! Thanks!

[kp]

Move free operations outside the lock

[glebius]

I don't think comments are valuable at this point. The resulting code is clear by itself. Up to you whether to leave them or not. Thanks!

sys/netpfil/pf/pf.c

 
 #define Malloc(foo) malloc(foo, M_UNIT, M_WAITOK | M_ZERO)
 #define Free(foo) free(foo, M_UNIT)
 
 static struct mtx unitmtx;
 
 MTX_SYSINIT(unit, &unitmtx, "unit# allocation", MTX_DEF);
 
+#ifdef UNR64_LOCKED
+uint64_t
+alloc_unr64(struct unrhdr64 *unr64)
+{
+        uint64_t item;
+
+        mtx_lock(&unitmtx);
+        item = unr64->counter++;
+        mtx_unlock(&unitmtx);
+        return (item);
+}
+#endif
+
 #else /* ...USERLAND */
 
 #include <bitstring.h>
 #include <err.h>
 #include <errno.h>
 #include <getopt.h>
 #include <stdbool.h>
 #include <stdio.h>