Messages in this thread | | | Date | Fri, 12 Mar 2004 10:50:33 -0800 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH] Light-weight Auditing Framework |
| |
On Thu, Mar 11, 2004 at 09:25:46AM -0500, Rik Faith wrote: > Below is a patch against 2.6.4 that provides a low-overhead system-call > auditing framework for Linux that is usable by LSM components (e.g., > SELinux). This is an update of the patch discussed in this thread: > http://marc.theaimsgroup.com/?t=107815888100001&r=1&w=2
[ . . . ]
Great application of RCU -- audit rules should not change often, but could be referenced quite frequently!
A couple of comments:
o I don't see any rcu_read_lock() or rcu_read_unlock() calls. These are needed on the read side in order to (1) let the people reading the code know the extent of the read-side critical section and (2) disable preemption in CONFIG_PREEMPT kernels. Without the former, someone will end up putting a blocking primitive in the wrong place. Without the latter, the kernel will do the dirty work all by itself. Either way, you get breakage.
For example, I suspect that audit_filter_task() needs to read as follows:
static enum audit_state audit_filter_task(struct task_struct *tsk) { 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)) { rcu_read_unlock(); return state; } } rcu_read_unlock(); return AUDIT_BUILD_CONTEXT; }
Alternatively, you could put the rcu_read_lock() and rcu_read_unlock() around the single call to audit_filter_task() from audit_alloc().
All of the list_for_each_.*_rcu() macros need to be enclosed by rcu_read_lock() and rcu_read_unlock() calls.
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.
Once this locking is present, the list_for_each_entry_rcu() in audit_del_rule() should be changed to list_for_each_entry(), as it cannot race with deletion, since it -is- deletion. The list_del_rcu() is correct, and should remain.
If you are using some sort of implicit locking, then please inject a clue...
o The audit_add_rule() function also needs something to prevent races with other audit_add_rule() and audit_del_rule() instances. Again, this might be happening somehow in the netlink_kernel_create() mechanism, but I don't immediately see it. Then again, I do not claim to fully understand how the netlink_kernel_create() mechanism functions.
Again, looks like a promising application of RCU!
Thanx, Paul - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |