Re: [PATCH 2/2] audit: Enable cacheing of cmdline in audit_context

On Tue, Nov 19, 2013 at 7:40 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > On Mon, Nov 18, 2013 at 04:41:20PM -0800, William Roberts wrote: >> Not enitrely sure if this is getting us any benefit, as in my >> environment, these contexts are getting free'd immediately. >> >> Change-Id: Ia0d432bc4aba8588840f0dc0026a1e9483e5b485 >> Signed-off-by: William Roberts <wroberts(a)tresys.com&gt; >> --- >> kernel/auditsc.c | 48 +++++++++++++++++++++++++++++++++++++----------- >> 1 file changed, 37 insertions(+), 11 deletions(-) >> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c >> index 45fd3d0..4b30c5d 100644 >> --- a/kernel/auditsc.c >> +++ b/kernel/auditsc.c >> @@ -270,6 +270,7 @@ struct audit_context { >> } mmap; >> }; >> int fds[2]; >> + unsigned long cmdline; > > Would this be better declared to avoid all the casts?: This is indirectly coupled with my todo as well. So the value could be up to a page big, and I allocate and cache a page (which is returned as an usnigned long). But, often times, the value is much smaller. Should I just alloc a seperate and smaller buffer with kmalloc, and cache that as a char * to avoid the casts? Then free the page?

Either way, I have no objection to taking the interface to a char *, I battled that decision in my head while I was coding.

> char *cmdline; > >> #if AUDIT_DEBUG >> int put_count; >> @@ -1044,6 +1045,14 @@ static inline void audit_free_aux(struct audit_context *context) >> } >> } >> >> +static inline void audit_cmdline_free(struct audit_context *ctx) >> +{ >> + if (!ctx->cmdline) >> + return; >> + free_page(ctx->cmdline); >> + ctx->cmdline = 0; > > ctx->cmdline = NULL; > >> +} >> + >> static inline void audit_zero_context(struct audit_context *context, >> enum audit_state state) >> { >> @@ -1118,6 +1127,7 @@ static inline void audit_free_context(struct audit_context *context) >> audit_free_aux(context); >> kfree(context->filterkey); >> kfree(context->sockaddr); >> + audit_cmdline_free(context); >> kfree(context); >> context = previous; >> } while (context); >> @@ -1154,35 +1164,51 @@ error_path: >> >> EXPORT_SYMBOL(audit_log_task_context); >> >> -static void audit_log_add_cmdline(struct audit_buffer *ab, >> +static unsigned long audit_cmdline_get_page(struct audit_buffer *ab, > > static char* audit_cmdline_get_page(struct audit_buffer *ab, > >> struct task_struct *tsk) >> { >> int len; >> unsigned long page; >> - char *msg = "(null)"; >> - >> - audit_log_format(ab, " cmdline="); >> >> /* Get the process cmdline */ >> page = __get_free_page(GFP_TEMPORARY); >> if (!page) { >> - audit_log_untrustedstring(ab, msg); >> - return; >> + return 0; > > return NULL; > >> } >> len = proc_pid_cmdline(tsk, (char *)page); >> if (len <= 0) { >> free_page(page); >> - audit_log_untrustedstring(ab, msg); >> - return; >> + return 0; > > return NULL; > >> } >> /* >> * Ensure NULL terminated! Application could >> * could be using setproctitle(3). >> */ >> ((char *)page)[len-1] = '\0'; >> - msg = (char *)page; >> + >> + /* TODO: Re-alloc to something smaller then a page here? */ >> + return page; >> +} >> + >> +static void audit_log_cmdline(struct audit_buffer *ab, struct task_struct *tsk, >> + struct audit_context *context) >> +{ >> + char *msg = "(null)"; >> + >> + audit_log_format(ab, " cmdline="); >> + >> + /* Already cached */ >> + if (context->cmdline) { >> + msg = (char *)context->cmdline; > > msg = context->cmdline; > >> + goto out; >> + } >> + /* Not cached yet */ >> + context->cmdline = audit_cmdline_get_page(ab, tsk); >> + if (!context->cmdline) >> + goto out; >> + msg = (char *)context->cmdline; > > msg = context->cmdline; > >> +out: >> audit_log_untrustedstring(ab, msg); >> - free_page(page); >> } >> >> static void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk) >> @@ -1211,7 +1237,6 @@ static void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk >> } >> up_read(&mm->mmap_sem); >> } >> - audit_log_add_cmdline(ab, tsk); >> audit_log_task_context(ab); >> } >> >> @@ -1679,6 +1704,7 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts >> >> >> audit_log_task_info(ab, tsk); >> + audit_log_cmdline(ab, tsk, context); >> audit_log_key(ab, context->filterkey); >> audit_log_end(ab); >> >> -- >> 1.7.9.5 >> > > - RGB > > -- > Richard Guy Briggs <rbriggs(a)redhat.com&gt; > Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat > Remote, Ottawa, Canada > Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545 -- Respectfully, William C Roberts