Re: [PATCH 2/2] audit string fields interface + consumer
On Wed, Jan 18, 2006 at 12:02:10PM -0600, Serge E. Hallyn wrote:
Quoting Timothy R. Chavez (tinytim(a)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