Re: pf: "set skip on tap" is lost when vmd destroys and recreates tap(4)

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 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;