On 2026-04-19 12:53, Alexandr Nedvedicky wrote:
Hello,
I agree this is a bug.
On Sat, Apr 18, 2026 at 08:33:44PM +0300, Atanas Vladimirov wrote:
</snip>
Index: sys/net/pf_if.c
===================================================================
--- sys/net/pf_if.c
+++ sys/net/pf_if.c
@@ -852,8 +852,17 @@ pfi_set_flags(const char *name, int flags)
} else
panic("%s pfi_kif_get() returned NULL\n",
__func__);
- } else
+ } else {
p->pfik_flags_new = p->pfik_flags | flags;
+ /*
+ * The kif already existed (created by
+ * pfi_attach_ifnet()/pfi_attach_ifgroup()). Take a
+ * flag ref so it survives a transient disappearance
+ * of the backing ifnet/ifgroup, e.g. vmd(8) destroying
+ * and recreating tap(4) across a VM reboot.
+ */
+ if (ISSET(flags, PFI_IFLAG_SKIP) &&
+ p->pfik_flagrefs == 0)
+ pfi_kif_ref(p, PFI_KIF_REF_FLAG);
+ }
} else {
RB_FOREACH(p, pfi_ifhead, &pfi_ifs)
p->pfik_flags_new = p->pfik_flags | flags;
however diff above triggered an assert() on one of my test boxes:
kernel diagnostic assertion "(p->pfik_flagrefs == 0) ||
(p->pfik_flagref
s == 1)" failed: file "/home/sashan/src.sashan/sys/net/pf_if.c",
line 901
Stopped at db_enter+0x14: popq %rbp
TID PID UID PRFLAGS PFLAGS CPU COMMAND
* 30662 18016 0 0x3 0 0K pfctl
db_enter() at db_enter+0x14
panic(ffffffff82672a84) at panic+0xd5
__assert(ffffffff826aeb78,ffffffff8270f923,385,ffffffff82628d6b) at
__assert+0x
29
pfi_clear_flags(ffff80001c8d6200,100) at pfi_clear_flags+0x206
pfioctl(14900,c028445a,ffff80001c8d6200,3,ffff80001c7f14d8) at
pfioctl+0xe0f
VOP_IOCTL(fffffd802b6e6cd0,c028445a,ffff80001c8d6200,3,fffffd8004ffda90,ffff800
01c7f14d8) at VOP_IOCTL+0x60
vn_ioctl(fffffd802909ebd0,c028445a,ffff80001c8d6200,ffff80001c7f14d8)
at vn_ioc
tl+0x86
sys_ioctl(ffff80001c7f14d8,ffff80001c8d63b0,ffff80001c8d6330) at
sys_ioctl+0x30
e
syscall(ffff80001c8d63b0) at syscall+0x5f9
Xsyscall() at Xsyscall+0x128
the /etc/pf.conf on that test box contained two 'set skip on'
statements
for lo0 interface. Diff below avoids the assert on that box.
Taking a closer look at pfi_set_flags() function I think the branch
with RB_FOREACH() should be removed. The pfctl(8) always fills name
so that code is never reached on behalf of pfctl. Also the
RB_FOREACH()
loop body suffers from the same bug we attempt to fix here.
OK ?
Thanks, Alexandr!
I'll try to test your diff and report back.
thanks and
regards
sashan
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/net/pf_if.c b/sys/net/pf_if.c
index 0c6681e757e..bba7237c904 100644
--- a/sys/net/pf_if.c
+++ b/sys/net/pf_if.c
@@ -854,8 +854,22 @@ pfi_set_flags(const char *name, int flags)
} else
panic("%s pfi_kif_get() returned NULL\n",
__func__);
- } else
+ } else {
+ /*
+ * pf.conf may accidentally contain two set skip on ...
+ * statements. For example:
+ * set skip lo
+ * set skip lo
+ * We need to grab reference only when skip flag is
+ * set to avoid tripping assert pfi_clear_flags()
+ */
+ if (ISSET(flags, PFI_IFLAG_SKIP) &&
+ !ISSET(p->pfik_flags_new, PFI_IFLAG_SKIP) &&
+ !ISSET(p->pfik_flags, PFI_IFLAG_SKIP))
+ pfi_kif_ref(p, PFI_KIF_REF_FLAG);
+
p->pfik_flags_new = p->pfik_flags | flags;
+ }
} else {
RB_FOREACH(p, pfi_ifhead, &pfi_ifs)
p->pfik_flags_new = p->pfik_flags | flags;
