Skip site navigation (1)Skip section navigation (2) 
Date:      Mon, 5 Nov 2012 17:42:06 -0600
From:      Guy Helmer <guy.helmer@gmail.com>
To:        "freebsd-net@freebsd.org" <freebsd-net@freebsd.org>
Subject:   Panic in bpf.c catchpacket()
Message-ID:  <B9C9A713-D51A-4B6D-8690-7D603C918CBC@gmail.com>
next in thread | raw e-mail | index | archive | help 
Still working this problem I've previously mentioned on the list, and my =
working theory now is a race between catchpacket() and this code in =
bpfread():

        /*
         * At this point, we know we have something in the hold slot.
         */
        BPFD_UNLOCK(d);

        /*
         * Move data from hold buffer into user space.
         * We know the entire buffer is transferred since
         * we checked above that the read buffer is bpf_bufsize bytes.
         *
         * XXXRW: More synchronization needed here: what if a second =
thread
         * issues a read on the same fd at the same time?  Don't want =
this
         * getting invalidated.
         */
        error =3D bpf_uiomove(d, d->bd_hbuf, d->bd_hlen, uio);
=20
        BPFD_LOCK(d);
        d->bd_fbuf =3D d->bd_hbuf;
        d->bd_hbuf =3D NULL;
        d->bd_hlen =3D 0;
        bpf_buf_reclaimed(d);
       BPFD_UNLOCK(d);

Assuming it's unwise to hold the descriptor lock over the uiomove() =
call, it seems we at least need to check to make sure bd_hbuf is still =
valid:

@@ -809,10 +948,15 @@ bpfread(struct cdev *dev, struct uio *uio, int iof
        error =3D bpf_uiomove(d, d->bd_hbuf, d->bd_hlen, uio);
=20
        BPFD_LOCK(d);
-       d->bd_fbuf =3D d->bd_hbuf;
-       d->bd_hbuf =3D NULL;
-       d->bd_hlen =3D 0;
-       bpf_buf_reclaimed(d);
+       if (d->bd_hbuf =3D=3D NULL) {
+               printf("bpfread: lost race: bd_hbuf=3D%p bd_sbuf=3D%p =
bd_fbuf=3D%p\n",
+                      d->bd_hbuf, d->bd_sbuf, d->bd_fbuf);
+       } else {
+               d->bd_fbuf =3D d->bd_hbuf;
+               d->bd_hbuf =3D NULL;
+               d->bd_hlen =3D 0;
+               bpf_buf_reclaimed(d);
+       }
        BPFD_UNLOCK(d);
=20
        return (error);

This still seems vulnerable to me -- a ROTATE_BUFFERS() or reset_d() =
could be done between the BPFD_UNLOCK() and the bpf_uiomove(). Would a =
new lock to protect the buffers be necessary, or a flag+condition =
variable to indicate "hold buffer in use"?

Guy=

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?B9C9A713-D51A-4B6D-8690-7D603C918CBC>