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

On 2026-04-19 18:45, Atanas Vladimirov wrote:
> 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.

I confirm that the patch works as it should.
The tap group and tap0 are skipped and the VM gets
dhcp lease immediately on boot.

Once again thanks for your effort!

Best wishes,
Atanas

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