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 02/13] audit: add container id Date: Fri, 21 Aug 2020 15:36:49 -0400 [thread overview] Message-ID: <CAHC9VhQTiu+yY6cY8tvBf-1ZtZrre3Ljs+Zd6Jf9ZM766bhUYQ@mail.gmail.com> (raw) In-Reply-To: <20200729200545.5apwc7fashwsnglj@madcap2.tricolour.ca> On Wed, Jul 29, 2020 at 4:06 PM Richard Guy Briggs <rgb@redhat.com> wrote: > On 2020-07-05 11:09, Paul Moore wrote: > > On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs <rgb@redhat.com> wrote: ... > > > @@ -212,6 +219,33 @@ void __init audit_task_init(void) > > > 0, SLAB_PANIC, NULL); > > > } > > > > > > +/* rcu_read_lock must be held by caller unless new */ > > > +static struct audit_contobj *_audit_contobj_hold(struct audit_contobj *cont) > > > +{ > > > + if (cont) > > > + refcount_inc(&cont->refcount); > > > + return cont; > > > +} > > > + > > > +static struct audit_contobj *_audit_contobj_get(struct task_struct *tsk) > > > +{ > > > + if (!tsk->audit) > > > + return NULL; > > > + return _audit_contobj_hold(tsk->audit->cont); > > > +} > > > + > > > +/* rcu_read_lock must be held by caller */ > > > +static void _audit_contobj_put(struct audit_contobj *cont) > > > +{ > > > + if (!cont) > > > + return; > > > + if (refcount_dec_and_test(&cont->refcount)) { > > > + put_task_struct(cont->owner); > > > + list_del_rcu(&cont->list); > > > > You should check your locking; I'm used to seeing exclusive locks > > (e.g. the spinlock) around list adds/removes, it just reads/traversals > > that can be done with just the RCU lock held. > > Ok, I've redone the locking yet again. I knew this on one level but > that didn't translate consistently to code... > > > > + kfree_rcu(cont, rcu); > > > + } > > > +} > > > > Another nitpick, but it might be nice to have similar arguments to the > > _get() and _put() functions, e.g. struct audit_contobj, but that is > > some serious bikeshedding (basically rename _hold() to _get() and > > rename _hold to audit_task_contid_hold() or similar). > > I have some idea what you are trying to say, but I think you misspoke. > Did you mean rename _hold to _get, rename _get to > audit_task_contobj_hold()? It reads okay to me, but I know what I'm intending here :) I agree it could be a bit confusing. Let me try to put my suggestion into some quick pseudo-code function prototypes to make things a bit more concrete. The _audit_contobj_hold() function would become: struct audit_contobj *_audit_contobj_hold(struct task_struct *tsk); The _audit_contobj_get() function would become: struct audit_contobj *_audit_contobj_get(struct audit_contobj *cont); The _audit_contobj_put() function would become: void _audit_contobj_put(struct audit_contobj *cont); Basically swap the _get() and _hold() function names so that the arguments are the same for both the _get() and _set() functions. Does this make more sense? > > > /** > > > * audit_alloc - allocate an audit info block for a task > > > * @tsk: task > > > @@ -232,6 +266,9 @@ int audit_alloc(struct task_struct *tsk) > > > } > > > info->loginuid = audit_get_loginuid(current); > > > info->sessionid = audit_get_sessionid(current); > > > + rcu_read_lock(); > > > + info->cont = _audit_contobj_get(current); > > > + rcu_read_unlock(); > > > > The RCU locks aren't strictly necessary here, are they? In fact I > > suppose we could probably just replace the _get() call with a > > refcount_set(1) just as we do in audit_set_contid(), yes? > > I don't understand what you are getting at here. It needs a *contobj, > along with bumping up the refcount of the existing contobj. Sorry, you can disregard. My mental definition for audit_alloc() is permanently messed up; I usually double check myself before commenting on related code, but I must have forgotten here. -- 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 02/13] audit: add container id Date: Fri, 21 Aug 2020 15:36:49 -0400 [thread overview] Message-ID: <CAHC9VhQTiu+yY6cY8tvBf-1ZtZrre3Ljs+Zd6Jf9ZM766bhUYQ@mail.gmail.com> (raw) In-Reply-To: <20200729200545.5apwc7fashwsnglj@madcap2.tricolour.ca> On Wed, Jul 29, 2020 at 4:06 PM Richard Guy Briggs <rgb@redhat.com> wrote: > On 2020-07-05 11:09, Paul Moore wrote: > > On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs <rgb@redhat.com> wrote: ... > > > @@ -212,6 +219,33 @@ void __init audit_task_init(void) > > > 0, SLAB_PANIC, NULL); > > > } > > > > > > +/* rcu_read_lock must be held by caller unless new */ > > > +static struct audit_contobj *_audit_contobj_hold(struct audit_contobj *cont) > > > +{ > > > + if (cont) > > > + refcount_inc(&cont->refcount); > > > + return cont; > > > +} > > > + > > > +static struct audit_contobj *_audit_contobj_get(struct task_struct *tsk) > > > +{ > > > + if (!tsk->audit) > > > + return NULL; > > > + return _audit_contobj_hold(tsk->audit->cont); > > > +} > > > + > > > +/* rcu_read_lock must be held by caller */ > > > +static void _audit_contobj_put(struct audit_contobj *cont) > > > +{ > > > + if (!cont) > > > + return; > > > + if (refcount_dec_and_test(&cont->refcount)) { > > > + put_task_struct(cont->owner); > > > + list_del_rcu(&cont->list); > > > > You should check your locking; I'm used to seeing exclusive locks > > (e.g. the spinlock) around list adds/removes, it just reads/traversals > > that can be done with just the RCU lock held. > > Ok, I've redone the locking yet again. I knew this on one level but > that didn't translate consistently to code... > > > > + kfree_rcu(cont, rcu); > > > + } > > > +} > > > > Another nitpick, but it might be nice to have similar arguments to the > > _get() and _put() functions, e.g. struct audit_contobj, but that is > > some serious bikeshedding (basically rename _hold() to _get() and > > rename _hold to audit_task_contid_hold() or similar). > > I have some idea what you are trying to say, but I think you misspoke. > Did you mean rename _hold to _get, rename _get to > audit_task_contobj_hold()? It reads okay to me, but I know what I'm intending here :) I agree it could be a bit confusing. Let me try to put my suggestion into some quick pseudo-code function prototypes to make things a bit more concrete. The _audit_contobj_hold() function would become: struct audit_contobj *_audit_contobj_hold(struct task_struct *tsk); The _audit_contobj_get() function would become: struct audit_contobj *_audit_contobj_get(struct audit_contobj *cont); The _audit_contobj_put() function would become: void _audit_contobj_put(struct audit_contobj *cont); Basically swap the _get() and _hold() function names so that the arguments are the same for both the _get() and _set() functions. Does this make more sense? > > > /** > > > * audit_alloc - allocate an audit info block for a task > > > * @tsk: task > > > @@ -232,6 +266,9 @@ int audit_alloc(struct task_struct *tsk) > > > } > > > info->loginuid = audit_get_loginuid(current); > > > info->sessionid = audit_get_sessionid(current); > > > + rcu_read_lock(); > > > + info->cont = _audit_contobj_get(current); > > > + rcu_read_unlock(); > > > > The RCU locks aren't strictly necessary here, are they? In fact I > > suppose we could probably just replace the _get() call with a > > refcount_set(1) just as we do in audit_set_contid(), yes? > > I don't understand what you are getting at here. It needs a *contobj, > along with bumping up the refcount of the existing contobj. Sorry, you can disregard. My mental definition for audit_alloc() is permanently messed up; I usually double check myself before commenting on related code, but I must have forgotten here. -- 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-08-21 19:37 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 [this message] 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 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=CAHC9VhQTiu+yY6cY8tvBf-1ZtZrre3Ljs+Zd6Jf9ZM766bhUYQ@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.