Skip site navigation (1)Skip section navigation (2) 
Date:      Fri, 2 Nov 2012 09:30:40 -0500
From:      Guy Helmer <guy.helmer@gmail.com>
To:        freebsd-hackers@freebsd.org, "freebsd-net@freebsd.org" <freebsd-net@freebsd.org>
Subject:   Panic in bpf.c catchpacket()
Message-ID:  <C292B23C-E9EE-40EB-B1AE-09666C5B42CA@gmail.com>
next in thread | raw e-mail | index | archive | help 

Still working this problem I've previously mentioned, 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 = bpf_uiomove(d, d->bd_hbuf, d->bd_hlen, uio);
 
        BPFD_LOCK(d);
        d->bd_fbuf = d->bd_hbuf;
        d->bd_hbuf = NULL;
        d->bd_hlen = 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 = bpf_uiomove(d, d->bd_hbuf, d->bd_hlen, uio);
 
        BPFD_LOCK(d);
-       d->bd_fbuf = d->bd_hbuf;
-       d->bd_hbuf = NULL;
-       d->bd_hlen = 0;
-       bpf_buf_reclaimed(d);
+       if (d->bd_hbuf == NULL) {
+               printf("bpfread: lost race: bd_hbuf=%p bd_sbuf=%p bd_fbuf=%p\n",
+                      d->bd_hbuf, d->bd_sbuf, d->bd_fbuf);
+       } else {
+               d->bd_fbuf = d->bd_hbuf;
+               d->bd_hbuf = NULL;
+               d->bd_hlen = 0;
+               bpf_buf_reclaimed(d);
+       }
        BPFD_UNLOCK(d);
 
        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?C292B23C-E9EE-40EB-B1AE-09666C5B42CA>