diff options
| author | Kristof Provost <kp@FreeBSD.org> | 2024-08-30 11:36:39 +0000 |
|---|---|---|
| committer | Kristof Provost <kp@FreeBSD.org> | 2024-09-04 08:38:15 +0000 |
| commit | 38f74de7184ac3ad7acc48055551aaa9ec9cded9 (patch) | |
| tree | 4d8c033067cc863902e44bd5a68394a35223dedf | |
| parent | b822e3fab468ffbe941d0758d960e1aa46069a38 (diff) | |
pf: rework pf_icmp_state_lookup() failure mode
If pf_icmp_state_lookup() finds a state but rejects it for not matching the
expected direction we should unlock the state (and NULL out *state). This
simplifies life for callers, and also ensures there's no confusion about what a
non-NULL returned state means.
Previously it could have been left in there by the caller, resulting in callers
unlocking the same state twice.
MFC after: 1 week
Sponsored by: Rubicon Communications, LLC ("Netgate")
(cherry picked from commit 0578fe492284ded4745167060be794032e6e22f0)
| -rw-r--r-- | sys/net/pfvar.h | 4 | ||||
| -rw-r--r-- | sys/netpfil/pf/pf.c | 20 |
2 files changed, 15 insertions, 9 deletions
diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h index d33c747efa7e..290f339e7d00 100644 --- a/sys/net/pfvar.h +++ b/sys/net/pfvar.h @@ -359,8 +359,8 @@ struct pfi_dynaddr { mtx_unlock(_s->lock); \ } while (0) #else -#define PF_STATE_LOCK(s) mtx_lock(s->lock) -#define PF_STATE_UNLOCK(s) mtx_unlock(s->lock) +#define PF_STATE_LOCK(s) mtx_lock((s)->lock) +#define PF_STATE_UNLOCK(s) mtx_unlock((s)->lock) #endif #ifdef INVARIANTS diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c index e94856b011bf..e39abc7e7ad2 100644 --- a/sys/netpfil/pf/pf.c +++ b/sys/netpfil/pf/pf.c @@ -6704,6 +6704,8 @@ pf_icmp_state_lookup(struct pf_state_key_cmp *key, struct pf_pdesc *pd, pf_print_state(*state); printf("\n"); } + PF_STATE_UNLOCK(*state); + *state = NULL; return (PF_DROP); } return (-1); @@ -6756,15 +6758,16 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif, kif, virtual_id, virtual_type, icmp_dir, &iidx, PF_ICMP_MULTI_NONE, 0); if (ret >= 0) { + MPASS(*state == NULL); if (ret == PF_DROP && pd->af == AF_INET6 && icmp_dir == PF_OUT) { - if (*state != NULL) - PF_STATE_UNLOCK((*state)); ret = pf_icmp_state_lookup(&key, pd, state, m, off, pd->dir, kif, virtual_id, virtual_type, icmp_dir, &iidx, multi, 0); - if (ret >= 0) + if (ret >= 0) { + MPASS(*state == NULL); return (ret); + } } else return (ret); } @@ -7171,8 +7174,10 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif, ret = pf_icmp_state_lookup(&key, &pd2, state, m, off, pd2.dir, kif, virtual_id, virtual_type, icmp_dir, &iidx, PF_ICMP_MULTI_NONE, 1); - if (ret >= 0) + if (ret >= 0) { + MPASS(*state == NULL); return (ret); + } /* translate source/destination address, if necessary */ if ((*state)->key[PF_SK_WIRE] != @@ -7227,16 +7232,17 @@ pf_test_state_icmp(struct pf_kstate **state, struct pfi_kkif *kif, pd->dir, kif, virtual_id, virtual_type, icmp_dir, &iidx, PF_ICMP_MULTI_NONE, 1); if (ret >= 0) { + MPASS(*state == NULL); if (ret == PF_DROP && pd2.af == AF_INET6 && icmp_dir == PF_OUT) { - if (*state != NULL) - PF_STATE_UNLOCK((*state)); ret = pf_icmp_state_lookup(&key, &pd2, state, m, off, pd->dir, kif, virtual_id, virtual_type, icmp_dir, &iidx, multi, 1); - if (ret >= 0) + if (ret >= 0) { + MPASS(*state == NULL); return (ret); + } } else return (ret); } |