One other question, should I append any new fields to structs to the end, as to preserve alignment?
Bill On Thu, May 16, 2013 at 11:16 AM, William Roberts <[email protected]>wrote: > > > > On Thu, May 16, 2013 at 5:45 AM, Stephen Smalley <[email protected]>wrote: > >> On 05/15/2013 09:52 PM, William Roberts wrote: >> >>> From eb7a0d5e711b555d38e3cd19c754e4**a866bb07a4 Mon Sep 17 00:00:00 2001 >>>> >>> From: William Roberts <[email protected]> >>> Date: Wed, 15 May 2013 18:12:31 -0700 >>> Subject: [PATCH] Enable splitting the logs to both auditd and kernel >>> simultaneously >>> >>> Allow the audit subsystem to send audit events to both the kernel >>> message buffer and auditd at the same time. >>> >>> Change-Id: I3107322c845a4cfb001352e152c08**66b8a73f02d >>> Signed-off-by: William Roberts <[email protected]> >>> >> >> The message included the patch in html (as well as plain) and used DOS >> line encodings. See Documentation/**SubmittingPatches and >> Documentation/email-clients.**txt for some tips or just send directly >> using git send-email. >> >> > I have two factor auth on my gmail, I need to generate a unique password > for my git client. So I was lazy and copy + pasted it into an email. > > > >> Indentation is wrong; scripts/checkpatch.pl complains. >> See Documentation/CodingStyle. >> >> Now, for more substantive comments: >> >> >> --- >>> include/linux/audit.h | 5 +++++ >>> kernel/audit.c | 44 ++++++++++++++++++++++++++++++** >>> +++++++++----- >>> 2 files changed, 44 insertions(+), 5 deletions(-) >>> >>> diff --git a/include/linux/audit.h b/include/linux/audit.h >>> index ed3ef19..ae8083e 100644 >>> --- a/include/linux/audit.h >>> +++ b/include/linux/audit.h >>> @@ -314,6 +314,10 @@ enum { >>> #define AUDIT_STATUS_PID 0x0004 >>> #define AUDIT_STATUS_RATE_LIMIT 0x0008 >>> #define AUDIT_STATUS_BACKLOG_LIMIT 0x0010 >>> +#define AUDIT_STATUS_LOGSPLIT 0x0020 >>> + /* Split log actions */ >>> +#define AUDIT_LOGSPLIT_OFF 0 >>> +#define AUDIT_LOGSPLIT_ON 1 >>> /* Failure-to-log actions */ >>> #define AUDIT_FAIL_SILENT 0 >>> #define AUDIT_FAIL_PRINTK 1 >>> @@ -359,6 +363,7 @@ struct audit_status { >>> __u32 mask; /* Bit mask for valid entries */ >>> __u32 enabled; /* 1 = enabled, 0 = disabled */ >>> __u32 failure; /* Failure-to-log action */ >>> + __u32 logsplit; /* Logsplit action */ >>> __u32 pid; /* pid of auditd process */ >>> __u32 rate_limit; /* messages rate limit (per second) */ >>> __u32 backlog_limit; /* waiting messages limit */ >>> >> >> Note that these definitions have moved in the mainline kernel to >> include/uapi/linux/audit.h as part of splitting userspace API definitions >> from kernel-internal definitions and prototypes. So you will need to >> adjust the patch for mainline. >> >> >> diff --git a/kernel/audit.c b/kernel/audit.c >>> index 4096bcc..10f0457 100644 >>> --- a/kernel/audit.c >>> +++ b/kernel/audit.c >>> @@ -74,6 +74,10 @@ static int audit_initialized; >>> #define AUDIT_OFF 0 >>> #define AUDIT_ON 1 >>> #define AUDIT_LOCKED 2 >>> + >>> +#define AUDIT_PRINTK_NOHOLD 0 >>> +#define AUDIT_PRINTK_HOLD 1 >>> + >>> >> >> These definitions don't seem to be used by your patch; leftovers of an >> earlier version? >> >> Yes > >> >> @@ -357,6 +364,16 @@ static int audit_set_failure(int state, uid_t >>> loginuid, u32 sessionid, u32 sid) >>> loginuid, sessionid, sid); >>> } >>> >>> +static int audit_set_logsplit(int state, uid_t loginuid, u32 sessionid, >>> u32 sid) >>> +{ >>> + if (state != AUDIT_LOGSPLIT_OFF >>> + && state != AUDIT_LOGSPLIT_ON) >>> + return -EINVAL; >>> + >>> + return audit_do_config_change("audit_**logsplit", &audit_logsplit, >>> state, >>> + loginuid, sessionid, sid); >>> +} >>> >> >> FYI, you won't need to pass down the loginuid, sessionid, or sid to this >> function when you port to current mainline. No longer required by >> audit_do_config_change(). >> >> Good to know... >> >>> >>> @@ -725,6 +748,15 @@ static int audit_receive_msg(struct sk_buff *skb, >>> struct nlmsghdr *nlh) >>> if (status_get->mask & AUDIT_STATUS_BACKLOG_LIMIT) >>> err = audit_set_backlog_limit(**status_get->backlog_limit, >>> loginuid, sessionid, sid); >>> + >>> + if (status_get->mask & AUDIT_STATUS_LOGSPLIT) { >>> + err = audit_set_logsplit(status_get-**>logsplit, >>> + loginuid, sessionid, sid); >>> + >>> + if (err < 0) { >>> + return err; >>> + } >>> + } >>> >> >> Looks like this will lose any error from audit_set_backlog_limit(). So >> you can either move your code before the AUDIT_STATUS_BACKLOG_LIMIT case, >> or add an if (err < 0) return err; to that block. Also you should not use >> { } around a single statement (as per Documentation/CodingStyle), so don't >> do that in your if (err < 0)... statement. >> >> I caught this after I sent it out, already corrected. >> > > >> break; >>> case AUDIT_USER: >>> case AUDIT_FIRST_USER_MSG ... AUDIT_LAST_USER_MSG: >>> @@ -1464,6 +1496,8 @@ void audit_log_end(struct audit_buffer *ab) >>> nlh->nlmsg_len = ab->skb->len - NLMSG_SPACE(0); >>> >>> if (audit_pid) { >>> + if(audit_logsplit == AUDIT_LOGSPLIT_ON) >>> + __audit_printk_skb(ab->skb); >>> skb_queue_tail(&audit_skb_**queue, ab->skb); >>> wake_up_interruptible(&**kauditd_wait); >>> } else { >>> >> >> Coding style - put space between if and (. >> >> Will do... > > > -- > Respectfully, > > William C Roberts > > -- Respectfully, William C Roberts
