Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
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 V8 13/16] audit: track container nesting
Date: Fri, 13 Mar 2020 12:47:34 -0400
Message-ID: <CAHC9VhTqWdXMsbSbsWJzRRvVbSaaFBmnFFsVutM7XSx5NT_FJA@mail.gmail.com> (raw)
In-Reply-To: <20200312205147.plxs4czjeuu4davj@madcap2.tricolour.ca>

On Thu, Mar 12, 2020 at 4:52 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2020-02-13 16:49, Paul Moore wrote:
> > On Wed, Feb 5, 2020 at 6:51 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2020-02-05 18:05, Paul Moore wrote:
> > > > On Thu, Jan 30, 2020 at 2:28 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > On 2020-01-22 16:29, Paul Moore wrote:
> > > > > > On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > > >
> > > > > > > Track the parent container of a container to be able to filter and
> > > > > > > report nesting.
> > > > > > >
> > > > > > > Now that we have a way to track and check the parent container of a
> > > > > > > container, modify the contid field format to be able to report that
> > > > > > > nesting using a carrat ("^") separator to indicate nesting.  The
> > > > > > > original field format was "contid=<contid>" for task-associated records
> > > > > > > and "contid=<contid>[,<contid>[...]]" for network-namespace-associated
> > > > > > > records.  The new field format is
> > > > > > > "contid=<contid>[^<contid>[...]][,<contid>[...]]".
> > > > > >
> > > > > > Let's make sure we always use a comma as a separator, even when
> > > > > > recording the parent information, for example:
> > > > > > "contid=<contid>[,^<contid>[...]][,<contid>[...]]"
> > > > >
> > > > > The intent here is to clearly indicate and separate nesting from
> > > > > parallel use of several containers by one netns.  If we do away with
> > > > > that distinction, then we lose that inheritance accountability and
> > > > > should really run the list through a "uniq" function to remove the
> > > > > produced redundancies.  This clear inheritance is something Steve was
> > > > > looking for since tracking down individual events/records to show that
> > > > > inheritance was not aways feasible due to rolled logs or search effort.
> > > >
> > > > Perhaps my example wasn't clear.  I'm not opposed to the little
> > > > carat/hat character indicating a container's parent, I just think it
> > > > would be good to also include a comma *in*addition* to the carat/hat.
> > >
> > > Ah, ok.  Well, I'd offer that it would be slightly shorter, slightly
> > > less cluttered and having already written the parser in userspace, I
> > > think the parser would be slightly simpler.
> > >
> > > I must admit, I was a bit puzzled by your snippet of code that was used
> > > as a prefix to the next item rather than as a postfix to the given item.
> > >
> > > Can you say why you prefer the comma in addition?
> >
> > Generally speaking, I believe that a single delimiter is both easier
> > for the eyes to parse, and easier/safer for machines to parse as well.
> > In this particular case I think of the comma as a delimiter and the
> > carat as a modifier, reusing the carat as a delimiter seems like a bad
> > idea to me.
>
> I'm not crazy about this idea, but I'll have a look at how much work it
> is to recode the userspace search tools.  It also adds extra characters
> and noise into the string format that seems counterproductive.

If anything the parser should be *easier* (although both parsers
should fall into the "trivial" category).  The comma is the one and
only delimiter, and if the ACID starts with a carat then it is a
parent of the preceding ACID.

> > > > > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > > > > index ef8e07524c46..68be59d1a89b 100644
> > > > > > > --- a/kernel/audit.c
> > > > > > > +++ b/kernel/audit.c
> > > > > >
> > > > > > > @@ -492,6 +493,7 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> > > > > > >                 audit_netns_contid_add(new->net_ns, contid);
> > > > > > >  }
> > > > > > >
> > > > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid);
> > > > > >
> > > > > > If we need a forward declaration, might as well just move it up near
> > > > > > the top of the file with the rest of the declarations.
> > > > >
> > > > > Ok.
> > > > >
> > > > > > > +void audit_log_contid(struct audit_buffer *ab, u64 contid)
> > > > > > > +{
> > > > > > > +       struct audit_contobj *cont = NULL, *prcont = NULL;
> > > > > > > +       int h;
> > > > > >
> > > > > > It seems safer to pass the audit container ID object and not the u64.
> > > > >
> > > > > It would also be faster, but in some places it isn't available such as
> > > > > for ptrace and signal targets.  This also links back to the drop record
> > > > > refcounts to hold onto the contobj until process exit, or signal
> > > > > delivery.
> > > > >
> > > > > What we could do is to supply two potential parameters, a contobj and/or
> > > > > a contid, and have it use the contobj if it is valid, otherwise, use the
> > > > > contid, as is done for names and paths supplied to audit_log_name().
> > > >
> > > > Let's not do multiple parameters, that begs for misuse, let's take the
> > > > wrapper function route:
> > > >
> > > >  func a(int id) {
> > > >    // important stuff
> > > >  }
> > > >
> > > >  func ao(struct obj) {
> > > >    a(obj.id);
> > > >  }
> > > >
> > > > ... and we can add a comment that you *really* should be using the
> > > > variant that passes an object.
> > >
> > > I was already doing that where it available, and dereferencing the id
> > > for the call.  But I see an advantage to having both parameters supplied
> > > to the function, since it saves us the trouble of dereferencing it,
> > > searching for the id in the hash list and re-locating the object if the
> > > object is already available.
> >
> > I strongly prefer we not do multiple parameters for the same "thing";
>
> So do I, ideally.  However...
>
> > I would much rather do the wrapper approach as described above.  I
> > would also like to see us use the audit container ID object as much as
> > possible, using a bare integer should be a last resort.
>
> It is not clear to me that you understood what I wrote above.  I can't
> use the object pointer where preferable because there are a few cases
> where only the ID is available.  If only the ID is available, I would
> have to make a best effort to look up the object pointer and am not
> guaranteed to find it (invalid, stale, signal info...).  If I am forced
> to use only one, it becomes the ID that is used, and I no longer have
> the benefit of already having the object pointer for certainty and
> saving work.  For all cases where I have the object pointer, which is
> most cases, and most frequently used cases, I will have to dereference
> the object pointer to an ID, then go through the work again to re-locate
> the object pointer.  This is less certain, and more work.  Reluctantly,
> the only practical solution I see here is to supply both, favouring the
> object pointer if it is valid, then falling back on the ID from the next
> parameter.

It has been a while since I last looked at the patchset, but my
concern over the prefered use of the ACID number vs the ACID object is
that the number offers no reuse protection where the object does.  I
really would like us to use the object everywhere it is possible.

-- 
paul moore
www.paul-moore.com

  reply index

Thread overview: 99+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-31 19:48 [PATCH ghak90 V8 00/16] audit: implement container identifier Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 01/16] audit: collect audit task parameters Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 02/16] audit: add container id Richard Guy Briggs
2020-01-22 21:28   ` Paul Moore
2020-01-30 17:53     ` Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 03/16] audit: read container ID of a process Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 04/16] audit: convert to contid list to check for orch/engine ownership Richard Guy Briggs
2020-01-22 21:28   ` Paul Moore
2020-02-04 22:51     ` Richard Guy Briggs
2020-02-05 22:40       ` Paul Moore
2019-12-31 19:48 ` [PATCH ghak90 V8 05/16] audit: log drop of contid on exit of last task Richard Guy Briggs
2020-01-22 21:28   ` Paul Moore
2020-02-04 23:02     ` Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 06/16] audit: log container info of syscalls Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 07/16] audit: add contid support for signalling the audit daemon Richard Guy Briggs
2020-01-22 21:28   ` Paul Moore
2020-01-23 16:29     ` Richard Guy Briggs
2020-01-23 17:09       ` Paul Moore
2020-01-23 20:04         ` Richard Guy Briggs
2020-01-23 21:35           ` Paul Moore
2020-02-04 23:14             ` Richard Guy Briggs
2020-02-05 22:50               ` Paul Moore
2020-02-12 22:38                 ` Steve Grubb
2020-02-13  0:09                   ` Paul Moore
2020-02-13 21:44                     ` Paul Moore
2020-03-12 19:30                       ` Richard Guy Briggs
2020-03-13 16:29                         ` Paul Moore
2020-03-13 18:59                           ` Richard Guy Briggs
2020-03-18 20:56                             ` Paul Moore
2020-03-18 21:26                               ` Richard Guy Briggs
2020-03-18 21:42                                 ` Paul Moore
2020-03-18 21:55                                   ` Richard Guy Briggs
2020-03-18 22:06                                     ` Paul Moore
2020-03-19 22:02                                       ` Richard Guy Briggs
2020-03-24  0:16                                         ` Paul Moore
2020-03-24 21:01                                           ` Richard Guy Briggs
2020-03-29  3:11                                             ` Paul Moore
2020-03-30 13:47                                               ` Richard Guy Briggs
2020-03-30 14:26                                                 ` Paul Moore
2020-03-30 16:21                                                   ` Richard Guy Briggs
2020-03-30 17:34                                                     ` Paul Moore
2020-03-30 17:49                                                       ` Richard Guy Briggs
2020-03-30 19:55                                                         ` Paul Moore
2020-04-16 20:33                                                           ` Eric W. Biederman
2020-04-16 21:53                                                             ` Paul Moore
2020-04-17 22:23                                                               ` Eric W. Biederman
2020-04-22 17:24                                                                 ` Paul Moore
2020-03-12 20:27                     ` Richard Guy Briggs
2020-03-13 16:42                       ` Paul Moore
2020-03-13 16:45                         ` Steve Grubb
2020-03-13 16:49                           ` Paul Moore
2020-03-13 19:23                         ` Richard Guy Briggs
2020-03-18 21:01                           ` Paul Moore
2020-03-18 21:41                             ` Richard Guy Briggs
2020-03-18 21:47                               ` Paul Moore
2020-03-19 21:47                                 ` Richard Guy Briggs
2020-03-20 21:56                                   ` Paul Moore
2020-03-25 12:29                                     ` Richard Guy Briggs
2020-03-29  3:17                                       ` Paul Moore
2020-03-30 15:23                                         ` Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 08/16] audit: add support for non-syscall auxiliary records Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 09/16] audit: add containerid support for user records Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 10/16] audit: add containerid filtering Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 11/16] audit: add support for containerid to network namespaces Richard Guy Briggs
2020-01-22 21:28   ` Paul Moore
2020-02-04 23:42     ` Richard Guy Briggs
2020-02-05 22:51       ` Paul Moore
2019-12-31 19:48 ` [PATCH ghak90 V8 12/16] audit: contid check descendancy and nesting Richard Guy Briggs
2020-01-22 21:29   ` Paul Moore
2020-01-23 21:02     ` Richard Guy Briggs
2020-01-23 21:47       ` Paul Moore
2019-12-31 19:48 ` [PATCH ghak90 V8 13/16] audit: track container nesting Richard Guy Briggs
2020-01-22 21:29   ` Paul Moore
2020-01-30 19:27     ` Richard Guy Briggs
2020-02-05 23:05       ` Paul Moore
2020-02-05 23:50         ` Richard Guy Briggs
2020-02-13 21:49           ` Paul Moore
2020-03-12 20:51             ` Richard Guy Briggs
2020-03-13 16:47               ` Paul Moore [this message]
2020-03-14 22:42                 ` Richard Guy Briggs
2020-03-17 18:28                   ` Richard Guy Briggs
2020-03-18 21:08                   ` Paul Moore
2020-01-31 14:50     ` Steve Grubb
2020-02-04 13:19       ` Richard Guy Briggs
2020-02-04 15:47         ` Steve Grubb
2020-02-04 15:52           ` Paul Moore
2020-02-04 18:12             ` Steve Grubb
2020-02-05 22:57               ` Paul Moore
2019-12-31 19:48 ` [PATCH ghak90 V8 14/16] audit: check contid depth and add limit config param Richard Guy Briggs
2020-01-22 21:29   ` Paul Moore
2019-12-31 19:48 ` [PATCH ghak90 V8 15/16] audit: check contid count per netns and add config param limit Richard Guy Briggs
2020-01-22 21:29   ` Paul Moore
2019-12-31 19:48 ` [PATCH ghak90 V8 16/16] audit: add capcontid to set contid outside init_user_ns Richard Guy Briggs
2020-01-22 21:29   ` Paul Moore
2020-02-05  0:39     ` Richard Guy Briggs
2020-02-05 22:56       ` Paul Moore
2020-02-06 12:51         ` Richard Guy Briggs
2020-02-13 21:58           ` Paul Moore
2020-03-12 21:58             ` 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=CAHC9VhTqWdXMsbSbsWJzRRvVbSaaFBmnFFsVutM7XSx5NT_FJA@mail.gmail.com \
    --to=paul@paul-moore.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=mpatel@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=netfilter-devel@vger.kernel.org \
    --cc=nhorman@tuxdriver.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

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/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 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


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