linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Neil Horman <nhorman@tuxdriver.com>
To: Richard Guy Briggs <rgb@redhat.com>
Cc: Paul Moore <paul@paul-moore.com>,
	Ondrej Mosnacek <omosnace@redhat.com>,
	linux-api@vger.kernel.org, containers@lists.linux-foundation.org,
	LKML <linux-kernel@vger.kernel.org>,
	David Howells <dhowells@redhat.com>,
	Linux-Audit Mailing List <linux-audit@redhat.com>,
	netfilter-devel@vger.kernel.org,
	"Eric W . Biederman" <ebiederm@xmission.com>,
	Simo Sorce <simo@redhat.com>,
	netdev@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	Eric Paris <eparis@parisplace.org>,
	"Serge E. Hallyn" <serge@hallyn.com>
Subject: Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
Date: Fri, 29 Mar 2019 10:50:17 -0400	[thread overview]
Message-ID: <20190329120403.GA30213@hmswarspite.think-freely.org> (raw)
In-Reply-To: <20190328214023.qpszfvxbrjlldmmt@madcap2.tricolour.ca>

On Thu, Mar 28, 2019 at 05:40:23PM -0400, Richard Guy Briggs wrote:
> On 2019-03-28 11:46, Paul Moore wrote:
> > On Wed, Mar 27, 2019 at 9:12 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > >
> > > On 2019-03-27 23:42, Ondrej Mosnacek wrote:
> > > > On Fri, Mar 15, 2019 at 7:35 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > Audit events could happen in a network namespace outside of a task
> > > > > context due to packets received from the net that trigger an auditing
> > > > > rule prior to being associated with a running task.  The network
> > > > > namespace could be in use by multiple containers by association to the
> > > > > tasks in that network namespace.  We still want a way to attribute
> > > > > these events to any potential containers.  Keep a list per network
> > > > > namespace to track these audit container identifiiers.
> > > > >
> > > > > Add/increment the audit container identifier on:
> > > > > - initial setting of the audit container identifier via /proc
> > > > > - clone/fork call that inherits an audit container identifier
> > > > > - unshare call that inherits an audit container identifier
> > > > > - setns call that inherits an audit container identifier
> > > > > Delete/decrement the audit container identifier on:
> > > > > - an inherited audit container identifier dropped when child set
> > > > > - process exit
> > > > > - unshare call that drops a net namespace
> > > > > - setns call that drops a net namespace
> > > > >
> > > > > See: https://github.com/linux-audit/audit-kernel/issues/92
> > > > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > > > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > ---
> > > > >  include/linux/audit.h | 19 ++++++++++++
> > > > >  kernel/audit.c        | 86 +++++++++++++++++++++++++++++++++++++++++++++++++--
> > > > >  kernel/nsproxy.c      |  4 +++
> > > > >  3 files changed, 106 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > > > index fa19fa408931..70255c2dfb9f 100644
> > > > > --- a/include/linux/audit.h
> > > > > +++ b/include/linux/audit.h
> > > > > @@ -27,6 +27,7 @@
> > > > >  #include <linux/ptrace.h>
> > > > >  #include <linux/namei.h>  /* LOOKUP_* */
> > > > >  #include <uapi/linux/audit.h>
> > > > > +#include <linux/refcount.h>
> > > > >
> > > > >  #define AUDIT_INO_UNSET ((unsigned long)-1)
> > > > >  #define AUDIT_DEV_UNSET ((dev_t)-1)
> > > > > @@ -99,6 +100,13 @@ struct audit_task_info {
> > > > >
> > > > >  extern struct audit_task_info init_struct_audit;
> > > > >
> > > > > +struct audit_contid {
> > > > > +       struct list_head        list;
> > > > > +       u64                     id;
> > > > > +       refcount_t              refcount;
> > > >
> > > > Hm, since we only ever touch the refcount under a spinlock, I wonder
> > > > if we could just make it a regular unsigned int (we don't need the
> > > > atomicity guarantees). OTOH, refcount_t comes with some extra overflow
> > > > checking, so it's probably better to leave it as is...
> > >
> > > Since the update is done using rcu-safe methods, do we even need the
> > > spin_lock?  Neil?  Paul?
> > 
> > As discussed, the refcount field is protected against simultaneous
> > writes by the spinlock that protects additions/removals from the list
> > as a whole so I don't believe the refcount_t atomicity is critical in
> > this regard.
> > 
> > Where it gets tricky, and I can't say I'm 100% confident on my answer
> > here, is if refcount was a regular int and we wanted to access it
> > outside of a spinlock (to be clear, it doesn't look like this patch
> > currently does this).  With RCU, if refcount was a regular int
> > (unsigned or otherwise), I believe it would be possible for different
> > threads of execution to potentially see different values of refcount
> > (assuming one thread was adding/removing from the list).  Using a
> > refcount_t would protect against this, alternatively, taking the
> > spinlock should also protect against this.
> 
> Ok, from the above it isn't clear to me if you are happy with the
> current code or would prefer any changes, or from below that you still
> need to work it through to make a pronouncement.  It sounds to me you
> would be ok with *either* spinlock *or* refcount_t, but don't see the
> need for both.
> 
I'll reiterate I think we should keep the refcount type just as it is, not for
safetys sake, but for readability and convienience.

Because the refcount currently only is used on add and delete operations
(implying it is only read in paths where its also modified), we need to
guarantee atomicity against multiple parallel writes.  We already have that
guarantee because every path in which we call
refcount_set/refcount_inc/refcount_dec_and_test occurs under the protection of
the list spin lock, and so from that standpoint we don't need the additional
guarantees offered by the refcount_t type.

However, if it were to be converted to an int type, we would have to replace the
refcount_dec_and_test call with this:
if (x == 0)
   warn_on_underflow
   return
x -= 1;
if (x == 0)
   preform_operations_to_free_list_entry

I find refcount_dec_and_test to be far easier to read and maintain, and you
still have to do all of this under the protection of a spin lock, to protect
against multiple writes.  And if you ever find that you are adding a pure read
side query of the refcount, you would need to hold the spin lock there as well,
instead of just using the available refcount_read api call

Yeah, you would save a few cycles if you didn't use an atomic type here, but
we're only talking about paths from user space making system calls executing
here (no high volume per packet receive paths or anything), and these paths have
already taken a few locks (the list lock, the task lock, etc), so eliminating
this one atomic isn't going to amount to anything.  Lets leave it as it is and
buy ourselves the extra code readability.

Neil

> > As we all know, RCU can be tricky at times, so I may be off on the
> > above; if I am, please provide an explanation so I (and likely others
> > as well) can learn a little bit more. :)
> > 
> > -- 
> > paul moore
> > www.paul-moore.com
> 
> - 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
> 
> 

  parent reply	other threads:[~2019-03-29 14:51 UTC|newest]

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-15 18:29 [PATCH ghak90 V5 00/10] audit: implement container identifier Richard Guy Briggs
2019-03-15 18:29 ` [PATCH ghak90 V5 01/10] audit: collect audit task parameters Richard Guy Briggs
2019-03-16 19:57   ` Neil Horman
2019-03-27 20:33   ` Ondrej Mosnacek
2019-03-15 18:29 ` [PATCH ghak90 V5 02/10] audit: add container id Richard Guy Briggs
2019-03-16 20:00   ` Neil Horman
2019-03-27 20:38   ` Ondrej Mosnacek
2019-03-27 20:44     ` Richard Guy Briggs
2019-03-15 18:29 ` [PATCH ghak90 V5 03/10] audit: read container ID of a process Richard Guy Briggs
2019-03-18 11:10   ` Neil Horman
2019-03-18 18:17     ` Richard Guy Briggs
2019-03-18 18:48       ` Neil Horman
2019-03-18 18:54         ` Richard Guy Briggs
2019-03-27 20:44   ` Ondrej Mosnacek
2019-03-15 18:29 ` [PATCH ghak90 V5 04/10] audit: log container info of syscalls Richard Guy Briggs
2019-03-16 22:44   ` Neil Horman
2019-03-27 21:01   ` Ondrej Mosnacek
2019-03-27 22:10     ` Richard Guy Briggs
2019-03-15 18:29 ` [PATCH ghak90 V5 05/10] audit: add containerid support for ptrace and signals Richard Guy Briggs
2019-03-18 19:04   ` Neil Horman
2019-03-18 19:29     ` Richard Guy Briggs
2019-03-27 21:17   ` Ondrej Mosnacek
2019-03-28  2:04     ` Richard Guy Briggs
2019-03-30 12:55       ` Richard Guy Briggs
2019-03-15 18:29 ` [PATCH ghak90 V5 06/10] audit: add support for non-syscall auxiliary records Richard Guy Briggs
2019-03-18 19:34   ` Neil Horman
2019-03-27 21:22   ` Ondrej Mosnacek
2019-04-01 14:49   ` Paul Moore
2019-04-01 17:44     ` Richard Guy Briggs
2019-04-01 18:57       ` Paul Moore
2019-04-01 20:43         ` Richard Guy Briggs
2019-03-15 18:29 ` [PATCH ghak90 V5 07/10] audit: add containerid support for user records Richard Guy Briggs
2019-03-18 19:41   ` Neil Horman
2019-03-27 21:30   ` Ondrej Mosnacek
2019-03-15 18:29 ` [PATCH ghak90 V5 08/10] audit: add containerid filtering Richard Guy Briggs
2019-03-18 20:02   ` Ondrej Mosnacek
2019-03-18 23:47     ` Richard Guy Briggs
2019-03-27 21:41       ` Ondrej Mosnacek
2019-03-27 22:00         ` Richard Guy Briggs
2019-03-18 20:39   ` Neil Horman
2019-03-15 18:29 ` [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces Richard Guy Briggs
2019-03-18 20:56   ` Neil Horman
2019-03-27 22:42   ` Ondrej Mosnacek
2019-03-28  1:12     ` Richard Guy Briggs
2019-03-28  8:01       ` Ondrej Mosnacek
2019-03-28 15:46       ` Paul Moore
2019-03-28 21:40         ` Richard Guy Briggs
2019-03-28 22:00           ` Paul Moore
2019-03-31  2:11             ` Neil Horman
2019-03-29 14:50           ` Neil Horman [this message]
2019-03-29 14:49       ` Neil Horman
2019-04-01 14:50   ` Paul Moore
2019-04-01 20:41     ` Richard Guy Briggs
2019-04-02 11:31     ` Neil Horman
2019-04-02 13:31       ` Paul Moore
2019-04-02 14:28         ` Neil Horman
2019-04-04 21:40       ` Richard Guy Briggs
2019-04-05  2:06         ` Paul Moore
2019-04-05 11:32         ` Neil Horman
2019-03-15 18:29 ` [PATCH ghak90 V5 10/10] audit: NETFILTER_PKT: record each container ID associated with a netNS Richard Guy Briggs
2019-03-18 20:58   ` Neil Horman
2019-03-27 22:52   ` Ondrej Mosnacek
2019-04-01 14:50   ` Paul Moore
2019-04-01 17:50     ` Richard Guy Briggs

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=20190329120403.GA30213@hmswarspite.think-freely.org \
    --to=nhorman@tuxdriver.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=netdev@vger.kernel.org \
    --cc=netfilter-devel@vger.kernel.org \
    --cc=omosnace@redhat.com \
    --cc=paul@paul-moore.com \
    --cc=rgb@redhat.com \
    --cc=serge@hallyn.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: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).