From: Paul Moore <paul@paul-moore.com> To: rgb@redhat.com Cc: containers@lists.linux-foundation.org, linux-api@vger.kernel.org, linux-audit@redhat.com, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, netfilter-devel@vger.kernel.org, ebiederm@xmission.com, luto@kernel.org, carlos@redhat.com, dhowells@redhat.com, viro@zeniv.linux.org.uk, simo@redhat.com, Eric Paris <eparis@parisplace.org>, Serge Hallyn <serge@hallyn.com> Subject: Re: [PATCH ghak90 (was ghak32) V4 01/10] audit: collect audit task parameters Date: Fri, 19 Oct 2018 19:15:56 -0400 [thread overview] Message-ID: <CAHC9VhQFqs1SyvximK+8XJG5Fk3p9WsWhEV5sY6kksN9tc6eKw@mail.gmail.com> (raw) In-Reply-To: <8e617ab568df28a66dfbe3284452de186b42fb0f.1533065887.git.rgb@redhat.com> On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs <rgb@redhat.com> wrote: > The audit-related parameters in struct task_struct should ideally be > collected together and accessed through a standard audit API. > > Collect the existing loginuid, sessionid and audit_context together in a > new struct audit_task_info called "audit" in struct task_struct. > > Use kmem_cache to manage this pool of memory. > Un-inline audit_free() to be able to always recover that memory. > > See: https://github.com/linux-audit/audit-kernel/issues/81 > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > include/linux/audit.h | 34 ++++++++++++++++++++++++---------- > include/linux/sched.h | 5 +---- > init/init_task.c | 3 +-- > init/main.c | 2 ++ > kernel/auditsc.c | 51 ++++++++++++++++++++++++++++++++++++++++++--------- > kernel/fork.c | 4 +++- > 6 files changed, 73 insertions(+), 26 deletions(-) ... > diff --git a/include/linux/audit.h b/include/linux/audit.h > index 9334fbe..8964332 100644 > --- a/include/linux/audit.h > +++ b/include/linux/audit.h > @@ -219,8 +219,15 @@ static inline void audit_log_task_info(struct audit_buffer *ab, > > /* These are defined in auditsc.c */ > /* Public API */ > +struct audit_task_info { > + kuid_t loginuid; > + unsigned int sessionid; > + struct audit_context *ctx; > +}; ... > diff --git a/include/linux/sched.h b/include/linux/sched.h > index 87bf02d..e117272 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -873,10 +872,8 @@ struct task_struct { > > struct callback_head *task_works; > > - struct audit_context *audit_context; > #ifdef CONFIG_AUDITSYSCALL > - kuid_t loginuid; > - unsigned int sessionid; > + struct audit_task_info *audit; > #endif > struct seccomp seccomp; Prior to this patch audit_context was available regardless of CONFIG_AUDITSYSCALL, after this patch the corresponding audit_context is only available when CONFIG_AUDITSYSCALL is defined. > diff --git a/init/main.c b/init/main.c > index 3b4ada1..6aba171 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -92,6 +92,7 @@ > #include <linux/rodata_test.h> > #include <linux/jump_label.h> > #include <linux/mem_encrypt.h> > +#include <linux/audit.h> > > #include <asm/io.h> > #include <asm/bugs.h> > @@ -721,6 +722,7 @@ asmlinkage __visible void __init start_kernel(void) > nsfs_init(); > cpuset_init(); > cgroup_init(); > + audit_task_init(); > taskstats_init_early(); > delayacct_init(); It seems like we would need either init_struct_audit or audit_task_init(), but not both, yes? > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index fb20746..88779a7 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -841,7 +841,7 @@ static inline struct audit_context *audit_take_context(struct task_struct *tsk, > int return_valid, > long return_code) > { > - struct audit_context *context = tsk->audit_context; > + struct audit_context *context = tsk->audit->ctx; > > if (!context) > return NULL; > @@ -926,6 +926,15 @@ static inline struct audit_context *audit_alloc_context(enum audit_state state) > return context; > } > > +static struct kmem_cache *audit_task_cache; > + > +void __init audit_task_init(void) > +{ > + audit_task_cache = kmem_cache_create("audit_task", > + sizeof(struct audit_task_info), > + 0, SLAB_PANIC, NULL); > +} This is somewhat related to the CONFIG_AUDITSYSCALL comment above, but since the audit_task_info contains generic audit state (not just syscall related state), it seems like this, and the audit_task_info accessors/helpers, should live in kernel/audit.c. There are probably a few other things that should move to kernel/audit.c too, e.g. audit_alloc(). Have you verified that this builds/runs correctly on architectures that define CONFIG_AUDIT but not CONFIG_AUDITSYSCALL? > /** > * audit_alloc - allocate an audit context block for a task > * @tsk: task > @@ -940,17 +949,28 @@ int audit_alloc(struct task_struct *tsk) > struct audit_context *context; > enum audit_state state; > char *key = NULL; > + struct audit_task_info *info; > + > + info = kmem_cache_zalloc(audit_task_cache, GFP_KERNEL); > + if (!info) > + return -ENOMEM; > + info->loginuid = audit_get_loginuid(current); > + info->sessionid = audit_get_sessionid(current); > + tsk->audit = info; > > if (likely(!audit_ever_enabled)) > return 0; /* Return if not auditing. */ I don't view this as necessary for initial acceptance, and synchronization/locking might render this undesirable, but it would be curious to see if we could do something clever with refcnts and copy-on-write to minimize the number of kmem_cache objects in use in the !audit_ever_enabled (and possibly the AUDIT_DISABLED) case. > state = audit_filter_task(tsk, &key); > if (state == AUDIT_DISABLED) { > + audit_set_context(tsk, NULL); It's already NULL, isn't it? > clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT); > return 0; > } > > if (!(context = audit_alloc_context(state))) { > + tsk->audit = NULL; > + kmem_cache_free(audit_task_cache, info); > kfree(key); > audit_log_lost("out of memory in audit_alloc"); > return -ENOMEM; > @@ -962,6 +982,12 @@ int audit_alloc(struct task_struct *tsk) > return 0; > } > > +struct audit_task_info init_struct_audit = { > + .loginuid = INVALID_UID, > + .sessionid = AUDIT_SID_UNSET, > + .ctx = NULL, > +}; > + > static inline void audit_free_context(struct audit_context *context) > { > audit_free_names(context); -- paul moore www.paul-moore.com
WARNING: multiple messages have this Message-ID (diff)
From: Paul Moore <paul@paul-moore.com> To: rgb@redhat.com Cc: containers@lists.linux-foundation.org, linux-api@vger.kernel.org, linux-audit@redhat.com, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org, netfilter-devel@vger.kernel.org, ebiederm@xmission.com, luto@kernel.org, carlos@redhat.com, dhowells@redhat.com, viro@zeniv.linux.org.uk, simo@redhat.com, Eric Paris <eparis@parisplace.org>, Serge Hallyn <serge@hallyn.com> Subject: Re: [PATCH ghak90 (was ghak32) V4 01/10] audit: collect audit task parameters Date: Fri, 19 Oct 2018 19:15:56 -0400 [thread overview] Message-ID: <CAHC9VhQFqs1SyvximK+8XJG5Fk3p9WsWhEV5sY6kksN9tc6eKw@mail.gmail.com> (raw) In-Reply-To: <8e617ab568df28a66dfbe3284452de186b42fb0f.1533065887.git.rgb@redhat.com> On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs <rgb@redhat.com> wrote: > The audit-related parameters in struct task_struct should ideally be > collected together and accessed through a standard audit API. > > Collect the existing loginuid, sessionid and audit_context together in a > new struct audit_task_info called "audit" in struct task_struct. > > Use kmem_cache to manage this pool of memory. > Un-inline audit_free() to be able to always recover that memory. > > See: https://github.com/linux-audit/audit-kernel/issues/81 > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > include/linux/audit.h | 34 ++++++++++++++++++++++++---------- > include/linux/sched.h | 5 +---- > init/init_task.c | 3 +-- > init/main.c | 2 ++ > kernel/auditsc.c | 51 ++++++++++++++++++++++++++++++++++++++++++--------- > kernel/fork.c | 4 +++- > 6 files changed, 73 insertions(+), 26 deletions(-) ... > diff --git a/include/linux/audit.h b/include/linux/audit.h > index 9334fbe..8964332 100644 > --- a/include/linux/audit.h > +++ b/include/linux/audit.h > @@ -219,8 +219,15 @@ static inline void audit_log_task_info(struct audit_buffer *ab, > > /* These are defined in auditsc.c */ > /* Public API */ > +struct audit_task_info { > + kuid_t loginuid; > + unsigned int sessionid; > + struct audit_context *ctx; > +}; ... > diff --git a/include/linux/sched.h b/include/linux/sched.h > index 87bf02d..e117272 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -873,10 +872,8 @@ struct task_struct { > > struct callback_head *task_works; > > - struct audit_context *audit_context; > #ifdef CONFIG_AUDITSYSCALL > - kuid_t loginuid; > - unsigned int sessionid; > + struct audit_task_info *audit; > #endif > struct seccomp seccomp; Prior to this patch audit_context was available regardless of CONFIG_AUDITSYSCALL, after this patch the corresponding audit_context is only available when CONFIG_AUDITSYSCALL is defined. > diff --git a/init/main.c b/init/main.c > index 3b4ada1..6aba171 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -92,6 +92,7 @@ > #include <linux/rodata_test.h> > #include <linux/jump_label.h> > #include <linux/mem_encrypt.h> > +#include <linux/audit.h> > > #include <asm/io.h> > #include <asm/bugs.h> > @@ -721,6 +722,7 @@ asmlinkage __visible void __init start_kernel(void) > nsfs_init(); > cpuset_init(); > cgroup_init(); > + audit_task_init(); > taskstats_init_early(); > delayacct_init(); It seems like we would need either init_struct_audit or audit_task_init(), but not both, yes? > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index fb20746..88779a7 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -841,7 +841,7 @@ static inline struct audit_context *audit_take_context(struct task_struct *tsk, > int return_valid, > long return_code) > { > - struct audit_context *context = tsk->audit_context; > + struct audit_context *context = tsk->audit->ctx; > > if (!context) > return NULL; > @@ -926,6 +926,15 @@ static inline struct audit_context *audit_alloc_context(enum audit_state state) > return context; > } > > +static struct kmem_cache *audit_task_cache; > + > +void __init audit_task_init(void) > +{ > + audit_task_cache = kmem_cache_create("audit_task", > + sizeof(struct audit_task_info), > + 0, SLAB_PANIC, NULL); > +} This is somewhat related to the CONFIG_AUDITSYSCALL comment above, but since the audit_task_info contains generic audit state (not just syscall related state), it seems like this, and the audit_task_info accessors/helpers, should live in kernel/audit.c. There are probably a few other things that should move to kernel/audit.c too, e.g. audit_alloc(). Have you verified that this builds/runs correctly on architectures that define CONFIG_AUDIT but not CONFIG_AUDITSYSCALL? > /** > * audit_alloc - allocate an audit context block for a task > * @tsk: task > @@ -940,17 +949,28 @@ int audit_alloc(struct task_struct *tsk) > struct audit_context *context; > enum audit_state state; > char *key = NULL; > + struct audit_task_info *info; > + > + info = kmem_cache_zalloc(audit_task_cache, GFP_KERNEL); > + if (!info) > + return -ENOMEM; > + info->loginuid = audit_get_loginuid(current); > + info->sessionid = audit_get_sessionid(current); > + tsk->audit = info; > > if (likely(!audit_ever_enabled)) > return 0; /* Return if not auditing. */ I don't view this as necessary for initial acceptance, and synchronization/locking might render this undesirable, but it would be curious to see if we could do something clever with refcnts and copy-on-write to minimize the number of kmem_cache objects in use in the !audit_ever_enabled (and possibly the AUDIT_DISABLED) case. > state = audit_filter_task(tsk, &key); > if (state == AUDIT_DISABLED) { > + audit_set_context(tsk, NULL); It's already NULL, isn't it? > clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT); > return 0; > } > > if (!(context = audit_alloc_context(state))) { > + tsk->audit = NULL; > + kmem_cache_free(audit_task_cache, info); > kfree(key); > audit_log_lost("out of memory in audit_alloc"); > return -ENOMEM; > @@ -962,6 +982,12 @@ int audit_alloc(struct task_struct *tsk) > return 0; > } > > +struct audit_task_info init_struct_audit = { > + .loginuid = INVALID_UID, > + .sessionid = AUDIT_SID_UNSET, > + .ctx = NULL, > +}; > + > static inline void audit_free_context(struct audit_context *context) > { > audit_free_names(context);
next prev parent reply other threads:[~2018-10-19 23:16 UTC|newest] Thread overview: 72+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-07-31 20:07 [PATCH ghak90 (was ghak32) V4 00/10] audit: implement container identifier Richard Guy Briggs 2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 01/10] audit: collect audit task parameters Richard Guy Briggs 2018-10-19 23:15 ` Paul Moore [this message] 2018-10-19 23:15 ` Paul Moore 2018-11-01 22:07 ` Richard Guy Briggs 2019-01-03 20:10 ` Paul Moore 2019-01-03 20:29 ` Richard Guy Briggs 2019-01-03 20:29 ` Richard Guy Briggs 2019-01-03 20:33 ` Paul Moore 2019-01-03 20:38 ` Richard Guy Briggs 2019-01-24 20:36 ` Richard Guy Briggs 2019-01-04 2:50 ` Guenter Roeck 2019-01-04 14:57 ` Richard Guy Briggs 2019-01-04 22:04 ` Guenter Roeck 2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 02/10] audit: add container id Richard Guy Briggs 2018-07-31 20:07 ` Richard Guy Briggs 2018-08-24 16:01 ` Steve Grubb 2018-10-19 19:38 ` Paul Moore 2018-10-19 19:40 ` Paul Moore 2018-10-19 21:50 ` Richard Guy Briggs 2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 03/10] audit: log container info of syscalls Richard Guy Briggs 2018-08-24 16:01 ` Steve Grubb 2018-08-24 16:01 ` Steve Grubb 2018-10-19 23:16 ` Paul Moore 2018-10-24 15:14 ` Richard Guy Briggs 2018-10-24 20:55 ` Paul Moore 2018-10-25 0:42 ` Richard Guy Briggs 2018-10-25 6:06 ` Steve Grubb 2018-10-25 10:49 ` Paul Moore 2018-10-25 12:27 ` Richard Guy Briggs 2018-10-25 12:27 ` Richard Guy Briggs 2018-10-25 15:57 ` Steve Grubb 2018-10-25 17:38 ` Richard Guy Briggs 2018-10-25 20:40 ` Paul Moore 2018-10-25 21:55 ` Steve Grubb 2018-10-26 8:09 ` Casey Schaufler 2018-10-28 7:53 ` Paul Moore 2018-10-25 6:13 ` Paul Moore 2018-10-25 6:13 ` Paul Moore 2018-10-25 6:13 ` Paul Moore 2018-10-25 12:22 ` Richard Guy Briggs 2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 04/10] audit: add containerid support for ptrace and signals Richard Guy Briggs 2018-10-19 23:16 ` Paul Moore 2018-10-26 22:15 ` Richard Guy Briggs 2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 05/10] audit: add support for non-syscall auxiliary records Richard Guy Briggs 2018-10-19 23:17 ` Paul Moore 2018-11-01 18:48 ` Richard Guy Briggs 2019-01-03 20:10 ` Paul Moore 2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 06/10] audit: add containerid support for tty_audit Richard Guy Briggs 2018-10-19 23:17 ` Paul Moore 2018-10-31 21:17 ` Richard Guy Briggs 2019-01-03 20:11 ` Paul Moore 2019-01-10 22:58 ` Richard Guy Briggs 2019-01-11 1:12 ` Paul Moore 2019-01-11 3:38 ` Richard Guy Briggs 2019-01-11 23:16 ` Paul Moore 2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 07/10] audit: add containerid filtering Richard Guy Briggs 2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 08/10] audit: add support for containerid to network namespaces Richard Guy Briggs 2018-07-31 20:07 ` Richard Guy Briggs 2018-10-19 23:18 ` Paul Moore 2018-10-19 23:18 ` Paul Moore 2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 09/10] audit: NETFILTER_PKT: record each container ID associated with a netNS Richard Guy Briggs 2018-10-19 23:18 ` Paul Moore 2018-10-31 19:30 ` Richard Guy Briggs 2018-12-27 15:33 ` Richard Guy Briggs 2018-12-27 22:54 ` Paul Moore 2018-07-31 20:07 ` [PATCH ghak90 (was ghak32) V4 10/10] debug audit: read container ID of a process Richard Guy Briggs 2019-01-03 16:15 ` [PATCH ghak90 (was ghak32) V4 00/10] audit: implement container identifier Guenter Roeck 2019-01-03 17:36 ` Richard Guy Briggs 2019-01-03 18:58 ` Guenter Roeck 2019-01-03 20:20 ` Richard Guy Briggs 2019-01-03 20:12 ` Paul Moore
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAHC9VhQFqs1SyvximK+8XJG5Fk3p9WsWhEV5sY6kksN9tc6eKw@mail.gmail.com \ --to=paul@paul-moore.com \ --cc=carlos@redhat.com \ --cc=containers@lists.linux-foundation.org \ --cc=dhowells@redhat.com \ --cc=ebiederm@xmission.com \ --cc=eparis@parisplace.org \ --cc=linux-api@vger.kernel.org \ --cc=linux-audit@redhat.com \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=luto@kernel.org \ --cc=netdev@vger.kernel.org \ --cc=netfilter-devel@vger.kernel.org \ --cc=rgb@redhat.com \ --cc=serge@hallyn.com \ --cc=simo@redhat.com \ --cc=viro@zeniv.linux.org.uk \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.