[PATCH] [AUDIT] auditfilter.c cleanup/const-ification

[PATCH] [AUDIT] auditfilter.c cleanup/const-ification

[Mitchell Blank Jr]

(Not sure if this will make it to the linux-audit mailing list or not; I
tried subscribing to it via the web page a couple days ago but haven't heard
back from mailman)

I noticed the gcc 4.X warning:
	kernel/auditfilter.c: In function "audit_filter_user"
	kernel/auditfilter.c:588: warning: "state" may be used uninitialized in this function
...and thought I'd take a quick look.

The gcc warning isn't correct (since audit_filter_user() only looked at state
if audit_filter_user_rules() returned non-zero, in which case 'state' would
have been initialized)  However the code was needlessly complex --
audit_filter_user_rules() carefully populated the "enum audit_state *state"
with various value but it's only caller just cares if it's AUDIT_DISABLED
or not.  It's shorter and simpler to just let audit_filter_user_rules()
modify its caller's return value more directly.  As an added bonus this
also removes the warning.

While I was looking at auditfilter.c I did some other minor cleanup

  * const-ified pointers where possible

  * both audit_data_to_entry() and audit_krule_to_data() had an unused
    variable called "void *bufp" which I removed

  * [minor] I changed some variables from "int" to "unsigned int" if
    they can't be negative.  Since ->field_count is unsigned I think it's
    a little cleaner to use an unsigned type to iterate through it

I'm not actually using the audit subsystem currently but this at least
compiles and boots ok.  I believe it should all be safe.

Signed-off-by: Mitchell Blank Jr <mitch@sfgoth.com>

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 1c47c59..c70049e 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -363,10 +363,11 @@ extern void		    audit_log_d_path(struct
 					     struct dentry *dentry,
 					     struct vfsmount *vfsmnt);
 				/* Private API (for audit.c only) */
-extern int audit_filter_user(struct netlink_skb_parms *cb, int type);
+extern int audit_filter_user(const struct netlink_skb_parms *cb, int type);
 extern int audit_filter_type(int type);
 extern int  audit_receive_filter(int type, int pid, int uid, int seq,
-				 void *data, size_t datasz, uid_t loginuid);
+				 const void *data, size_t datasz,
+				 uid_t loginuid);
 #else
 #define audit_log(c,g,t,f,...) do { ; } while (0)
 #define audit_log_start(c,g,t) ({ NULL; })
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index d3a8539..c9932a7 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -80,12 +80,13 @@ static __attribute__((unused)) char *aud
 }
 
 /* Common user-space to kernel rule translation. */
