Re: [PATCH 06/11] Task watchers: Register audit task watcher

[Matt Helsley <matthltc@us.ibm.com>]

On Wed, 2006-06-14 at 10:46 -0400, Alexander Viro wrote:
> On Tue, Jun 13, 2006 at 04:54:46PM -0700, Matt Helsley wrote:
> > Adapt audit to use task watchers.
> 
> audit_free(p) really expects that either p is a stillborn (never ran)
> *or* that p == current.

	Makes sense. I think the task watcher patches are consistent with this.
I think the first patch of this series helps explain how this patch
remains consistent with the above. I should have cc'd linux-audit when
posting that patch -- here's a link for now:
http://www.ussg.iu.edu/hypermail/linux/kernel/0606.1/1800.html

	In copy_process() and do_exit() notify_watchers() passes the same
pointers as audit_alloc() and audit_free() used before. The patches also
do not introduce or remove calls to audit_alloc() or audit_free(). The
patches trigger these calls with notify_watchers() while passing
WATCH_TASK_INIT and WATCH_TASK_FREE for audit_alloc() and audit_free()
respectively. WATCH_TASK_INIT (and hence audit_alloc()) only happens in
copy_process(). WATCH_TASK_FREE (and hence audit_free()) happens in
copy_process()'s error recovery path and in do_exit().

	This results in the same calls to audit_alloc() and audit_free() except
with an additional function call preceding them on the stack.

	Are you concerned that future modifications of task watchers would pass
in task structs that violate these expectations? I can alter the patches
to incorporate these restrictions:

copy_process()
{
	...
	notify_watchers(WATCH_TASK_INIT, p);
	...
	if (<succeeding>)
		notify_watchers(WATCH_TASK_CLONE, p);
	...
bad_foo:
	...
- 	notify_watchers(WATCH_TASK_FREE, p);
+ 	notify_watchers(WATCH_TASK_ABORT, p);
	...
}

<change all other notify_watchers() invocations to pass NULL as
the second parameter, e.g.>

do_exit()
{
	...
	notify_watchers(WATCH_TSK_FREE, NULL); /* callees must use current */
}

However this requires that I modify each user of task watchers with
something like:

int foo (struct notifier_block *nb, unsigned long val, void *v)
{
-	struct task_struct *tsk = v;
+	struct task_struct *tsk = current;
	...
	switch(get_watch_event(val)) {
	case WATCH_TASK_INIT:
+ 		tsk = v; /* INIT and ABORT use v, the rest use current */
	...
+ 	case WATCH_TASK_ABORT:
+ 		tsk = v; /* fall through */
	case WATCH_TASK_FREE:
		...
	}
	...
}

Which seems a bit more complicated. Is this worth it?

Cheers,
	-Matt Helsley