Hi Darrel,
I didn't notice this patch in my inbox until just recently.  I've put
a few comments inline.
On Fri, Feb 24, 2006 at 04:26:13PM -0600, Darrel Goeddel wrote:
 diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
 index 3712295..752e2bb 100644
 --- a/kernel/auditfilter.c
 +++ b/kernel/auditfilter.c
 @@ -25,6 +25,7 @@
 #include <linux/fs.h>
 #include <linux/namei.h>
 #include <linux/netlink.h>
 +#include <linux/selinux.h>
 #include "audit.h"
 
 /* There are three lists of rules -- one to search at task creation
 @@ -50,6 +51,13 @@ static inline void audit_free_watch(stru
 
 static inline void audit_free_rule(struct audit_entry *e)
 {
 +	int i;
 +	if (e->rule.fields)
 +		for (i = 0; i < e->rule.field_count; i++) {
 +			struct audit_field *f = &e->rule.fields[i];
 +			kfree(f->se_str);
 +			selinux_audit_rule_free(f->se_rule);
 +		}
 	kfree(e->rule.fields);
 	kfree(e);
 }
 @@ -192,7 +200,12 @@ static struct audit_entry *audit_rule_to
 		f->val = rule->values[i];
 
 		if (f->type & AUDIT_UNUSED_BITS ||
 -		    f->type == AUDIT_WATCH) {
 +		    f->type == AUDIT_WATCH ||
 +		    f->type == AUDIT_SE_USER ||
 +		    f->type == AUDIT_SE_ROLE ||
 +		    f->type == AUDIT_SE_TYPE ||
 +		    f->type == AUDIT_SE_SEN ||
 +		    f->type == AUDIT_SE_CLR) {
 			err = -EINVAL;
 			goto exit_free;
 		}
 @@ -222,7 +235,7 @@ static struct audit_entry *audit_data_to
 	void *bufp;
 	size_t remain = datasz - sizeof(struct audit_rule_data);
 	int i;
 -	char *path;
 +	char *str;
 
 	entry = audit_to_entry_common((struct audit_rule *)data);
 	if (IS_ERR(entry))
 @@ -241,16 +254,42 @@ static struct audit_entry *audit_data_to
 		f->op = data->fieldflags[i] & AUDIT_OPERATORS;
 		f->type = data->fields[i];
 		f->val = data->values[i];
 +		f->se_str = NULL;
 +		f->se_rule = NULL;
 		switch(f->type) {
 +		case AUDIT_SE_USER:
 +		case AUDIT_SE_ROLE:
 +		case AUDIT_SE_TYPE:
 +		case AUDIT_SE_SEN:
 +		case AUDIT_SE_CLR:
 +			str = audit_unpack_string(&bufp, &remain, f->val);
 +			if (IS_ERR(str))
 +				goto exit_free;
 +			entry->rule.buflen += f->val;
 +
 +			err = selinux_audit_rule_init(f->type, f->op, str,
 +			                              &f->se_rule);
 +			/* Keep currently invalid fields around in case they
 +			   become valid after a policy reload. */
 +			if (err == -EINVAL) {
 +				printk(KERN_WARNING "selinux audit rule for 
 item %s is invalid\n", str);
 +				err = 0;
 +			}
 +			if (err) {
 +				kfree(str);
 +				goto exit_free;
 +			} else
 +				f->se_str = str;
 +			break;
 		case AUDIT_WATCH:
 -			path = audit_unpack_string(&bufp, &remain, f->val);
 -			if (IS_ERR(path))
 +			str = audit_unpack_string(&bufp, &remain, f->val);
 +			if (IS_ERR(str))
 				goto exit_free;
 			entry->rule.buflen += f->val;
 
 -			err = audit_to_watch(path, &entry->rule, i);
 +			err = audit_to_watch(str, &entry->rule, i);
 			if (err) {
 -				kfree(path);
 +				kfree(str);
 				goto exit_free;
 			}
 			break;
 @@ -333,6 +372,14 @@ static struct audit_rule_data *audit_kru
 			data->buflen += data->values[i] =
 				audit_pack_string(&bufp, krule->watch->path);
 			break;
 +		case AUDIT_SE_USER:
 +		case AUDIT_SE_ROLE:
 +		case AUDIT_SE_TYPE:
 +		case AUDIT_SE_SEN:
 +		case AUDIT_SE_CLR:
 +			data->buflen += data->values[i] =
 +				audit_pack_string(&bufp, f->se_str);
 +			break;
 		default:
 			data->values[i] = f->val;
 		}
 @@ -370,6 +417,14 @@ static int audit_compare_rule(struct aud
 			if (audit_compare_watch(a->watch, b->watch))
 				return 1;
 			break;
 +		case AUDIT_SE_USER:
 +		case AUDIT_SE_ROLE:
 +		case AUDIT_SE_TYPE:
 +		case AUDIT_SE_SEN:
 +		case AUDIT_SE_CLR:
 +			if (strcmp(a->fields[i].se_str, b->fields[i].se_str))
 +				return 1;
 +			break;
 		default:
 			if (a->fields[i].val != b->fields[i].val)
 				return 1;
 @@ -640,6 +695,9 @@ int audit_comparator(const u32 left, con
 	default:
 		return -EINVAL;
 	}
 +	/* should NEVER get here */
 +	BUG();
 +	return 0;
 }
 
 
 @@ -726,3 +784,143 @@ unlock_and_return:
 	rcu_read_unlock();
 	return result;
 }
 +
 +/* Check to see if the rule contains any selinux fields.  Returns 1 if 
 there
 +   are selinux fields specified in the rule, 0 otherwise. */
 +static inline int audit_rule_has_selinux(struct audit_krule *rule)
 +{
 +	int i;
 +
 +	for (i = 0; i < rule->field_count; i++) {
 +		struct audit_field *f = &rule->fields[i];
 +		switch (f->type) {
 +		case AUDIT_SE_USER:
 +		case AUDIT_SE_ROLE:
 +		case AUDIT_SE_TYPE:
 +		case AUDIT_SE_SEN:
 +		case AUDIT_SE_CLR:
 +			return 1;
 +		}
 +	}
 +
 +	return 0;
 +}
 +
 +/* Make a copy of src in dest.  This will be a deep copy with the exception
 +   of the watch - that pointer is carried over.  The selinux specific 
 fields
 +   will be updated in the copy.  The point is to be able to replace the src
 +   rule with the dest rule in the list, then free the dest rule. */
 +static inline int selinux_audit_rule_update_helper(struct audit_krule 
 *dest,
 +                                                   struct audit_krule *src)
 +{
 +	int i, err = 0;
 +
 +	dest->vers_ops = src->vers_ops;
 +	dest->flags = src->flags;
 +	dest->listnr = src->listnr;
 +	dest->action = src->action;
 +	for (i = 0; i < AUDIT_BITMASK_SIZE; i++)
 +		dest->mask[i] = src->mask[i];
 +	dest->buflen = src->buflen;
 +	dest->field_count = src->field_count;
 +
 +	/* deep copy this information, updating the se_rule fields, because
 +	   the originals will all be freed when the old rule is freed. */
 +	dest->fields = kzalloc(sizeof(struct audit_field) * 
 dest->field_count,
 +	                       GFP_ATOMIC);
 +	if (!dest->fields)
 +		return -ENOMEM; 
It seems like it would be cleaner to group the memory allocations for
the rule entry and the fields into a single function, as any audit
rule must always have both.  This would allow you to retain the
assumption of the existence of fields in audit_free_rule.
 +	memcpy(dest->fields, src->fields,
 +	       sizeof(struct audit_field) * dest->field_count);
 +	for (i = 0; i < dest->field_count; i++) {
 +		struct audit_field *df = &dest->fields[i];
 +		struct audit_field *sf = &src->fields[i];
 +		switch (df->type) {
 +		case AUDIT_SE_USER:
 +		case AUDIT_SE_ROLE:
 +		case AUDIT_SE_TYPE:
 +		case AUDIT_SE_SEN:
 +		case AUDIT_SE_CLR:
 +			/* our own copy of se_str */
 +			df->se_str = kstrdup(sf->se_str, GFP_ATOMIC); 
I realize failure is unlikely here, but shouldn't you check the return
value?  Later on you'll end up passing this to strcmp() which won't
like it if it's NULL.  
 +			/* our own (refreshed) copy of se_rule */
 +			err = selinux_audit_rule_init(df->type, df->op,
 +			                              df->se_str, 
 &df->se_rule);
 +			/* Keep currently invalid fields around in case they
 +			   become valid after a policy reload. */
 +			if (err == -EINVAL) {
 +				printk(KERN_WARNING "selinux audit rule for 
 item %s is invalid\n", df->se_str);
 +				err = 0;
 +			}
 +			if (err)
 +				return err;
 +		}
 +	}
 +
 +	/* we can shallow copy the watch because we will not be freeing it 
 via
 +	   selinux_audit_rule_update (and we do nto modify it) */ 
On the flipside, when audit watches are updated, the se_str and
se_rule will need to be deep copied since they are freed in
audit_free_rule().  Is this what you intend?
 +	dest->watch = src->watch;
 +	dest->rlist = src->rlist; 
 +
 +	return 0;
 +}
 +
 +/* This function will re-initialize the se_rule field of all applicable 
 rules.
 +   It will traverse the filter lists serarching for rules that contain 
 selinux
 +   specific filter fields.  When such a rule is found, it is copied, the
 +   selinux field is re-initialized, and the old rule is replaced with the
 +   updated rule. */
 +/* XXX: is the error handling below appropriate */
 +static int selinux_audit_rule_update(void)
 +{
 +	struct audit_entry *entry, *nentry;
 +	int i, err = 0, tmperr;
 +
 +	/* audit_netlink_sem synchronizes the writers */
 +	down(&audit_netlink_sem);
 +
 +	for (i = 0; i < AUDIT_NR_FILTERS; i++) {
 +		list_for_each_entry(entry, &audit_filter_list[i], list) {
 +			if (!audit_rule_has_selinux(&entry->rule))
 +				continue;
 +
 +			nentry = kmalloc(sizeof(*entry), GFP_ATOMIC);
 +			if (!nentry) {
 +				/* save the first error encountered for the
 +				   return value */
 +				if (!err)
 +					err = -ENOMEM;
 +				audit_panic("error updating selinux 
 filters");
 +				continue;
 +			}
 +
 +			tmperr = 
 selinux_audit_rule_update_helper(&nentry->rule,
 +			                                          
 &entry->rule);
 +			if (!nentry) { 
How would we end up with !nentry here?  Maybe you mean if (temperr) ?
 +				/* save the first error encountered for the
 +				   return value */
 +				if (!err)
 +					err = -ENOMEM; 
You don't want to hardcode the -ENOMEM as selinux_audit_rule_init()
can also return an error.
 +				audit_free_rule(nentry);
 +				audit_panic("error updating selinux 
 filters");
 +				continue;
 +			}
 +			list_replace_rcu(&entry->list, &nentry->list);
 +			call_rcu(&entry->rcu, audit_free_rule_rcu);
 +		}
 +	}
 +
 +	up(&audit_netlink_sem);
 +
 +	return err;
 +}
 +
 +/* Register the callback with selinux.  This callback will be invoked when 
 a
 +   new policy is loaded. */
 +static int __init register_selinux_update_callback(void)
 +{
 +	selinux_audit_set_callback(&selinux_audit_rule_update);
 +	return 0;
 +}
 +__initcall(register_selinux_update_callback);
 + 
I don't know about anyone else, but I would prefer to keep all of the
initialization for the audit subsystem in audit.c:audit_init().  This
makes the audit initialization path more easily synchronized and
readable.
Regards,
Amy