-static inline struct audit_entry *audit_to_entry_common(struct audit_rule *rule)
+static inline struct audit_entry *audit_to_entry_common(const struct audit_rule *rule)
 {
 	unsigned listnr;
 	struct audit_entry *entry;
 	struct audit_field *fields;
-	int i, err;
+	unsigned int i;
+	int err;
 
 	err = -EINVAL;
 	listnr = rule->flags & ~AUDIT_FILTER_PREPEND;
@@ -137,11 +138,11 @@ exit_err:
 
 /* Translate struct audit_rule to kernel's rule respresentation.
  * Exists for backward compatibility with userspace. */
-static struct audit_entry *audit_rule_to_entry(struct audit_rule *rule)
+static struct audit_entry *audit_rule_to_entry(const struct audit_rule *rule)
 {
 	struct audit_entry *entry;
 	int err = 0;
-	int i;
+	unsigned int i;
 
 	entry = audit_to_entry_common(rule);
 	if (IS_ERR(entry))
@@ -182,20 +183,18 @@ exit_free:
 }
 
 /* Translate struct audit_rule_data to kernel's rule respresentation. */
-static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
+static struct audit_entry *audit_data_to_entry(const struct audit_rule_data *data,
 					       size_t datasz)
 {
 	int err = 0;
 	struct audit_entry *entry;
-	void *bufp;
 	/* size_t remain = datasz - sizeof(struct audit_rule_data); */
-	int i;
+	unsigned int i;
 
-	entry = audit_to_entry_common((struct audit_rule *)data);
+	entry = audit_to_entry_common((const struct audit_rule *) data);
 	if (IS_ERR(entry))
 		goto exit_nofree;
 
-	bufp = data->buf;
 	entry->rule.vers_ops = 2;
 	for (i = 0; i < data->field_count; i++) {
 		struct audit_field *f = &entry->rule.fields[i];
@@ -223,7 +222,7 @@ exit_free:
 }
 
 /* Pack a filter field's string representation into data block. */
-static inline size_t audit_pack_string(void **bufp, char *str)
+static inline size_t audit_pack_string(void **bufp, const char *str)
 {
 	size_t len = strlen(str);
 
@@ -235,10 +234,10 @@ static inline size_t audit_pack_string(v
 
 /* Translate kernel rule respresentation to struct audit_rule.
  * Exists for backward compatibility with userspace. */
-static struct audit_rule *audit_krule_to_rule(struct audit_krule *krule)
+static struct audit_rule *audit_krule_to_rule(const struct audit_krule *krule)
 {
 	struct audit_rule *rule;
-	int i;
+	unsigned int i;
 
 	rule = kmalloc(sizeof(*rule), GFP_KERNEL);
 	if (unlikely(!rule))
@@ -265,11 +264,10 @@ static struct audit_rule *audit_krule_to
 }
 
 /* Translate kernel rule respresentation to struct audit_rule_data. */
-static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule)
+static struct audit_rule_data *audit_krule_to_data(const struct audit_krule *krule)
 {
 	struct audit_rule_data *data;
-	void *bufp;
-	int i;
+	unsigned int i;
 
 	data = kmalloc(sizeof(*data) + krule->buflen, GFP_KERNEL);
 	if (unlikely(!data))
@@ -279,7 +277,6 @@ static struct audit_rule_data *audit_kru
 	data->flags = krule->flags | krule->listnr;
 	data->action = krule->action;
 	data->field_count = krule->field_count;
-	bufp = data->buf;
 	for (i = 0; i < data->field_count; i++) {
 		struct audit_field *f = &krule->fields[i];
 
@@ -298,9 +295,10 @@ static struct audit_rule_data *audit_kru
 
 /* Compare two rules in kernel format.  Considered success if rules
  * don't match. */
-static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
+static int audit_compare_rule(const struct audit_krule *a,
+			      const struct audit_krule *b)
 {
-	int i;
+	unsigned int i;
 
 	if (a->flags != b->flags ||
 	    a->listnr != b->listnr ||
@@ -353,7 +351,7 @@ static inline int audit_add_rule(struct 
 
 /* Remove an existing rule from filterlist.  Protected by
  * audit_netlink_mutex. */
-static inline int audit_del_rule(struct audit_entry *entry,
+static inline int audit_del_rule(const struct audit_entry *entry,
 				 struct list_head *list)
 {
 	struct audit_entry  *e;
@@ -376,8 +374,8 @@ static int audit_list(void *_dest)
 {
 	int pid, seq;
 	int *dest = _dest;
-	struct audit_entry *entry;
-	int i;
+	const struct audit_entry *entry;
+	unsigned int i;
 
 	pid = dest[0];
 	seq = dest[1];
@@ -410,8 +408,8 @@ static int audit_list_rules(void *_dest)
 {
 	int pid, seq;
 	int *dest = _dest;
-	struct audit_entry *e;
-	int i;
+	const struct audit_entry *e;
+	unsigned int i;
 
 	pid = dest[0];
 	seq = dest[1];
@@ -449,10 +447,10 @@ static int audit_list_rules(void *_dest)
  * @datasz: size of payload data
  * @loginuid: loginuid of sender
  */
-int audit_receive_filter(int type, int pid, int uid, int seq, void *data,
+int audit_receive_filter(int type, int pid, int uid, int seq, const void *data,
 			 size_t datasz, uid_t loginuid)
 {
-	struct task_struct *tsk;
+	const struct task_struct *tsk;
 	int *dest;
 	int err = 0;
 	struct audit_entry *entry;
@@ -546,14 +544,14 @@ int audit_comparator(const u32 left, con
 
 
 
-static int audit_filter_user_rules(struct netlink_skb_parms *cb,
-				   struct audit_krule *rule,
-				   enum audit_state *state)
+static int audit_filter_user_rules(const struct netlink_skb_parms *cb,
+				   const struct audit_krule *rule,
+				   int *enabledp)
 {
-	int i;
+	unsigned int i;
 
 	for (i = 0; i < rule->field_count; i++) {
-		struct audit_field *f = &rule->fields[i];
+		const struct audit_field *f = &rule->fields[i];
 		int result = 0;
 
 		switch (f->type) {
@@ -574,28 +572,20 @@ static int audit_filter_user_rules(struc
 		if (!result)
 			return 0;
 	}
-	switch (rule->action) {
-	case AUDIT_NEVER:    *state = AUDIT_DISABLED;	    break;
-	case AUDIT_POSSIBLE: *state = AUDIT_BUILD_CONTEXT;  break;
-	case AUDIT_ALWAYS:   *state = AUDIT_RECORD_CONTEXT; break;
-	}
+	if (rule->action == AUDIT_NEVER)
+		*enabledp = 0;
 	return 1;
 }
 
-int audit_filter_user(struct netlink_skb_parms *cb, int type)
+int audit_filter_user(const struct netlink_skb_parms *cb, int type)
 {
-	struct audit_entry *e;
-	enum audit_state   state;
+	const struct audit_entry *e;
 	int ret = 1;
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_USER], list) {
-		if (audit_filter_user_rules(cb, &e->rule, &state)) {
-			if (state == AUDIT_DISABLED)
-				ret = 0;
+	list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_USER], list)
+		if (audit_filter_user_rules(cb, &e->rule, &ret))
 			break;
-		}
-	}
 	rcu_read_unlock();
 
 	return ret; /* Audit by default */
@@ -603,7 +593,7 @@ int audit_filter_user(struct netlink_skb
 
 int audit_filter_type(int type)
 {
-	struct audit_entry *e;
+	const struct audit_entry *e;
 	int result = 0;
 	
 	rcu_read_lock();
@@ -612,9 +602,9 @@ int audit_filter_type(int type)
 
 	list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TYPE],
 				list) {
-		int i;
+		unsigned int i;
 		for (i = 0; i < e->rule.field_count; i++) {
-			struct audit_field *f = &e->rule.fields[i];
+			const struct audit_field *f = &e->rule.fields[i];
 			if (f->type == AUDIT_MSGTYPE) {
 				result = audit_comparator(type, f->op, f->val);
 				if (!result)

Re: [PATCH] [AUDIT] auditfilter.c cleanup/const-ification

[Steve Grubb]

Hi Mitchell,

Thanks for looking at this. 

On Monday 03 April 2006 08:51, Mitchell Blank Jr wrote:
> The gcc warning isn't correct (since audit_filter_user() only looked at
> state if audit_filter_user_rules() returned non-zero, in which case 'state'
> would have been initialized)  However the code was needlessly complex --
> audit_filter_user_rules() carefully populated the "enum audit_state *state"
> with various value but it's only caller just cares if it's AUDIT_DISABLED
> or not.

IIRC, this was done to mirror the filtering of syscalls. I think we discussed 
this last June/July. Anyways it was a long time ago.

> It's shorter and simpler to just let audit_filter_user_rules() 
> modify its caller's return value more directly.  As an added bonus this
> also removes the warning.

Changes to the rule matcher have to be carefully tested just in case something 
obscure needs it. In this case, I don't think so since its a user space 
originating message.

> While I was looking at auditfilter.c I did some other minor cleanup
>
>   * const-ified pointers where possible
>
>   * both audit_data_to_entry() and audit_krule_to_data() had an unused
>     variable called "void *bufp" which I removed
>
>   * [minor] I changed some variables from "int" to "unsigned int" if
>     they can't be negative.  Since ->field_count is unsigned I think it's
>     a little cleaner to use an unsigned type to iterate through it

These are good cleanups. In a way, I wished this was 2 patches instead of 1. 
I'd take all these cleanups immediately. The other one I'd probably want to 
put in the test kernel for a week or two just to make sure nothing relied on 
the state.

Thanks,
-Steve

Re: [PATCH] [AUDIT] auditfilter.c cleanup/const-ification

[Mitchell Blank Jr]

Steve Grubb wrote:
> These are good cleanups. In a way, I wished this was 2 patches instead of 1. 

Yeah, sorry. I got a little carried away I guess.  I can split them if that
helps.

> The other one I'd probably want to 
> put in the test kernel for a week or two just to make sure nothing relied on 
> the state.

It's pretty clear from inspection.  Look:

| static int audit_filter_user_rules(struct netlink_skb_parms *cb,
|                                    struct audit_krule *rule,
|                                    enum audit_state *state)

OK, so it's a static function... and it has only one caller, where:

|       enum audit_state   state;
[...]
|	if (audit_filter_user_rules(cb, &e->rule, &state)) {
|                        if (state == AUDIT_DISABLED)
|	                                ret = 0;

That's all the code that looks at "state".  It's never stored anywhere;
it's just lost after that test.

The only other possibility where it could be used is if
audit_filter_user_rules() were relying on it to be stable each time its run
but that's clearly not the case because audit_filter_user_rules() only writes
to "*state"; it never reads from it.

So I'm pretty confident that this is a safe cleanup.

-Mitch

Re: [PATCH] [AUDIT] auditfilter.c cleanup/const-ification

[Amy Griffis]

Mitchell Blank Jr wrote:     [Mon Apr 03 2006, 08:51:28AM EDT]
> While I was looking at auditfilter.c I did some other minor cleanup
> 
>   * const-ified pointers where possible
> 
>   * both audit_data_to_entry() and audit_krule_to_data() had an unused
>     variable called "void *bufp" which I removed

A patch in -mm is using 'bufp', so it might not be worthwhile to
remove at this point.

Thanks,
Amy

Re: [PATCH] [AUDIT] auditfilter.c cleanup/const-ification

[Mitchell Blank Jr]

Amy Griffis wrote:
> Mitchell Blank Jr wrote:     [Mon Apr 03 2006, 08:51:28AM EDT]
> > While I was looking at auditfilter.c I did some other minor cleanup
> [...]
> >   * both audit_data_to_entry() and audit_krule_to_data() had an unused
> >     variable called "void *bufp" which I removed
> 
> A patch in -mm is using 'bufp', so it might not be worthwhile to
> remove at this point.

Do you have any info on this patch?  The latest -mm (17-rc1-mm1) doesn't
touch auditfilter.c at all.

-Mitch

Re: [PATCH] [AUDIT] auditfilter.c cleanup/const-ification

[Steve Grubb]

On Wednesday 05 April 2006 07:41, Mitchell Blank Jr wrote:
> Do you have any info on this patch?  The latest -mm (17-rc1-mm1) doesn't
> touch auditfilter.c at all.

We loosely sync to -mm as our working group's patches are ready for wider 
testing. There is a git tree at:

git://git.kernel.org/pub/scm/linux/kernel/git/viro/audit-current.git/

This is what we work against.

-Steve

Re: [PATCH] [AUDIT] auditfilter.c cleanup/const-ification

[Amy Griffis]

On Wed, Apr 05, 2006 at 04:41:52AM -0700, Mitchell Blank Jr wrote:
> Amy Griffis wrote:
> > Mitchell Blank Jr wrote:     [Mon Apr 03 2006, 08:51:28AM EDT]
> > > While I was looking at auditfilter.c I did some other minor cleanup
> > [...]
> > >   * both audit_data_to_entry() and audit_krule_to_data() had an unused
> > >     variable called "void *bufp" which I removed
> > 
> > A patch in -mm is using 'bufp', so it might not be worthwhile to
> > remove at this point.
> 
> Do you have any info on this patch?  The latest -mm (17-rc1-mm1) doesn't
> touch auditfilter.c at all.

Sorry about that, I thought it was in -mm at this point.  Apparently
the latest audit master branch wasn't pulled for 17-rc1-mm1, but
AFAIKT it should be in the next -mm.

Re: [PATCH] [AUDIT] auditfilter.c cleanup/const-ification

[Mitchell Blank Jr]

Steve Grubb wrote:
> We loosely sync to -mm as our working group's patches are ready for wider 
> testing. There is a git tree at:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/audit-current.git/

OK, I cloned that.  I'll try to rebase my changes against that when I get
some spare moments.

I still don't see the change that Amy mentioned though: the only divergence
in auditfilter.c from mainline is some added "#ifdef CONFIG_AUDIT_SYSCALL"
stuff... nothing touches those "bufp" variables that I can see.

-Mitch

Re: [PATCH] [AUDIT] auditfilter.c cleanup/const-ification

[Mitchell Blank Jr]

Mitchell Blank Jr wrote:
> Steve Grubb wrote:
> > git://git.kernel.org/pub/scm/linux/kernel/git/viro/audit-current.git/
> 
> I still don't see the change that Amy mentioned though: the only divergence
> in auditfilter.c from mainline is some added "#ifdef CONFIG_AUDIT_SYSCALL"
> stuff... nothing touches those "bufp" variables that I can see.

Ah, I see the problem I was having... the "git clone" left HEAD pointing at
the "master.b2" branch.  It seems that "audit.b4" is where the current
action is, correct?

-Mitch

Re: [PATCH] [AUDIT] auditfilter.c cleanup/const-ification

[Alexander Viro]

On Thu, Apr 06, 2006 at 06:43:51AM -0700, Mitchell Blank Jr wrote:
> Mitchell Blank Jr wrote:
> > Steve Grubb wrote:
> > > git://git.kernel.org/pub/scm/linux/kernel/git/viro/audit-current.git/
> > 
> > I still don't see the change that Amy mentioned though: the only divergence
> > in auditfilter.c from mainline is some added "#ifdef CONFIG_AUDIT_SYSCALL"
> > stuff... nothing touches those "bufp" variables that I can see.
> 
> Ah, I see the problem I was having... the "git clone" left HEAD pointing at
> the "master.b2" branch.  It seems that "audit.b4" is where the current
> action is, correct?

lspp.b5, soon to switch to lspp.b6