rx: Use atomics for rx_securityClass refcounts

Currently, the refCount in struct rx_securityClass is not protected by
any locks. Thus, if two threads create or destroy a connection using
the same rx_securityClass at the same time (or call rxs_Release), the
refCount can become inaccurate. If the refCount is undercounted, we
can prematurely free it while it's still referenced by other
connections or services, leading to segfaults, data corruption, etc.

For client connections, this can happen between any threads that
create and destroy a connection using the same security class struct.
For server connections, only two threads can race in this way: the rx
listener thread (which creates connections), and the rx event thread
(which destroys idle connections in rxi_ReapConnections).

To fix this, ideally we would change the refCount field to be an
rx_atomic_t. However, struct rx_securityClass is declared in the
public installed rx.h header, which cannot include rx_atomic.h. So
instead, change refCount users to go through a few new functions:
rxs_Ref(), rxs_DecRef(), and rxs_SetRefs(). These functions interpret
the refCount as an rx_atomic_t, and so allows callers to use safe
refcounting without needing to call rx_atomic_* functions directly.

Rename the existing refCount field to refCount_data, and declare it as
a char[8]. This gives us enough space to use it as an rx_atomic_t, but
avoids using rx_atomic_t in a public header, and discourages callers
from manipulating the refCount directly.

Thanks to mvitale@sinenomine.net for helping investigate the relevant
issue.

Change-Id: I55094218c79e8bc5498a6d2c1daa5620b1fceaff
Reviewed-on: https://gerrit.openafs.org/15158
Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Michael Meffie <mmeffie@sinenomine.net>

src/rx/rx.c

@@ -57,6 +57,7 @@ extern afs_int32 afs_termState;
 #  include "sys/lock_def.h"
 # endif /* AFS_AIX41_ENV */
 # include "afs/rxgen_consts.h"
+# include <afs/opr.h>
 #else /* KERNEL */
 # include <roken.h>
 
@@ -7392,21 +7393,70 @@ rxi_ReapConnections(struct rxevent *unused, void *unused1, void *unused2,
 }
 
 
-/* rxs_Release - This isn't strictly necessary but, since the macro name from
+/*
- * rx.h is sort of strange this is better.  This is called with a security
+ * rxs_Release - This is called with a security object before it is discarded.
- * object before it is discarded.  Each connection using a security object has
+ * Each connection using a security object has its own refcount to the object
- * its own refcount to the object so it won't actually be freed until the last
+ * so it won't actually be freed until the last connection is destroyed.
- * connection is destroyed.
+ */
- *
- * This is the only rxs module call.  A hold could also be written but no one
- * needs it. */
-
 int
 rxs_Release(struct rx_securityClass *aobj)
 {
     return RXS_Close(aobj);
 }
 
+static_inline rx_atomic_t *
+sc2refCount(struct rx_securityClass *aobj)
+{
+    opr_StaticAssert(sizeof(rx_atomic_t) <= sizeof(aobj->refCount_data));
+    return (rx_atomic_t *)&aobj->refCount_data;
+}
+
+/**
+ * Get a reference to a security object.
+ *
+ * @param[in] aobj  The security object.
+ * @returns The given security object.
+ */
+struct rx_securityClass *
+rxs_Ref(struct rx_securityClass *aobj)
+{
+    rx_atomic_t *refCount = sc2refCount(aobj);
+    rx_atomic_inc(refCount);
+    return aobj;
+}
+
+/**
+ * Puts a reference to a security object.
+ *
+ * This should only be used by securityClass implementations; users of a
+ * security class should use rxs_Release to release a reference to a security
+ * object.
+ *
+ * @param[in] aobj  The security object.
+ * @returns The refcount of the security object (post-decrement).
+ */
+int
+rxs_DecRef(struct rx_securityClass *aobj)
+{
+    rx_atomic_t *refCount = sc2refCount(aobj);
+    return rx_atomic_dec_and_read(refCount);
+}
+
+/**
+ * Set the reference count on a security object.
+ *
+ * This should only be used by securityClass implementations.
+ *
+ * @param[in] aobj  The security object.
+ * @param[in] refs  The refCount to set.
+ */
+void
+rxs_SetRefs(struct rx_securityClass *aobj, int refs)
+{
+    rx_atomic_t *refCount = sc2refCount(aobj);
+    rx_atomic_set(refCount, refs);
+}
+
 void
 rxi_DebugInit(void)
 {

src/rx/rx.h

@@ -604,7 +604,13 @@ struct rx_securityClass {
 	int (*op_Spare3) (void);
     } *ops;
     void *privateData;
-    int refCount;
+
+    /*
+     * Don't use this field directly. To manipulate the refcount, callers
+     * should use rxs_Ref()/rxs_Release(), and (for security class
+     * implementations only) rxs_DecRef()/rxs_SetRefs().
+     */
+    char refCount_data[8];
 };
 
 #define RXS_OP(obj,op,args) ((obj && (obj->ops->op_ ## op)) ? \

src/rx/rx_null.c

@@ -25,7 +25,7 @@
 /* The null security object.  No authentication, no nothing. */
 
 static struct rx_securityOps null_ops;
-static struct rx_securityClass null_object = { &null_ops, 0, 0 };
+static struct rx_securityClass null_object = { &null_ops, 0, {0} };
 
 struct rx_securityClass *
 rxnull_NewServerSecurityObject(void)

src/rx/rx_prototypes.h

@@ -88,6 +88,9 @@ extern void rxi_CallError(struct rx_call *call, afs_int32 error);
 extern void rx_SetConnSecondsUntilNatPing(struct rx_connection *conn,
 					  afs_int32 seconds);
 extern int rxs_Release(struct rx_securityClass *aobj);
