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));
}
/*
