Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
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, 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 V7 04/21] audit: convert to contid list to check for orch/engine ownership
Date: Fri, 8 Nov 2019 13:26:18 -0500
Message-ID: <CAHC9VhRMJkeC7HkAMr1TwymtT7eHOB_B=_28R6zVfQQk-gW-DA@mail.gmail.com> (raw)
In-Reply-To: <20191025210004.jzkenjg6jrka22ak@madcap2.tricolour.ca>

On Fri, Oct 25, 2019 at 5:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-10-10 20:38, Paul Moore wrote:
> > On Wed, Sep 18, 2019 at 9:24 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > Store the audit container identifier in a refcounted kernel object that
> > > is added to the master list of audit container identifiers.  This will
> > > allow multiple container orchestrators/engines to work on the same
> > > machine without danger of inadvertantly re-using an existing identifier.
> > > It will also allow an orchestrator to inject a process into an existing
> > > container by checking if the original container owner is the one
> > > injecting the task.  A hash table list is used to optimize searches.
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > >  include/linux/audit.h | 26 ++++++++++++++--
> > >  kernel/audit.c        | 86 ++++++++++++++++++++++++++++++++++++++++++++++++---
> > >  kernel/audit.h        |  8 +++++
> > >  3 files changed, 112 insertions(+), 8 deletions(-)
> >
> > One general comment before we go off into the weeds on this ... I can
> > understand why you wanted to keep this patch separate from the earlier
> > patches, but as we get closer to having mergeable code this should get
> > folded into the previous patches.  For example, there shouldn't be a
> > change in audit_task_info where you change the contid field from a u64
> > to struct pointer, it should be a struct pointer from the start.
>
> I should have marked this patchset as RFC even though it was v7 due to a
> lot of new ideas/code that was added with uncertainties needing comment
> and direction.
>
> > It's also disappointing that idr appears to only be for 32-bit ID
> > values, if we had a 64-bit idr I think we could simplify this greatly.
>
> Perhaps.  I do still see value in letting the orchestrator choose the
> value.

Agreed.  I was just thinking out loud that it seems like much of what
we need could be a generic library mechanism similar to, but not quite
like, the existing idr code.

> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index f2e3b81f2942..e317807cdd3e 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -95,10 +95,18 @@ struct audit_ntp_data {
> > >  struct audit_ntp_data {};
> > >  #endif
> > >
> > > +struct audit_cont {
> > > +       struct list_head        list;
> > > +       u64                     id;
> > > +       struct task_struct      *owner;
> > > +       refcount_t              refcount;
> > > +       struct rcu_head         rcu;
> > > +};
> >
> > It seems as though in most of the code you are using "contid", any
> > reason why didn't stick with that naming scheme here, e.g. "struct
> > audit_contid"?
>
> I was using contid to refer to the value itself and cont to refer to the
> refcounted object.  I find cont a bit too terse, so I'm still thinking
> of changing it.  Perhaps contobj?

Yes, just "cont" is a bit too ambiguous considering we have both
integer values and structures being passed around.  Whatever you
decide on, a common base with separate suffixes seems like a good
idea.

FWIW, I still think the "audit container ID" : "ACID" thing is kinda funny ;)

> > > @@ -203,11 +211,15 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
> > >
> > >  static inline u64 audit_get_contid(struct task_struct *tsk)
> > >  {
> > > -       if (!tsk->audit)
> > > +       if (!tsk->audit || !tsk->audit->cont)
> > >                 return AUDIT_CID_UNSET;
> > > -       return tsk->audit->contid;
> > > +       return tsk->audit->cont->id;
> > >  }
> >
> > Assuming for a moment that we implement an audit_contid_get() (see
> > Neil's comment as well as mine below), we probably need to name this
> > something different so we don't all lose our minds when we read this
> > code.  On the plus side we can probably preface it with an underscore
> > since it is a static, in which case _audit_contid_get() might be okay,
> > but I'm open to suggestions.
>
> I'm fine with the "_" prefix, can you point to precedent or convention?

Generally kernel functions which are "special"/private/unsafe/etc.
have a one, or two, underscore prefix.  If you don't want to add the
prefix, that's fine, but please change the name as mentioned
previously.

> > > @@ -231,7 +235,9 @@ int audit_alloc(struct task_struct *tsk)
> > >         }
> > >         info->loginuid = audit_get_loginuid(current);
> > >         info->sessionid = audit_get_sessionid(current);
> > > -       info->contid = audit_get_contid(current);
> > > +       info->cont = audit_cont(current);
> > > +       if (info->cont)
> > > +               refcount_inc(&info->cont->refcount);
> >
> > See the other comments about a "get" function, but I think we need a
> > RCU read lock around the above, no?
>
> The rcu read lock is to protect the list rather than the cont object
> itself, the latter of which is protected by its refcount.

