git: 0578fe492284 - main - pf: rework pf_icmp_state_lookup() failure mode

The branch main has been updated by kp:

URL: https://cgit.FreeBSD.org/src/commit/?id=0578fe492284ded4745167060be794032e6e22f0

commit 0578fe492284ded4745167060be794032e6e22f0
Author:     Kristof Provost <kp@FreeBSD.org>
AuthorDate: 2024-08-30 11:36:39 +0000
Commit:     Kristof Provost <kp@FreeBSD.org>
CommitDate: 2024-09-01 15:05:29 +0000

    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")
---
 sys/net/pfvar.h     |  4 ++--
 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 43d4b908a407..7b3c1c49696a 100644
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -374,8 +374,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 e28bad8750f9..f7fe75184efd 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -6754,6 +6754,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);
@@ -6806,15 +6808,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);
 		}
@@ -7221,8 +7224,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] !=
@@ -7277,16 +7282,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);
 			}