use-after-free in fd code?

[Mouse <mouse%Rodents-Montreal.ORG@localhost>]

I've been working on code that involves meddling with unp_internalize
and unp_externalize (specifically, a way to pass memory, not just file
descriptors, through AF_LOCAL sockets), and I think I may have found a
use-after-free bug, one that appears to be present in even 8.0 (I'm
targeting my work for my 5.2 derivative).

I got my stuff "working", but when testing the codepath for dealing
with descriptors dropped while in transit (not memory blocks - I was
testing to make sure I hadn't broken descriptor passing), I found that
very occasionally, on the order of one in 5-10 million cases, a KASSERT
in unp_gc would trip.  Comparing my uipc_usrreq.c with the 8.0 version
from a work machine, I find that the KASSERT in question is gone,
replaced with a comment "XXX: closef() doesn't pay attention to
FDEFER", an unlock, and a continue.  This might fix the symptom, but I
think there's something worse lurking.

The debugging code I added in an attempt to figure this out clearly
shows that, in most cases, file_ts are used after being freed.  Here's
an example.  The first string is a short summary of the operation (--
and ++ are decrement and increment f_count; A is a KASSERT testing the
indicated condition; the a or b after -- disambiguates two decrements
that would otherwise be indistinguishable); test is a more complicated
case where, for example, f_count is compared to f_msgcount.  The number
in () is the value of f_count.  The number in [] is the CPU the record
was made on.  The file and line are what they look like, though the
line numbers won't match up with anything because I've got a lot of
debugging code in those files.  The stuff on the right is manually
added, describing what the file-and-line indicates.
(internalize_rights and externalize_process_rights are SCM_RIGHTS
processing pulled out into their own functions as part of adding memory
passing.)

init (1) [0]: uipc_usrreq.c, line 1800		internalize_rights
++ (2) [0]: uipc_usrreq.c, line 1802		internalize_rights
A >0 (2) [0]: kern_descrip.c, line 648		fd_close
--a (1) [0]: kern_descrip.c, line 648		fd_close
A !=0 (1) [1]: uipc_usrreq.c, line 2378		unp_gc (scan 1)
A !=0 (1) [1]: uipc_usrreq.c, line 2343		unp_gc (FDEFER)
++ (2) [1]: uipc_usrreq.c, line 2369		unp_gc (hold)
++ (3) [0]: kern_descrip.c, line 1063		fd_affix
A >0 (3) [0]: uipc_usrreq.c, line 1387		externalize_process_rights
--a (2) [0]: uipc_usrreq.c, line 1387		externalize_process_rights
A >0 (2) [1]: uipc_usrreq.c, line 2390		unp_gc (unhold)
--a (1) [1]: uipc_usrreq.c, line 2390		unp_gc (unhold)
test (1) [1]: uipc_usrreq.c, line 2421		unp_gc (sweep)
A >0 (1) [0]: kern_descrip.c, line 648		fd_close
--b (0) [0]: kern_descrip.c, line 648		fd_close
A ==0 (0) [0]: kern_descrip.c, line 648		fd_close 
A ==0 (0) [0]: kern_descrip.c, line 764		ffree (closef)
test (0) [1]: uipc_usrreq.c, line 2346		unp_gc (!FDEFER)
test (0) [1]: uipc_usrreq.c, line 2421		unp_gc (sweep)

Now, this particular case didn't trip the KASSERT; that is in what
appears here as "unp_gc (FDEFER)", which ran while f_count was still
nonzero.  But it _is_ quite clear that unp_gc referred to fp->f_count
twice *after* fd_close called closef called ffree on it.  (The actual
trace logs include the file_t pointer; the above is just the lines for
one particular file_t.)  My trace includes 55 examples; this happens in
all 55 of them, and then crashes with

init (1) [0]: uipc_usrreq.c, line 1800		internalize_rights
++ (2) [0]: uipc_usrreq.c, line 1802		internalize_rights
A >0 (2) [0]: kern_descrip.c, line 648		fd_close
--a (1) [0]: kern_descrip.c, line 648		fd_close
A !=0 (1) [1]: uipc_usrreq.c, line 2378		unp_gc (scan 1)
++ (2) [0]: kern_descrip.c, line 1063		fd_affix
A >0 (2) [0]: uipc_usrreq.c, line 1387		externalize_process_rights
--a (1) [0]: uipc_usrreq.c, line 1387		externalize_process_rights
A >0 (1) [0]: kern_descrip.c, line 648		fd_close
--b (0) [0]: kern_descrip.c, line 648		fd_close
A ==0 (0) [0]: kern_descrip.c, line 648		fd_close
A !=0 (0) [1]: uipc_usrreq.c, line 2343		unp_gc (FDEFER)

In my tests I've never seen a file_t reallocated before unp_gc stops
looking at it, but I don't see any reason that couldn't happen,
especially if the system is close to ENFILE.  (I suspect it hasn't
happened to me in part because my tester is the only thing creating or
closing file descriptors during these runs, it's doing only one
transaction at a time, and the system is nowhere close to ENFILE.)  It
looks to me like a classic use-after-free, and, comparing my
uipc_usrreq.c to 8.0's, I don't see any reason it can't happen there
too.

Of course, it's possible this arises from something I've changed
(though I can't see how; I didn't change the basic control flow).  It's
also possible there's something I've missed that makes this particular
use-after-free safe somehow.  But it's also possible, it seems to me,
that this is a real bug that just hasn't been turned up before because
nobody's really stress-tested fd passing, the way file_ts are allocated
means that file_t memory is never reused for anything else, and it's a
pretty rare race to begin with.

So, am I missing something, or is this a real bug?  I'll be staring at
uipc_usrreq.c trying to figure out where to add and drop references to
keep this from happening.  But I would welcome any thoughts anyone has
on either the (apparent) use-after-free or on possible ways to keep it
from happening.

/~\ The ASCII				  Mouse
\ / Ribbon Campaign
 X  Against HTML		mouse%rodents-montreal.org@localhost
/ \ Email!	     7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B