On Thu, Oct 24, 2019 at 5:23 PM Richard Guy Briggs [off-list ref] wrote:

On 2019-10-10 20:38, Paul Moore wrote:

quoted

On Fri, Sep 27, 2019 at 8:52 AM Neil Horman [off-list ref] wrote:

quoted

On Wed, Sep 18, 2019 at 09:22:23PM -0400, Richard Guy Briggs wrote:

quoted

Set an arbitrary limit on the number of audit container identifiers to
limit abuse.

Signed-off-by: Richard Guy Briggs <redacted>
---
 kernel/audit.c | 8 ++++++++
 kernel/audit.h | 4 ++++
 2 files changed, 12 insertions(+)

diff --git a/kernel/audit.c b/kernel/audit.c
index 53d13d638c63..329916534dd2 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c

...

quoted

quoted

@@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct *task, u64 contid)
                              newcont->owner = current;
                              refcount_set(&newcont->refcount, 1);
                              list_add_rcu(&newcont->list, &audit_contid_hash[h]);
+                             audit_contid_count++;
                      } else {
                              rc = -ENOMEM;
                              goto conterror;

diff --git a/kernel/audit.h b/kernel/audit.h
index 162de8366b32..543f1334ba47 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h

@@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 contid)
      return (contid & (AUDIT_CONTID_BUCKETS-1));
 }

+extern int audit_contid_count;
+
+#define AUDIT_CONTID_COUNT   1 << 16
+

Just to ask the question, since it wasn't clear in the changelog, what
abuse are you avoiding here?  Ostensibly you should be able to create as
many container ids as you have space for, and the simple creation of
container ids doesn't seem like the resource strain I would be concerned
about here, given that an orchestrator can still create as many
containers as the system will otherwise allow, which will consume
significantly more ram/disk/etc.

I've got a similar question.  Up to this point in the patchset, there
is a potential issue of hash bucket chain lengths and traversing them
with a spinlock held, but it seems like we shouldn't be putting an
arbitrary limit on audit container IDs unless we have a good reason
for it.  If for some reason we do want to enforce a limit, it should
probably be a tunable value like a sysctl, or similar.

Can you separate and clarify the concerns here?

"Why are you doing this?" is about as simple as I can pose the question.

I plan to move this patch to the end of the patchset and make it
optional, possibly adding a tuning mechanism.  Like the migration from
/proc to netlink for loginuid/sessionid/contid/capcontid, this was Eric
Biederman's concern and suggested mitigation.

Okay, let's just drop it.  I *really* don't like this approach of
tossing questionable stuff at the end of the patchset; I get why you
are doing it, but I think we really need to focus on keeping this
changeset small.  If the number of ACIDs (heh) become unwieldy the
right solution is to improve the algorithms/structures, if we can't do
that for some reason, *then* we can fall back to a limiting knob in a
latter release.

As for the first issue of the bucket chain length traversal while
holding the list spin-lock, would you prefer to use the rcu lock to
traverse the list and then only hold the spin-lock when modifying the
list, and possibly even make the spin-lock more fine-grained per list?

Until we have a better idea of how this is going to be used, I think
it's okay for now.  It's also internal to the kernel so we can change
it at any time.  My comments about the locking/structs was only to try
and think of some reason why one might want to limit the number of
ACIDs since neither you or Eric provided any reasoning that I could
see.

-- 
paul moore
www.paul-moore.com