What protects you from info->cont going away between when you fetch
the pointer via audit_cont() to when you dereference it in
refcount_inc()?

> > > @@ -2397,8 +2438,43 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> > >         else if (audit_contid_set(task))
> > >                 rc = -ECHILD;
> > >         read_unlock(&tasklist_lock);
> > > -       if (!rc)
> > > -               task->audit->contid = contid;
> > > +       if (!rc) {
> > > +               struct audit_cont *oldcont = audit_cont(task);
> >
> > Previously we held the tasklist_lock to protect the audit container ID
> > associated with the struct, should we still be holding it here?
>
> We held the tasklist_lock to protect access to the target task's
> child/parent/thread relationships.

What protects us in the case of simultaneous calls to audit_set_contid()?

> > Regardless, I worry that the lock dependencies between the
> > tasklist_lock and the audit_contid_list_lock are going to be tricky.
> > It might be nice to document the relationship in a comment up near
> > where you declare audit_contid_list_lock.
>
> I don't think there should be a conflict between the two.
>
> The contid_list_lock doesn't care if the cont object is associated to a
> particular task.

Please document the relationship between the two, I worry we could
easily run into lockdep problems without a clearly defined ordering.

> > > +               struct audit_cont *cont = NULL;
> > > +               struct audit_cont *newcont = NULL;
> > > +               int h = audit_hash_contid(contid);
> > > +
> > > +               spin_lock(&audit_contid_list_lock);
> > > +               list_for_each_entry_rcu(cont, &audit_contid_hash[h], list)
> > > +                       if (cont->id == contid) {
> > > +                               /* task injection to existing container */
> > > +                               if (current == cont->owner) {
> >
> > I understand the desire to limit a given audit container ID to the
> > orchestrator that created it, but are we certain that we can track
> > audit container ID "ownership" via a single instance of a task_struct?
>
> Are you suggesting that a task_struct representing a task may be
> replaced for a specific task?  I don't believe that will ever happen.
>
> >  What happens when the orchestrator stops/restarts/crashes?  Do we
> > even care?
>
> Reap all of its containers?

These were genuine questions, I'm not suggesting anything in
particular, I'm just curious about how we handle an orchestrator that
isn't continuously running ... is this possible?  Do we care?

-- 
paul moore
www.paul-moore.com

  reply index

Thread overview: 69+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-19  1:22 [PATCH ghak90 V7 00/21] audit: implement container identifier Richard Guy Briggs
2019-09-19  1:22 ` [PATCH ghak90 V7 01/21] audit: collect audit task parameters Richard Guy Briggs
2019-09-19  1:22 ` [PATCH ghak90 V7 02/21] audit: add container id Richard Guy Briggs
2019-09-19  1:22 ` [PATCH ghak90 V7 03/21] audit: read container ID of a process Richard Guy Briggs
2019-09-19  1:22 ` [PATCH ghak90 V7 04/21] audit: convert to contid list to check for orch/engine ownership Richard Guy Briggs
2019-09-26 14:46   ` Neil Horman
2019-10-25 20:00     ` Richard Guy Briggs
2019-10-28 12:20       ` Neil Horman
2019-10-11  0:38   ` Paul Moore
2019-10-25 21:00     ` Richard Guy Briggs
2019-11-08 18:26       ` Paul Moore [this message]
2019-09-19  1:22 ` [PATCH ghak90 V7 05/21] audit: log drop of contid on exit of last task Richard Guy Briggs
2019-10-11  0:38   ` Paul Moore
2019-10-25 19:43     ` Richard Guy Briggs
2019-09-19  1:22 ` [PATCH ghak90 V7 06/21] audit: contid limit of 32k imposed to avoid DoS Richard Guy Briggs
2019-09-27 12:51   ` Neil Horman
2019-10-11  0:38     ` Paul Moore
2019-10-24 21:23       ` Richard Guy Briggs
2019-11-08 17:49         ` Paul Moore
2019-10-25 20:15     ` Richard Guy Briggs
2019-09-19  1:22 ` [PATCH ghak90 V7 07/21] audit: log container info of syscalls Richard Guy Briggs
2019-09-19  1:22 ` [PATCH ghak90 V7 08/21] audit: add contid support for signalling the audit daemon Richard Guy Briggs
2019-10-11  0:39   ` Paul Moore
2019-10-25 19:20     ` Richard Guy Briggs
2019-11-08 17:41       ` Paul Moore
2019-09-19  1:22 ` [PATCH ghak90 V7 09/21] audit: add support for non-syscall auxiliary records Richard Guy Briggs
2019-09-19  1:22 ` [PATCH ghak90 V7 10/21] audit: add containerid support for user records Richard Guy Briggs
2019-09-19  1:22 ` [PATCH ghak90 V7 11/21] audit: add containerid filtering Richard Guy Briggs
2019-09-19  1:22 ` [PATCH ghak90 V7 12/21] audit: add support for containerid to network namespaces Richard Guy Briggs
2019-10-11  0:39   ` Paul Moore
2019-09-19  1:22 ` [PATCH ghak90 V7 13/21] audit: NETFILTER_PKT: record each container ID associated with a netNS Richard Guy Briggs
2019-10-11  0:39   ` Paul Moore
2019-09-19  1:22 ` [PATCH ghak90 V7 14/21] audit: contid check descendancy and nesting Richard Guy Briggs
2019-10-11  0:40   ` Paul Moore
2019-10-24 22:08     ` Richard Guy Briggs
2019-10-30 20:32       ` Paul Moore
2019-09-19  1:22 ` [PATCH ghak90 V7 15/21] sched: pull task_is_descendant into kernel/sched/core.c Richard Guy Briggs
2019-10-11  0:40   ` Paul Moore
2019-09-19  1:22 ` [PATCH ghak90 V7 16/21] audit: add support for contid set/get by netlink Richard Guy Briggs
2019-10-11  0:40   ` Paul Moore
2019-09-19  1:22 ` [PATCH ghak90 V7 17/21] audit: add support for loginuid/sessionid " Richard Guy Briggs
2019-10-11  0:40   ` Paul Moore
2019-09-19  1:22 ` [PATCH ghak90 V7 18/21] audit: track container nesting Richard Guy Briggs
2019-10-11  0:40   ` Paul Moore
2019-09-19  1:22 ` [PATCH ghak90 V7 19/21] audit: check cont depth Richard Guy Briggs
2019-09-19  1:22 ` [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns Richard Guy Briggs
2019-10-19  1:39   ` Richard Guy Briggs
2019-10-21 19:53     ` Paul Moore
2019-10-21 21:38       ` Richard Guy Briggs
2019-10-21 21:43         ` Paul Moore
2019-10-21 23:57           ` Richard Guy Briggs
2019-10-22  0:31             ` Paul Moore
2019-10-22 12:13               ` Neil Horman
2019-10-22 14:04                 ` Paul Moore
2019-10-22 20:06                 ` Richard Guy Briggs
2019-10-22 14:27               ` Richard Guy Briggs
2019-10-22 14:34                 ` Paul Moore
2019-10-24 21:00               ` Richard Guy Briggs
2019-10-30 20:27                 ` Paul Moore
2019-10-30 22:03                   ` Richard Guy Briggs
2019-10-31 13:59                     ` Paul Moore
2019-10-31 14:50                     ` Steve Grubb
2019-10-31 23:37                       ` Paul Moore
2019-11-01  1:02                       ` Duncan Roe
2019-11-01 15:09                       ` Richard Guy Briggs
2019-11-01 15:13                         ` Steve Grubb
2019-11-01 15:21                           ` Richard Guy Briggs
2019-11-01 16:22                         ` Paul Moore
2019-09-19  1:22 ` [PATCH ghak90 V7 21/21] audit: add proc interface for capcontid Richard Guy Briggs

Reply instructions:

You may reply publically 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='CAHC9VhRMJkeC7HkAMr1TwymtT7eHOB_B=_28R6zVfQQk-gW-DA@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: link

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git