From 6a88e22728d285c4df17216515ce2b8d1e5a6835 Mon Sep 17 00:00:00 2001
From: Kristof Provost <kp@FreeBSD.org>
Date: Fri, 16 Aug 2024 14:55:31 +0200
Subject: [PATCH] pfctl: pfik_ifp is always NULL

The pfik_ifp field is not provided by the kernel, it is always NULL. Do not
check for it. This caused us to not clear the skip flag on interfaces, leading
to unexpected behaviour when a 'set skip' was removed.

PR:		280834
Sponsored by:	Rubicon Communications, LLC ("Netgate")
Differential Revision:	https://reviews.freebsd.org/D46311
---
 sbin/pfctl/pfctl.c               |  7 +---
 tests/sys/netpfil/pf/set_skip.sh | 61 ++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 6 deletions(-)

diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index b60e64fba33..45bfdf31f8d 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -394,8 +394,6 @@ pfctl_check_skip_ifaces(char *ifname)
 				continue;
 
 			for (n = h; n != NULL; n = n->next) {
-				if (p->pfik_ifp == NULL)
-					continue;
 				if (strncmp(p->pfik_name, ifname, IFNAMSIZ))
 					continue;
 
@@ -422,9 +420,6 @@ pfctl_adjust_skip_ifaces(struct pfctl *pf)
 
 		for (n = h; n != NULL; n = n->next)
 			PFRB_FOREACH(pp, &skip_b) {
-				if (pp->pfik_ifp == NULL)
-					continue;
-
 				if (strncmp(pp->pfik_name, n->ifname, IFNAMSIZ))
 					continue;
 
@@ -437,7 +432,7 @@ pfctl_adjust_skip_ifaces(struct pfctl *pf)
 	}
 
 	PFRB_FOREACH(p, &skip_b) {
-		if (p->pfik_ifp == NULL || ! (p->pfik_flags & PFI_IFLAG_SKIP))
+		if (! (p->pfik_flags & PFI_IFLAG_SKIP))
 			continue;
 
 		pfctl_set_interface_flags(pf, p->pfik_name, PFI_IFLAG_SKIP, 0);
diff --git a/tests/sys/netpfil/pf/set_skip.sh b/tests/sys/netpfil/pf/set_skip.sh
index e5b1440360e..e984377721b 100644
--- a/tests/sys/netpfil/pf/set_skip.sh
+++ b/tests/sys/netpfil/pf/set_skip.sh
@@ -26,6 +26,50 @@
 
 . $(atf_get_srcdir)/utils.subr
 
+atf_test_case "unset" "cleanup"
+unset_head()
+{
+	atf_set descr 'Unset set skip test'
+	atf_set require.user root
+}
+
+unset_body()
+{
+	pft_init
+
+	vnet_mkjail alcatraz
+	jexec alcatraz ifconfig lo0 127.0.0.1/8 up
+	jexec alcatraz pfctl -e
+	pft_set_rules alcatraz "set skip on lo0" \
+		"block in proto icmp"
+
+	echo "set skip"
+	jexec alcatraz pfctl -v -sI
+
+	jexec alcatraz ifconfig
+	atf_check -s exit:0 -o ignore jexec alcatraz ping -c 1 127.0.0.1
+
+	# Unset the skip on the group
+	pft_set_rules noflush alcatraz \
+	    "block in proto icmp"
+
+	echo "No setskip"
+	jexec alcatraz pfctl -v -sI
+
+	# Do flush states
+	jexec alcatraz pfctl -Fs
+
+	# And now our ping is blocked
+	atf_check -s exit:2 -o ignore jexec alcatraz ping -c 1 127.0.0.1
+
+	jexec alcatraz pfctl -v -sI
+}
+
+unset_cleanup()
+{
+	pft_cleanup
+}
+
 atf_test_case "set_skip_group" "cleanup"
 set_skip_group_head()
 {
@@ -45,8 +89,24 @@ set_skip_group_body()
 	pft_set_rules alcatraz "set skip on foo" \
 		"block in proto icmp"
 
+	echo "set skip"
+	jexec alcatraz pfctl -v -sI
+
 	jexec alcatraz ifconfig
 	atf_check -s exit:0 -o ignore jexec alcatraz ping -c 1 127.0.0.1
+
+	# Unset the skip on the group
+	pft_set_rules noflush alcatraz \
+	    "block in proto icmp"
+
+	# Do flush states
+	jexec alcatraz pfctl -Fs
+
+	# And now our ping is blocked
+	atf_check -s exit:2 -o ignore jexec alcatraz ping -c 1 127.0.0.1
+
+	echo "No setskip"
+	jexec alcatraz pfctl -v -sI
 }
 
 set_skip_group_cleanup()
@@ -163,6 +223,7 @@ pr255852_cleanup()
 
 atf_init_test_cases()
 {
+	atf_add_test_case "unset"
 	atf_add_test_case "set_skip_group"
 	atf_add_test_case "set_skip_group_lo"
 	atf_add_test_case "set_skip_dynamic"