bpfilter_create kassert panic

[Alexander Bluhm]

Hi,

When running some scapy regression tests, a current i386 machine
triggered this panic.

bluhm

panic: kernel diagnostic assertion "bpfilter_lookup(unit) == NULL" failed: file "../../../../net/bpf.c", line 1645
Stopped at Debugger+0x7: leave
TID PID UID PRFLAGS PFLAGS CPU COMMAND
*18583 18583 0 0x2 0 1 python2.7
39680 39680 0 0x14000 0x210 2 softnet
Debugger(d0a01fd4,f570cb98,d09dca30,f570cb98,20) at Debugger+0x7
panic(d09dca30,d0959766,d09e37dc,d09e37fa,66d) at panic+0x71
__assert(d0959766,d09e37fa,66d,d09e37dc,db60f7c0) at __assert+0x2e
bpfilter_create(200,3f8,f570cc5c,db529af4,db6f0cec) at bpfilter_create+0x8b
bpfopen(21700,2,2000,db6f0cec,f570ce48) at bpfopen+0x29
spec_open_clone(f570ccb8,2180,0,0,db6f0cec) at spec_open_clone+0x158
spec_open(f570ccb8,48,f570ccd8,f570ccd4,5d568c47) at spec_open+0x1c3
VOP_OPEN(db529af4,2,db7ee960,db6f0cec,dad6d424) at VOP_OPEN+0x3f
vn_open(f570ce48,2,0,0,ffffff9c) at vn_open+0x16b
doopenat(db6f0cec,ffffff9c,cf7cbe68,1,0) at doopenat+0x176
sys_open(db6f0cec,f570cf5c,f570cf7c,0,f570cfa8) at sys_open+0x37
syscall() at syscall+0x24c
--- syscall (number 1) ---
0x7:
http://www.openbsd.org/ddb.html describes the minimum info required in bug
reports. Insufficient info makes it difficult to find and fix bugs.

ddb{1}> show register
ds 0x10
es 0x10
fs 0x20 clean_idt
gs 0
edi 0x100 clean_idt+0xe0
esi 0xd09dca30 __func__.6218+0x27d
ebp 0xf570cb4c
ebx 0xf570cb98
edx 0x1
ecx 0xd0b5727c kprintf_mutex
eax 0x1
eip 0xd055fe27 Debugger+0x7
cs 0x8
eflags 0x200286 start_phys+0x1a6
esp 0xf570cb3c
ss 0x10
Debugger+0x7: leave

