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

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;