Re: RHEL4 panic when renaming with a watched file as the target

["Timothy R. Chavez" <tim.chavez@linux.vnet.ibm.com>]

On Mon, 05 Feb 2007 14:45:20 -0500
Eric Paris <eparis@redhat.com> wrote:

<snip>

Eric,

Good work.  Though I agree that the general solution you've provided
is good and is probably the easiest to implement, I'd prefer to have
actual wrapper calls on, say, a private function called __audit_data_get
which takes as arguments, the inode and the flags (as separate integers,
a bitmask, or an array). Then that function can be wrapped with public
calls like:

audit_data_get(inode, intent=[0|1])
	return __audit_data_get(inode, 1, intent)
 
audit_data_get_noalloc(inode):
        return __audit_data_get(inode, 0, 1)

I also think using the word "intent" (or "context") to describe the
parameter is better than "remove" as it's telling the caller to describe
how (or in what context) it intends to use the audit_inode_data it may
receive back. This would of course require you to expose a new function
to the kernel called audit_data_get_noalloc(), but that may not be
a bad thing.  Even in your patch, you're calling audit_data_get()'s that
look different but are consequentially equivalent:

audit_get_data(inode, 0, 0)
audit_get_data(inode, 0, 1)

In both, if the inode is not found in the audit_inode_data hashtable,
you'll exit the function then due to the fact that allocate=0,
regardless of what the "remove" flag was set too.

Just my two Lincolns (Thanks Casey) :)

-tim

> diff -Naupr linux-2.6.9/fs/dcache.c linux-2.6.9/fs/dcache.c
> --- linux-2.6.9/fs/dcache.c	2007-02-05 13:12:01.000000000 -0500
> +++ linux-2.6.9/fs/dcache.c	2007-02-05 13:09:36.000000000 -0500
> @@ -1292,6 +1292,7 @@ void d_move(struct dentry * dentry, stru
>  	}
> 
>  	audit_update_watch(dentry, 1);
> +	audit_update_watch(target, 1);
> 
>  	/* Move the dentry to the target hash queue, if on different bucket */
>  	if (dentry->d_flags & DCACHE_UNHASHED)
> diff -Naupr linux-2.6.9/kernel/auditfs.c linux-2.6.9/kernel/auditfs.c
> --- linux-2.6.9/kernel/auditfs.c	2007-02-05 13:11:49.000000000 -0500
> +++ linux-2.6.9/kernel/auditfs.c	2007-02-05 13:09:09.000000000 -0500
> @@ -110,7 +110,8 @@ static void audit_data_pool_shrink(void)
>  	spin_unlock(&auditfs_hash_lock);
>  }
> 
> -static struct audit_inode_data *audit_data_get(struct inode *inode, int allocate)
> +static struct audit_inode_data *audit_data_get(struct inode *inode, int allocate,
> +						int remove)
>  {
>  	struct audit_inode_data **list;
>  	struct audit_inode_data *ret = NULL;
> @@ -142,7 +143,7 @@ static struct audit_inode_data *audit_da
> 
>  	if (ret) {
>  		ret->count++;
> -	} else if (allocate) {
> +	} else if (allocate && !remove) {
>  		ret = audit_data_pool;
>  		audit_data_pool = ret->next_hash;
>  		audit_pool_size--;
> @@ -410,7 +411,7 @@ static inline int audit_insert_watch(str
>  	if (nd.last_type != LAST_NORM || !nd.last.name)
>  		goto release;
> 
> -	pdata = audit_data_get(nd.dentry->d_inode, 1);
> +	pdata = audit_data_get(nd.dentry->d_inode, 1, 0);
>  	if (!pdata)
>  		goto put_pdata;
> 
> @@ -478,7 +479,7 @@ static inline int audit_remove_watch(str
>  	if (nd.last_type != LAST_NORM || !nd.last.name)
>  		goto audit_remove_watch_release;
> 
> -	data = audit_data_get(nd.dentry->d_inode, 0);
> +	data = audit_data_get(nd.dentry->d_inode, 0, 1);
>  	if (!data)
>  		goto audit_remove_watch_release;
> 
> @@ -562,7 +563,7 @@ void audit_update_watch(struct dentry *d
> 
>  	/* If there's no audit data on the parent inode, then there can
>  	   be no watches to add or remove */
> -	parent = audit_data_get(dentry->d_parent->d_inode, 0);
> +	parent = audit_data_get(dentry->d_parent->d_inode, 0, 0);
>  	if (!parent)
>  		return;
> 
> @@ -571,7 +572,7 @@ void audit_update_watch(struct dentry *d
>  	/* Fetch audit data, using the preallocated one from the watch if
>  	   there is actually a relevant watch and the inode didn't already
>  	   have any audit data */
> -	data = audit_data_get(dentry->d_inode, !!watch);
> +	data = audit_data_get(dentry->d_inode, !!watch, remove);
> 
>  	/* If there's no data, then there wasn't a watch either.
>  	   Nothing to see here; move along */
> @@ -786,7 +787,7 @@ void audit_inode_free(struct inode *inod
>  {
>  	struct audit_watch *watch;
>  	struct hlist_node *pos, *tmp;
> -	struct audit_inode_data *data = audit_data_get(inode, 0);
> +	struct audit_inode_data *data = audit_data_get(inode, 0, 1);
> 
>  	if (data) {
>  		spin_lock(&auditfs_hash_lock);
> @@ -851,7 +852,7 @@ void audit_notify_watch(struct inode *in
>  	if (!inode || !current->audit_context)
>  		return;
> 
> -	data = audit_data_get(inode, 0);
> +	data = audit_data_get(inode, 0, 0);
>  	if (!data)
>  		return;
> 
> 
>