Re: [Crash-utility] feature to dump audit logs in vmcore

-----Original Message----- From: crash-utility-bounces(a)redhat.com [mailto:crash-utility-bounces@redhat.com] On Behalf Of Dave Anderson Sent: Tuesday, March 14, 2017 4:18 AM To: Discussion list for crash utility usage, maintenance and development <crash-utility(a)redhat.com&gt; Subject: Re: [Crash-utility] feature to dump audit logs in vmcore ----- Original Message ----- > I've written the first version of the patch adding a feature to dump kernel > audit logs as log -a. > Could you review this patch? > > I made this patch on top of today's latest commit on github crash utility > repository: > > https://github.com/crash-utility/crash/commit/ed60e97e319a1cfc9e2779aa1baa c305677393d8 > > Thanks. > HATAYAMA, Daisuke > ----- Original Message ----- > What the extension module does is so simple that it retrives audit > logs from a queue. Looking back into kernel git logs, this design that > audit logs are held in the queue, was introduced at the following > patch to introduce kauditd kernel thread and have never changed until > now: > > # git describe b7d1125817c9a46cc46f57db89d9c195e7af22f8 > v2.6.12-rc4-48-gb7d1125 > > # git log -1 -p b7d1125817c9a46cc46f57db89d9c195e7af22f8 > commit b7d1125817c9a46cc46f57db89d9c195e7af22f8 > Author: David Woodhouse <dwmw2 shinybook infradead org> > Date: Thu May 19 10:56:58 2005 +0100 > > AUDIT: Send netlink messages from a separate kernel thread > > netlink_unicast() will attempt to reallocate and will free messages if > the socket's rcvbuf limit is reached unless we give it an infinite > timeout. So do that, from a kernel thread which is dedicated to spewing > stuff up the netlink socket. > > Signed-off-by: David Woodhouse <dwmw2 infradead org> > > I confirmed the comamnd works well on fedora 4.8 kernel, RHEL7.3, > RHEL6.8 and RHEL5.11; RHEL6.8 was tested both on x86_64 and x86_32. I tested this on ~250 sample vmcores I keep around, but absolutely none of them had any audit data stored. So "log -a" either quietly did nothing, or in a handful of cases, failed with "log: -a option not supported or applicable on this architecture or kernel". Perhaps it would be helpful if you could also print a message if there is no audit data stored in the kernel?

Also, with respect to "have never changed until now", your patch does this:

+static void +__dump_audit(char *symname) +{ + if (symbol_exists(symname)) { + if (CRASHDEBUG(1)) + fprintf(fp, "# %s:\n", symname); + dump_audit_skb_queue(symbol_value(symname)); + } +} + +static void +dump_audit(void) +{ + if (!symbol_exists("audit_skb_queue")) + option_not_supported('a'); + + __dump_audit("audit_skb_hold_queue"); + __dump_audit("audit_skb_queue"); +} But in 4.10, the command fails due to this commit, which changed the names of both of those two symbols above: commit af8b824f283de5acc9b9ae8dbb60e4adacff721b Author: Paul Moore <paul(a)paul-moore.com&gt; Date: Tue Nov 29 16:53:24 2016 -0500 audit: rename the queues and kauditd related functions The audit queue names can be shortened and the record sending helpers associated with the kauditd task could be named better, do these small cleanups now to make life easier once we start reworking the queues and kauditd code. Signed-off-by: Paul Moore <paul(a)paul-moore.com&gt; diff --git a/kernel/audit.c b/kernel/audit.c index 801247a..6ac1df1 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -138,9 +138,9 @@ static int audit_freelist_count; static LIST_HEAD(audit_freelist); -static struct sk_buff_head audit_skb_queue; +static struct sk_buff_head audit_queue; /* queue of skbs to send to auditd when/if it comes back */ -static struct sk_buff_head audit_skb_hold_queue; +static struct sk_buff_head audit_hold_queue; static struct task_struct *kauditd_task; static DECLARE_WAIT_QUEUE_HEAD(kauditd_wait); static DECLARE_WAIT_QUEUE_HEAD(audit_backlog_wait); ... I don't know what else may have changed with the patch above, but the "reworking the queuees and kauditd code" comment above sounds like the whole infrastructure is due for some signficant changes soon. In any case, it would be unfortunate to check in a new option that doesn't at least work on the current stable upstream kernel.

With respect to the size_table changes, I understand why you might want to store the structure member sizes of nlmsghdr.nlmsg_len, nlmsghdr.nlmsg_type and and sk_buff_head.qlen, but it's overkill to bother storing the sizes of sk_buff_head.next, sk_buff.data and sk_buff.next: @@ -2126,6 +2132,13 @@ struct size_table { /* stash of commonly-used sizes */ long task_struct_flags; long timer_base; long taint_flag; + long nlmsghdr; + long nlmsghdr_nlmsg_len; + long nlmsghdr_nlmsg_type; + long sk_buff_head_next; + long sk_buff_head_qlen; + long sk_buff_data; + long sk_buff_next; }; Given that those 3 members are all pointer types, you could just use "sizeof(void *)" for the readmem() calls, as is done throughout the crash code. Note that there are not any MEMBER_SIZE_INIT() callers for structure members that are known, guaranteed, pointer values.

Also, can you please display the new offset_table[] and size_table[] members in dump_offset_table() for use by "help -o"?