| Message ID | 6abeb26e64489fc29b00c86b60b501c8b7316424.1593198710.git.rgb@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | audit: implement container identifier | expand |
On Sat, Jun 27, 2020 at 9:21 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. > > Please see the upstream github issue > https://github.com/linux-audit/audit-kernel/issues/81 > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > Acked-by: Neil Horman <nhorman@tuxdriver.com> > Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > include/linux/audit.h | 49 +++++++++++++++++++++++------------ > include/linux/sched.h | 7 +---- > init/init_task.c | 3 +-- > init/main.c | 2 ++ > kernel/audit.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++-- > kernel/audit.h | 5 ++++ > kernel/auditsc.c | 26 ++++++++++--------- > kernel/fork.c | 1 - > 8 files changed, 124 insertions(+), 40 deletions(-) > > diff --git a/include/linux/audit.h b/include/linux/audit.h > index 3fcd9ee49734..c2150415f9df 100644 > --- a/include/linux/audit.h > +++ b/include/linux/audit.h > @@ -100,6 +100,16 @@ enum audit_nfcfgop { > AUDIT_XT_OP_UNREGISTER, > }; > > +struct audit_task_info { > + kuid_t loginuid; > + unsigned int sessionid; > +#ifdef CONFIG_AUDITSYSCALL > + struct audit_context *ctx; > +#endif > +}; > + > +extern struct audit_task_info init_struct_audit; > + > extern int is_audit_feature_set(int which); > > extern int __init audit_register_class(int class, unsigned *list); ... > diff --git a/include/linux/sched.h b/include/linux/sched.h > index b62e6aaf28f0..2213ac670386 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -34,7 +34,6 @@ > #include <linux/kcsan.h> > > /* task_struct member predeclarations (sorted alphabetically): */ > -struct audit_context; > struct backing_dev_info; > struct bio_list; > struct blk_plug; > @@ -946,11 +945,7 @@ struct task_struct { > struct callback_head *task_works; > > #ifdef CONFIG_AUDIT > -#ifdef CONFIG_AUDITSYSCALL > - struct audit_context *audit_context; > -#endif > - kuid_t loginuid; > - unsigned int sessionid; > + struct audit_task_info *audit; > #endif > struct seccomp seccomp; In the early days of this patchset we talked a lot about how to handle the task_struct and the changes that would be necessary, ultimately deciding that encapsulating all of the audit fields into an audit_task_info struct. However, what is puzzling me a bit at this moment is why we are only including audit_task_info in task_info by reference *and* making it a build time conditional (via CONFIG_AUDIT). If audit is enabled at build time it would seem that we are always going to allocate an audit_task_info struct, so I have to wonder why we don't simply embed it inside the task_info struct (similar to the seccomp struct in the snippet above? Of course the audit_context struct needs to remain as is, I'm talking only about the task_info/audit_task_info struct. Richard, I'm sure you can answer this off the top of your head, but I'd have to go digging through the archives to pull out the relevant discussions so I figured I would just ask you for a reminder ... ? I imagine it's also possible things have changed a bit since those early discussions and the solution we arrived at then no longer makes as much sense as it did before. > diff --git a/init/init_task.c b/init/init_task.c > index 15089d15010a..92d34c4b7702 100644 > --- a/init/init_task.c > +++ b/init/init_task.c > @@ -130,8 +130,7 @@ struct task_struct init_task > .thread_group = LIST_HEAD_INIT(init_task.thread_group), > .thread_node = LIST_HEAD_INIT(init_signals.thread_head), > #ifdef CONFIG_AUDIT > - .loginuid = INVALID_UID, > - .sessionid = AUDIT_SID_UNSET, > + .audit = &init_struct_audit, > #endif > #ifdef CONFIG_PERF_EVENTS > .perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex), > diff --git a/init/main.c b/init/main.c > index 0ead83e86b5a..349470ad7458 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -96,6 +96,7 @@ > #include <linux/jump_label.h> > #include <linux/mem_encrypt.h> > #include <linux/kcsan.h> > +#include <linux/audit.h> > > #include <asm/io.h> > #include <asm/bugs.h> > @@ -1028,6 +1029,7 @@ asmlinkage __visible void __init start_kernel(void) > nsfs_init(); > cpuset_init(); > cgroup_init(); > + audit_task_init(); > taskstats_init_early(); > delayacct_init(); > > diff --git a/kernel/audit.c b/kernel/audit.c > index 8c201f414226..5d8147a29291 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -203,6 +203,73 @@ struct audit_reply { > struct sk_buff *skb; > }; > > +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); > +} > + > +/** > + * audit_alloc - allocate an audit info block for a task > + * @tsk: task > + * > + * Call audit_alloc_syscall to filter on the task information and > + * allocate a per-task audit context if necessary. This is called from > + * copy_process, so no lock is needed. > + */ > +int audit_alloc(struct task_struct *tsk) > +{ > + int ret = 0; > + struct audit_task_info *info; > + > + info = kmem_cache_alloc(audit_task_cache, GFP_KERNEL); > + if (!info) { > + ret = -ENOMEM; > + goto out; > + } > + info->loginuid = audit_get_loginuid(current); > + info->sessionid = audit_get_sessionid(current); > + tsk->audit = info; > + > + ret = audit_alloc_syscall(tsk); > + if (ret) { > + tsk->audit = NULL; > + kmem_cache_free(audit_task_cache, info); > + } > +out: > + return ret; > +} This is a big nitpick, and I'm only mentioning this in the case you need to respin this patchset: the "out" label is unnecessary in the function above. Simply return the error code, there is no need to jump to "out" only to immediately return the error code there and nothing more. > +struct audit_task_info init_struct_audit = { > + .loginuid = INVALID_UID, > + .sessionid = AUDIT_SID_UNSET, > +#ifdef CONFIG_AUDITSYSCALL > + .ctx = NULL, > +#endif > +}; > + > +/** > + * audit_free - free per-task audit info > + * @tsk: task whose audit info block to free > + * > + * Called from copy_process and do_exit > + */ > +void audit_free(struct task_struct *tsk) > +{ > + struct audit_task_info *info = tsk->audit; > + > + audit_free_syscall(tsk); > + /* Freeing the audit_task_info struct must be performed after > + * audit_log_exit() due to need for loginuid and sessionid. > + */ > + info = tsk->audit; > + tsk->audit = NULL; > + kmem_cache_free(audit_task_cache, info); Another nitpick, and this one may even become a moot point given the question posed above. However, is there any reason we couldn't get rid of "info" and simplify this a bit? audit_free_syscall(tsk); kmem_cache_free(audit_task_cache, tsk->audit); tsk->audit = NULL; > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index 468a23390457..f00c1da587ea 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -1612,7 +1615,6 @@ void __audit_free(struct task_struct *tsk) > if (context->current_state == AUDIT_RECORD_CONTEXT) > audit_log_exit(); > } > - > audit_set_context(tsk, NULL); > audit_free_context(context); > } This nitpick is barely worth the time it is taking me to write this, but the whitespace change above isn't strictly necessary. -- paul moore www.paul-moore.com
On 2020-07-05 11:09, Paul Moore wrote: > On Sat, Jun 27, 2020 at 9:21 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. > > > > Please see the upstream github issue > > https://github.com/linux-audit/audit-kernel/issues/81 > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > Acked-by: Neil Horman <nhorman@tuxdriver.com> > > Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com> > > --- > > include/linux/audit.h | 49 +++++++++++++++++++++++------------ > > include/linux/sched.h | 7 +---- > > init/init_task.c | 3 +-- > > init/main.c | 2 ++ > > kernel/audit.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++-- > > kernel/audit.h | 5 ++++ > > kernel/auditsc.c | 26 ++++++++++--------- > > kernel/fork.c | 1 - > > 8 files changed, 124 insertions(+), 40 deletions(-) > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > index 3fcd9ee49734..c2150415f9df 100644 > > --- a/include/linux/audit.h > > +++ b/include/linux/audit.h > > @@ -100,6 +100,16 @@ enum audit_nfcfgop { > > AUDIT_XT_OP_UNREGISTER, > > }; > > > > +struct audit_task_info { > > + kuid_t loginuid; > > + unsigned int sessionid; > > +#ifdef CONFIG_AUDITSYSCALL > > + struct audit_context *ctx; > > +#endif > > +}; > > + > > +extern struct audit_task_info init_struct_audit; > > + > > extern int is_audit_feature_set(int which); > > > > extern int __init audit_register_class(int class, unsigned *list); > > ... > > > diff --git a/include/linux/sched.h b/include/linux/sched.h > > index b62e6aaf28f0..2213ac670386 100644 > > --- a/include/linux/sched.h > > +++ b/include/linux/sched.h > > @@ -34,7 +34,6 @@ > > #include <linux/kcsan.h> > > > > /* task_struct member predeclarations (sorted alphabetically): */ > > -struct audit_context; > > struct backing_dev_info; > > struct bio_list; > > struct blk_plug; > > @@ -946,11 +945,7 @@ struct task_struct { > > struct callback_head *task_works; > > > > #ifdef CONFIG_AUDIT > > -#ifdef CONFIG_AUDITSYSCALL > > - struct audit_context *audit_context; > > -#endif > > - kuid_t loginuid; > > - unsigned int sessionid; > > + struct audit_task_info *audit; > > #endif > > struct seccomp seccomp; > > In the early days of this patchset we talked a lot about how to handle > the task_struct and the changes that would be necessary, ultimately > deciding that encapsulating all of the audit fields into an > audit_task_info struct. However, what is puzzling me a bit at this > moment is why we are only including audit_task_info in task_info by > reference *and* making it a build time conditional (via CONFIG_AUDIT). > > If audit is enabled at build time it would seem that we are always > going to allocate an audit_task_info struct, so I have to wonder why > we don't simply embed it inside the task_info struct (similar to the > seccomp struct in the snippet above? Of course the audit_context > struct needs to remain as is, I'm talking only about the > task_info/audit_task_info struct. I agree that including the audit_task_info struct in the struct task_struct would have been preferred to simplify allocation and free, but the reason it was included by reference instead was to make the task_struct size independent of audit so that future changes would not cause as many kABI challenges. This first change will cause kABI challenges regardless, but it was future ones that we were trying to ease. Does that match with your recollection? > Richard, I'm sure you can answer this off the top of your head, but > I'd have to go digging through the archives to pull out the relevant > discussions so I figured I would just ask you for a reminder ... ? I > imagine it's also possible things have changed a bit since those early > discussions and the solution we arrived at then no longer makes as > much sense as it did before. Agreed, it doesn't make as much sense now as it did when proposed, but will make more sense in the future depending on when this change gets accepted upstream. This is why I wanted this patch to go through as part of ghak81 at the time the rest of it did so that future kABI issues would be easier to handle, but that ship has long sailed. I didn't make that argument then and I regret it now that I realize and recall some of the thinking behind the change. Your reasons at the time were that contid was the only user of that change but there have been some CONFIG_AUDIT and CONFIG_AUDITSYSCALL changes since that were related. > > diff --git a/init/init_task.c b/init/init_task.c > > index 15089d15010a..92d34c4b7702 100644 > > --- a/init/init_task.c > > +++ b/init/init_task.c > > @@ -130,8 +130,7 @@ struct task_struct init_task > > .thread_group = LIST_HEAD_INIT(init_task.thread_group), > > .thread_node = LIST_HEAD_INIT(init_signals.thread_head), > > #ifdef CONFIG_AUDIT > > - .loginuid = INVALID_UID, > > - .sessionid = AUDIT_SID_UNSET, > > + .audit = &init_struct_audit, > > #endif > > #ifdef CONFIG_PERF_EVENTS > > .perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex), > > diff --git a/init/main.c b/init/main.c > > index 0ead83e86b5a..349470ad7458 100644 > > --- a/init/main.c > > +++ b/init/main.c > > @@ -96,6 +96,7 @@ > > #include <linux/jump_label.h> > > #include <linux/mem_encrypt.h> > > #include <linux/kcsan.h> > > +#include <linux/audit.h> > > > > #include <asm/io.h> > > #include <asm/bugs.h> > > @@ -1028,6 +1029,7 @@ asmlinkage __visible void __init start_kernel(void) > > nsfs_init(); > > cpuset_init(); > > cgroup_init(); > > + audit_task_init(); > > taskstats_init_early(); > > delayacct_init(); > > > > diff --git a/kernel/audit.c b/kernel/audit.c > > index 8c201f414226..5d8147a29291 100644 > > --- a/kernel/audit.c > > +++ b/kernel/audit.c > > @@ -203,6 +203,73 @@ struct audit_reply { > > struct sk_buff *skb; > > }; > > > > +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); > > +} > > + > > +/** > > + * audit_alloc - allocate an audit info block for a task > > + * @tsk: task > > + * > > + * Call audit_alloc_syscall to filter on the task information and > > + * allocate a per-task audit context if necessary. This is called from > > + * copy_process, so no lock is needed. > > + */ > > +int audit_alloc(struct task_struct *tsk) > > +{ > > + int ret = 0; > > + struct audit_task_info *info; > > + > > + info = kmem_cache_alloc(audit_task_cache, GFP_KERNEL); > > + if (!info) { > > + ret = -ENOMEM; > > + goto out; > > + } > > + info->loginuid = audit_get_loginuid(current); > > + info->sessionid = audit_get_sessionid(current); > > + tsk->audit = info; > > + > > + ret = audit_alloc_syscall(tsk); > > + if (ret) { > > + tsk->audit = NULL; > > + kmem_cache_free(audit_task_cache, info); > > + } > > +out: > > + return ret; > > +} > > This is a big nitpick, and I'm only mentioning this in the case you > need to respin this patchset: the "out" label is unnecessary in the > function above. Simply return the error code, there is no need to > jump to "out" only to immediately return the error code there and > nothing more. Agreed. This must have been due to some restructuring that no longer needed an exit cleanup action. > > +struct audit_task_info init_struct_audit = { > > + .loginuid = INVALID_UID, > > + .sessionid = AUDIT_SID_UNSET, > > +#ifdef CONFIG_AUDITSYSCALL > > + .ctx = NULL, > > +#endif > > +}; > > + > > +/** > > + * audit_free - free per-task audit info > > + * @tsk: task whose audit info block to free > > + * > > + * Called from copy_process and do_exit > > + */ > > +void audit_free(struct task_struct *tsk) > > +{ > > + struct audit_task_info *info = tsk->audit; > > + > > + audit_free_syscall(tsk); > > + /* Freeing the audit_task_info struct must be performed after > > + * audit_log_exit() due to need for loginuid and sessionid. > > + */ > > + info = tsk->audit; > > + tsk->audit = NULL; > > + kmem_cache_free(audit_task_cache, info); > > Another nitpick, and this one may even become a moot point given the > question posed above. However, is there any reason we couldn't get > rid of "info" and simplify this a bit? That info allocation and assignment does now seem pointless, I agree... > audit_free_syscall(tsk); > kmem_cache_free(audit_task_cache, tsk->audit); > tsk->audit = NULL; > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index 468a23390457..f00c1da587ea 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -1612,7 +1615,6 @@ void __audit_free(struct task_struct *tsk) > > if (context->current_state == AUDIT_RECORD_CONTEXT) > > audit_log_exit(); > > } > > - > > audit_set_context(tsk, NULL); > > audit_free_context(context); > > } > > This nitpick is barely worth the time it is taking me to write this, > but the whitespace change above isn't strictly necessary. Sure, it is a harmless but noisy cleanup when the function was being cleaned up and renamed. It wasn't an accident, but a style preference. Do you prefer a vertical space before cleanup actions at the end of functions and more versus less vertical whitespace in general? > paul moore - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On Mon, Jul 6, 2020 at 10:50 PM Richard Guy Briggs <rgb@redhat.com> wrote: > On 2020-07-05 11:09, Paul Moore wrote: > > On Sat, Jun 27, 2020 at 9:21 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. > > > > > > Please see the upstream github issue > > > https://github.com/linux-audit/audit-kernel/issues/81 > > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > Acked-by: Neil Horman <nhorman@tuxdriver.com> > > > Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com> > > > --- > > > include/linux/audit.h | 49 +++++++++++++++++++++++------------ > > > include/linux/sched.h | 7 +---- > > > init/init_task.c | 3 +-- > > > init/main.c | 2 ++ > > > kernel/audit.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++-- > > > kernel/audit.h | 5 ++++ > > > kernel/auditsc.c | 26 ++++++++++--------- > > > kernel/fork.c | 1 - > > > 8 files changed, 124 insertions(+), 40 deletions(-) > > > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > > index 3fcd9ee49734..c2150415f9df 100644 > > > --- a/include/linux/audit.h > > > +++ b/include/linux/audit.h > > > @@ -100,6 +100,16 @@ enum audit_nfcfgop { > > > AUDIT_XT_OP_UNREGISTER, > > > }; > > > > > > +struct audit_task_info { > > > + kuid_t loginuid; > > > + unsigned int sessionid; > > > +#ifdef CONFIG_AUDITSYSCALL > > > + struct audit_context *ctx; > > > +#endif > > > +}; > > > + > > > +extern struct audit_task_info init_struct_audit; > > > + > > > extern int is_audit_feature_set(int which); > > > > > > extern int __init audit_register_class(int class, unsigned *list); > > > > ... > > > > > diff --git a/include/linux/sched.h b/include/linux/sched.h > > > index b62e6aaf28f0..2213ac670386 100644 > > > --- a/include/linux/sched.h > > > +++ b/include/linux/sched.h > > > @@ -34,7 +34,6 @@ > > > #include <linux/kcsan.h> > > > > > > /* task_struct member predeclarations (sorted alphabetically): */ > > > -struct audit_context; > > > struct backing_dev_info; > > > struct bio_list; > > > struct blk_plug; > > > @@ -946,11 +945,7 @@ struct task_struct { > > > struct callback_head *task_works; > > > > > > #ifdef CONFIG_AUDIT > > > -#ifdef CONFIG_AUDITSYSCALL > > > - struct audit_context *audit_context; > > > -#endif > > > - kuid_t loginuid; > > > - unsigned int sessionid; > > > + struct audit_task_info *audit; > > > #endif > > > struct seccomp seccomp; > > > > In the early days of this patchset we talked a lot about how to handle > > the task_struct and the changes that would be necessary, ultimately > > deciding that encapsulating all of the audit fields into an > > audit_task_info struct. However, what is puzzling me a bit at this > > moment is why we are only including audit_task_info in task_info by > > reference *and* making it a build time conditional (via CONFIG_AUDIT). > > > > If audit is enabled at build time it would seem that we are always > > going to allocate an audit_task_info struct, so I have to wonder why > > we don't simply embed it inside the task_info struct (similar to the > > seccomp struct in the snippet above? Of course the audit_context > > struct needs to remain as is, I'm talking only about the > > task_info/audit_task_info struct. > > I agree that including the audit_task_info struct in the struct > task_struct would have been preferred to simplify allocation and free, > but the reason it was included by reference instead was to make the > task_struct size independent of audit so that future changes would not > cause as many kABI challenges. This first change will cause kABI > challenges regardless, but it was future ones that we were trying to > ease. > > Does that match with your recollection? I guess, sure. I suppose what I was really asking was if we had a "good" reason for not embedding the audit_task_info struct. Regardless, thanks for the explanation, that was helpful. From an upstream perspective, I think embedding the audit_task_info struct is the Right Thing To Do. The code is cleaner and more robust if we embed the struct. > > Richard, I'm sure you can answer this off the top of your head, but > > I'd have to go digging through the archives to pull out the relevant > > discussions so I figured I would just ask you for a reminder ... ? I > > imagine it's also possible things have changed a bit since those early > > discussions and the solution we arrived at then no longer makes as > > much sense as it did before. > > Agreed, it doesn't make as much sense now as it did when proposed, but > will make more sense in the future depending on when this change gets > accepted upstream. This is why I wanted this patch to go through as > part of ghak81 at the time the rest of it did so that future kABI issues > would be easier to handle, but that ship has long sailed. To be clear, kABI issues with task_struct really aren't an issue with the upstream kernel. I know that you know all of this already Richard, I'm mostly talking to everyone else on the To/CC line in case they are casually watching this discussion. While I'm sympathetic to long-lifetime enterprise distros such as RHEL, my responsibility is to ensure the upstream kernel is as good as we can make it, and in this case I believe that means embedding audit_task_info into the task_struct. > I didn't make > that argument then and I regret it now that I realize and recall some of > the thinking behind the change. Your reasons at the time were that > contid was the only user of that change but there have been some > CONFIG_AUDIT and CONFIG_AUDITSYSCALL changes since that were related. Agreed that there are probably some common goals and benefits with those changes and the audit container ID work, however, I believe that discussion quickly goes back to upstream vs RHEL. > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > > index 468a23390457..f00c1da587ea 100644 > > > --- a/kernel/auditsc.c > > > +++ b/kernel/auditsc.c > > > @@ -1612,7 +1615,6 @@ void __audit_free(struct task_struct *tsk) > > > if (context->current_state == AUDIT_RECORD_CONTEXT) > > > audit_log_exit(); > > > } > > > - > > > audit_set_context(tsk, NULL); > > > audit_free_context(context); > > > } > > > > This nitpick is barely worth the time it is taking me to write this, > > but the whitespace change above isn't strictly necessary. > > Sure, it is a harmless but noisy cleanup when the function was being > cleaned up and renamed. It wasn't an accident, but a style preference. > Do you prefer a vertical space before cleanup actions at the end of > functions and more versus less vertical whitespace in general? As I mentioned above, this really was barely worth mentioning, but I made the comment simply because I feel this patchset is going to draw a lot of attention once it is merged and I feel keeping the patchset as small, and as focused, as possible is a good thing. However, I'm not going to lose even a second of sleep over a single blank line gone missing ;)
On 2020-07-07 21:42, Paul Moore wrote: > On Mon, Jul 6, 2020 at 10:50 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > On 2020-07-05 11:09, Paul Moore wrote: > > > On Sat, Jun 27, 2020 at 9:21 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. > > > > > > > > Please see the upstream github issue > > > > https://github.com/linux-audit/audit-kernel/issues/81 > > > > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > > > Acked-by: Neil Horman <nhorman@tuxdriver.com> > > > > Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com> > > > > --- > > > > include/linux/audit.h | 49 +++++++++++++++++++++++------------ > > > > include/linux/sched.h | 7 +---- > > > > init/init_task.c | 3 +-- > > > > init/main.c | 2 ++ > > > > kernel/audit.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++-- > > > > kernel/audit.h | 5 ++++ > > > > kernel/auditsc.c | 26 ++++++++++--------- > > > > kernel/fork.c | 1 - > > > > 8 files changed, 124 insertions(+), 40 deletions(-) > > > > > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > > > index 3fcd9ee49734..c2150415f9df 100644 > > > > --- a/include/linux/audit.h > > > > +++ b/include/linux/audit.h > > > > @@ -100,6 +100,16 @@ enum audit_nfcfgop { > > > > AUDIT_XT_OP_UNREGISTER, > > > > }; > > > > > > > > +struct audit_task_info { > > > > + kuid_t loginuid; > > > > + unsigned int sessionid; > > > > +#ifdef CONFIG_AUDITSYSCALL > > > > + struct audit_context *ctx; > > > > +#endif > > > > +}; > > > > + > > > > +extern struct audit_task_info init_struct_audit; > > > > + > > > > extern int is_audit_feature_set(int which); > > > > > > > > extern int __init audit_register_class(int class, unsigned *list); > > > > > > ... > > > > > > > diff --git a/include/linux/sched.h b/include/linux/sched.h > > > > index b62e6aaf28f0..2213ac670386 100644 > > > > --- a/include/linux/sched.h > > > > +++ b/include/linux/sched.h > > > > @@ -34,7 +34,6 @@ > > > > #include <linux/kcsan.h> > > > > > > > > /* task_struct member predeclarations (sorted alphabetically): */ > > > > -struct audit_context; > > > > struct backing_dev_info; > > > > struct bio_list; > > > > struct blk_plug; > > > > @@ -946,11 +945,7 @@ struct task_struct { > > > > struct callback_head *task_works; > > > > > > > > #ifdef CONFIG_AUDIT > > > > -#ifdef CONFIG_AUDITSYSCALL > > > > - struct audit_context *audit_context; > > > > -#endif > > > > - kuid_t loginuid; > > > > - unsigned int sessionid; > > > > + struct audit_task_info *audit; > > > > #endif > > > > struct seccomp seccomp; > > > > > > In the early days of this patchset we talked a lot about how to handle > > > the task_struct and the changes that would be necessary, ultimately > > > deciding that encapsulating all of the audit fields into an > > > audit_task_info struct. However, what is puzzling me a bit at this > > > moment is why we are only including audit_task_info in task_info by > > > reference *and* making it a build time conditional (via CONFIG_AUDIT). > > > > > > If audit is enabled at build time it would seem that we are always > > > going to allocate an audit_task_info struct, so I have to wonder why > > > we don't simply embed it inside the task_info struct (similar to the > > > seccomp struct in the snippet above? Of course the audit_context > > > struct needs to remain as is, I'm talking only about the > > > task_info/audit_task_info struct. > > > > I agree that including the audit_task_info struct in the struct > > task_struct would have been preferred to simplify allocation and free, > > but the reason it was included by reference instead was to make the > > task_struct size independent of audit so that future changes would not > > cause as many kABI challenges. This first change will cause kABI > > challenges regardless, but it was future ones that we were trying to > > ease. > > > > Does that match with your recollection? > > I guess, sure. I suppose what I was really asking was if we had a > "good" reason for not embedding the audit_task_info struct. > Regardless, thanks for the explanation, that was helpful. Making it dynamic was actually your idea back in the spring of 2018: https://lkml.org/lkml/2018/4/18/759 The first two iterations were embedded to more quickly prove the idea: https://lkml.org/lkml/2018/5/12/173 https://lkml.org/lkml/2018/5/12/168 And then switched as strongly recommended to a dynamic pointer: https://lkml.org/lkml/2018/5/16/461 https://lkml.org/lkml/2018/5/16/457 I was initially concerned about switching to a dynamically allocated structure, but those concerns are a couple of years behind us. What significant change has happenned since then to alter your perspective? > From an upstream perspective, I think embedding the audit_task_info > struct is the Right Thing To Do. The code is cleaner and more robust > if we embed the struct. I would agree if the audit subsystem were done. It isn't. > > > Richard, I'm sure you can answer this off the top of your head, but > > > I'd have to go digging through the archives to pull out the relevant > > > discussions so I figured I would just ask you for a reminder ... ? I > > > imagine it's also possible things have changed a bit since those early > > > discussions and the solution we arrived at then no longer makes as > > > much sense as it did before. > > > > Agreed, it doesn't make as much sense now as it did when proposed, but > > will make more sense in the future depending on when this change gets > > accepted upstream. This is why I wanted this patch to go through as > > part of ghak81 at the time the rest of it did so that future kABI issues > > would be easier to handle, but that ship has long sailed. > > To be clear, kABI issues with task_struct really aren't an issue with > the upstream kernel. I know that you know all of this already > Richard, I'm mostly talking to everyone else on the To/CC line in case > they are casually watching this discussion. kABI issues may not as much of an upstream issue, but part of the goal here was upstream kernel issues, isolating the kernel audit changes to its own subsystem and affect struct task_struct as little as possible in the future and to protect it from "abuse" (as you had expressed serious concerns) from the rest of the kernel. include/linux/sched.h will need to know more about struct audit_task_info if it is embedded, making it more suceptible to abuse. > While I'm sympathetic to long-lifetime enterprise distros such as > RHEL, my responsibility is to ensure the upstream kernel is as good as > we can make it, and in this case I believe that means embedding > audit_task_info into the task_struct. Keeping audit_task_info dynamic will also make embedding struct audit_context as a zero-length array at the end of it possible in the future as an internal audit subsystem optimization whereas largely preclude that if it were embedded. Any change to audit_task_info in the future will change struct task_struct which is what we had agreed was a good thing to avoid to keep audit as isolated and independent as possible. This method has been well exercised over the last two years of development, testing and rebases, so I'm not particularly concerned about its dynamic nature any more. It works well. At this point this change seems to be more gratuitously disruptive than helpful. > > I didn't make > > that argument then and I regret it now that I realize and recall some of > > the thinking behind the change. Your reasons at the time were that > > contid was the only user of that change but there have been some > > CONFIG_AUDIT and CONFIG_AUDITSYSCALL changes since that were related. > > Agreed that there are probably some common goals and benefits with > those changes and the audit container ID work, however, I believe that > discussion quickly goes back to upstream vs RHEL. I did't think things were quite so cut and dried with respect to upstream vs downstream. > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > > > index 468a23390457..f00c1da587ea 100644 > > > > --- a/kernel/auditsc.c > > > > +++ b/kernel/auditsc.c > > > > @@ -1612,7 +1615,6 @@ void __audit_free(struct task_struct *tsk) > > > > if (context->current_state == AUDIT_RECORD_CONTEXT) > > > > audit_log_exit(); > > > > } > > > > - > > > > audit_set_context(tsk, NULL); > > > > audit_free_context(context); > > > > } > > > > > > This nitpick is barely worth the time it is taking me to write this, > > > but the whitespace change above isn't strictly necessary. > > > > Sure, it is a harmless but noisy cleanup when the function was being > > cleaned up and renamed. It wasn't an accident, but a style preference. > > Do you prefer a vertical space before cleanup actions at the end of > > functions and more versus less vertical whitespace in general? > > As I mentioned above, this really was barely worth mentioning, but I > made the comment simply because I feel this patchset is going to draw > a lot of attention once it is merged and I feel keeping the patchset > as small, and as focused, as possible is a good thing. Is this concern also affecting the perspective on the change from pointer to embedded above? > However, I'm not going to lose even a second of sleep over a single > blank line gone missing ;) > > paul moore - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
On Mon, Jul 13, 2020 at 4:30 PM Richard Guy Briggs <rgb@redhat.com> wrote: > On 2020-07-07 21:42, Paul Moore wrote: > > On Mon, Jul 6, 2020 at 10:50 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > On 2020-07-05 11:09, Paul Moore wrote: > > > > On Sat, Jun 27, 2020 at 9:21 AM Richard Guy Briggs <rgb@redhat.com> wrote: ... > > > > In the early days of this patchset we talked a lot about how to handle > > > > the task_struct and the changes that would be necessary, ultimately > > > > deciding that encapsulating all of the audit fields into an > > > > audit_task_info struct. However, what is puzzling me a bit at this > > > > moment is why we are only including audit_task_info in task_info by > > > > reference *and* making it a build time conditional (via CONFIG_AUDIT). > > > > > > > > If audit is enabled at build time it would seem that we are always > > > > going to allocate an audit_task_info struct, so I have to wonder why > > > > we don't simply embed it inside the task_info struct (similar to the > > > > seccomp struct in the snippet above? Of course the audit_context > > > > struct needs to remain as is, I'm talking only about the > > > > task_info/audit_task_info struct. > > > > > > I agree that including the audit_task_info struct in the struct > > > task_struct would have been preferred to simplify allocation and free, > > > but the reason it was included by reference instead was to make the > > > task_struct size independent of audit so that future changes would not > > > cause as many kABI challenges. This first change will cause kABI > > > challenges regardless, but it was future ones that we were trying to > > > ease. > > > > > > Does that match with your recollection? > > > > I guess, sure. I suppose what I was really asking was if we had a > > "good" reason for not embedding the audit_task_info struct. > > Regardless, thanks for the explanation, that was helpful. > > Making it dynamic was actually your idea back in the spring of 2018: > https://lkml.org/lkml/2018/4/18/759 If you read my comments from 2018 carefully, or even not so carefully I think, you'll notice that my primary motivation for using a pointer was to "hide" the audit_task_info struct contents so that they couldn't be abused by other kernel subsystems looking for a general container identifier inside the kernel. As we've discussed many times before, this patchset is not a general purpose container identifier, this is an ***audit*** container ID; limiting the scope and usage of this identifier is what has allowed us to gain the begrudging acceptance we've had thus far and I believe it is the key to success. For whatever it is worth, this patchset doesn't hide the audit_task_struct definition in a kernel/audit*.c file, it lives in a header file which is easily accessed by other subsystems. In my opinion we should pick one of two options: leave it as a pointer reference and "hide" the struct definition, or just embed the struct and simplify the code. I see little value in openly defining the audit_task_info struct and using a pointer reference; if you believe you have a valid argument for why this makes sense I'm open to hearing it, but your comments thus far have been unconvincing. > > > > Richard, I'm sure you can answer this off the top of your head, but > > > > I'd have to go digging through the archives to pull out the relevant > > > > discussions so I figured I would just ask you for a reminder ... ? I > > > > imagine it's also possible things have changed a bit since those early > > > > discussions and the solution we arrived at then no longer makes as > > > > much sense as it did before. > > > > > > Agreed, it doesn't make as much sense now as it did when proposed, but > > > will make more sense in the future depending on when this change gets > > > accepted upstream. This is why I wanted this patch to go through as > > > part of ghak81 at the time the rest of it did so that future kABI issues > > > would be easier to handle, but that ship has long sailed. > > > > To be clear, kABI issues with task_struct really aren't an issue with > > the upstream kernel. I know that you know all of this already > > Richard, I'm mostly talking to everyone else on the To/CC line in case > > they are casually watching this discussion. > > kABI issues may not as much of an upstream issue, but part of the goal > here was upstream kernel issues, isolating the kernel audit changes > to its own subsystem and affect struct task_struct as little as possible > in the future and to protect it from "abuse" (as you had expressed > serious concerns) from the rest of the kernel. include/linux/sched.h > will need to know more about struct audit_task_info if it is embedded, > making it more suceptible to abuse. I define "abuse" in this context as other kernel subsystems inspecting the contents of the audit_task_struct, most likely to try and approximate a general container identifier. Better separation between the audit subsystem and the task_struct, while conceptually nice, isn't critical and is easily changed upstream with each kernel release as it isn't part of the kernel/userspace API. Regardless, a basic conceptual separation is achieved by the audit_task_struct regardless of if it is embedded into the task_struct or included by a pointer reference. > > While I'm sympathetic to long-lifetime enterprise distros such as > > RHEL, my responsibility is to ensure the upstream kernel is as good as > > we can make it, and in this case I believe that means embedding > > audit_task_info into the task_struct. > > Keeping audit_task_info dynamic will also make embedding struct > audit_context as a zero-length array at the end of it possible in the > future as an internal audit subsystem optimization whereas largely > preclude that if it were embedded. Predicting the future is hard, but I would be comfortable giving up on a variable length audit_task_info struct. Besides, if we *really* had to do that in the future we could, it's not part of the kernel/userspace API. > This method has been well exercised over the last two years of > development, testing and rebases, so I'm not particularly concerned > about its dynamic nature any more. It works well. At this point this > change seems to be more gratuitously disruptive than helpful. It may not seem like it, but at this point in this patchset's life I do try to limit my comments to only those things which I feel are substantive. In the cases where I think something is borderline I'll mention that in my comments. The trivial cases I'll generally call out as "nitpicks". I assure you my comments are not gratuitous. I look forward to reviewing another round of this patchset about as much as I expect you look forward to writing, testing, and submitting it. > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > > > > index 468a23390457..f00c1da587ea 100644 > > > > > --- a/kernel/auditsc.c > > > > > +++ b/kernel/auditsc.c > > > > > @@ -1612,7 +1615,6 @@ void __audit_free(struct task_struct *tsk) > > > > > if (context->current_state == AUDIT_RECORD_CONTEXT) > > > > > audit_log_exit(); > > > > > } > > > > > - > > > > > audit_set_context(tsk, NULL); > > > > > audit_free_context(context); > > > > > } > > > > > > > > This nitpick is barely worth the time it is taking me to write this, > > > > but the whitespace change above isn't strictly necessary. > > > > > > Sure, it is a harmless but noisy cleanup when the function was being > > > cleaned up and renamed. It wasn't an accident, but a style preference. > > > Do you prefer a vertical space before cleanup actions at the end of > > > functions and more versus less vertical whitespace in general? > > > > As I mentioned above, this really was barely worth mentioning, but I > > made the comment simply because I feel this patchset is going to draw > > a lot of attention once it is merged and I feel keeping the patchset > > as small, and as focused, as possible is a good thing. > > Is this concern also affecting the perspective on the change from > pointer to embedded above? Keeping this particular patchset small and focused has always been a goal; I know we talked about this at least once, likely more than that, while I was still at RH and we were talking offline. If something is going to be contentious, it is better to be small and focused on the contention.
diff --git a/include/linux/audit.h b/include/linux/audit.h index 3fcd9ee49734..c2150415f9df 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -100,6 +100,16 @@ enum audit_nfcfgop { AUDIT_XT_OP_UNREGISTER, }; +struct audit_task_info { + kuid_t loginuid; + unsigned int sessionid; +#ifdef CONFIG_AUDITSYSCALL + struct audit_context *ctx; +#endif +}; + +extern struct audit_task_info init_struct_audit; + extern int is_audit_feature_set(int which); extern int __init audit_register_class(int class, unsigned *list); @@ -136,6 +146,9 @@ enum audit_nfcfgop { #ifdef CONFIG_AUDIT /* These are defined in audit.c */ /* Public API */ +extern int audit_alloc(struct task_struct *task); +extern void audit_free(struct task_struct *task); +extern void __init audit_task_init(void); extern __printf(4, 5) void audit_log(struct audit_context *ctx, gfp_t gfp_mask, int type, const char *fmt, ...); @@ -179,12 +192,16 @@ extern void audit_log_path_denied(int type, static inline kuid_t audit_get_loginuid(struct task_struct *tsk) { - return tsk->loginuid; + if (!tsk->audit) + return INVALID_UID; + return tsk->audit->loginuid; } static inline unsigned int audit_get_sessionid(struct task_struct *tsk) { - return tsk->sessionid; + if (!tsk->audit) + return AUDIT_SID_UNSET; + return tsk->audit->sessionid; } extern u32 audit_enabled; @@ -192,6 +209,14 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk) extern int audit_signal_info(int sig, struct task_struct *t); #else /* CONFIG_AUDIT */ +static inline int audit_alloc(struct task_struct *task) +{ + return 0; +} +static inline void audit_free(struct task_struct *task) +{ } +static inline void __init audit_task_init(void) +{ } static inline __printf(4, 5) void audit_log(struct audit_context *ctx, gfp_t gfp_mask, int type, const char *fmt, ...) @@ -267,8 +292,6 @@ static inline int audit_signal_info(int sig, struct task_struct *t) /* These are defined in auditsc.c */ /* Public API */ -extern int audit_alloc(struct task_struct *task); -extern void __audit_free(struct task_struct *task); extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1, unsigned long a2, unsigned long a3); extern void __audit_syscall_exit(int ret_success, long ret_value); @@ -288,12 +311,14 @@ extern void audit_seccomp_actions_logged(const char *names, static inline void audit_set_context(struct task_struct *task, struct audit_context *ctx) { - task->audit_context = ctx; + task->audit->ctx = ctx; } static inline struct audit_context *audit_context(void) { - return current->audit_context; + if (!current->audit) + return NULL; + return current->audit->ctx; } static inline bool audit_dummy_context(void) @@ -301,11 +326,7 @@ static inline bool audit_dummy_context(void) void *p = audit_context(); return !p || *(int *)p; } -static inline void audit_free(struct task_struct *task) -{ - if (unlikely(task->audit_context)) - __audit_free(task); -} + static inline void audit_syscall_entry(int major, unsigned long a0, unsigned long a1, unsigned long a2, unsigned long a3) @@ -533,12 +554,6 @@ static inline void audit_log_nfcfg(const char *name, u8 af, extern int audit_n_rules; extern int audit_signals; #else /* CONFIG_AUDITSYSCALL */ -static inline int audit_alloc(struct task_struct *task) -{ - return 0; -} -static inline void audit_free(struct task_struct *task) -{ } static inline void audit_syscall_entry(int major, unsigned long a0, unsigned long a1, unsigned long a2, unsigned long a3) diff --git a/include/linux/sched.h b/include/linux/sched.h index b62e6aaf28f0..2213ac670386 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -34,7 +34,6 @@ #include <linux/kcsan.h> /* task_struct member predeclarations (sorted alphabetically): */ -struct audit_context; struct backing_dev_info; struct bio_list; struct blk_plug; @@ -946,11 +945,7 @@ struct task_struct { struct callback_head *task_works; #ifdef CONFIG_AUDIT -#ifdef CONFIG_AUDITSYSCALL - struct audit_context *audit_context; -#endif - kuid_t loginuid; - unsigned int sessionid; + struct audit_task_info *audit; #endif struct seccomp seccomp; diff --git a/init/init_task.c b/init/init_task.c index 15089d15010a..92d34c4b7702 100644 --- a/init/init_task.c +++ b/init/init_task.c @@ -130,8 +130,7 @@ struct task_struct init_task .thread_group = LIST_HEAD_INIT(init_task.thread_group), .thread_node = LIST_HEAD_INIT(init_signals.thread_head), #ifdef CONFIG_AUDIT - .loginuid = INVALID_UID, - .sessionid = AUDIT_SID_UNSET, + .audit = &init_struct_audit, #endif #ifdef CONFIG_PERF_EVENTS .perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex), diff --git a/init/main.c b/init/main.c index 0ead83e86b5a..349470ad7458 100644 --- a/init/main.c +++ b/init/main.c @@ -96,6 +96,7 @@ #include <linux/jump_label.h> #include <linux/mem_encrypt.h> #include <linux/kcsan.h> +#include <linux/audit.h> #include <asm/io.h> #include <asm/bugs.h> @@ -1028,6 +1029,7 @@ asmlinkage __visible void __init start_kernel(void) nsfs_init(); cpuset_init(); cgroup_init(); + audit_task_init(); taskstats_init_early(); delayacct_init(); diff --git a/kernel/audit.c b/kernel/audit.c index 8c201f414226..5d8147a29291 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -203,6 +203,73 @@ struct audit_reply { struct sk_buff *skb; }; +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); +} + +/** + * audit_alloc - allocate an audit info block for a task + * @tsk: task + * + * Call audit_alloc_syscall to filter on the task information and + * allocate a per-task audit context if necessary. This is called from + * copy_process, so no lock is needed. + */ +int audit_alloc(struct task_struct *tsk) +{ + int ret = 0; + struct audit_task_info *info; + + info = kmem_cache_alloc(audit_task_cache, GFP_KERNEL); + if (!info) { + ret = -ENOMEM; + goto out; + } + info->loginuid = audit_get_loginuid(current); + info->sessionid = audit_get_sessionid(current); + tsk->audit = info; + + ret = audit_alloc_syscall(tsk); + if (ret) { + tsk->audit = NULL; + kmem_cache_free(audit_task_cache, info); + } +out: + return ret; +} + +struct audit_task_info init_struct_audit = { + .loginuid = INVALID_UID, + .sessionid = AUDIT_SID_UNSET, +#ifdef CONFIG_AUDITSYSCALL + .ctx = NULL, +#endif +}; + +/** + * audit_free - free per-task audit info + * @tsk: task whose audit info block to free + * + * Called from copy_process and do_exit + */ +void audit_free(struct task_struct *tsk) +{ + struct audit_task_info *info = tsk->audit; + + audit_free_syscall(tsk); + /* Freeing the audit_task_info struct must be performed after + * audit_log_exit() due to need for loginuid and sessionid. + */ + info = tsk->audit; + tsk->audit = NULL; + kmem_cache_free(audit_task_cache, info); +} + /** * auditd_test_task - Check to see if a given task is an audit daemon * @task: the task to check @@ -2309,8 +2376,8 @@ int audit_set_loginuid(kuid_t loginuid) sessionid = (unsigned int)atomic_inc_return(&session_id); } - current->sessionid = sessionid; - current->loginuid = loginuid; + current->audit->sessionid = sessionid; + current->audit->loginuid = loginuid; out: audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid, sessionid, rc); return rc; diff --git a/kernel/audit.h b/kernel/audit.h index f0233dc40b17..9bee09757068 100644 --- a/kernel/audit.h +++ b/kernel/audit.h @@ -251,6 +251,8 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab, extern unsigned int audit_serial(void); extern int auditsc_get_stamp(struct audit_context *ctx, struct timespec64 *t, unsigned int *serial); +extern int audit_alloc_syscall(struct task_struct *tsk); +extern void audit_free_syscall(struct task_struct *tsk); extern void audit_put_watch(struct audit_watch *watch); extern void audit_get_watch(struct audit_watch *watch); @@ -299,6 +301,9 @@ static inline void audit_clear_dummy(struct audit_context *ctx) #else /* CONFIG_AUDITSYSCALL */ #define auditsc_get_stamp(c, t, s) 0 +#define audit_alloc_syscall(t) 0 +#define audit_free_syscall(t) {} + #define audit_put_watch(w) {} #define audit_get_watch(w) {} #define audit_to_watch(k, p, l, o) (-EINVAL) diff --git a/kernel/auditsc.c b/kernel/auditsc.c index 468a23390457..f00c1da587ea 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -914,23 +914,25 @@ static inline struct audit_context *audit_alloc_context(enum audit_state state) return context; } -/** - * audit_alloc - allocate an audit context block for a task +/* + * audit_alloc_syscall - allocate an audit context block for a task * @tsk: task * * Filter on the task information and allocate a per-task audit context * if necessary. Doing so turns on system call auditing for the - * specified task. This is called from copy_process, so no lock is - * needed. + * specified task. This is called from copy_process via audit_alloc, so + * no lock is needed. */ -int audit_alloc(struct task_struct *tsk) +int audit_alloc_syscall(struct task_struct *tsk) { struct audit_context *context; enum audit_state state; char *key = NULL; - if (likely(!audit_ever_enabled)) + if (likely(!audit_ever_enabled)) { + audit_set_context(tsk, NULL); return 0; /* Return if not auditing. */ + } state = audit_filter_task(tsk, &key); if (state == AUDIT_DISABLED) { @@ -940,7 +942,7 @@ int audit_alloc(struct task_struct *tsk) if (!(context = audit_alloc_context(state))) { kfree(key); - audit_log_lost("out of memory in audit_alloc"); + audit_log_lost("out of memory in audit_alloc_syscall"); return -ENOMEM; } context->filterkey = key; @@ -1582,14 +1584,15 @@ static void audit_log_exit(void) } /** - * __audit_free - free a per-task audit context + * audit_free_syscall - free per-task audit context info * @tsk: task whose audit context block to free * - * Called from copy_process and do_exit + * Called from audit_free */ -void __audit_free(struct task_struct *tsk) +void audit_free_syscall(struct task_struct *tsk) { - struct audit_context *context = tsk->audit_context; + struct audit_task_info *info = tsk->audit; + struct audit_context *context = info->ctx; if (!context) return; @@ -1612,7 +1615,6 @@ void __audit_free(struct task_struct *tsk) if (context->current_state == AUDIT_RECORD_CONTEXT) audit_log_exit(); } - audit_set_context(tsk, NULL); audit_free_context(context); } diff --git a/kernel/fork.c b/kernel/fork.c index 142b23645d82..bacbff0e8a75 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -2019,7 +2019,6 @@ static __latent_entropy struct task_struct *copy_process( posix_cputimers_init(&p->posix_cputimers); p->io_context = NULL; - audit_set_context(p, NULL); cgroup_fork(p); #ifdef CONFIG_NUMA p->mempolicy = mpol_dup(p->mempolicy);