* [RFC PATCH 0/4] Assorted audit fixes/improvements
@ 2017-03-21 18:58 Paul Moore
2017-03-21 18:58 ` [RFC PATCH 1/4] audit: combine audit_receive() and audit_receive_skb() Paul Moore
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Paul Moore @ 2017-03-21 18:58 UTC (permalink / raw)
To: linux-audit
Some miscellaneous audit patches that layer on top of the lock/queue
patch I posted earlier today. Unlike the lock/queue fix patch these
patches would go into the next branch and not the stable-4.11 branch.
I'll probably need to rebase the audit/next branch to merge these
patches, but that is something we'll worry about when the time comes.
---
Paul Moore (4):
audit: combine audit_receive() and audit_receive_skb()
audit: kernel generated netlink traffic should have a portid of 0
audit: store the auditd PID as a pid struct instead of pid_t
audit: use kmem_cache to manage the audit_buffer cache
include/linux/audit.h | 3 -
kernel/audit.c | 190 ++++++++++++++++++++++---------------------------
kernel/audit.h | 5 +
kernel/auditfilter.c | 14 ++--
4 files changed, 96 insertions(+), 116 deletions(-)
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH 1/4] audit: combine audit_receive() and audit_receive_skb()
2017-03-21 18:58 [RFC PATCH 0/4] Assorted audit fixes/improvements Paul Moore
@ 2017-03-21 18:58 ` Paul Moore
2017-04-10 3:38 ` Richard Guy Briggs
2017-03-21 18:58 ` [RFC PATCH 2/4] audit: kernel generated netlink traffic should have a portid of 0 Paul Moore
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: Paul Moore @ 2017-03-21 18:58 UTC (permalink / raw)
To: linux-audit
From: Paul Moore <paul@paul-moore.com>
There is no reason to have both of these functions, combine the two.
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
kernel/audit.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 2f4964cfde0b..4037869b4b64 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1381,11 +1381,14 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
return err < 0 ? err : 0;
}
-/*
- * Get message from skb. Each message is processed by audit_receive_msg.
- * Malformed skbs with wrong length are discarded silently.
+/**
+ * audit_receive - receive messages from a netlink control socket
+ * @skb: the message buffer
+ *
+ * Parse the provided skb and deal with any messages that may be present,
+ * malformed skbs are discarded.
*/
-static void audit_receive_skb(struct sk_buff *skb)
+static void audit_receive(struct sk_buff *skb)
{
struct nlmsghdr *nlh;
/*
@@ -1398,6 +1401,7 @@ static void audit_receive_skb(struct sk_buff *skb)
nlh = nlmsg_hdr(skb);
len = skb->len;
+ mutex_lock(&audit_cmd_mutex);
while (nlmsg_ok(nlh, len)) {
err = audit_receive_msg(skb, nlh);
/* if err or if this message says it wants a response */
@@ -1406,13 +1410,6 @@ static void audit_receive_skb(struct sk_buff *skb)
nlh = nlmsg_next(nlh, &len);
}
-}
-
-/* Receive messages from netlink socket. */
-static void audit_receive(struct sk_buff *skb)
-{
- mutex_lock(&audit_cmd_mutex);
- audit_receive_skb(skb);
mutex_unlock(&audit_cmd_mutex);
}
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC PATCH 2/4] audit: kernel generated netlink traffic should have a portid of 0
2017-03-21 18:58 [RFC PATCH 0/4] Assorted audit fixes/improvements Paul Moore
2017-03-21 18:58 ` [RFC PATCH 1/4] audit: combine audit_receive() and audit_receive_skb() Paul Moore
@ 2017-03-21 18:58 ` Paul Moore
2017-04-10 3:41 ` Richard Guy Briggs
2017-03-21 18:59 ` [RFC PATCH 3/4] audit: store the auditd PID as a pid struct instead of pid_t Paul Moore
2017-03-21 18:59 ` [RFC PATCH 4/4] audit: use kmem_cache to manage the audit_buffer cache Paul Moore
3 siblings, 1 reply; 16+ messages in thread
From: Paul Moore @ 2017-03-21 18:58 UTC (permalink / raw)
To: linux-audit
From: Paul Moore <paul@paul-moore.com>
We were setting the portid incorrectly in the netlink message headers,
fix that to always be 0 (nlmsg_pid = 0).
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
include/linux/audit.h | 3 +--
kernel/audit.c | 23 ++++++-----------------
kernel/audit.h | 3 +--
kernel/auditfilter.c | 14 ++++++--------
4 files changed, 14 insertions(+), 29 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 504e784b7ffa..cc0497c39472 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -163,8 +163,7 @@ extern void audit_log_task_info(struct audit_buffer *ab,
extern int audit_update_lsm_rules(void);
/* Private API (for audit.c only) */
-extern int audit_rule_change(int type, __u32 portid, int seq,
- void *data, size_t datasz);
+extern int audit_rule_change(int type, int seq, void *data, size_t datasz);
extern int audit_list_rules_send(struct sk_buff *request_skb, int seq);
extern u32 audit_enabled;
diff --git a/kernel/audit.c b/kernel/audit.c
index 4037869b4b64..6cbf47a372e8 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -251,14 +251,6 @@ static struct sock *audit_get_sk(const struct net *net)
return aunet->sk;
}
-static void audit_set_portid(struct audit_buffer *ab, __u32 portid)
-{
- if (ab) {
- struct nlmsghdr *nlh = nlmsg_hdr(ab->skb);
- nlh->nlmsg_pid = portid;
- }
-}
-
void audit_panic(const char *message)
{
switch (audit_failure) {
@@ -819,7 +811,7 @@ int audit_send_list(void *_dest)
return 0;
}
-struct sk_buff *audit_make_reply(__u32 portid, int seq, int type, int done,
+struct sk_buff *audit_make_reply(int seq, int type, int done,
int multi, const void *payload, int size)
{
struct sk_buff *skb;
@@ -832,7 +824,7 @@ struct sk_buff *audit_make_reply(__u32 portid, int seq, int type, int done,
if (!skb)
return NULL;
- nlh = nlmsg_put(skb, portid, seq, t, size, flags);
+ nlh = nlmsg_put(skb, 0, seq, t, size, flags);
if (!nlh)
goto out_kfree_skb;
data = nlmsg_data(nlh);
@@ -876,7 +868,6 @@ static int audit_send_reply_thread(void *arg)
static void audit_send_reply(struct sk_buff *request_skb, int seq, int type, int done,
int multi, const void *payload, int size)
{
- u32 portid = NETLINK_CB(request_skb).portid;
struct net *net = sock_net(NETLINK_CB(request_skb).sk);
struct sk_buff *skb;
struct task_struct *tsk;
@@ -886,12 +877,12 @@ static void audit_send_reply(struct sk_buff *request_skb, int seq, int type, int
if (!reply)
return;
- skb = audit_make_reply(portid, seq, type, done, multi, payload, size);
+ skb = audit_make_reply(seq, type, done, multi, payload, size);
if (!skb)
goto out;
reply->net = get_net(net);
- reply->portid = portid;
+ reply->portid = NETLINK_CB(request_skb).portid;
reply->skb = skb;
tsk = kthread_run(audit_send_reply_thread, reply, "audit_send_reply");
@@ -1075,7 +1066,7 @@ static int audit_replace(pid_t pid)
{
struct sk_buff *skb;
- skb = audit_make_reply(0, 0, AUDIT_REPLACE, 0, 0, &pid, sizeof(pid));
+ skb = audit_make_reply(0, AUDIT_REPLACE, 0, 0, &pid, sizeof(pid));
if (!skb)
return -ENOMEM;
return auditd_send_unicast_skb(skb);
@@ -1245,7 +1236,6 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
size--;
audit_log_n_untrustedstring(ab, data, size);
}
- audit_set_portid(ab, NETLINK_CB(skb).portid);
audit_log_end(ab);
}
break;
@@ -1259,8 +1249,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
audit_log_end(ab);
return -EPERM;
}
- err = audit_rule_change(msg_type, NETLINK_CB(skb).portid,
- seq, data, nlmsg_len(nlh));
+ err = audit_rule_change(msg_type, seq, data, nlmsg_len(nlh));
break;
case AUDIT_LIST_RULES:
err = audit_list_rules_send(skb, seq);
diff --git a/kernel/audit.h b/kernel/audit.h
index 0f1cf6d1878a..c21b74dd7ff2 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -237,8 +237,7 @@ extern int audit_uid_comparator(kuid_t left, u32 op, kuid_t right);
extern int audit_gid_comparator(kgid_t left, u32 op, kgid_t right);
extern int parent_len(const char *path);
extern int audit_compare_dname_path(const char *dname, const char *path, int plen);
-extern struct sk_buff *audit_make_reply(__u32 portid, int seq, int type,
- int done, int multi,
+extern struct sk_buff *audit_make_reply(int seq, int type, int done, int multi,
const void *payload, int size);
extern void audit_panic(const char *message);
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 880519d6cf2a..81cdf8d8f319 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1033,7 +1033,7 @@ int audit_del_rule(struct audit_entry *entry)
}
/* List rules using struct audit_rule_data. */
-static void audit_list_rules(__u32 portid, int seq, struct sk_buff_head *q)
+static void audit_list_rules(int seq, struct sk_buff_head *q)
{
struct sk_buff *skb;
struct audit_krule *r;
@@ -1048,15 +1048,15 @@ static void audit_list_rules(__u32 portid, int seq, struct sk_buff_head *q)
data = audit_krule_to_data(r);
if (unlikely(!data))
break;
- skb = audit_make_reply(portid, seq, AUDIT_LIST_RULES,
- 0, 1, data,
+ skb = audit_make_reply(seq, AUDIT_LIST_RULES, 0, 1,
+ data,
sizeof(*data) + data->buflen);
if (skb)
skb_queue_tail(q, skb);
kfree(data);
}
}
- skb = audit_make_reply(portid, seq, AUDIT_LIST_RULES, 1, 1, NULL, 0);
+ skb = audit_make_reply(seq, AUDIT_LIST_RULES, 1, 1, NULL, 0);
if (skb)
skb_queue_tail(q, skb);
}
@@ -1085,13 +1085,11 @@ static void audit_log_rule_change(char *action, struct audit_krule *rule, int re
/**
* audit_rule_change - apply all rules to the specified message type
* @type: audit message type
- * @portid: target port id for netlink audit messages
* @seq: netlink audit message sequence (serial) number
* @data: payload data
* @datasz: size of payload data
*/
-int audit_rule_change(int type, __u32 portid, int seq, void *data,
- size_t datasz)
+int audit_rule_change(int type, int seq, void *data, size_t datasz)
{
int err = 0;
struct audit_entry *entry;
@@ -1150,7 +1148,7 @@ int audit_list_rules_send(struct sk_buff *request_skb, int seq)
skb_queue_head_init(&dest->q);
mutex_lock(&audit_filter_mutex);
- audit_list_rules(portid, seq, &dest->q);
+ audit_list_rules(seq, &dest->q);
mutex_unlock(&audit_filter_mutex);
tsk = kthread_run(audit_send_list, dest, "audit_send_list");
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC PATCH 3/4] audit: store the auditd PID as a pid struct instead of pid_t
2017-03-21 18:58 [RFC PATCH 0/4] Assorted audit fixes/improvements Paul Moore
2017-03-21 18:58 ` [RFC PATCH 1/4] audit: combine audit_receive() and audit_receive_skb() Paul Moore
2017-03-21 18:58 ` [RFC PATCH 2/4] audit: kernel generated netlink traffic should have a portid of 0 Paul Moore
@ 2017-03-21 18:59 ` Paul Moore
2017-04-10 4:30 ` Richard Guy Briggs
2017-03-21 18:59 ` [RFC PATCH 4/4] audit: use kmem_cache to manage the audit_buffer cache Paul Moore
3 siblings, 1 reply; 16+ messages in thread
From: Paul Moore @ 2017-03-21 18:59 UTC (permalink / raw)
To: linux-audit
From: Paul Moore <paul@paul-moore.com>
This is arguably the right thing to do, and will make it easier when
we start supporting multiple audit daemons in different namespaces.
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
kernel/audit.c | 84 ++++++++++++++++++++++++++++++++++++++------------------
kernel/audit.h | 2 +
2 files changed, 58 insertions(+), 28 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 6cbf47a372e8..b718bf3a73f8 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -58,6 +58,7 @@
#include <linux/rcupdate.h>
#include <linux/mutex.h>
#include <linux/gfp.h>
+#include <linux/pid.h>
#include <linux/audit.h>
@@ -117,7 +118,7 @@ struct audit_net {
* or the included spinlock for writing.
*/
static struct auditd_connection {
- int pid;
+ struct pid *pid;
u32 portid;
struct net *net;
spinlock_t lock;
@@ -221,18 +222,41 @@ struct audit_reply {
* Description:
* Return 1 if the task is a registered audit daemon, 0 otherwise.
*/
-int auditd_test_task(const struct task_struct *task)
+int auditd_test_task(struct task_struct *task)
{
int rc;
rcu_read_lock();
- rc = (auditd_conn.pid && task->tgid == auditd_conn.pid ? 1 : 0);
+ rc = (auditd_conn.pid && auditd_conn.pid == task_tgid(task) ? 1 : 0);
rcu_read_unlock();
return rc;
}
/**
+ * auditd_pid_vnr - Return the auditd PID relative to the namespace
+ * @auditd: the auditd connection
+ *
+ * Description:
+ * Returns the PID in relation to the namespace, 0 on failure. This function
+ * takes the RCU read lock internally, but if the caller needs to protect the
+ * auditd_connection pointer it should take the RCU read lock as well.
+ */
+static pid_t auditd_pid_vnr(const struct auditd_connection *auditd)
+{
+ pid_t pid;
+
+ rcu_read_lock();
+ if (!auditd || !auditd->pid)
+ pid = 0;
+ else
+ pid = pid_vnr(auditd->pid);
+ rcu_read_unlock();
+
+ return pid;
+}
+
+/**
* audit_get_sk - Return the audit socket for the given network namespace
* @net: the destination network namespace
*
@@ -429,12 +453,17 @@ static int audit_set_failure(u32 state)
* This function will obtain and drop network namespace references as
* necessary.
*/
-static void auditd_set(int pid, u32 portid, struct net *net)
+static void auditd_set(struct pid *pid, u32 portid, struct net *net)
{
unsigned long flags;
spin_lock_irqsave(&auditd_conn.lock, flags);
- auditd_conn.pid = pid;
+ if (auditd_conn.pid)
+ put_pid(auditd_conn.pid);
+ if (pid)
+ auditd_conn.pid = get_pid(pid);
+ else
+ auditd_conn.pid = NULL;
auditd_conn.portid = portid;
if (auditd_conn.net)
put_net(auditd_conn.net);
@@ -1062,11 +1091,13 @@ static int audit_set_feature(struct sk_buff *skb)
return 0;
}
-static int audit_replace(pid_t pid)
+static int audit_replace(struct pid *pid)
{
+ pid_t pvnr;
struct sk_buff *skb;
- skb = audit_make_reply(0, AUDIT_REPLACE, 0, 0, &pid, sizeof(pid));
+ pvnr = pid_vnr(pid);
+ skb = audit_make_reply(0, AUDIT_REPLACE, 0, 0, &pvnr, sizeof(pvnr));
if (!skb)
return -ENOMEM;
return auditd_send_unicast_skb(skb);
@@ -1096,9 +1127,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
memset(&s, 0, sizeof(s));
s.enabled = audit_enabled;
s.failure = audit_failure;
- rcu_read_lock();
- s.pid = auditd_conn.pid;
- rcu_read_unlock();
+ /* NOTE: use pid_vnr() so the PID is relative to the current
+ * namespace */
+ s.pid = auditd_pid_vnr(&auditd_conn);
s.rate_limit = audit_rate_limit;
s.backlog_limit = audit_backlog_limit;
s.lost = atomic_read(&audit_lost);
@@ -1124,36 +1155,36 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
return err;
}
if (s.mask & AUDIT_STATUS_PID) {
- /* NOTE: we are using task_tgid_vnr() below because
- * the s.pid value is relative to the namespace
- * of the caller; at present this doesn't matter
- * much since you can really only run auditd
- * from the initial pid namespace, but something
- * to keep in mind if this changes */
- int new_pid = s.pid;
+ /* NOTE: we are using the vnr PID functions below
+ * because the s.pid value is relative to the
+ * namespace of the caller; at present this
+ * doesn't matter much since you can really only
+ * run auditd from the initial pid namespace, but
+ * something to keep in mind if this changes */
+ pid_t new_pid = s.pid;
pid_t auditd_pid;
- pid_t requesting_pid = task_tgid_vnr(current);
+ struct pid *req_pid = task_tgid(current);
+
+ /* sanity check - PID values must match */
+ if (new_pid != pid_vnr(req_pid))
+ return -EINVAL;
/* test the auditd connection */
- audit_replace(requesting_pid);
+ audit_replace(req_pid);
- rcu_read_lock();
- auditd_pid = auditd_conn.pid;
+ auditd_pid = auditd_pid_vnr(&auditd_conn);
/* only the current auditd can unregister itself */
- if ((!new_pid) && (requesting_pid != auditd_pid)) {
- rcu_read_unlock();
+ if ((!new_pid) && (new_pid != auditd_pid)) {
audit_log_config_change("audit_pid", new_pid,
auditd_pid, 0);
return -EACCES;
}
/* replacing a healthy auditd is not allowed */
if (auditd_pid && new_pid) {
- rcu_read_unlock();
audit_log_config_change("audit_pid", new_pid,
auditd_pid, 0);
return -EEXIST;
}
- rcu_read_unlock();
if (audit_enabled != AUDIT_OFF)
audit_log_config_change("audit_pid", new_pid,
@@ -1161,8 +1192,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
if (new_pid) {
/* register a new auditd connection */
- auditd_set(new_pid,
- NETLINK_CB(skb).portid,
+ auditd_set(req_pid, NETLINK_CB(skb).portid,
sock_net(NETLINK_CB(skb).sk));
/* try to process any backlog */
wake_up_interruptible(&kauditd_wait);
diff --git a/kernel/audit.h b/kernel/audit.h
index c21b74dd7ff2..d9b9af769128 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -218,7 +218,7 @@ extern void audit_log_name(struct audit_context *context,
struct audit_names *n, const struct path *path,
int record_num, int *call_panic);
-extern int auditd_test_task(const struct task_struct *task);
+extern int auditd_test_task(struct task_struct *task);
#define AUDIT_INODE_BUCKETS 32
extern struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC PATCH 4/4] audit: use kmem_cache to manage the audit_buffer cache
2017-03-21 18:58 [RFC PATCH 0/4] Assorted audit fixes/improvements Paul Moore
` (2 preceding siblings ...)
2017-03-21 18:59 ` [RFC PATCH 3/4] audit: store the auditd PID as a pid struct instead of pid_t Paul Moore
@ 2017-03-21 18:59 ` Paul Moore
2017-03-21 19:04 ` Paul Moore
2017-04-10 4:04 ` Richard Guy Briggs
3 siblings, 2 replies; 16+ messages in thread
From: Paul Moore @ 2017-03-21 18:59 UTC (permalink / raw)
To: linux-audit
From: Paul Moore <paul@paul-moore.com>
The audit subsystem implemented its own buffer cache mechanism which
is a bit silly these days when we could use the kmem_cache construct.
Some credit is due to Florian Westphal for originally proposing that
we remove the audit cache implementation in favor of simple
kmalloc()/kfree() calls, but I would rather have a dedicated slab
cache to ease debugging and future stats/performance work.
Cc: Florian Westphal <fw@strlen.de>
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
kernel/audit.c | 66 ++++++++++++++------------------------------------------
1 file changed, 17 insertions(+), 49 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index b718bf3a73f8..f78cdd75a4d2 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -59,6 +59,7 @@
#include <linux/mutex.h>
#include <linux/gfp.h>
#include <linux/pid.h>
+#include <linux/slab.h>
#include <linux/audit.h>
@@ -152,12 +153,7 @@ static atomic_t audit_lost = ATOMIC_INIT(0);
/* Hash for inode-based rules */
struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
-/* The audit_freelist is a list of pre-allocated audit buffers (if more
- * than AUDIT_MAXFREE are in use, the audit buffer is freed instead of
- * being placed on the freelist). */
-static DEFINE_SPINLOCK(audit_freelist_lock);
-static int audit_freelist_count;
-static LIST_HEAD(audit_freelist);
+static struct kmem_cache *audit_buffer_cache;
/* queue msgs to send via kauditd_task */
static struct sk_buff_head audit_queue;
@@ -193,17 +189,12 @@ DEFINE_MUTEX(audit_cmd_mutex);
* should be at least that large. */
#define AUDIT_BUFSIZ 1024
-/* AUDIT_MAXFREE is the number of empty audit_buffers we keep on the
- * audit_freelist. Doing so eliminates many kmalloc/kfree calls. */
-#define AUDIT_MAXFREE (2*NR_CPUS)
-
/* The audit_buffer is used when formatting an audit record. The caller
* locks briefly to get the record off the freelist or to allocate the
* buffer, and locks briefly to send the buffer to the netlink layer or
* to place it on a transmit queue. Multiple audit_buffers can be in
* use simultaneously. */
struct audit_buffer {
- struct list_head list;
struct sk_buff *skb; /* formatted skb ready to send */
struct audit_context *ctx; /* NULL or associated context */
gfp_t gfp_mask;
@@ -1489,6 +1480,10 @@ static int __init audit_init(void)
if (audit_initialized == AUDIT_DISABLED)
return 0;
+ audit_buffer_cache = kmem_cache_create("audit_buffer",
+ sizeof(struct audit_buffer),
+ 0, SLAB_PANIC, NULL);
+
memset(&auditd_conn, 0, sizeof(auditd_conn));
spin_lock_init(&auditd_conn.lock);
@@ -1557,60 +1552,33 @@ __setup("audit_backlog_limit=", audit_backlog_limit_set);
static void audit_buffer_free(struct audit_buffer *ab)
{
- unsigned long flags;
-
if (!ab)
return;
kfree_skb(ab->skb);
- spin_lock_irqsave(&audit_freelist_lock, flags);
- if (audit_freelist_count > AUDIT_MAXFREE)
- kfree(ab);
- else {
- audit_freelist_count++;
- list_add(&ab->list, &audit_freelist);
- }
- spin_unlock_irqrestore(&audit_freelist_lock, flags);
+ kmem_cache_free(audit_buffer_cache, ab);
}
-static struct audit_buffer * audit_buffer_alloc(struct audit_context *ctx,
- gfp_t gfp_mask, int type)
+static struct audit_buffer *audit_buffer_alloc(struct audit_context *ctx,
+ gfp_t gfp_mask, int type)
{
- unsigned long flags;
- struct audit_buffer *ab = NULL;
- struct nlmsghdr *nlh;
-
- spin_lock_irqsave(&audit_freelist_lock, flags);
- if (!list_empty(&audit_freelist)) {
- ab = list_entry(audit_freelist.next,
- struct audit_buffer, list);
- list_del(&ab->list);
- --audit_freelist_count;
- }
- spin_unlock_irqrestore(&audit_freelist_lock, flags);
-
- if (!ab) {
- ab = kmalloc(sizeof(*ab), gfp_mask);
- if (!ab)
- goto err;
- }
+ struct audit_buffer *ab;
- ab->ctx = ctx;
- ab->gfp_mask = gfp_mask;
+ ab = kmem_cache_alloc(audit_buffer_cache, gfp_mask);
+ if (!ab)
+ return NULL;
ab->skb = nlmsg_new(AUDIT_BUFSIZ, gfp_mask);
if (!ab->skb)
goto err;
+ if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
+ goto err;
- nlh = nlmsg_put(ab->skb, 0, 0, type, 0, 0);
- if (!nlh)
- goto out_kfree_skb;
+ ab->ctx = ctx;
+ ab->gfp_mask = gfp_mask;
return ab;
-out_kfree_skb:
- kfree_skb(ab->skb);
- ab->skb = NULL;
err:
audit_buffer_free(ab);
return NULL;
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 4/4] audit: use kmem_cache to manage the audit_buffer cache
2017-03-21 18:59 ` [RFC PATCH 4/4] audit: use kmem_cache to manage the audit_buffer cache Paul Moore
@ 2017-03-21 19:04 ` Paul Moore
2017-04-10 4:04 ` Richard Guy Briggs
1 sibling, 0 replies; 16+ messages in thread
From: Paul Moore @ 2017-03-21 19:04 UTC (permalink / raw)
To: Florian Westphal; +Cc: linux-audit
... and I forgot to explicitly add Florian to the email, fixing that now.
On Tue, Mar 21, 2017 at 2:59 PM, Paul Moore <pmoore@redhat.com> wrote:
> From: Paul Moore <paul@paul-moore.com>
>
> The audit subsystem implemented its own buffer cache mechanism which
> is a bit silly these days when we could use the kmem_cache construct.
>
> Some credit is due to Florian Westphal for originally proposing that
> we remove the audit cache implementation in favor of simple
> kmalloc()/kfree() calls, but I would rather have a dedicated slab
> cache to ease debugging and future stats/performance work.
>
> Cc: Florian Westphal <fw@strlen.de>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
> kernel/audit.c | 66 ++++++++++++++------------------------------------------
> 1 file changed, 17 insertions(+), 49 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index b718bf3a73f8..f78cdd75a4d2 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -59,6 +59,7 @@
> #include <linux/mutex.h>
> #include <linux/gfp.h>
> #include <linux/pid.h>
> +#include <linux/slab.h>
>
> #include <linux/audit.h>
>
> @@ -152,12 +153,7 @@ static atomic_t audit_lost = ATOMIC_INIT(0);
> /* Hash for inode-based rules */
> struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
>
> -/* The audit_freelist is a list of pre-allocated audit buffers (if more
> - * than AUDIT_MAXFREE are in use, the audit buffer is freed instead of
> - * being placed on the freelist). */
> -static DEFINE_SPINLOCK(audit_freelist_lock);
> -static int audit_freelist_count;
> -static LIST_HEAD(audit_freelist);
> +static struct kmem_cache *audit_buffer_cache;
>
> /* queue msgs to send via kauditd_task */
> static struct sk_buff_head audit_queue;
> @@ -193,17 +189,12 @@ DEFINE_MUTEX(audit_cmd_mutex);
> * should be at least that large. */
> #define AUDIT_BUFSIZ 1024
>
> -/* AUDIT_MAXFREE is the number of empty audit_buffers we keep on the
> - * audit_freelist. Doing so eliminates many kmalloc/kfree calls. */
> -#define AUDIT_MAXFREE (2*NR_CPUS)
> -
> /* The audit_buffer is used when formatting an audit record. The caller
> * locks briefly to get the record off the freelist or to allocate the
> * buffer, and locks briefly to send the buffer to the netlink layer or
> * to place it on a transmit queue. Multiple audit_buffers can be in
> * use simultaneously. */
> struct audit_buffer {
> - struct list_head list;
> struct sk_buff *skb; /* formatted skb ready to send */
> struct audit_context *ctx; /* NULL or associated context */
> gfp_t gfp_mask;
> @@ -1489,6 +1480,10 @@ static int __init audit_init(void)
> if (audit_initialized == AUDIT_DISABLED)
> return 0;
>
> + audit_buffer_cache = kmem_cache_create("audit_buffer",
> + sizeof(struct audit_buffer),
> + 0, SLAB_PANIC, NULL);
> +
> memset(&auditd_conn, 0, sizeof(auditd_conn));
> spin_lock_init(&auditd_conn.lock);
>
> @@ -1557,60 +1552,33 @@ __setup("audit_backlog_limit=", audit_backlog_limit_set);
>
> static void audit_buffer_free(struct audit_buffer *ab)
> {
> - unsigned long flags;
> -
> if (!ab)
> return;
>
> kfree_skb(ab->skb);
> - spin_lock_irqsave(&audit_freelist_lock, flags);
> - if (audit_freelist_count > AUDIT_MAXFREE)
> - kfree(ab);
> - else {
> - audit_freelist_count++;
> - list_add(&ab->list, &audit_freelist);
> - }
> - spin_unlock_irqrestore(&audit_freelist_lock, flags);
> + kmem_cache_free(audit_buffer_cache, ab);
> }
>
> -static struct audit_buffer * audit_buffer_alloc(struct audit_context *ctx,
> - gfp_t gfp_mask, int type)
> +static struct audit_buffer *audit_buffer_alloc(struct audit_context *ctx,
> + gfp_t gfp_mask, int type)
> {
> - unsigned long flags;
> - struct audit_buffer *ab = NULL;
> - struct nlmsghdr *nlh;
> -
> - spin_lock_irqsave(&audit_freelist_lock, flags);
> - if (!list_empty(&audit_freelist)) {
> - ab = list_entry(audit_freelist.next,
> - struct audit_buffer, list);
> - list_del(&ab->list);
> - --audit_freelist_count;
> - }
> - spin_unlock_irqrestore(&audit_freelist_lock, flags);
> -
> - if (!ab) {
> - ab = kmalloc(sizeof(*ab), gfp_mask);
> - if (!ab)
> - goto err;
> - }
> + struct audit_buffer *ab;
>
> - ab->ctx = ctx;
> - ab->gfp_mask = gfp_mask;
> + ab = kmem_cache_alloc(audit_buffer_cache, gfp_mask);
> + if (!ab)
> + return NULL;
>
> ab->skb = nlmsg_new(AUDIT_BUFSIZ, gfp_mask);
> if (!ab->skb)
> goto err;
> + if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
> + goto err;
>
> - nlh = nlmsg_put(ab->skb, 0, 0, type, 0, 0);
> - if (!nlh)
> - goto out_kfree_skb;
> + ab->ctx = ctx;
> + ab->gfp_mask = gfp_mask;
>
> return ab;
>
> -out_kfree_skb:
> - kfree_skb(ab->skb);
> - ab->skb = NULL;
> err:
> audit_buffer_free(ab);
> return NULL;
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/4] audit: combine audit_receive() and audit_receive_skb()
2017-03-21 18:58 ` [RFC PATCH 1/4] audit: combine audit_receive() and audit_receive_skb() Paul Moore
@ 2017-04-10 3:38 ` Richard Guy Briggs
2017-04-11 19:47 ` Paul Moore
0 siblings, 1 reply; 16+ messages in thread
From: Richard Guy Briggs @ 2017-04-10 3:38 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit
On 2017-03-21 14:58, Paul Moore wrote:
> From: Paul Moore <paul@paul-moore.com>
>
> There is no reason to have both of these functions, combine the two.
>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
Reviewed-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> kernel/audit.c | 19 ++++++++-----------
> 1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 2f4964cfde0b..4037869b4b64 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1381,11 +1381,14 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> return err < 0 ? err : 0;
> }
>
> -/*
> - * Get message from skb. Each message is processed by audit_receive_msg.
> - * Malformed skbs with wrong length are discarded silently.
> +/**
> + * audit_receive - receive messages from a netlink control socket
> + * @skb: the message buffer
> + *
> + * Parse the provided skb and deal with any messages that may be present,
> + * malformed skbs are discarded.
> */
> -static void audit_receive_skb(struct sk_buff *skb)
> +static void audit_receive(struct sk_buff *skb)
> {
> struct nlmsghdr *nlh;
> /*
> @@ -1398,6 +1401,7 @@ static void audit_receive_skb(struct sk_buff *skb)
> nlh = nlmsg_hdr(skb);
> len = skb->len;
>
> + mutex_lock(&audit_cmd_mutex);
> while (nlmsg_ok(nlh, len)) {
> err = audit_receive_msg(skb, nlh);
> /* if err or if this message says it wants a response */
> @@ -1406,13 +1410,6 @@ static void audit_receive_skb(struct sk_buff *skb)
>
> nlh = nlmsg_next(nlh, &len);
> }
> -}
> -
> -/* Receive messages from netlink socket. */
> -static void audit_receive(struct sk_buff *skb)
> -{
> - mutex_lock(&audit_cmd_mutex);
> - audit_receive_skb(skb);
> mutex_unlock(&audit_cmd_mutex);
> }
>
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
- 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/4] audit: kernel generated netlink traffic should have a portid of 0
2017-03-21 18:58 ` [RFC PATCH 2/4] audit: kernel generated netlink traffic should have a portid of 0 Paul Moore
@ 2017-04-10 3:41 ` Richard Guy Briggs
2017-04-11 19:49 ` Paul Moore
0 siblings, 1 reply; 16+ messages in thread
From: Richard Guy Briggs @ 2017-04-10 3:41 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit
On 2017-03-21 14:58, Paul Moore wrote:
> From: Paul Moore <paul@paul-moore.com>
>
> We were setting the portid incorrectly in the netlink message headers,
> fix that to always be 0 (nlmsg_pid = 0).
>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
Reviewed-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 3 +--
> kernel/audit.c | 23 ++++++-----------------
> kernel/audit.h | 3 +--
> kernel/auditfilter.c | 14 ++++++--------
> 4 files changed, 14 insertions(+), 29 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 504e784b7ffa..cc0497c39472 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -163,8 +163,7 @@ extern void audit_log_task_info(struct audit_buffer *ab,
> extern int audit_update_lsm_rules(void);
>
> /* Private API (for audit.c only) */
> -extern int audit_rule_change(int type, __u32 portid, int seq,
> - void *data, size_t datasz);
> +extern int audit_rule_change(int type, int seq, void *data, size_t datasz);
> extern int audit_list_rules_send(struct sk_buff *request_skb, int seq);
>
> extern u32 audit_enabled;
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 4037869b4b64..6cbf47a372e8 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -251,14 +251,6 @@ static struct sock *audit_get_sk(const struct net *net)
> return aunet->sk;
> }
>
> -static void audit_set_portid(struct audit_buffer *ab, __u32 portid)
> -{
> - if (ab) {
> - struct nlmsghdr *nlh = nlmsg_hdr(ab->skb);
> - nlh->nlmsg_pid = portid;
> - }
> -}
> -
> void audit_panic(const char *message)
> {
> switch (audit_failure) {
> @@ -819,7 +811,7 @@ int audit_send_list(void *_dest)
> return 0;
> }
>
> -struct sk_buff *audit_make_reply(__u32 portid, int seq, int type, int done,
> +struct sk_buff *audit_make_reply(int seq, int type, int done,
> int multi, const void *payload, int size)
> {
> struct sk_buff *skb;
> @@ -832,7 +824,7 @@ struct sk_buff *audit_make_reply(__u32 portid, int seq, int type, int done,
> if (!skb)
> return NULL;
>
> - nlh = nlmsg_put(skb, portid, seq, t, size, flags);
> + nlh = nlmsg_put(skb, 0, seq, t, size, flags);
> if (!nlh)
> goto out_kfree_skb;
> data = nlmsg_data(nlh);
> @@ -876,7 +868,6 @@ static int audit_send_reply_thread(void *arg)
> static void audit_send_reply(struct sk_buff *request_skb, int seq, int type, int done,
> int multi, const void *payload, int size)
> {
> - u32 portid = NETLINK_CB(request_skb).portid;
> struct net *net = sock_net(NETLINK_CB(request_skb).sk);
> struct sk_buff *skb;
> struct task_struct *tsk;
> @@ -886,12 +877,12 @@ static void audit_send_reply(struct sk_buff *request_skb, int seq, int type, int
> if (!reply)
> return;
>
> - skb = audit_make_reply(portid, seq, type, done, multi, payload, size);
> + skb = audit_make_reply(seq, type, done, multi, payload, size);
> if (!skb)
> goto out;
>
> reply->net = get_net(net);
> - reply->portid = portid;
> + reply->portid = NETLINK_CB(request_skb).portid;
> reply->skb = skb;
>
> tsk = kthread_run(audit_send_reply_thread, reply, "audit_send_reply");
> @@ -1075,7 +1066,7 @@ static int audit_replace(pid_t pid)
> {
> struct sk_buff *skb;
>
> - skb = audit_make_reply(0, 0, AUDIT_REPLACE, 0, 0, &pid, sizeof(pid));
> + skb = audit_make_reply(0, AUDIT_REPLACE, 0, 0, &pid, sizeof(pid));
> if (!skb)
> return -ENOMEM;
> return auditd_send_unicast_skb(skb);
> @@ -1245,7 +1236,6 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> size--;
> audit_log_n_untrustedstring(ab, data, size);
> }
> - audit_set_portid(ab, NETLINK_CB(skb).portid);
> audit_log_end(ab);
> }
> break;
> @@ -1259,8 +1249,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> audit_log_end(ab);
> return -EPERM;
> }
> - err = audit_rule_change(msg_type, NETLINK_CB(skb).portid,
> - seq, data, nlmsg_len(nlh));
> + err = audit_rule_change(msg_type, seq, data, nlmsg_len(nlh));
> break;
> case AUDIT_LIST_RULES:
> err = audit_list_rules_send(skb, seq);
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 0f1cf6d1878a..c21b74dd7ff2 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -237,8 +237,7 @@ extern int audit_uid_comparator(kuid_t left, u32 op, kuid_t right);
> extern int audit_gid_comparator(kgid_t left, u32 op, kgid_t right);
> extern int parent_len(const char *path);
> extern int audit_compare_dname_path(const char *dname, const char *path, int plen);
> -extern struct sk_buff *audit_make_reply(__u32 portid, int seq, int type,
> - int done, int multi,
> +extern struct sk_buff *audit_make_reply(int seq, int type, int done, int multi,
> const void *payload, int size);
> extern void audit_panic(const char *message);
>
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 880519d6cf2a..81cdf8d8f319 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -1033,7 +1033,7 @@ int audit_del_rule(struct audit_entry *entry)
> }
>
> /* List rules using struct audit_rule_data. */
> -static void audit_list_rules(__u32 portid, int seq, struct sk_buff_head *q)
> +static void audit_list_rules(int seq, struct sk_buff_head *q)
> {
> struct sk_buff *skb;
> struct audit_krule *r;
> @@ -1048,15 +1048,15 @@ static void audit_list_rules(__u32 portid, int seq, struct sk_buff_head *q)
> data = audit_krule_to_data(r);
> if (unlikely(!data))
> break;
> - skb = audit_make_reply(portid, seq, AUDIT_LIST_RULES,
> - 0, 1, data,
> + skb = audit_make_reply(seq, AUDIT_LIST_RULES, 0, 1,
> + data,
> sizeof(*data) + data->buflen);
> if (skb)
> skb_queue_tail(q, skb);
> kfree(data);
> }
> }
> - skb = audit_make_reply(portid, seq, AUDIT_LIST_RULES, 1, 1, NULL, 0);
> + skb = audit_make_reply(seq, AUDIT_LIST_RULES, 1, 1, NULL, 0);
> if (skb)
> skb_queue_tail(q, skb);
> }
> @@ -1085,13 +1085,11 @@ static void audit_log_rule_change(char *action, struct audit_krule *rule, int re
> /**
> * audit_rule_change - apply all rules to the specified message type
> * @type: audit message type
> - * @portid: target port id for netlink audit messages
> * @seq: netlink audit message sequence (serial) number
> * @data: payload data
> * @datasz: size of payload data
> */
> -int audit_rule_change(int type, __u32 portid, int seq, void *data,
> - size_t datasz)
> +int audit_rule_change(int type, int seq, void *data, size_t datasz)
> {
> int err = 0;
> struct audit_entry *entry;
> @@ -1150,7 +1148,7 @@ int audit_list_rules_send(struct sk_buff *request_skb, int seq)
> skb_queue_head_init(&dest->q);
>
> mutex_lock(&audit_filter_mutex);
> - audit_list_rules(portid, seq, &dest->q);
> + audit_list_rules(seq, &dest->q);
> mutex_unlock(&audit_filter_mutex);
>
> tsk = kthread_run(audit_send_list, dest, "audit_send_list");
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
- 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 4/4] audit: use kmem_cache to manage the audit_buffer cache
2017-03-21 18:59 ` [RFC PATCH 4/4] audit: use kmem_cache to manage the audit_buffer cache Paul Moore
2017-03-21 19:04 ` Paul Moore
@ 2017-04-10 4:04 ` Richard Guy Briggs
2017-04-11 20:07 ` Paul Moore
1 sibling, 1 reply; 16+ messages in thread
From: Richard Guy Briggs @ 2017-04-10 4:04 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, Florian Westphal
On 2017-03-21 14:59, Paul Moore wrote:
> From: Paul Moore <paul@paul-moore.com>
> The audit subsystem implemented its own buffer cache mechanism which
> is a bit silly these days when we could use the kmem_cache construct.
>
> Some credit is due to Florian Westphal for originally proposing that
> we remove the audit cache implementation in favor of simple
> kmalloc()/kfree() calls, but I would rather have a dedicated slab
> cache to ease debugging and future stats/performance work.
>
> Cc: Florian Westphal <fw@strlen.de>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
> kernel/audit.c | 66 ++++++++++++++------------------------------------------
> 1 file changed, 17 insertions(+), 49 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index b718bf3a73f8..f78cdd75a4d2 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -59,6 +59,7 @@
> #include <linux/mutex.h>
> #include <linux/gfp.h>
> #include <linux/pid.h>
> +#include <linux/slab.h>
>
> #include <linux/audit.h>
>
> @@ -152,12 +153,7 @@ static atomic_t audit_lost = ATOMIC_INIT(0);
> /* Hash for inode-based rules */
> struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
>
> -/* The audit_freelist is a list of pre-allocated audit buffers (if more
> - * than AUDIT_MAXFREE are in use, the audit buffer is freed instead of
> - * being placed on the freelist). */
> -static DEFINE_SPINLOCK(audit_freelist_lock);
> -static int audit_freelist_count;
> -static LIST_HEAD(audit_freelist);
> +static struct kmem_cache *audit_buffer_cache;
>
> /* queue msgs to send via kauditd_task */
> static struct sk_buff_head audit_queue;
> @@ -193,17 +189,12 @@ DEFINE_MUTEX(audit_cmd_mutex);
> * should be at least that large. */
> #define AUDIT_BUFSIZ 1024
>
> -/* AUDIT_MAXFREE is the number of empty audit_buffers we keep on the
> - * audit_freelist. Doing so eliminates many kmalloc/kfree calls. */
> -#define AUDIT_MAXFREE (2*NR_CPUS)
> -
> /* The audit_buffer is used when formatting an audit record. The caller
> * locks briefly to get the record off the freelist or to allocate the
> * buffer, and locks briefly to send the buffer to the netlink layer or
> * to place it on a transmit queue. Multiple audit_buffers can be in
> * use simultaneously. */
> struct audit_buffer {
> - struct list_head list;
> struct sk_buff *skb; /* formatted skb ready to send */
> struct audit_context *ctx; /* NULL or associated context */
> gfp_t gfp_mask;
> @@ -1489,6 +1480,10 @@ static int __init audit_init(void)
> if (audit_initialized == AUDIT_DISABLED)
> return 0;
>
> + audit_buffer_cache = kmem_cache_create("audit_buffer",
> + sizeof(struct audit_buffer),
> + 0, SLAB_PANIC, NULL);
> +
> memset(&auditd_conn, 0, sizeof(auditd_conn));
> spin_lock_init(&auditd_conn.lock);
>
> @@ -1557,60 +1552,33 @@ __setup("audit_backlog_limit=", audit_backlog_limit_set);
>
> static void audit_buffer_free(struct audit_buffer *ab)
> {
> - unsigned long flags;
> -
> if (!ab)
> return;
>
> kfree_skb(ab->skb);
> - spin_lock_irqsave(&audit_freelist_lock, flags);
> - if (audit_freelist_count > AUDIT_MAXFREE)
> - kfree(ab);
> - else {
> - audit_freelist_count++;
> - list_add(&ab->list, &audit_freelist);
> - }
> - spin_unlock_irqrestore(&audit_freelist_lock, flags);
> + kmem_cache_free(audit_buffer_cache, ab);
> }
>
> -static struct audit_buffer * audit_buffer_alloc(struct audit_context *ctx,
> - gfp_t gfp_mask, int type)
> +static struct audit_buffer *audit_buffer_alloc(struct audit_context *ctx,
> + gfp_t gfp_mask, int type)
> {
> - unsigned long flags;
> - struct audit_buffer *ab = NULL;
> - struct nlmsghdr *nlh;
> -
> - spin_lock_irqsave(&audit_freelist_lock, flags);
> - if (!list_empty(&audit_freelist)) {
> - ab = list_entry(audit_freelist.next,
> - struct audit_buffer, list);
> - list_del(&ab->list);
> - --audit_freelist_count;
> - }
> - spin_unlock_irqrestore(&audit_freelist_lock, flags);
> -
> - if (!ab) {
> - ab = kmalloc(sizeof(*ab), gfp_mask);
> - if (!ab)
> - goto err;
> - }
> + struct audit_buffer *ab;
>
> - ab->ctx = ctx;
> - ab->gfp_mask = gfp_mask;
> + ab = kmem_cache_alloc(audit_buffer_cache, gfp_mask);
> + if (!ab)
> + return NULL;
>
> ab->skb = nlmsg_new(AUDIT_BUFSIZ, gfp_mask);
> if (!ab->skb)
> goto err;
> + if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
> + goto err;
>
> - nlh = nlmsg_put(ab->skb, 0, 0, type, 0, 0);
> - if (!nlh)
> - goto out_kfree_skb;
Is there a reason to care about an error returned from nlmsg_put() if
you aren't going to free the skb that was allocated? If you think
nlmsg_put() can't fail due to extremely simple calling arguments then
there is no need to check its return code.
If nlmsg_new() succeeds, it has allocated an skb. If nlmsg_put() fails,
you free the audit_buffer and the skb is now a memory leak.
Have I read this correctly?
Otherwise, I like the intent of this simplification.
> + ab->ctx = ctx;
> + ab->gfp_mask = gfp_mask;
>
> return ab;
>
> -out_kfree_skb:
> - kfree_skb(ab->skb);
> - ab->skb = NULL;
> err:
> audit_buffer_free(ab);
> return NULL;
- 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 3/4] audit: store the auditd PID as a pid struct instead of pid_t
2017-03-21 18:59 ` [RFC PATCH 3/4] audit: store the auditd PID as a pid struct instead of pid_t Paul Moore
@ 2017-04-10 4:30 ` Richard Guy Briggs
2017-04-11 19:56 ` Paul Moore
0 siblings, 1 reply; 16+ messages in thread
From: Richard Guy Briggs @ 2017-04-10 4:30 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit
On 2017-03-21 14:59, Paul Moore wrote:
> From: Paul Moore <paul@paul-moore.com>
>
> This is arguably the right thing to do, and will make it easier when
> we start supporting multiple audit daemons in different namespaces.
I had tried this several years ago inspired by Eric Biederman's work for
the same reasons:
https://www.redhat.com/archives/linux-audit/2014-February/msg00116.html
A lot has changed since then... A couple of comments in-line...
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
> kernel/audit.c | 84 ++++++++++++++++++++++++++++++++++++++------------------
> kernel/audit.h | 2 +
> 2 files changed, 58 insertions(+), 28 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 6cbf47a372e8..b718bf3a73f8 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -58,6 +58,7 @@
> #include <linux/rcupdate.h>
> #include <linux/mutex.h>
> #include <linux/gfp.h>
> +#include <linux/pid.h>
>
> #include <linux/audit.h>
>
> @@ -117,7 +118,7 @@ struct audit_net {
> * or the included spinlock for writing.
> */
> static struct auditd_connection {
> - int pid;
> + struct pid *pid;
> u32 portid;
> struct net *net;
> spinlock_t lock;
> @@ -221,18 +222,41 @@ struct audit_reply {
> * Description:
> * Return 1 if the task is a registered audit daemon, 0 otherwise.
> */
> -int auditd_test_task(const struct task_struct *task)
> +int auditd_test_task(struct task_struct *task)
Does the compiler complain if this is left as const?
> {
> int rc;
>
> rcu_read_lock();
> - rc = (auditd_conn.pid && task->tgid == auditd_conn.pid ? 1 : 0);
> + rc = (auditd_conn.pid && auditd_conn.pid == task_tgid(task) ? 1 : 0);
> rcu_read_unlock();
>
> return rc;
> }
>
> /**
> + * auditd_pid_vnr - Return the auditd PID relative to the namespace
> + * @auditd: the auditd connection
> + *
> + * Description:
> + * Returns the PID in relation to the namespace, 0 on failure. This function
> + * takes the RCU read lock internally, but if the caller needs to protect the
> + * auditd_connection pointer it should take the RCU read lock as well.
> + */
> +static pid_t auditd_pid_vnr(const struct auditd_connection *auditd)
> +{
> + pid_t pid;
> +
> + rcu_read_lock();
> + if (!auditd || !auditd->pid)
> + pid = 0;
> + else
> + pid = pid_vnr(auditd->pid);
> + rcu_read_unlock();
> +
> + return pid;
> +}
> +
> +/**
> * audit_get_sk - Return the audit socket for the given network namespace
> * @net: the destination network namespace
> *
> @@ -429,12 +453,17 @@ static int audit_set_failure(u32 state)
> * This function will obtain and drop network namespace references as
> * necessary.
> */
> -static void auditd_set(int pid, u32 portid, struct net *net)
> +static void auditd_set(struct pid *pid, u32 portid, struct net *net)
> {
> unsigned long flags;
>
> spin_lock_irqsave(&auditd_conn.lock, flags);
> - auditd_conn.pid = pid;
> + if (auditd_conn.pid)
> + put_pid(auditd_conn.pid);
> + if (pid)
> + auditd_conn.pid = get_pid(pid);
> + else
> + auditd_conn.pid = NULL;
> auditd_conn.portid = portid;
> if (auditd_conn.net)
> put_net(auditd_conn.net);
> @@ -1062,11 +1091,13 @@ static int audit_set_feature(struct sk_buff *skb)
> return 0;
> }
>
> -static int audit_replace(pid_t pid)
> +static int audit_replace(struct pid *pid)
> {
> + pid_t pvnr;
> struct sk_buff *skb;
>
> - skb = audit_make_reply(0, AUDIT_REPLACE, 0, 0, &pid, sizeof(pid));
> + pvnr = pid_vnr(pid);
> + skb = audit_make_reply(0, AUDIT_REPLACE, 0, 0, &pvnr, sizeof(pvnr));
> if (!skb)
> return -ENOMEM;
> return auditd_send_unicast_skb(skb);
> @@ -1096,9 +1127,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> memset(&s, 0, sizeof(s));
> s.enabled = audit_enabled;
> s.failure = audit_failure;
> - rcu_read_lock();
> - s.pid = auditd_conn.pid;
> - rcu_read_unlock();
> + /* NOTE: use pid_vnr() so the PID is relative to the current
> + * namespace */
> + s.pid = auditd_pid_vnr(&auditd_conn);
I thought this had been fixed earlier (maybe it was in an abandonned
patch) or more likely due to the current state of CAP_AUDIT_CONTROL and
initial PID namespace requirements it was deemed unnecessary.
> s.rate_limit = audit_rate_limit;
> s.backlog_limit = audit_backlog_limit;
> s.lost = atomic_read(&audit_lost);
> @@ -1124,36 +1155,36 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> return err;
> }
> if (s.mask & AUDIT_STATUS_PID) {
> - /* NOTE: we are using task_tgid_vnr() below because
> - * the s.pid value is relative to the namespace
> - * of the caller; at present this doesn't matter
> - * much since you can really only run auditd
> - * from the initial pid namespace, but something
> - * to keep in mind if this changes */
> - int new_pid = s.pid;
> + /* NOTE: we are using the vnr PID functions below
> + * because the s.pid value is relative to the
> + * namespace of the caller; at present this
> + * doesn't matter much since you can really only
> + * run auditd from the initial pid namespace, but
> + * something to keep in mind if this changes */
> + pid_t new_pid = s.pid;
> pid_t auditd_pid;
> - pid_t requesting_pid = task_tgid_vnr(current);
> + struct pid *req_pid = task_tgid(current);
> +
> + /* sanity check - PID values must match */
> + if (new_pid != pid_vnr(req_pid))
> + return -EINVAL;
>
> /* test the auditd connection */
> - audit_replace(requesting_pid);
> + audit_replace(req_pid);
>
> - rcu_read_lock();
> - auditd_pid = auditd_conn.pid;
> + auditd_pid = auditd_pid_vnr(&auditd_conn);
> /* only the current auditd can unregister itself */
> - if ((!new_pid) && (requesting_pid != auditd_pid)) {
> - rcu_read_unlock();
> + if ((!new_pid) && (new_pid != auditd_pid)) {
> audit_log_config_change("audit_pid", new_pid,
> auditd_pid, 0);
> return -EACCES;
> }
> /* replacing a healthy auditd is not allowed */
> if (auditd_pid && new_pid) {
> - rcu_read_unlock();
> audit_log_config_change("audit_pid", new_pid,
> auditd_pid, 0);
> return -EEXIST;
> }
> - rcu_read_unlock();
>
> if (audit_enabled != AUDIT_OFF)
> audit_log_config_change("audit_pid", new_pid,
> @@ -1161,8 +1192,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>
> if (new_pid) {
> /* register a new auditd connection */
> - auditd_set(new_pid,
> - NETLINK_CB(skb).portid,
> + auditd_set(req_pid, NETLINK_CB(skb).portid,
> sock_net(NETLINK_CB(skb).sk));
> /* try to process any backlog */
> wake_up_interruptible(&kauditd_wait);
> diff --git a/kernel/audit.h b/kernel/audit.h
> index c21b74dd7ff2..d9b9af769128 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -218,7 +218,7 @@ extern void audit_log_name(struct audit_context *context,
> struct audit_names *n, const struct path *path,
> int record_num, int *call_panic);
>
> -extern int auditd_test_task(const struct task_struct *task);
> +extern int auditd_test_task(struct task_struct *task);
>
> #define AUDIT_INODE_BUCKETS 32
> extern struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
Reviewed-by: Richard Guy Briggs <rgb@redhat.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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/4] audit: combine audit_receive() and audit_receive_skb()
2017-04-10 3:38 ` Richard Guy Briggs
@ 2017-04-11 19:47 ` Paul Moore
0 siblings, 0 replies; 16+ messages in thread
From: Paul Moore @ 2017-04-11 19:47 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit
On Sun, Apr 9, 2017 at 11:38 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-03-21 14:58, Paul Moore wrote:
>> From: Paul Moore <paul@paul-moore.com>
>>
>> There is no reason to have both of these functions, combine the two.
>>
>> Signed-off-by: Paul Moore <paul@paul-moore.com>
>
> Reviewed-by: Richard Guy Briggs <rgb@redhat.com>
Thanks. I just merged this to audit/next.
>> ---
>> kernel/audit.c | 19 ++++++++-----------
>> 1 file changed, 8 insertions(+), 11 deletions(-)
>>
>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index 2f4964cfde0b..4037869b4b64 100644
>> --- a/kernel/audit.c
>> +++ b/kernel/audit.c
>> @@ -1381,11 +1381,14 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>> return err < 0 ? err : 0;
>> }
>>
>> -/*
>> - * Get message from skb. Each message is processed by audit_receive_msg.
>> - * Malformed skbs with wrong length are discarded silently.
>> +/**
>> + * audit_receive - receive messages from a netlink control socket
>> + * @skb: the message buffer
>> + *
>> + * Parse the provided skb and deal with any messages that may be present,
>> + * malformed skbs are discarded.
>> */
>> -static void audit_receive_skb(struct sk_buff *skb)
>> +static void audit_receive(struct sk_buff *skb)
>> {
>> struct nlmsghdr *nlh;
>> /*
>> @@ -1398,6 +1401,7 @@ static void audit_receive_skb(struct sk_buff *skb)
>> nlh = nlmsg_hdr(skb);
>> len = skb->len;
>>
>> + mutex_lock(&audit_cmd_mutex);
>> while (nlmsg_ok(nlh, len)) {
>> err = audit_receive_msg(skb, nlh);
>> /* if err or if this message says it wants a response */
>> @@ -1406,13 +1410,6 @@ static void audit_receive_skb(struct sk_buff *skb)
>>
>> nlh = nlmsg_next(nlh, &len);
>> }
>> -}
>> -
>> -/* Receive messages from netlink socket. */
>> -static void audit_receive(struct sk_buff *skb)
>> -{
>> - mutex_lock(&audit_cmd_mutex);
>> - audit_receive_skb(skb);
>> mutex_unlock(&audit_cmd_mutex);
>> }
>>
>>
>> --
>> Linux-audit mailing list
>> Linux-audit@redhat.com
>> https://www.redhat.com/mailman/listinfo/linux-audit
>
> - 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
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/4] audit: kernel generated netlink traffic should have a portid of 0
2017-04-10 3:41 ` Richard Guy Briggs
@ 2017-04-11 19:49 ` Paul Moore
0 siblings, 0 replies; 16+ messages in thread
From: Paul Moore @ 2017-04-11 19:49 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit
On Sun, Apr 9, 2017 at 11:41 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-03-21 14:58, Paul Moore wrote:
>> From: Paul Moore <paul@paul-moore.com>
>>
>> We were setting the portid incorrectly in the netlink message headers,
>> fix that to always be 0 (nlmsg_pid = 0).
>>
>> Signed-off-by: Paul Moore <paul@paul-moore.com>
>
> Reviewed-by: Richard Guy Briggs <rgb@redhat.com>
Thanks. Also merged.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 3/4] audit: store the auditd PID as a pid struct instead of pid_t
2017-04-10 4:30 ` Richard Guy Briggs
@ 2017-04-11 19:56 ` Paul Moore
0 siblings, 0 replies; 16+ messages in thread
From: Paul Moore @ 2017-04-11 19:56 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit
On Mon, Apr 10, 2017 at 12:30 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-03-21 14:59, Paul Moore wrote:
>> From: Paul Moore <paul@paul-moore.com>
>>
>> This is arguably the right thing to do, and will make it easier when
>> we start supporting multiple audit daemons in different namespaces.
>
> I had tried this several years ago inspired by Eric Biederman's work for
> the same reasons:
> https://www.redhat.com/archives/linux-audit/2014-February/msg00116.html
>
> A lot has changed since then... A couple of comments in-line...
>
>> Signed-off-by: Paul Moore <paul@paul-moore.com>
>> ---
>> kernel/audit.c | 84 ++++++++++++++++++++++++++++++++++++++------------------
>> kernel/audit.h | 2 +
>> 2 files changed, 58 insertions(+), 28 deletions(-)
>>
>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index 6cbf47a372e8..b718bf3a73f8 100644
>> --- a/kernel/audit.c
>> +++ b/kernel/audit.c
>> @@ -58,6 +58,7 @@
>> #include <linux/rcupdate.h>
>> #include <linux/mutex.h>
>> #include <linux/gfp.h>
>> +#include <linux/pid.h>
>>
>> #include <linux/audit.h>
>>
>> @@ -117,7 +118,7 @@ struct audit_net {
>> * or the included spinlock for writing.
>> */
>> static struct auditd_connection {
>> - int pid;
>> + struct pid *pid;
>> u32 portid;
>> struct net *net;
>> spinlock_t lock;
>> @@ -221,18 +222,41 @@ struct audit_reply {
>> * Description:
>> * Return 1 if the task is a registered audit daemon, 0 otherwise.
>> */
>> -int auditd_test_task(const struct task_struct *task)
>> +int auditd_test_task(struct task_struct *task)
>
> Does the compiler complain if this is left as const?
Yep, it runs afoul with the task_tgid() call.
> Reviewed-by: Richard Guy Briggs <rgb@redhat.com>
Thanks. Merged.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 4/4] audit: use kmem_cache to manage the audit_buffer cache
2017-04-10 4:04 ` Richard Guy Briggs
@ 2017-04-11 20:07 ` Paul Moore
2017-04-12 4:59 ` Richard Guy Briggs
0 siblings, 1 reply; 16+ messages in thread
From: Paul Moore @ 2017-04-11 20:07 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, Florian Westphal
On Mon, Apr 10, 2017 at 12:04 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-03-21 14:59, Paul Moore wrote:
>> From: Paul Moore <paul@paul-moore.com>
>> The audit subsystem implemented its own buffer cache mechanism which
>> is a bit silly these days when we could use the kmem_cache construct.
>>
>> Some credit is due to Florian Westphal for originally proposing that
>> we remove the audit cache implementation in favor of simple
>> kmalloc()/kfree() calls, but I would rather have a dedicated slab
>> cache to ease debugging and future stats/performance work.
>>
>> Cc: Florian Westphal <fw@strlen.de>
>> Signed-off-by: Paul Moore <paul@paul-moore.com>
>> ---
>> kernel/audit.c | 66 ++++++++++++++------------------------------------------
>> 1 file changed, 17 insertions(+), 49 deletions(-)
>>
>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index b718bf3a73f8..f78cdd75a4d2 100644
>> --- a/kernel/audit.c
>> +++ b/kernel/audit.c
>> @@ -59,6 +59,7 @@
>> #include <linux/mutex.h>
>> #include <linux/gfp.h>
>> #include <linux/pid.h>
>> +#include <linux/slab.h>
>>
>> #include <linux/audit.h>
>>
>> @@ -152,12 +153,7 @@ static atomic_t audit_lost = ATOMIC_INIT(0);
>> /* Hash for inode-based rules */
>> struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
>>
>> -/* The audit_freelist is a list of pre-allocated audit buffers (if more
>> - * than AUDIT_MAXFREE are in use, the audit buffer is freed instead of
>> - * being placed on the freelist). */
>> -static DEFINE_SPINLOCK(audit_freelist_lock);
>> -static int audit_freelist_count;
>> -static LIST_HEAD(audit_freelist);
>> +static struct kmem_cache *audit_buffer_cache;
>>
>> /* queue msgs to send via kauditd_task */
>> static struct sk_buff_head audit_queue;
>> @@ -193,17 +189,12 @@ DEFINE_MUTEX(audit_cmd_mutex);
>> * should be at least that large. */
>> #define AUDIT_BUFSIZ 1024
>>
>> -/* AUDIT_MAXFREE is the number of empty audit_buffers we keep on the
>> - * audit_freelist. Doing so eliminates many kmalloc/kfree calls. */
>> -#define AUDIT_MAXFREE (2*NR_CPUS)
>> -
>> /* The audit_buffer is used when formatting an audit record. The caller
>> * locks briefly to get the record off the freelist or to allocate the
>> * buffer, and locks briefly to send the buffer to the netlink layer or
>> * to place it on a transmit queue. Multiple audit_buffers can be in
>> * use simultaneously. */
>> struct audit_buffer {
>> - struct list_head list;
>> struct sk_buff *skb; /* formatted skb ready to send */
>> struct audit_context *ctx; /* NULL or associated context */
>> gfp_t gfp_mask;
>> @@ -1489,6 +1480,10 @@ static int __init audit_init(void)
>> if (audit_initialized == AUDIT_DISABLED)
>> return 0;
>>
>> + audit_buffer_cache = kmem_cache_create("audit_buffer",
>> + sizeof(struct audit_buffer),
>> + 0, SLAB_PANIC, NULL);
>> +
>> memset(&auditd_conn, 0, sizeof(auditd_conn));
>> spin_lock_init(&auditd_conn.lock);
>>
>> @@ -1557,60 +1552,33 @@ __setup("audit_backlog_limit=", audit_backlog_limit_set);
>>
>> static void audit_buffer_free(struct audit_buffer *ab)
>> {
>> - unsigned long flags;
>> -
>> if (!ab)
>> return;
>>
>> kfree_skb(ab->skb);
>> - spin_lock_irqsave(&audit_freelist_lock, flags);
>> - if (audit_freelist_count > AUDIT_MAXFREE)
>> - kfree(ab);
>> - else {
>> - audit_freelist_count++;
>> - list_add(&ab->list, &audit_freelist);
>> - }
>> - spin_unlock_irqrestore(&audit_freelist_lock, flags);
>> + kmem_cache_free(audit_buffer_cache, ab);
>> }
>>
>> -static struct audit_buffer * audit_buffer_alloc(struct audit_context *ctx,
>> - gfp_t gfp_mask, int type)
>> +static struct audit_buffer *audit_buffer_alloc(struct audit_context *ctx,
>> + gfp_t gfp_mask, int type)
>> {
>> - unsigned long flags;
>> - struct audit_buffer *ab = NULL;
>> - struct nlmsghdr *nlh;
>> -
>> - spin_lock_irqsave(&audit_freelist_lock, flags);
>> - if (!list_empty(&audit_freelist)) {
>> - ab = list_entry(audit_freelist.next,
>> - struct audit_buffer, list);
>> - list_del(&ab->list);
>> - --audit_freelist_count;
>> - }
>> - spin_unlock_irqrestore(&audit_freelist_lock, flags);
>> -
>> - if (!ab) {
>> - ab = kmalloc(sizeof(*ab), gfp_mask);
>> - if (!ab)
>> - goto err;
>> - }
>> + struct audit_buffer *ab;
>>
>> - ab->ctx = ctx;
>> - ab->gfp_mask = gfp_mask;
>> + ab = kmem_cache_alloc(audit_buffer_cache, gfp_mask);
>> + if (!ab)
>> + return NULL;
>>
>> ab->skb = nlmsg_new(AUDIT_BUFSIZ, gfp_mask);
>> if (!ab->skb)
>> goto err;
>> + if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
>> + goto err;
>>
>> - nlh = nlmsg_put(ab->skb, 0, 0, type, 0, 0);
>> - if (!nlh)
>> - goto out_kfree_skb;
>
> Is there a reason to care about an error returned from nlmsg_put() if
> you aren't going to free the skb that was allocated? If you think
> nlmsg_put() can't fail due to extremely simple calling arguments then
> there is no need to check its return code.
>
> If nlmsg_new() succeeds, it has allocated an skb. If nlmsg_put() fails,
> you free the audit_buffer and the skb is now a memory leak.
>
> Have I read this correctly?
Check my math, but in the patched code if the nlmsg_put() call fails
then we jump to "err" which calls audit_buffer_free() which in turn
calls kfree_skb() on ab->skb so I don't believe we have a memory leak
on error ... I'll hold off on merging this in case I'm missing
something, but I'm pretty sure we're okay here.
> Otherwise, I like the intent of this simplification.
>
>> + ab->ctx = ctx;
>> + ab->gfp_mask = gfp_mask;
>>
>> return ab;
>>
>> -out_kfree_skb:
>> - kfree_skb(ab->skb);
>> - ab->skb = NULL;
>> err:
>> audit_buffer_free(ab);
>> return NULL;
>
> - 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
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 4/4] audit: use kmem_cache to manage the audit_buffer cache
2017-04-11 20:07 ` Paul Moore
@ 2017-04-12 4:59 ` Richard Guy Briggs
2017-04-12 15:51 ` Paul Moore
0 siblings, 1 reply; 16+ messages in thread
From: Richard Guy Briggs @ 2017-04-12 4:59 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, Florian Westphal
On 2017-04-11 16:07, Paul Moore wrote:
> On Mon, Apr 10, 2017 at 12:04 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2017-03-21 14:59, Paul Moore wrote:
> >> From: Paul Moore <paul@paul-moore.com>
> >> The audit subsystem implemented its own buffer cache mechanism which
> >> is a bit silly these days when we could use the kmem_cache construct.
> >>
> >> Some credit is due to Florian Westphal for originally proposing that
> >> we remove the audit cache implementation in favor of simple
> >> kmalloc()/kfree() calls, but I would rather have a dedicated slab
> >> cache to ease debugging and future stats/performance work.
> >>
> >> Cc: Florian Westphal <fw@strlen.de>
> >> Signed-off-by: Paul Moore <paul@paul-moore.com>
> >> ---
> >> kernel/audit.c | 66 ++++++++++++++------------------------------------------
> >> 1 file changed, 17 insertions(+), 49 deletions(-)
> >>
> >> diff --git a/kernel/audit.c b/kernel/audit.c
> >> index b718bf3a73f8..f78cdd75a4d2 100644
> >> --- a/kernel/audit.c
> >> +++ b/kernel/audit.c
> >> @@ -59,6 +59,7 @@
> >> #include <linux/mutex.h>
> >> #include <linux/gfp.h>
> >> #include <linux/pid.h>
> >> +#include <linux/slab.h>
> >>
> >> #include <linux/audit.h>
> >>
> >> @@ -152,12 +153,7 @@ static atomic_t audit_lost = ATOMIC_INIT(0);
> >> /* Hash for inode-based rules */
> >> struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
> >>
> >> -/* The audit_freelist is a list of pre-allocated audit buffers (if more
> >> - * than AUDIT_MAXFREE are in use, the audit buffer is freed instead of
> >> - * being placed on the freelist). */
> >> -static DEFINE_SPINLOCK(audit_freelist_lock);
> >> -static int audit_freelist_count;
> >> -static LIST_HEAD(audit_freelist);
> >> +static struct kmem_cache *audit_buffer_cache;
> >>
> >> /* queue msgs to send via kauditd_task */
> >> static struct sk_buff_head audit_queue;
> >> @@ -193,17 +189,12 @@ DEFINE_MUTEX(audit_cmd_mutex);
> >> * should be at least that large. */
> >> #define AUDIT_BUFSIZ 1024
> >>
> >> -/* AUDIT_MAXFREE is the number of empty audit_buffers we keep on the
> >> - * audit_freelist. Doing so eliminates many kmalloc/kfree calls. */
> >> -#define AUDIT_MAXFREE (2*NR_CPUS)
> >> -
> >> /* The audit_buffer is used when formatting an audit record. The caller
> >> * locks briefly to get the record off the freelist or to allocate the
> >> * buffer, and locks briefly to send the buffer to the netlink layer or
> >> * to place it on a transmit queue. Multiple audit_buffers can be in
> >> * use simultaneously. */
> >> struct audit_buffer {
> >> - struct list_head list;
> >> struct sk_buff *skb; /* formatted skb ready to send */
> >> struct audit_context *ctx; /* NULL or associated context */
> >> gfp_t gfp_mask;
> >> @@ -1489,6 +1480,10 @@ static int __init audit_init(void)
> >> if (audit_initialized == AUDIT_DISABLED)
> >> return 0;
> >>
> >> + audit_buffer_cache = kmem_cache_create("audit_buffer",
> >> + sizeof(struct audit_buffer),
> >> + 0, SLAB_PANIC, NULL);
> >> +
> >> memset(&auditd_conn, 0, sizeof(auditd_conn));
> >> spin_lock_init(&auditd_conn.lock);
> >>
> >> @@ -1557,60 +1552,33 @@ __setup("audit_backlog_limit=", audit_backlog_limit_set);
> >>
> >> static void audit_buffer_free(struct audit_buffer *ab)
> >> {
> >> - unsigned long flags;
> >> -
> >> if (!ab)
> >> return;
> >>
> >> kfree_skb(ab->skb);
> >> - spin_lock_irqsave(&audit_freelist_lock, flags);
> >> - if (audit_freelist_count > AUDIT_MAXFREE)
> >> - kfree(ab);
> >> - else {
> >> - audit_freelist_count++;
> >> - list_add(&ab->list, &audit_freelist);
> >> - }
> >> - spin_unlock_irqrestore(&audit_freelist_lock, flags);
> >> + kmem_cache_free(audit_buffer_cache, ab);
> >> }
> >>
> >> -static struct audit_buffer * audit_buffer_alloc(struct audit_context *ctx,
> >> - gfp_t gfp_mask, int type)
> >> +static struct audit_buffer *audit_buffer_alloc(struct audit_context *ctx,
> >> + gfp_t gfp_mask, int type)
> >> {
> >> - unsigned long flags;
> >> - struct audit_buffer *ab = NULL;
> >> - struct nlmsghdr *nlh;
> >> -
> >> - spin_lock_irqsave(&audit_freelist_lock, flags);
> >> - if (!list_empty(&audit_freelist)) {
> >> - ab = list_entry(audit_freelist.next,
> >> - struct audit_buffer, list);
> >> - list_del(&ab->list);
> >> - --audit_freelist_count;
> >> - }
> >> - spin_unlock_irqrestore(&audit_freelist_lock, flags);
> >> -
> >> - if (!ab) {
> >> - ab = kmalloc(sizeof(*ab), gfp_mask);
> >> - if (!ab)
> >> - goto err;
> >> - }
> >> + struct audit_buffer *ab;
> >>
> >> - ab->ctx = ctx;
> >> - ab->gfp_mask = gfp_mask;
> >> + ab = kmem_cache_alloc(audit_buffer_cache, gfp_mask);
> >> + if (!ab)
> >> + return NULL;
> >>
> >> ab->skb = nlmsg_new(AUDIT_BUFSIZ, gfp_mask);
> >> if (!ab->skb)
> >> goto err;
> >> + if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
> >> + goto err;
> >>
> >> - nlh = nlmsg_put(ab->skb, 0, 0, type, 0, 0);
> >> - if (!nlh)
> >> - goto out_kfree_skb;
> >
> > Is there a reason to care about an error returned from nlmsg_put() if
> > you aren't going to free the skb that was allocated? If you think
> > nlmsg_put() can't fail due to extremely simple calling arguments then
> > there is no need to check its return code.
> >
> > If nlmsg_new() succeeds, it has allocated an skb. If nlmsg_put() fails,
> > you free the audit_buffer and the skb is now a memory leak.
> >
> > Have I read this correctly?
>
> Check my math, but in the patched code if the nlmsg_put() call fails
> then we jump to "err" which calls audit_buffer_free() which in turn
> calls kfree_skb() on ab->skb so I don't believe we have a memory leak
> on error ... I'll hold off on merging this in case I'm missing
> something, but I'm pretty sure we're okay here.
Ok, yes, you're right. This is ringing a bell... I think there was
another place recently that the extra free_skb() was dropped and I had
missed audit_buffer_free() doing the right thing then.
> > Otherwise, I like the intent of this simplification.
Looks good,
Reviewed-by: Richard Guy Briggs <rgb@redhat.com>
> >> + ab->ctx = ctx;
> >> + ab->gfp_mask = gfp_mask;
> >>
> >> return ab;
> >>
> >> -out_kfree_skb:
> >> - kfree_skb(ab->skb);
> >> - ab->skb = NULL;
> >> err:
> >> audit_buffer_free(ab);
> >> return NULL;
> >
> > - RGB
>
> paul moore
- 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 4/4] audit: use kmem_cache to manage the audit_buffer cache
2017-04-12 4:59 ` Richard Guy Briggs
@ 2017-04-12 15:51 ` Paul Moore
0 siblings, 0 replies; 16+ messages in thread
From: Paul Moore @ 2017-04-12 15:51 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, Florian Westphal
On Wed, Apr 12, 2017 at 12:59 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-04-11 16:07, Paul Moore wrote:
>> On Mon, Apr 10, 2017 at 12:04 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > On 2017-03-21 14:59, Paul Moore wrote:
>> >> From: Paul Moore <paul@paul-moore.com>
>> >> The audit subsystem implemented its own buffer cache mechanism which
>> >> is a bit silly these days when we could use the kmem_cache construct.
>> >>
>> >> Some credit is due to Florian Westphal for originally proposing that
>> >> we remove the audit cache implementation in favor of simple
>> >> kmalloc()/kfree() calls, but I would rather have a dedicated slab
>> >> cache to ease debugging and future stats/performance work.
>> >>
>> >> Cc: Florian Westphal <fw@strlen.de>
>> >> Signed-off-by: Paul Moore <paul@paul-moore.com>
>> >> ---
>> >> kernel/audit.c | 66 ++++++++++++++------------------------------------------
>> >> 1 file changed, 17 insertions(+), 49 deletions(-)
>> >>
>> >> diff --git a/kernel/audit.c b/kernel/audit.c
>> >> index b718bf3a73f8..f78cdd75a4d2 100644
>> >> --- a/kernel/audit.c
>> >> +++ b/kernel/audit.c
>> >> @@ -59,6 +59,7 @@
>> >> #include <linux/mutex.h>
>> >> #include <linux/gfp.h>
>> >> #include <linux/pid.h>
>> >> +#include <linux/slab.h>
>> >>
>> >> #include <linux/audit.h>
>> >>
>> >> @@ -152,12 +153,7 @@ static atomic_t audit_lost = ATOMIC_INIT(0);
>> >> /* Hash for inode-based rules */
>> >> struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
>> >>
>> >> -/* The audit_freelist is a list of pre-allocated audit buffers (if more
>> >> - * than AUDIT_MAXFREE are in use, the audit buffer is freed instead of
>> >> - * being placed on the freelist). */
>> >> -static DEFINE_SPINLOCK(audit_freelist_lock);
>> >> -static int audit_freelist_count;
>> >> -static LIST_HEAD(audit_freelist);
>> >> +static struct kmem_cache *audit_buffer_cache;
>> >>
>> >> /* queue msgs to send via kauditd_task */
>> >> static struct sk_buff_head audit_queue;
>> >> @@ -193,17 +189,12 @@ DEFINE_MUTEX(audit_cmd_mutex);
>> >> * should be at least that large. */
>> >> #define AUDIT_BUFSIZ 1024
>> >>
>> >> -/* AUDIT_MAXFREE is the number of empty audit_buffers we keep on the
>> >> - * audit_freelist. Doing so eliminates many kmalloc/kfree calls. */
>> >> -#define AUDIT_MAXFREE (2*NR_CPUS)
>> >> -
>> >> /* The audit_buffer is used when formatting an audit record. The caller
>> >> * locks briefly to get the record off the freelist or to allocate the
>> >> * buffer, and locks briefly to send the buffer to the netlink layer or
>> >> * to place it on a transmit queue. Multiple audit_buffers can be in
>> >> * use simultaneously. */
>> >> struct audit_buffer {
>> >> - struct list_head list;
>> >> struct sk_buff *skb; /* formatted skb ready to send */
>> >> struct audit_context *ctx; /* NULL or associated context */
>> >> gfp_t gfp_mask;
>> >> @@ -1489,6 +1480,10 @@ static int __init audit_init(void)
>> >> if (audit_initialized == AUDIT_DISABLED)
>> >> return 0;
>> >>
>> >> + audit_buffer_cache = kmem_cache_create("audit_buffer",
>> >> + sizeof(struct audit_buffer),
>> >> + 0, SLAB_PANIC, NULL);
>> >> +
>> >> memset(&auditd_conn, 0, sizeof(auditd_conn));
>> >> spin_lock_init(&auditd_conn.lock);
>> >>
>> >> @@ -1557,60 +1552,33 @@ __setup("audit_backlog_limit=", audit_backlog_limit_set);
>> >>
>> >> static void audit_buffer_free(struct audit_buffer *ab)
>> >> {
>> >> - unsigned long flags;
>> >> -
>> >> if (!ab)
>> >> return;
>> >>
>> >> kfree_skb(ab->skb);
>> >> - spin_lock_irqsave(&audit_freelist_lock, flags);
>> >> - if (audit_freelist_count > AUDIT_MAXFREE)
>> >> - kfree(ab);
>> >> - else {
>> >> - audit_freelist_count++;
>> >> - list_add(&ab->list, &audit_freelist);
>> >> - }
>> >> - spin_unlock_irqrestore(&audit_freelist_lock, flags);
>> >> + kmem_cache_free(audit_buffer_cache, ab);
>> >> }
>> >>
>> >> -static struct audit_buffer * audit_buffer_alloc(struct audit_context *ctx,
>> >> - gfp_t gfp_mask, int type)
>> >> +static struct audit_buffer *audit_buffer_alloc(struct audit_context *ctx,
>> >> + gfp_t gfp_mask, int type)
>> >> {
>> >> - unsigned long flags;
>> >> - struct audit_buffer *ab = NULL;
>> >> - struct nlmsghdr *nlh;
>> >> -
>> >> - spin_lock_irqsave(&audit_freelist_lock, flags);
>> >> - if (!list_empty(&audit_freelist)) {
>> >> - ab = list_entry(audit_freelist.next,
>> >> - struct audit_buffer, list);
>> >> - list_del(&ab->list);
>> >> - --audit_freelist_count;
>> >> - }
>> >> - spin_unlock_irqrestore(&audit_freelist_lock, flags);
>> >> -
>> >> - if (!ab) {
>> >> - ab = kmalloc(sizeof(*ab), gfp_mask);
>> >> - if (!ab)
>> >> - goto err;
>> >> - }
>> >> + struct audit_buffer *ab;
>> >>
>> >> - ab->ctx = ctx;
>> >> - ab->gfp_mask = gfp_mask;
>> >> + ab = kmem_cache_alloc(audit_buffer_cache, gfp_mask);
>> >> + if (!ab)
>> >> + return NULL;
>> >>
>> >> ab->skb = nlmsg_new(AUDIT_BUFSIZ, gfp_mask);
>> >> if (!ab->skb)
>> >> goto err;
>> >> + if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
>> >> + goto err;
>> >>
>> >> - nlh = nlmsg_put(ab->skb, 0, 0, type, 0, 0);
>> >> - if (!nlh)
>> >> - goto out_kfree_skb;
>> >
>> > Is there a reason to care about an error returned from nlmsg_put() if
>> > you aren't going to free the skb that was allocated? If you think
>> > nlmsg_put() can't fail due to extremely simple calling arguments then
>> > there is no need to check its return code.
>> >
>> > If nlmsg_new() succeeds, it has allocated an skb. If nlmsg_put() fails,
>> > you free the audit_buffer and the skb is now a memory leak.
>> >
>> > Have I read this correctly?
>>
>> Check my math, but in the patched code if the nlmsg_put() call fails
>> then we jump to "err" which calls audit_buffer_free() which in turn
>> calls kfree_skb() on ab->skb so I don't believe we have a memory leak
>> on error ... I'll hold off on merging this in case I'm missing
>> something, but I'm pretty sure we're okay here.
>
> Ok, yes, you're right. This is ringing a bell... I think there was
> another place recently that the extra free_skb() was dropped and I had
> missed audit_buffer_free() doing the right thing then.
>
>> > Otherwise, I like the intent of this simplification.
>
> Looks good,
> Reviewed-by: Richard Guy Briggs <rgb@redhat.com>
Merged.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2017-04-12 15:51 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-21 18:58 [RFC PATCH 0/4] Assorted audit fixes/improvements Paul Moore
2017-03-21 18:58 ` [RFC PATCH 1/4] audit: combine audit_receive() and audit_receive_skb() Paul Moore
2017-04-10 3:38 ` Richard Guy Briggs
2017-04-11 19:47 ` Paul Moore
2017-03-21 18:58 ` [RFC PATCH 2/4] audit: kernel generated netlink traffic should have a portid of 0 Paul Moore
2017-04-10 3:41 ` Richard Guy Briggs
2017-04-11 19:49 ` Paul Moore
2017-03-21 18:59 ` [RFC PATCH 3/4] audit: store the auditd PID as a pid struct instead of pid_t Paul Moore
2017-04-10 4:30 ` Richard Guy Briggs
2017-04-11 19:56 ` Paul Moore
2017-03-21 18:59 ` [RFC PATCH 4/4] audit: use kmem_cache to manage the audit_buffer cache Paul Moore
2017-03-21 19:04 ` Paul Moore
2017-04-10 4:04 ` Richard Guy Briggs
2017-04-11 20:07 ` Paul Moore
2017-04-12 4:59 ` Richard Guy Briggs
2017-04-12 15:51 ` Paul Moore
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.