+extern struct rx_securityClass *rxs_Ref(struct rx_securityClass *aobj);
+extern int rxs_DecRef(struct rx_securityClass *aobj);
+extern void rxs_SetRefs(struct rx_securityClass *aobj, int refs);
 #ifndef KERNEL
 extern void rx_PrintTheseStats(FILE * file, struct rx_statistics *s, int size,
 			       afs_int32 freePackets, char version);

src/rxgk/rxgk_client.c

@@ -59,15 +59,6 @@ cprivate_destroy(struct rxgk_cprivate *cp)
     rxi_Free(cp, sizeof(*cp));
 }
 
-/*
- * Increment the reference count on the security object secobj.
- */
-static_inline void
-obj_ref(struct rx_securityClass *secobj)
-{
-    secobj->refCount++;
-}
-
 /*
  * Decrement the reference count on the security object secobj.
  * If the reference count falls to zero, release the underlying storage.
@@ -77,8 +68,7 @@ obj_rele(struct rx_securityClass *secobj)
 {
     struct rxgk_cprivate *cp;
 
-    secobj->refCount--;
+    if (rxs_DecRef(secobj) > 0) {
-    if (secobj->refCount > 0) {
 	/* still in use */
 	return;
     }
@@ -116,7 +106,7 @@ rxgk_NewClientConnection(struct rx_securityClass *aobj,
     if (rxgk_security_overhead(aconn, cp->level, cp->k0) != 0)
 	goto error;
     rx_SetSecurityData(aconn, cc);
-    obj_ref(aobj);
+    rxs_Ref(aobj);
     return 0;
 
  error:
@@ -502,7 +492,7 @@ rxgk_NewClientSecurityObject(RXGK_Level level, afs_int32 enctype, rxgk_key k0,
     if (cp == NULL)
 	goto error;
     sc->ops = &rxgk_client_ops;
-    sc->refCount = 1;
+    rxs_SetRefs(sc, 1);
     sc->privateData = cp;
 
     /* Now get the client-private data. */

src/rxgk/rxgk_server.c

@@ -47,15 +47,6 @@
 
 #include "rxgk_private.h"
 
-/*
- * Increment the reference count on the security object secobj.
- */
-static_inline void
-obj_ref(struct rx_securityClass *secobj)
-{
-    secobj->refCount++;
-}
-
 /*
  * Decrement the reference count on the security object secobj.
  * If the reference count falls to zero, release the underlying storage.
@@ -65,8 +56,7 @@ obj_rele(struct rx_securityClass *secobj)
 {
     struct rxgk_sprivate *sp;
 
-    secobj->refCount--;
+    if (rxs_DecRef(secobj) > 0) {
-    if (secobj->refCount > 0) {
 	/* still in use */
 	return;
     }
@@ -124,7 +114,7 @@ rxgk_NewServerConnection(struct rx_securityClass *aobj,
 
     sconn_set_noauth(sc);
     rx_SetSecurityData(aconn, sc);
-    obj_ref(aobj);
+    rxs_Ref(aobj);
     return 0;
 
  error:
@@ -753,7 +743,7 @@ rxgk_NewServerSecurityObject(void *getkey_rock, rxgk_getkey_func getkey)
 	return NULL;
     }
     sc->ops = &rxgk_server_ops;
-    sc->refCount = 1;
+    rxs_SetRefs(sc, 1);
     sc->privateData = sp;
 
     /* Now set the server-private data. */

src/rxkad/rxkad_client.c

@@ -87,7 +87,7 @@ rxkad_NewClientSecurityObject(rxkad_level level,
     size = sizeof(struct rx_securityClass);
     tsc = rxi_Alloc(size);
     memset((void *)tsc, 0, size);
-    tsc->refCount = 1;		/* caller gets one for free */
+    rxs_SetRefs(tsc, 1);		/* caller gets one for free */
     tsc->ops = &rxkad_client_ops;
 
     psize = PDATA_SIZE(ticketLen);

src/rxkad/rxkad_common.c

@@ -313,8 +313,6 @@ FreeObject(struct rx_securityClass *aobj)
 {
     struct rxkad_cprivate *tcp;	/* both structs start w/ type field */
 
-    if (aobj->refCount > 0)
-	return 0;		/* still in use */
     tcp = (struct rxkad_cprivate *)aobj->privateData;
     rxi_Free(aobj, sizeof(struct rx_securityClass));
     if (tcp->type & rxkad_client) {
@@ -335,10 +333,9 @@ FreeObject(struct rx_securityClass *aobj)
 int
 rxkad_Close(struct rx_securityClass *aobj)
 {
-    afs_int32 code;
+    if (rxs_DecRef(aobj) > 0)
-    aobj->refCount--;
+	return 0;		/* still in use */
-    code = FreeObject(aobj);
+    return FreeObject(aobj);
-    return code;
 }
 
 /* either: called to (re)create a new connection. */
@@ -371,7 +368,7 @@ rxkad_NewConnection(struct rx_securityClass *aobj,
 	INC_RXKAD_STATS(connections[rxkad_LevelIndex(tcp->level)]);
     }
 
-    aobj->refCount++;		/* attached connection */
+    rxs_Ref(aobj);		/* attached connection */
     return 0;
 }
 

src/rxkad/rxkad_server.c

@@ -147,7 +147,7 @@ rxkad_NewServerSecurityObject(rxkad_level level, void *get_key_rock,
     size = sizeof(struct rx_securityClass);
     tsc = rxi_Alloc(size);
     memset(tsc, 0, size);
-    tsc->refCount = 1;		/* caller has one reference */
+    rxs_SetRefs(tsc, 1);		/* caller has one reference */
     tsc->ops = &rxkad_server_ops;
     size = sizeof(struct rxkad_sprivate);
     tsp = rxi_Alloc(size);