Re: [PATCH ghak90 V7 04/21] audit: convert to contid list to check for orch/engine ownership
On Wed, Sep 18, 2019 at 9:24 PM Richard Guy Briggs 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
> ---
> 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.
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.
> 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_headlist;
> + 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"?
> struct audit_task_info {
> kuid_t loginuid;
> unsigned intsessionid;
> - u64 contid;
> + struct audit_cont *cont;
Same, why not stick with "contid"?
> #ifdef CONFIG_AUDITSYSCALL
> struct audit_context*ctx;
> #endif
> @@ -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.
> +extern struct audit_cont *audit_cont(struct task_struct *tsk);
> +
> +extern void audit_cont_put(struct audit_cont *cont);
More of the "contid" vs "cont".
> extern u32 audit_enabled;
>
> extern int audit_signal_info(int sig, struct task_struct *t);
> @@ -277,6 +289,14 @@ static inline u64 audit_get_contid(struct task_struct
> *tsk)
> return AUDIT_CID_UNSET;
> }
>
> +static inline struct audit_cont *audit_cont(struct task_struct *tsk)
> +{
> + return NULL;
> +}
> +
> +static inline void audit_cont_put(struct audit_cont *cont)
> +{ }
> +
> #define audit_enabled AUDIT_OFF
>
> static inline int audit_signal_info(int sig, struct task_struct *t)
> diff --git a/kernel/audit.c b/kernel/audit.c
> index a36ea57cbb61..ea0899130cc1 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -137,6 +137,8 @@ struct audit_net {
>
> /* Hash for inode-based rules */
> struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
> +/* Hash for contid-based rules */
> +struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
>
> static struct kmem_cache *audit_buffer_cache;
>
> @@ -204,6 +206,8 @@ struct audit_reply {
>
> static struct kmem_cache *audit_task_cache;
>
> +static DEFINE_SPINLOCK(audit_contid_list_lock);
Since it looks like this protectects audit_contid_hash, I think it
would be better to move it up underneath audit_contid_hash.
> void __init audit_task_init(void)
> {
> audit_task_cache = kmem_cache_create("audit_task",
> @@ -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(>cont->refcount);
See the other comments about a "get" function, but I think we need a
RCU read lock
[PATCH ghak90 V7 04/21] audit: convert to contid list to check for orch/engine ownership
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
---
include/linux/audit.h | 26 ++--
kernel/audit.c| 86 ---
kernel/audit.h| 8 +
3 files changed, 112 insertions(+), 8 deletions(-)
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_headlist;
+ u64 id;
+ struct task_struct *owner;
+ refcount_t refcount;
+ struct rcu_head rcu;
+};
+
struct audit_task_info {
kuid_t loginuid;
unsigned intsessionid;
- u64 contid;
+ struct audit_cont *cont;
#ifdef CONFIG_AUDITSYSCALL
struct audit_context*ctx;
#endif
@@ -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;
}
+extern struct audit_cont *audit_cont(struct task_struct *tsk);
+
+extern void audit_cont_put(struct audit_cont *cont);
+
extern u32 audit_enabled;
extern int audit_signal_info(int sig, struct task_struct *t);
@@ -277,6 +289,14 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
return AUDIT_CID_UNSET;
}
+static inline struct audit_cont *audit_cont(struct task_struct *tsk)
+{
+ return NULL;
+}
+
+static inline void audit_cont_put(struct audit_cont *cont)
+{ }
+
#define audit_enabled AUDIT_OFF
static inline int audit_signal_info(int sig, struct task_struct *t)
diff --git a/kernel/audit.c b/kernel/audit.c
index a36ea57cbb61..ea0899130cc1 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -137,6 +137,8 @@ struct audit_net {
/* Hash for inode-based rules */
struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
+/* Hash for contid-based rules */
+struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
static struct kmem_cache *audit_buffer_cache;
@@ -204,6 +206,8 @@ struct audit_reply {
static struct kmem_cache *audit_task_cache;
+static DEFINE_SPINLOCK(audit_contid_list_lock);
+
void __init audit_task_init(void)
{
audit_task_cache = kmem_cache_create("audit_task",
@@ -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(>cont->refcount);
tsk->audit = info;
ret = audit_alloc_syscall(tsk);
@@ -246,7 +252,7 @@ int audit_alloc(struct task_struct *tsk)
struct audit_task_info init_struct_audit = {
.loginuid = INVALID_UID,
.sessionid = AUDIT_SID_UNSET,
- .contid = AUDIT_CID_UNSET,
+ .cont = NULL,
#ifdef CONFIG_AUDITSYSCALL
.ctx = NULL,
#endif
@@ -266,6 +272,9 @@ void audit_free(struct task_struct *tsk)
/* Freeing the audit_task_info struct must be performed after
* audit_log_exit() due to need for loginuid and sessionid.
*/
+ spin_lock(_contid_list_lock);
+ audit_cont_put(tsk->audit->cont);
+ spin_unlock(_contid_list_lock);
info = tsk->audit;
tsk->audit = NULL;
kmem_cache_free(audit_task_cache, info);
@@ -1657,6 +1666,9 @@ static int __init audit_init(void)
for (i = 0; i < AUDIT_INODE_BUCKETS; i++)
INIT_LIST_HEAD(_inode_hash[i]);
+ for (i = 0; i < AUDIT_CONTID_BUCKETS; i++)
+ INIT_LIST_HEAD(_contid_hash[i]);
+
mutex_init(_cmd_mutex.lock);
audit_cmd_mutex.owner = NULL;
@@ -2356,6 +2368,32 @@ int audit_signal_info(int sig, struct task_struct *t)
return audit_signal_info_syscall(t);
}
+struct audit_cont *audit_cont(struct task_struct *tsk)
+{
+ if (!tsk->audit || !tsk->audit->cont)
+ return NULL;
+ return tsk->audit->cont;
+}
+
+/* audit_contid_list_lock must be held by caller */
+void audit_cont_put(struct audit_cont *cont)
+{
+ if (!cont)
+ return;
+ if
