Standardize rx_event usage

Standardize rx_event usage

Go over all consumers of the rx event framework and normalize its usage
according to the following principles:

rxevent_Post() is used to create an event, and it returns an event
handle (with a reference on the event structure) that can be used
to cancel the event before its timeout fires.  (There is also an
additional reference on the event held by the global event tree.)
In all(*) usage within the tree, that event handle is stored within
either an rx_connection or an rx_call.  Reads/writes to the member variable
that holds the event handle require either the conn_data_lock or call
lock, respectively -- that means that in most cases, callers of
rxevent_Post() and rxevent_Cancel() will be holding one of those
aforementioned locks.  The event handlers themselves will need to
modify the call/connection object according to the nature of the
event, which requires holding those same locks, and also a guarantee
that the call/connection is still a live object and has not been
deallocated!  Whether or not rxevent_Cancel() succeeds in cancelling
the event before it fires, whenever passed a non-NULL event structure
it will NULL out the supplied pointer and drop a reference on the
event structure.  This is the correct behavior, since the caller
has asked to cancel the event and has no further use for the event
handle or its reference on the event structure.  The caller of
rxevent_Cancel() must check its return value to know whether or
not the event was cancelled before its handler was able to run.

The interaction window between the call/connection lock and the lock
protecting the red/black tree of pending events opens up a somewhat
problematic race window.  Because the application thread is expected
to hold the call/connection lock around rxevent_Cancel() (to protect
the write to the field in the call/connection structure that holds
an event handle), and rxevent_Cancel() must take the lock protecting
the red/black tree of events, this establishes a lock order with the
call/connection lock taken before the eventTree lock.  This is in
conflict with the event handler thread, which must take the eventTree
lock first, in order to select an event to run (and thus know what
additional lock would need to be taken, by virtue of what handler
function is to be run).  The conflict is easy to resolve in the
standard way, by having a local pointer to the event that is obtained
while the event is removed from the red/black tree under the eventTree
lock, and then the eventTree lock can be dropped and the event run
based on the local variable referring to it.  The race window occurs
when the caller of rxevent_Cancel() holds the call/connection lock,
and rxevent_Cancel() obtains the eventTree lock just after the event
handler thread drops it in order to run the event.  The event handler
function begins to execute, and immediately blocks trying to obtain
the call/connection lock.  Now that rxevent_Cancel() has the eventTree
lock it can proceed to search the tree, fail to find the indicated event
in the tree, clear out the event pointer from the call/connection
data structure, drop its caller's reference to the event structure,
and return failure (the event was not cancelled).  Only then does the
caller of rxevent_Cancel() drop the call/connection lock and allow
the event handler to make progress.

This race is not necessarily problematic if appropriate care is taken,
but in the previous code such was not the case.  In particular, it
is a common idiom for the firing event to call rxevent_Put() on itself,
to release the handle stored in the call/connection that could have
been used to cancel the event before it fired.  Failing to do so would
result in a memory leak of event structures; however, rxevent_Put() does
not check for a NULL argument, so a segfault (NULL dereference) was
observed in the test suite when the race occurred and the event handler
tried to rxevent_Put() the reference that had already been released by
the unsuccessful rxevent_Cancel() call.  Upon inspection, many (but not
all) of the uses in rx.c were susceptible to a similar race condition
and crash.

The test suite also papers over a related issue in that the event handler
in the test suite always knows that the data structure containing the
event handle will remain live, since it is a global array that is allocated
for the entire scope of the test.  In rx.c, events are associated with
calls and connections that have a finite lifetime, so we need to take care
to ensure that the call/connection pointer stored in the event remains
valid for the duration of the event's lifecycle.  In particular, even an
attempt to take the call/connection lock to check whether the corresponding
event field is NULL is fraught with risk, as it could crash if the lock
(and containing call/connection) has already been destroyed!  There are
several potential ways to ensure the liveness of the associated
call/connection while the event handler runs, most notably to take care
in the call/connection destruction path to ensure that all associated
events are either successfully cancelled or run to completion before
tearing down the call/connection structure, and to give the pending event
its own reference on the associated call/connection.  Here, we opt for
the latter, acknowledging that this may result in the event handler thread
doing the full call/connection teardown and delay the firing of subsequent
events.  This is deemed acceptable, as pending events are for intentionally
delayed tasks, and some extra delay is probably acceptable.  (The various
keepalive events and the challenge event could delay the user experience
and/or security properties if significantly delayed, but I do not believe
that this change admits completely unbounded delay in the event handler
thread, so the practical risk seems minimal.)

Accordingly, this commit attempts to ensure that:

* Each event holds a formal reference on its associated call/connection.
* The appropriate lock is held for all accesses to event pointers in
  call/connection structures.
* Each event handler (after taking the appropriate lock) checks whether
  it raced with rxevent_Cancel() and only drops the call/connection's
  reference to the event if the race did not occur.
* Each event handler drops its reference to the associated call/connection
  *after* doing any actions that might access/modify the call/connection.
* The per-event reference on the associated call/connection is dropped by
  the thread that removes the event from the red/black tree.  That is,
  the event handler function if the event runs, or by the caller of
  rxevent_Cancel() when the cancellation succeed.
* No non-NULL event handles remain in a call/connection being destroyed,
  which would indicate a refcounting error.

(*) There is an additional event used in practice, to reap old connections,
    but it is effectively a background task that reschedules itself
    periodically, with no handle to the event retained so as to be able
    to cancel it.  As such, it is unaffected by the concerns raised here.

While here, standardize on the rx_GetConnection() function for incrementing
the reference count on a connection object, instead of inlining the
corresponding mutex lock/unlock and variable access.

Also enable refcount checking unconditionally on unix, as this is a
rather invasive change late in the 1.8.0 release process and we want
to get as much sanity checking coverage as possible.

Change-Id: I27bcb932ec200ff20364fb1b83ea811221f9871c
Reviewed-on: https://gerrit.openafs.org/12756
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>