[PATCH 2/2] audit string fields interface + consumer

[Amy Griffis]

On Wed, Jan 18, 2006 at 12:02:10PM -0600, Serge E. Hallyn wrote:
> Quoting Timothy R. Chavez (tinytim at us.ibm.com):
> > On Tue, 2006-01-17 at 17:19 -0500, Amy Griffis wrote:
> > > Here is an update that incorporates changes based on Tim's feedback:
> > >     - sanity check for path with trailing /
> > >     - call path_release
> > >     - use audit_compare_watch
> > > 
> > > and fixes panics due to the assumption of the existence of
> > > rule->watch.
> > >
> > > diff --git a/kernel/audit.h b/kernel/audit.h
> > > index f3b2a00..cc979e9 100644
> > > --- a/kernel/audit.h
> > > +++ b/kernel/audit.h
> > > @@ -52,6 +52,12 @@ enum audit_state {
> > >  };
> > >  
> > >  /* Rule lists */
> > > +struct audit_watch {
> > > +	char			*path; /* watch insertion path */
> > > +	struct list_head	mlist; /* entry in master_watchlist */
> > > +	struct list_head	rules; /* associated rules */
> > > +};
> > > +
> > >  struct audit_field {
> > >  	u32			type;
> > >  	u32			val;
> > > @@ -67,6 +73,8 @@ struct audit_krule {
> > >  	u32			buflen; /* for data alloc on list rules */
> > >  	u32			field_count;
> > >  	struct audit_field	fields[AUDIT_MAX_FIELDS];
> > > +	struct audit_watch	*watch; /* associated watch */
> > > +	struct list_head	rlist; /* entry in audit_watch.rules list */
> > >  };
> > 
> > This may not really be that important, but if you switch to hlist_head
> > you have a 4-byte savings, which is something...
> > 
> > AUDIT_MAX_FIELDS defaults to 64, sizeof(audit_field) is 12-bytes...
> > 768-bytes... 788-bytes per audit_krule?
> 
> Agree.

Sorry, I'm not following how the list_head applies to the fields[]
array.

> > Not sure if this qualifies as stack abuse though, which is why I
> > say it might not be that important.  I'm still a little concerned
> > about AUDIT_MAX_FIELDS though... How much of that stack space is
> > actually being used on average?
> 
> Why is it done that way?

Up to the present, the audit code had taken the more simplistic
approach of using a single data structure for representing an audit
rule for both the interface between kernel and userspace and the
kernel's internal representation.

The other patch in this set changes the code so that there are now
separate data structures used for the interface and the internal
representation.

> I assume ->fields would only need to be allocated or expanded when
> rules are being added?  So is there any reason not to make ->fields
> and ->values dynamically allocated as needed?

Yes, I think it makes sense to dynamically allocate the audit rule
fields in the kernel's representation.  I've been asked to focus on
getting some more functionality out for review, but I'll revisit this
as soon as I get a chance.

> More inanely, in
> 
> >+static int audit_to_watch(char *path, struct audit_krule *krule, int fidx)
> >+{
> >+	int err;
> ...
> >+	err = -EINVAL;
> >+	if (path[0] != '/' || path[f->val-1] == '/' ||
> >+	    krule->listnr != AUDIT_FILTER_EXIT ||
> >+	    f->op & ~AUDIT_EQUAL)
> >+		return err;
> ...
> >+	err = -ENOMEM;
> >+	watch = kmalloc(sizeof(*watch), GFP_KERNEL);
> >+	if (!watch)
> >+		return err;
> ...
> >+
> >+	return 0;
> >+}
> 
> There's really no point using err... :)

Heh, good point.

Thanks,
Amy