From: Rik Faith Paul E. McKenney wrote: > o I don't see any rcu_read_lock() or rcu_read_unlock() calls. Fixed. > o Presumably something surrounding netlink_kernel_create() > ensures that only one instance of audit_del_rule() will > be executing at a given time. If not, some locking is > needed. I was unable to find anything, so I added a semaphore to the receive routine, and comments to the add and delete routines. > o The audit_add_rule() function also needs something to prevent > races with other audit_add_rule() and audit_del_rule() > instances. This is also handled by the semaphore. Thanks for your comments! This patch is against 2.6.5-rc1-mm1. It also corrects a problem that could trigger a BUG() near boot time. --- 25-akpm/kernel/audit.c | 9 +++++++++ 25-akpm/kernel/auditsc.c | 38 +++++++++++++++++++++++++++++--------- 2 files changed, 38 insertions(+), 9 deletions(-) diff -puN kernel/audit.c~lightweight-auditing-framework-update-1 kernel/audit.c --- 25/kernel/audit.c~lightweight-auditing-framework-update-1 2004-03-17 02:27:52.401036032 -0800 +++ 25-akpm/kernel/audit.c 2004-03-17 02:27:52.407035120 -0800 @@ -111,6 +111,11 @@ static LIST_HEAD(audit_tsklist); static LIST_HEAD(audit_entlist); static LIST_HEAD(audit_extlist); +/* The netlink socket is only to be read by 1 CPU, which lets us assume + * that list additions and deletions never happen simultaneiously in + * auditsc.c */ +static DECLARE_MUTEX(audit_netlink_sem); + /* AUDIT_BUFSIZ is the size of the temporary buffer used for formatting * audit records. Since printk uses a 1024 byte buffer, this buffer * should be at least that large. */ @@ -427,6 +432,9 @@ static void audit_receive(struct sock *s { struct sk_buff *skb; + if (down_trylock(&audit_netlink_sem)) + return; + /* FIXME: this must not cause starvation */ while ((skb = skb_dequeue(&sk->sk_receive_queue))) { if (audit_receive_skb(skb) && skb->len) @@ -434,6 +442,7 @@ static void audit_receive(struct sock *s else kfree_skb(skb); } + up(&audit_netlink_sem); } /* Move data from tmp buffer into an skb. This is an extra copy, and diff -puN kernel/auditsc.c~lightweight-auditing-framework-update-1 kernel/auditsc.c --- 25/kernel/auditsc.c~lightweight-auditing-framework-update-1 2004-03-17 02:27:52.404035576 -0800 +++ 25-akpm/kernel/auditsc.c 2004-03-17 02:27:52.408034968 -0800 @@ -158,6 +158,9 @@ static int audit_compare_rule(struct aud return 0; } +/* Note that audit_add_rule and audit_del_rule are called via + * audit_receive() in audit.c, and are protected by + * audit_netlink_sem. */ static inline int audit_add_rule(struct audit_entry *entry, struct list_head *list) { @@ -175,12 +178,17 @@ static void audit_free_rule(void *arg) kfree(arg); } +/* Note that audit_add_rule and audit_del_rule are called via + * audit_receive() in audit.c, and are protected by + * audit_netlink_sem. */ static inline int audit_del_rule(struct audit_rule *rule, struct list_head *list) { struct audit_entry *e; - list_for_each_entry_rcu(e, list, list) { + /* Do not use the _rcu iterator here, since this is the only + * deletion routine. */ + list_for_each_entry(e, list, list) { if (!audit_compare_rule(rule, &e->rule)) { list_del_rcu(&e->list); call_rcu(&e->rcu, audit_free_rule, e); @@ -223,6 +231,7 @@ int audit_receive_filter(int type, int p switch (type) { case AUDIT_LIST: + rcu_read_lock(); list_for_each_entry_rcu(entry, &audit_tsklist, list) audit_send_reply(pid, seq, AUDIT_LIST, 0, 1, &entry->rule, sizeof(entry->rule)); @@ -232,6 +241,7 @@ int audit_receive_filter(int type, int p list_for_each_entry_rcu(entry, &audit_extlist, list) audit_send_reply(pid, seq, AUDIT_LIST, 0, 1, &entry->rule, sizeof(entry->rule)); + rcu_read_unlock(); audit_send_reply(pid, seq, AUDIT_LIST, 1, 1, NULL, 0); break; case AUDIT_ADD: @@ -382,10 +392,14 @@ static enum audit_state audit_filter_tas struct audit_entry *e; enum audit_state state; + rcu_read_lock(); list_for_each_entry_rcu(e, &audit_tsklist, list) { - if (audit_filter_rules(tsk, &e->rule, NULL, &state)) + if (audit_filter_rules(tsk, &e->rule, NULL, &state)) { + rcu_read_unlock(); return state; + } } + rcu_read_unlock(); return AUDIT_BUILD_CONTEXT; } @@ -403,11 +417,15 @@ static enum audit_state audit_filter_sys int word = AUDIT_WORD(ctx->major); int bit = AUDIT_BIT(ctx->major); + rcu_read_lock(); list_for_each_entry_rcu(e, list, list) { if ((e->rule.mask[word] & bit) == bit - && audit_filter_rules(tsk, &e->rule, ctx, &state)) + && audit_filter_rules(tsk, &e->rule, ctx, &state)) { + rcu_read_unlock(); return state; + } } + rcu_read_unlock(); return AUDIT_BUILD_CONTEXT; } @@ -753,8 +771,8 @@ void audit_getname(const char *name) BUG_ON(!context); if (!context->in_syscall) { #if AUDIT_DEBUG == 2 - printk(KERN_ERR "audit.c:%d(:%d): ignoring getname(%p)\n", - __LINE__, context->serial, name); + printk(KERN_ERR "%s:%d(:%d): ignoring getname(%p)\n", + __FILE__, __LINE__, context->serial, name); dump_stack(); #endif return; @@ -777,8 +795,8 @@ void audit_putname(const char *name) BUG_ON(!context); if (!context->in_syscall) { #if AUDIT_DEBUG == 2 - printk(KERN_ERR "audit.c:%d(:%d): __putname(%p)\n", - __LINE__, context->serial, name); + printk(KERN_ERR "%s:%d(:%d): __putname(%p)\n", + __FILE__, __LINE__, context->serial, name); if (context->name_count) { int i; for (i = 0; i < context->name_count; i++) @@ -793,10 +811,10 @@ void audit_putname(const char *name) else { ++context->put_count; if (context->put_count > context->name_count) { - printk(KERN_ERR "audit.c:%d(:%d): major=%d" + printk(KERN_ERR "%s:%d(:%d): major=%d" " in_syscall=%d putname(%p) name_count=%d" " put_count=%d\n", - __LINE__, + __FILE__, __LINE__, context->serial, context->major, context->in_syscall, name, context->name_count, context->put_count); @@ -813,6 +831,8 @@ void audit_inode(const char *name, unsig int idx; struct audit_context *context = current->audit_context; + if (!context->in_syscall) + return; if (context->name_count && context->names[context->name_count-1].name && context->names[context->name_count-1].name == name) _