ddb{1}> ps
TID PPID PGRP UID S FLAGS WAIT COMMAND
*18583 59926 82362 0 7 0x2 python2.7
59926 6716 82362 0 3 0x10008a pause sh
6716 16109 82362 0 3 0x100082 piperd make
16109 82362 82362 0 3 0x10008a pause sh
97424 87959 87959 57 3 0x100083 netio ftp
87959 42203 87959 0 3 0x83 piperd perl
82362 96135 82362 0 3 0x10008a pause make
43744 26800 7482 0 3 0x100082 piperd gzip
26800 96135 7482 0 3 0x100082 piperd pax
96135 7482 7482 0 3 0x82 piperd perl
7482 50744 7482 0 3 0x10008a pause ksh
50744 6457 50744 0 3 0x92 select sshd
42203 59249 42203 0 3 0x10008b pause ksh
59249 6457 59249 0 3 0x92 select sshd
91019 95762 95762 73 3 0x100090 kqread syslogd
95762 1 95762 0 3 0x100080 netio syslogd
63490 1 63490 0 3 0x100090 kqread inetd
6457 1 6457 0 3 0x80 select sshd
3755 1 3755 0 3 0x100083 ttyin ksh
94216 1 94216 0 3 0x100083 ttyin getty
84616 1 84616 0 3 0x100083 ttyin getty
99119 1 99119 0 3 0x100083 ttyin getty
37582 1 37582 0 3 0x100083 ttyin getty
61646 1 61646 0 3 0x100083 ttyin getty
77551 1 77551 0 3 0x100098 poll cron
92485 1 92485 99 3 0x100090 poll sndiod
19492 1 19492 110 3 0x100090 poll sndiod
75722 80348 80348 95 3 0x100092 kqread smtpd
13828 80348 80348 103 3 0x100092 kqread smtpd
97935 80348 80348 95 3 0x100092 kqread smtpd
12504 80348 80348 95 3 0x100092 kqread smtpd
5603 80348 80348 95 3 0x100092 kqread smtpd
90661 80348 80348 95 3 0x100092 kqread smtpd
80348 1 80348 0 3 0x100080 kqread smtpd
55186 0 0 0 3 0x14280 nfsidl nfsio
56545 0 0 0 3 0x14280 nfsidl nfsio
48066 0 0 0 3 0x14280 nfsidl nfsio
45832 0 0 0 3 0x14280 nfsidl nfsio
61138 97667 84727 83 3 0x100090 poll ntpd
97667 84727 84727 83 3 0x100090 poll ntpd
84727 1 84727 0 3 0x80 poll ntpd
54276 34610 34610 74 3 0x100090 bpf pflogd
34610 1 34610 0 3 0x80 netio pflogd
40161 0 0 0 3 0x14200 pgzero zerothread
86681 0 0 0 3 0x14200 aiodoned aiodoned
2152 0 0 0 3 0x14200 syncer update
32663 0 0 0 3 0x14200 cleaner cleaner
15241 0 0 0 3 0x14200 reaper reaper
65311 0 0 0 3 0x14200 pgdaemon pagedaemon
94872 0 0 0 3 0x14200 bored crynlk
83385 0 0 0 3 0x14200 bored crypto
24109 0 0 0 3 0x14200 pftm pfpurge
7935 0 0 0 3 0x14200 usbtsk usbtask
50848 0 0 0 3 0x14200 usbatsk usbatsk
78435 0 0 0 3 0x14200 bored sensors
56331 0 0 0 3 0x40014200 acpi0 acpi0
89784 0 0 0 7 0x40014200 idle3
15857 0 0 0 3 0x40014200 idle2
32118 0 0 0 3 0x40014200 idle1
39680 0 0 0 7 0x14210 softnet
6634 0 0 0 3 0x14200 bored systqmp
11510 0 0 0 3 0x14200 bored systq
41425 0 0 0 7 0x40014200 idle0
25178 0 0 0 3 0x14200 bored sbar
81068 0 0 0 3 0x14200 kmalloc kmthread
1 0 1 0 3 0x82 wait init
0 -1 0 0 3 0x10200 scheduler swapper

ddb{1}> x/s version
version: OpenBSD 6.0-beta (GENERIC.MP) #1958: Mon Jul 11 17:38:33 MDT 20
16\012 der...@i386.openbsd.org:/usr/src/sys/arch/i386/compile/GENERIC.MP\01
2

[Ted Unangst]

Alexander Bluhm wrote:
> Hi,
>
> When running some scapy regression tests, a current i386 machine
> triggered this panic.

dunno if this is best. maybe. the refcounting of units implies that they can
last longer than close(). therefore, if open() is expecting close() to remove
the unit from the list, we must make sure close does that. dangling refs then
will be freed when the refcount drops.

The bpfilter_destory function is obfuscation at this point.

Alas, no time to test right now.

