From: Paul Moore <paul@paul-moore.com> To: Richard Guy Briggs <rgb@redhat.com> Cc: containers@lists.linux-foundation.org, linux-api@vger.kernel.org, Linux-Audit Mailing List <linux-audit@redhat.com>, linux-fsdevel@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>, netdev@vger.kernel.org, netfilter-devel@vger.kernel.org, sgrubb@redhat.com, Ondrej Mosnacek <omosnace@redhat.com>, dhowells@redhat.com, simo@redhat.com, Eric Paris <eparis@parisplace.org>, Serge Hallyn <serge@hallyn.com>, ebiederm@xmission.com, nhorman@tuxdriver.com, Dan Walsh <dwalsh@redhat.com>, mpatel@redhat.com Subject: Re: [PATCH ghak90 V9 07/13] audit: add support for non-syscall auxiliary records Date: Sun, 5 Jul 2020 11:11:00 -0400 [thread overview] Message-ID: <CAHC9VhTEkhZqkH24hPEZgMtWcYy9qKhZdoiegDLhGefa_bxmuw@mail.gmail.com> (raw) In-Reply-To: <21e6c4e1ac179c8dcf35803e603899ccfc69300a.1593198710.git.rgb@redhat.com> On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs <rgb@redhat.com> wrote: > > Standalone audit records have the timestamp and serial number generated > on the fly and as such are unique, making them standalone. This new > function audit_alloc_local() generates a local audit context that will > be used only for a standalone record and its auxiliary record(s). The > context is discarded immediately after the local associated records are > produced. We've had some good discussions on the list about why we can't reuse the "in_syscall" field and need to add a "local" field, I think it would be good to address that here in the commit description. > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > Acked-by: Serge Hallyn <serge@hallyn.com> > Acked-by: Neil Horman <nhorman@tuxdriver.com> > Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > include/linux/audit.h | 8 ++++++++ > kernel/audit.h | 1 + > kernel/auditsc.c | 33 ++++++++++++++++++++++++++++----- > 3 files changed, 37 insertions(+), 5 deletions(-) ... > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index 9e79645e5c0e..935eb3d2cde9 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -908,11 +908,13 @@ static inline void audit_free_aux(struct audit_context *context) > } > } > > -static inline struct audit_context *audit_alloc_context(enum audit_state state) > +static inline struct audit_context *audit_alloc_context(enum audit_state state, > + gfp_t gfpflags) > { > struct audit_context *context; > > - context = kzalloc(sizeof(*context), GFP_KERNEL); > + /* We can be called in atomic context via audit_tg() */ At this point I think it's clear we need a respin so I'm not going to preface all of my nitpick comments as such, although this definitely would qualify ... I don't believe audit_tg() doesn't exist yet, likely coming later in this patchset, so please remove this comment as it doesn't make sense in this context. To be frank, don't re-add the comment later in the patchset either. Comments like these tend to be fragile and don't really add any great insight. The audit_tg() function can, and most likely will, be modified at some point in the future such that the comment above no longer applies, and there is a reasonable chance that when it does the above comment will not be updated. Further, anyone modifying the audit_alloc_context() is going to look at the callers (rather they *should* look at the callers) and will notice the no-sleep requirements. > @@ -960,8 +963,27 @@ int audit_alloc_syscall(struct task_struct *tsk) > return 0; > } > > -static inline void audit_free_context(struct audit_context *context) > +struct audit_context *audit_alloc_local(gfp_t gfpflags) > { > + struct audit_context *context = NULL; > + > + context = audit_alloc_context(AUDIT_RECORD_CONTEXT, gfpflags); > + if (!context) { > + audit_log_lost("out of memory in audit_alloc_local"); > + goto out; You might as well just return NULL here, no need to jump and then return NULL. > + } > + context->serial = audit_serial(); > + ktime_get_coarse_real_ts64(&context->ctime); > + context->local = true; > +out: > + return context; > +} > +EXPORT_SYMBOL(audit_alloc_local); -- paul moore www.paul-moore.com
WARNING: multiple messages have this Message-ID (diff)
From: Paul Moore <paul@paul-moore.com> To: Richard Guy Briggs <rgb@redhat.com> Cc: nhorman@tuxdriver.com, linux-api@vger.kernel.org, containers@lists.linux-foundation.org, LKML <linux-kernel@vger.kernel.org>, dhowells@redhat.com, Linux-Audit Mailing List <linux-audit@redhat.com>, netfilter-devel@vger.kernel.org, ebiederm@xmission.com, simo@redhat.com, netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org, Eric Paris <eparis@parisplace.org>, mpatel@redhat.com, Serge Hallyn <serge@hallyn.com> Subject: Re: [PATCH ghak90 V9 07/13] audit: add support for non-syscall auxiliary records Date: Sun, 5 Jul 2020 11:11:00 -0400 [thread overview] Message-ID: <CAHC9VhTEkhZqkH24hPEZgMtWcYy9qKhZdoiegDLhGefa_bxmuw@mail.gmail.com> (raw) In-Reply-To: <21e6c4e1ac179c8dcf35803e603899ccfc69300a.1593198710.git.rgb@redhat.com> On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs <rgb@redhat.com> wrote: > > Standalone audit records have the timestamp and serial number generated > on the fly and as such are unique, making them standalone. This new > function audit_alloc_local() generates a local audit context that will > be used only for a standalone record and its auxiliary record(s). The > context is discarded immediately after the local associated records are > produced. We've had some good discussions on the list about why we can't reuse the "in_syscall" field and need to add a "local" field, I think it would be good to address that here in the commit description. > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > Acked-by: Serge Hallyn <serge@hallyn.com> > Acked-by: Neil Horman <nhorman@tuxdriver.com> > Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > include/linux/audit.h | 8 ++++++++ > kernel/audit.h | 1 + > kernel/auditsc.c | 33 ++++++++++++++++++++++++++++----- > 3 files changed, 37 insertions(+), 5 deletions(-) ... > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index 9e79645e5c0e..935eb3d2cde9 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -908,11 +908,13 @@ static inline void audit_free_aux(struct audit_context *context) > } > } > > -static inline struct audit_context *audit_alloc_context(enum audit_state state) > +static inline struct audit_context *audit_alloc_context(enum audit_state state, > + gfp_t gfpflags) > { > struct audit_context *context; > > - context = kzalloc(sizeof(*context), GFP_KERNEL); > + /* We can be called in atomic context via audit_tg() */ At this point I think it's clear we need a respin so I'm not going to preface all of my nitpick comments as such, although this definitely would qualify ... I don't believe audit_tg() doesn't exist yet, likely coming later in this patchset, so please remove this comment as it doesn't make sense in this context. To be frank, don't re-add the comment later in the patchset either. Comments like these tend to be fragile and don't really add any great insight. The audit_tg() function can, and most likely will, be modified at some point in the future such that the comment above no longer applies, and there is a reasonable chance that when it does the above comment will not be updated. Further, anyone modifying the audit_alloc_context() is going to look at the callers (rather they *should* look at the callers) and will notice the no-sleep requirements. > @@ -960,8 +963,27 @@ int audit_alloc_syscall(struct task_struct *tsk) > return 0; > } > > -static inline void audit_free_context(struct audit_context *context) > +struct audit_context *audit_alloc_local(gfp_t gfpflags) > { > + struct audit_context *context = NULL; > + > + context = audit_alloc_context(AUDIT_RECORD_CONTEXT, gfpflags); > + if (!context) { > + audit_log_lost("out of memory in audit_alloc_local"); > + goto out; You might as well just return NULL here, no need to jump and then return NULL. > + } > + context->serial = audit_serial(); > + ktime_get_coarse_real_ts64(&context->ctime); > + context->local = true; > +out: > + return context; > +} > +EXPORT_SYMBOL(audit_alloc_local); -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
next prev parent reply other threads:[~2020-07-05 15:11 UTC|newest] Thread overview: 111+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-06-27 13:20 [PATCH ghak90 V9 00/13] audit: implement container identifier Richard Guy Briggs 2020-06-27 13:20 ` Richard Guy Briggs 2020-06-27 13:20 ` [PATCH ghak90 V9 01/13] audit: collect audit task parameters Richard Guy Briggs 2020-06-27 13:20 ` Richard Guy Briggs 2020-07-05 15:09 ` Paul Moore 2020-07-05 15:09 ` Paul Moore 2020-07-07 2:50 ` Richard Guy Briggs 2020-07-07 2:50 ` Richard Guy Briggs 2020-07-08 1:42 ` Paul Moore 2020-07-08 1:42 ` Paul Moore 2020-07-13 20:29 ` Richard Guy Briggs 2020-07-13 20:29 ` Richard Guy Briggs 2020-07-14 0:44 ` Paul Moore 2020-07-14 0:44 ` Paul Moore 2020-06-27 13:20 ` [PATCH ghak90 V9 02/13] audit: add container id Richard Guy Briggs 2020-06-27 13:20 ` Richard Guy Briggs 2020-07-04 13:29 ` Paul Moore 2020-07-04 13:29 ` Paul Moore 2020-07-04 13:30 ` Paul Moore 2020-07-04 13:30 ` Paul Moore 2020-07-05 15:09 ` Paul Moore 2020-07-05 15:09 ` Paul Moore 2020-07-29 20:05 ` Richard Guy Briggs 2020-07-29 20:05 ` Richard Guy Briggs 2020-08-21 19:36 ` Paul Moore 2020-08-21 19:36 ` Paul Moore 2020-06-27 13:20 ` [PATCH ghak90 V9 03/13] audit: read container ID of a process Richard Guy Briggs 2020-06-27 13:20 ` Richard Guy Briggs 2020-06-27 13:20 ` [PATCH ghak90 V9 04/13] audit: log drop of contid on exit of last task Richard Guy Briggs 2020-06-27 13:20 ` Richard Guy Briggs 2020-07-05 15:10 ` Paul Moore 2020-07-05 15:10 ` Paul Moore 2020-06-27 13:20 ` [PATCH ghak90 V9 05/13] audit: log container info of syscalls Richard Guy Briggs 2020-06-27 13:20 ` Richard Guy Briggs 2020-07-05 15:10 ` Paul Moore 2020-07-05 15:10 ` Paul Moore 2020-07-29 19:40 ` Richard Guy Briggs 2020-07-29 19:40 ` Richard Guy Briggs 2020-08-21 19:15 ` Paul Moore 2020-08-21 19:15 ` Paul Moore 2020-10-02 19:52 ` Richard Guy Briggs 2020-10-02 19:52 ` Richard Guy Briggs 2020-10-02 19:52 ` Richard Guy Briggs 2020-10-21 16:39 ` Richard Guy Briggs 2020-10-21 16:39 ` Richard Guy Briggs 2020-10-21 16:39 ` Richard Guy Briggs 2020-10-21 16:49 ` Steve Grubb 2020-10-21 16:49 ` Steve Grubb 2020-10-21 16:49 ` Steve Grubb 2020-10-21 17:53 ` Richard Guy Briggs 2020-10-21 17:53 ` Richard Guy Briggs 2020-10-21 17:53 ` Richard Guy Briggs 2020-10-23 1:21 ` Paul Moore 2020-10-23 1:21 ` Paul Moore 2020-10-23 1:21 ` Paul Moore 2020-10-23 20:40 ` Richard Guy Briggs 2020-10-23 20:40 ` Richard Guy Briggs 2020-10-23 20:40 ` Richard Guy Briggs 2020-10-28 1:35 ` Paul Moore 2020-10-28 1:35 ` Paul Moore 2020-10-28 1:35 ` Paul Moore 2020-06-27 13:20 ` [PATCH ghak90 V9 06/13] audit: add contid support for signalling the audit daemon Richard Guy Briggs 2020-06-27 13:20 ` Richard Guy Briggs 2020-07-05 15:10 ` Paul Moore 2020-07-05 15:10 ` Paul Moore 2020-07-29 19:00 ` Richard Guy Briggs 2020-07-29 19:00 ` Richard Guy Briggs 2020-08-21 18:48 ` Paul Moore 2020-08-21 18:48 ` Paul Moore 2020-10-02 19:25 ` Richard Guy Briggs 2020-10-02 19:25 ` Richard Guy Briggs 2020-10-02 19:25 ` Richard Guy Briggs 2020-06-27 13:20 ` [PATCH ghak90 V9 07/13] audit: add support for non-syscall auxiliary records Richard Guy Briggs 2020-06-27 13:20 ` Richard Guy Briggs 2020-07-05 15:11 ` Paul Moore [this message] 2020-07-05 15:11 ` Paul Moore 2020-06-27 13:20 ` [PATCH ghak90 V9 08/13] audit: add containerid support for user records Richard Guy Briggs 2020-06-27 13:20 ` Richard Guy Briggs 2020-07-05 15:11 ` Paul Moore 2020-07-05 15:11 ` Paul Moore 2020-07-18 0:43 ` Richard Guy Briggs 2020-07-18 0:43 ` Richard Guy Briggs 2020-08-21 18:34 ` Paul Moore 2020-08-21 18:34 ` Paul Moore 2020-06-27 13:20 ` [PATCH ghak90 V9 09/13] audit: add containerid filtering Richard Guy Briggs 2020-06-27 13:20 ` Richard Guy Briggs 2020-06-27 13:20 ` [PATCH ghak90 V9 10/13] audit: add support for containerid to network namespaces Richard Guy Briggs 2020-06-27 13:20 ` Richard Guy Briggs 2020-07-05 15:11 ` Paul Moore 2020-07-05 15:11 ` Paul Moore 2020-07-21 22:05 ` Richard Guy Briggs 2020-07-21 22:05 ` Richard Guy Briggs 2020-06-27 13:20 ` [PATCH ghak90 V9 11/13] audit: contid check descendancy and nesting Richard Guy Briggs 2020-06-27 13:20 ` Richard Guy Briggs 2020-07-05 15:11 ` Paul Moore 2020-07-05 15:11 ` Paul Moore 2020-08-07 17:10 ` Richard Guy Briggs 2020-08-07 17:10 ` Richard Guy Briggs 2020-08-21 20:13 ` Paul Moore 2020-08-21 20:13 ` Paul Moore 2020-10-06 20:03 ` Richard Guy Briggs 2020-10-06 20:03 ` Richard Guy Briggs 2020-10-06 20:03 ` Richard Guy Briggs 2020-06-27 13:20 ` [PATCH ghak90 V9 12/13] audit: track container nesting Richard Guy Briggs 2020-06-27 13:20 ` Richard Guy Briggs 2020-07-05 15:11 ` Paul Moore 2020-07-05 15:11 ` Paul Moore 2020-06-27 13:20 ` [PATCH ghak90 V9 13/13] audit: add capcontid to set contid outside init_user_ns Richard Guy Briggs 2020-06-27 13:20 ` Richard Guy Briggs 2020-07-05 15:11 ` Paul Moore 2020-07-05 15:11 ` 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=CAHC9VhTEkhZqkH24hPEZgMtWcYy9qKhZdoiegDLhGefa_bxmuw@mail.gmail.com \ --to=paul@paul-moore.com \ --cc=containers@lists.linux-foundation.org \ --cc=dhowells@redhat.com \ --cc=dwalsh@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=mpatel@redhat.com \ --cc=netdev@vger.kernel.org \ --cc=netfilter-devel@vger.kernel.org \ --cc=nhorman@tuxdriver.com \ --cc=omosnace@redhat.com \ --cc=rgb@redhat.com \ --cc=serge@hallyn.com \ --cc=sgrubb@redhat.com \ --cc=simo@redhat.com \ /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.