Re: [PATCH ghak105 V2] audit: remove audit_context when CONFIG_ AUDIT and not AUDITSYSCALL

On Tue, Jan 29, 2019 at 6:18 PM Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > On 2019-01-29 18:07, Paul Moore wrote: > > On Mon, Jan 28, 2019 at 1:33 PM Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > > > Remove audit_context from struct task_struct and struct audit_buffer > > > when CONFIG_AUDIT is enabled but CONFIG_AUDITSYSCALL is not. > > > > > > Also, audit_log_name() (and supporting inode and fcaps functions) should > > > have been put back in auditsc.c when soft and hard link logging was > > > normalized since it is only used by syscall auditing. > > > > > > See github issue https://github.com/linux-audit/audit-kernel/issues/105 > > > > > > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; > > > --- > > > Changelog: > > > v2: > > > - resolve merge conflicts from rebase on upstreamed ghak103 patch > > > - wrap task_struct audit_context in CONFIG_AUDITSYSCALL > > > > > > include/linux/sched.h | 4 +- > > > kernel/audit.c | 157 +++----------------------------------------------- > > > kernel/audit.h | 9 --- > > > kernel/auditsc.c | 150 +++++++++++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 161 insertions(+), 159 deletions(-) > > > > ... > > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > > index 3f3f1888cac7..15e41603fd34 100644 > > > --- a/kernel/audit.c > > > +++ b/kernel/audit.c > > > @@ -205,7 +205,9 @@ struct audit_net { > > > * use simultaneously. */ > > > struct audit_buffer { > > > struct sk_buff *skb; /* formatted skb ready to send */ > > > +#ifdef CONFIG_AUDITSYSCALL > > > struct audit_context *ctx; /* NULL or associated context */ > > > +#endif > > > gfp_t gfp_mask; > > > }; > > > > > > @@ -1696,7 +1698,9 @@ static struct audit_buffer *audit_buffer_alloc(struct audit_context *ctx, > > > if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0)) > > > goto err; > > > > > > +#ifdef CONFIG_AUDITSYSCALL > > > ab->ctx = ctx; > > > +#endif > > > > I vaguely remember reading/hearing something in the past about > > kmem_cache_alloc() not returning a zero'd out buffer in all cases, can > > you say for certain that "ab" in this case is always going to be > > zero'd out? This is an honest question. > > Ok, then maybe we should be using kmem_cache_zalloc() instead of > kmem_cache_alloc() in audit_buffer_alloc()? (as I've done in > the last patch of ghak81/first patch of ghak90) > > If this is too much overhead, then we can initialize ctx = NULL; We don't need zalloc() since we're setting all the fields, although more on this below ...

> > If we can't guarantee that "ab" is zero'd out, we should manually set > > ab->ctx to NULL when !CONFIG_AUDITSYSCALL. > > But ctx isn't part of struct audit_buffer when !CONFIG_AUDITSYSCALL. It > is #ifdef-ed out. What am I missing? You're not, I am. I saw the obvious bit where you removed it from the task_struct, but completely glossed over the bit where you also removed it from the audit_buffer struct. My mistake. Once the audit container ID stuff lands we are going to need to have the audit_context pointer in the audit_buffer regardless of the CONFIG_AUDITSYSCALL setting, right? Assuming the answer is yes, I think I'd just assume leave the pointer in the audit_buffer (setting it to NULL when !CONFIG_AUDITSYSCALL) so we don't have to have those #ifdef's in the middle of the functions (I generally like to avoid those if possible). I think it's still worth making the changes to task_struct, as that is the right thing to do and doesn't have the same level of impact.

paul moore