Re: [PATCH][RFC] Light-weight Auditing Framework

[Chris Wright <chrisw@osdl.org>]

* Rik Faith (faith@redhat.com) wrote:
> This note describes a patch against 2.6.4-rc1-bk2 that provides a
> low-overhead system-call auditing framework for Linux that is usable by
> LSM components (e.g., SELinux).  Comments will be appreciated.

Do you have an perf numbers for simply enabling the auditint framework?
Seems to be a lot of use of likely/unlikely.  Are these actually useful
optimizations?  Doesn't seem like CONFIG_AUDIT=n disables all the code.
Not sure if the rest of the patch lot is mainlineable or not, but here are
some simple nits/questions.

Some basic CodingStyle bits:

+static int         audit_initialized = 0;

no need to zero initialized static data.

+       if (in_irq()) BUG();
+       if (!context) BUG();

convert to BUG_ON(in_irq());
           BUG_ON(!context);

+#ifdef CONFIG_AUDIT
+/***********************************************************************/
+/** audit.c                                                           **/
+/***********************************************************************/

Noisy comments.

+                       audit_log_format(ab, " dev=%02x:%02x",
+                                        MAJOR(context->names[i].rdev),
+                                        MINOR(context->names[i].rdev));

format_dev_t() ?

+static int __init audit_enable(char *str)
+{
+       if (!strncmp(str, "on", 2) || !strncmp(str, "yes", 3))
+               audit_default = 1;
+       else if (!strncmp(str, "off", 3) || !strncmp(str, "no", 2))
+               audit_default = 0;
+       else
+               audit_default = simple_strtol(str, NULL, 0);

just pick one method, strings or int (int is so simple), and document the
method for enable/disable.

+void audit_log_vformat(struct audit_buffer *ab, const char *fmt, va_list args)

as of now, this could be static, and not exported, no?


+               ab = kmalloc(sizeof(*ab), GFP_ATOMIC);

need atomic allocations here?  also, i wasn't clear on what makes sure the
audit_buffers can't be overflown, the bits in vformat talking about 1024 bytes?


+               audit_log_format(ab, "<toolong>");

is it useful to at least get the final d_name?

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net