Index: bpf.c
===================================================================
RCS file: /cvs/src/sys/net/bpf.c,v
retrieving revision 1.142
diff -u -p -r1.142 bpf.c
--- bpf.c 10 Jun 2016 20:33:29 -0000 1.142
+++ bpf.c 24 Jul 2016 13:30:29 -0000
@@ -117,7 +117,6 @@ int bpf_sysctl_locked(int *, u_int, void

struct bpf_d *bpfilter_lookup(int);
struct bpf_d *bpfilter_create(int);
-void bpfilter_destroy(struct bpf_d *);

/*
* Reference count access to descriptor buffers
@@ -368,6 +367,7 @@ bpfclose(dev_t dev, int flag, int mode,
if (d->bd_bif)
bpf_detachd(d);
bpf_wakeup(d);
+ LIST_REMOVE(d, bd_list);
D_PUT(d);
splx(s);

@@ -1494,7 +1494,7 @@ bpf_freed(struct bpf_d *d)
srp_update_locked(&bpf_insn_gc, &d->bd_rfilter, NULL);
srp_update_locked(&bpf_insn_gc, &d->bd_wfilter, NULL);

- bpfilter_destroy(d);
+ free(d, M_DEVBUF, sizeof(*d));
}

/*
@@ -1651,12 +1651,6 @@ bpfilter_create(int unit)
return (bd);
}

-void
-bpfilter_destroy(struct bpf_d *bd)
-{
- LIST_REMOVE(bd, bd_list);
- free(bd, M_DEVBUF, sizeof(*bd));
-}

/*
* Get a list of available data link type of the interface.

[Martin Natano]

On Sun, Jul 24, 2016 at 09:31:08AM -0400, Ted Unangst wrote:
> Alexander Bluhm wrote:
> > Hi,
> >
> > When running some scapy regression tests, a current i386 machine
> > triggered this panic.
>
> dunno if this is best. maybe. the refcounting of units implies that they can
> last longer than close(). therefore, if open() is expecting close() to remove
> the unit from the list, we must make sure close does that. dangling refs then
> will be freed when the refcount drops.
>
> The bpfilter_destory function is obfuscation at this point.
>
> Alas, no time to test right now.

I completely missed that bpf devices can hang around after close. With
that in mind your diff makes sense to me. I think it would be wise to
remove the dependency from bpfdetach() on the bpf_d_list, as it is not
actually required since the introduction of bd_unit, to make sure the
early LIST_REMOVE() doesn't produce any side-effects. See updated diff
below.

Alexander, could you verify this fixes the panic on your i386 machine?

natano


Index: net/bpf.c

===================================================================
RCS file: /cvs/src/sys/net/bpf.c,v
retrieving revision 1.142
diff -u -p -r1.142 bpf.c

--- net/bpf.c 10 Jun 2016 20:33:29 -0000 1.142
+++ net/bpf.c 24 Jul 2016 16:30:30 -0000

@@ -117,7 +117,6 @@ int bpf_sysctl_locked(int *, u_int, void

struct bpf_d *bpfilter_lookup(int);
struct bpf_d *bpfilter_create(int);
-void bpfilter_destroy(struct bpf_d *);

/*
* Reference count access to descriptor buffers
@@ -368,6 +367,7 @@ bpfclose(dev_t dev, int flag, int mode,
if (d->bd_bif)
bpf_detachd(d);
bpf_wakeup(d);
+ LIST_REMOVE(d, bd_list);
D_PUT(d);
splx(s);

@@ -1494,7 +1494,7 @@ bpf_freed(struct bpf_d *d)
srp_update_locked(&bpf_insn_gc, &d->bd_rfilter, NULL);
srp_update_locked(&bpf_insn_gc, &d->bd_wfilter, NULL);

- bpfilter_destroy(d);
+ free(d, M_DEVBUF, sizeof(*d));
}

/*

@@ -1548,20 +1548,8 @@ bpfdetach(struct ifnet *ifp)
if (cdevsw[maj].d_open == bpfopen)
break;

- while ((bd = SRPL_FIRST_LOCKED(&bp->bif_dlist))) {
- struct bpf_d *d;
-
- /*
- * Locate the minor number and nuke the vnode
- * for any open instance.
- */
- LIST_FOREACH(d, &bpf_d_list, bd_list)
- if (d == bd) {
- vdevgone(maj, d->bd_unit,
- d->bd_unit, VCHR);
- break;
- }
- }
+ while ((bd = SRPL_FIRST_LOCKED(&bp->bif_dlist)))
+ vdevgone(maj, bd->bd_unit, bd->bd_unit, VCHR);

free(bp, M_DEVBUF, sizeof *bp);
} else
@@ -1649,13 +1637,6 @@ bpfilter_create(int unit)
LIST_INSERT_HEAD(&bpf_d_list, bd, bd_list);
}
return (bd);
-}
-

-void
-bpfilter_destroy(struct bpf_d *bd)
-{
- LIST_REMOVE(bd, bd_list);
- free(bd, M_DEVBUF, sizeof(*bd));
}

/*