----- Original Message ----- > > > > -----Original Message----- > > From: crash-utility-bounces@xxxxxxxxxx > > [mailto:crash-utility-bounces@xxxxxxxxxx] 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@xxxxxxxxxx> > > Subject: Re: 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? > > Thanks for your reviewing. > > Sure, I add such message such as ''kernel audit buffer is empty''. > > > > > Also, with respect to "have never changed until now", your patch does this: > > Sorry, 'never' was not sutable. > > I mean the relevant data structures such as sk_buff and the way handling them > are not changed. A kind of queues and their handling have varied a little. > So, I wanted to say it's not so much complicated to implement dumping feature > of kernel audit logs. > > > > > +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@xxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxx> > > > > 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. > > I've noticed this for the first time. I'll look into this. > > As the first look, it looks to me that this patch also doesn't change data > structures and the way of handling them. It may be enough to adapt to > changes of symbol names and addition of new kind of audit queues. > > > > > 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. > > I'm doing this considering x86 build as x86_64 binary where pointer size > differs, or is this unnecessary for such x86 build? Sorry, I didn't test > x86 build as x86_64 binary. With "make target=x86", crash is built with -m32 to create a 32-bit x86 binary that runs on an x86_64 host, so kernel pointer sizes will always be the same as the crash binary. Thanks, Dave -- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/crash-utility
