PERFORCE change 167094 for review
Ilias Marinos
marinosi at FreeBSD.org
Fri Aug 7 18:27:15 UTC 2009
http://perforce.freebsd.org/chv.cgi?CH=167094
Change 167094 by marinosi at marinosi_redrum on 2009/08/07 18:26:35
Fix several bugs -- restructure several things as pointed by rwatson.
Affected files ...
.. //depot/projects/soc2009/marinosi_appaudit/src/sys/security/audit/audit.c#13 edit
.. //depot/projects/soc2009/marinosi_appaudit/src/sys/security/audit/audit_slice.h#11 edit
Differences ...
==== //depot/projects/soc2009/marinosi_appaudit/src/sys/security/audit/audit.c#13 (text) ====
@@ -54,6 +54,7 @@
#include <sys/sysproto.h>
#include <sys/sysent.h>
#include <sys/systm.h>
+#include <sys/sx.h>
#include <sys/ucred.h>
#include <sys/uio.h>
#include <sys/un.h>
@@ -113,6 +114,18 @@
struct audit_slice_queue audit_slice_q;
/*
+ * Serialize the creation of audit slices using this lock to ensure it.
+ */
+static struct sx audit_slices_lock;
+
+#define AUDIT_SLICES_LOCK_INIT() sx_init(&audit_slices_lock, \
+ "audit_slices_lock");
+#define AUDIT_SLICES_LOCK_ASSERT() sx_assert(&audit_slices_lock, \
+ SA_XLOCKED)
+#define AUDIT_SLICES_LOCK() sx_xlock(&audit_slices_lock)
+#define AUDIT_SLICES_UNLOCK() sx_xunlock(&audit_slices_lock)
+
+/*
* Kernel audit information. This will store the current audit address
* or host information that the kernel will use when it's generating
* audit records. This data is modified by the A_GET{SET}KAUDIT auditon(2)
@@ -226,11 +239,21 @@
* base(no reason to be in the queue). We want the queue initialized
* once, even if there are no other slices except the base one and
* thus this is happening in audit_init().
- *
+ */
+ TAILQ_INIT(&audit_slice_q);
+
+ /*
+ * Initialize the lock that will be used to ensure the audit slice
+ * serial creation.
+ */
+ AUDIT_SLICES_LOCK_INIT();
+
+ KINFO_LOCK_INIT();
+
+ /*
* XXXRW: Why might audit_init() ever be called with a non-NULL base
* slice?
*/
- TAILQ_INIT(&audit_slice_q);
if ( audit_base_slice == NULL ) {
/*
* If base slice is null, allocate the base slice.
@@ -665,21 +688,21 @@
audit_slice_create(char *name)
{
struct audit_slice *as = NULL;
- int err;
/*
* XXXRW: With M_WAITOK, malloc(9) never fails, so no error handling
* needed here.
+ * FIXED.
*
* XXXRW: It might be desirable to wrap all of this in an sx lock
* to serialize the creation of audit slices, probably the same lock
* used to serialize access to audit_slice_q. That would prevent
* attempts to remove a slice while it was still being added, etc.
+ * IMPLEMENTED.
*/
- err = 0;
+ AUDIT_SLICES_LOCK();
+ AUDIT_SLICES_LOCK_ASSERT();
as = malloc(sizeof(*as), M_AUDITSLICE, M_WAITOK | M_ZERO);
- if ( as == NULL )
- err = 1; /* Failed to allocate slice */
/*
* XXXRW: Locking needed here. Possibly we should fully initialize
@@ -693,13 +716,17 @@
/*
* XXXRW: Possibly start worker before creating the device?
+ * FIXED.
*/
+ /* Start audit worker thread. */
+ audit_worker_init(as);
+
/* Create the special device node */
audit_slice_cdev_init(as);
- /* Start audit worker thread. */
- audit_worker_init(as);
+ AUDIT_SLICES_UNLOCK();
+
}
/*
@@ -748,10 +775,9 @@
/*
* XXXRW: KINFO_LOCK_INIT() should be in audit_init() rather than
* here conditionally?
+ * FIXED.
*/
mtx_init(&(as->audit_mtx), "audit_mtx", NULL, MTX_DEF);
- if ( as == audit_base_slice )
- KINFO_LOCK_INIT();
cv_init(&(as->audit_worker_cv), "audit_worker_cv");
cv_init(&(as->audit_watermark_cv), "audit_watermark_cv");
@@ -766,15 +792,20 @@
*
* XXXRW: likewise, should acquire an sx lock around all of this to avoid
* simultaneous add/remove being a problem.
+ * FIXED.
*
* XXXR: Does the caller validate that as != audit_base_slice? Perhaps
* audit_slice_destroy() should do the name->slice lookup and potentially
* return an error here.
+ * PENDING. Maybe A_REMOVESLICE should iterate in slice queue (doing the
+ * name->slice lookup) and call this function.This guarantees that 'as' will
+ * never be the base slice as it is not a slice queue element.
*/
-void
+int
audit_slice_destroy(struct audit_slice *as)
{
+ AUDIT_SLICES_LOCK();
/*
* XXXRW: Should either assert the record queue is empty, or drain
* it, depending on invariants.
@@ -783,33 +814,56 @@
* variables?
*/
if (as != NULL) {
+ AUDIT_SLICES_LOCK_ASSERT();
TAILQ_REMOVE(&audit_slice_q, as, as_q);
destroy_dev(as->as_dev);
free(as, M_AUDITSLICE);
}
+ AUDIT_SLICES_UNLOCK();
+ return (0); /* Success */
}
/*
* audit_slice_commit_rec() is used to commit a BSM record that was fetched
- * from a special device node, to the appropriate slice-owner.
+ * from a special device node, to the appropriate slice-owner. This function
+ * is not used for the base audit slice.
+ * Return '0' if record was committed successfully or else return the error
+ * code.
*/
-void
+int
audit_slice_commit_rec(void *rec, struct audit_slice *as)
{
struct kaudit_record *ar = NULL;
- struct thread *td = NULL; /* CHECK THIS */
int error;
+ struct thread *td = NULL;
/*
+ * XXXRW: This error value seems never to be used? Possibly we
+ * should validate the record before calling audit_new, and return
+ * the error to the caller so it can return to userspace?
+ *
+ * FIXED.
+ * Note: This function is called from the write method of the slice's
+ * special device node.We should find a way to return the error to
+ * userspace.
+ */
+ /* Verify the record. */
+ if (bsm_rec_verify(rec) == 0) {
+ error = EINVAL;
+ return (error);
+ }
+
+ /*
* XXXRW: Audit slices other than the base should probably never
* touch td->td_ar, so the below should unconditionally allocate the
* container record.
+ * FIXED.
*
* XXXRW: Asser that this isn't audit_base_slice, since we'll handle
* that improperly?
+ * Base slice should never call audit_slice_commit_rec.
*/
- /* struct thread uninitialized! */
if (ar == NULL) {
/*
@@ -824,25 +878,12 @@
* base slice may already be using it to record the write
* syscall.
*/
- td->td_ar = audit_new(AUE_NULL, td, as);
- if (td->td_ar == NULL)
- //return (ENOTSUP);
- return;
- td->td_pflags |= TDP_AUDITREC;
- ar = td->td_ar;
+ ar = audit_new(AUE_NULL, td, as);
+ if (ar == NULL)
+ return (1);
}
/*
- * XXXRW: This error value seems never to be used? Possibly we
- * should validate the record before calling audit_new, and return
- * the error to the caller so it can return to userspace?
- */
- /* Verify the record. */
- if (bsm_rec_verify(rec) == 0)
- error = EINVAL;
-
-
- /*
* Note: it could be that some records initiated while audit was
* enabled should still be committed?
*/
@@ -852,7 +893,7 @@
as->audit_pre_q_len--;
mtx_unlock(&(as->audit_mtx));
audit_free(ar);
- return;
+ return (1);
}
/*
@@ -869,4 +910,5 @@
as->audit_pre_q_len--;
cv_signal(&(as->audit_worker_cv));
mtx_unlock(&(as->audit_mtx));
+ return (0);
}
==== //depot/projects/soc2009/marinosi_appaudit/src/sys/security/audit/audit_slice.h#11 (text+ko) ====
@@ -191,8 +191,8 @@
void audit_worker_init(void *arg);
void audit_slice_init(struct audit_slice *as, char *name);
void audit_slice_create(char *name);
-void audit_slice_destroy(struct audit_slice *as);
+int audit_slice_destroy(struct audit_slice *as);
void audit_slice_cdev_init(struct audit_slice *as);
-void audit_slice_commit_rec(void *rec, struct audit_slice *as);
+int audit_slice_commit_rec(void *rec, struct audit_slice *as);
#endif /* ! _SECURITY_AUDIT_SLICE_H_ */
More information about the p4-projects
mailing list