lkml.org 
[lkml]   [2004]   [Mar]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/

\
 
 \ /
  Last update: 2005-03-22 14:01    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site