Updated pull request for review on bitbucket: https://bitbucket.org/seandroid/kernel-msm/pull-request/1/enable-splitting-the-logs-to-both-auditd/diff
On Thu, May 16, 2013 at 11:22 AM, William Roberts <[email protected]>wrote: > 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 > > -- Respectfully, William C Roberts
