* [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
* 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 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
* [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
* 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 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
* [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
* 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 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
* [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 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 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.