Date: Fri, 7 Aug 2009 18:27:13 GMT From: Ilias Marinos <marinosi@FreeBSD.org> To: Perforce Change Reviews <perforce@FreeBSD.org> Subject: PERFORCE change 167094 for review Message-ID: <200908071827.n77IRDIY070094@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=167094 Change 167094 by marinosi@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_ */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200908071827.n77IRDIY070094>