RHEL4 panic when renaming with a watched file as the target

RHEL4 panic when renaming with a watched file as the target

[Eric Paris]

Since the upstream filesystem auditing is different than RHEL4 this
problem (I believe) is RHEL4 specific.  Lets assume I add the rule

auditctl -w /tmp/watched_file

then I run

touch /tmp/unwatched_file
mv -f /tmp/watched_file

For the purposes of this discussion lets call the inode for the
original /tmp/watched_file i1 and the indoe for the
original /tmp/unwatched_fiel i2.

The RHEL4 kernel panics.  The reason is because it wants to allocate a
audit_inode_data to i2 but there is no audit_inode_data to allocate.
Thus we get the panic as seen in RH BZ 223129.  

When the watch was originally inserted the kernel mallocs 2
audit_inode_data structs.  One for the inode to be watched and one for
the parent of the that inode.  In our case the panic happens because
those 2 structs are allocated for i1 and for the inode associated
with /tmp.  From looking at the code I saw that in d_move we were
dropping the audit_inode_data associated with i2 (inside that function
i2 is pointed at by "dentry") at the beginning.  But we were not
dropping the watch on i1.  It appears that the watch was intended to
have been dropped on i1 later in i_put.  At the end of d_move we add the
watch for i2.  It was at this point that things blew up (as i_put had
not yet released the data structure.)

My first attempt at fixing this simply was to change d_move such that it
would drop the audit_inode_data for both i1 and i2 and then add it back
to i2 once i2 was the inode which needed the watch.  This however simply
moved the panic into i_put when it wanted to remove the audit_inode_data
from the original i1.  The reason it panics there is because when trying
to fetch the audit_inode_data to free it, it was actually trying to
allocate a new struct so it had something to free!  My solution simply
passes a new flag to audit_data_get which states if the intent of
requesting the audit_inode_data is to use it or to remove it.  If the
intent of getting the audit_inode_data was to simply remove that struct
from the inode there is no need to allocate a new one just to have it
removed.

This patch appears to fix the panic and to correctly watch the correct
inodes at the correct times.  I'd love comment as there may be other
solutions to either of the problems.  Maybe we like the removal of the
watch on i1 in d_move but we don't like the new flags.  In that case we
could just 'mess' around with i1 at the end of d_move such that it
wouldn't watch the watch in i_put and wouldn't ever get into
audit_data_get to cause the secondary panic.  Maybe there is something
I'm missing altogether.  Anyway comments please before I put this into a
RHEL4 kernel would be appreciated.

-Eric

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;
 

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

[Eric Paris]

On Mon, 2007-02-05 at 14:45 -0500, Eric Paris wrote:
> Since the upstream filesystem auditing is different than RHEL4 this
> problem (I believe) is RHEL4 specific.  Lets assume I add the rule
> 
> auditctl -w /tmp/watched_file
> 
> then I run
> 
> touch /tmp/unwatched_file
> mv -f /tmp/watched_file

*OBVIOUSLY* I meant to say

mv -f /tmp/unwatched_file /tmp/watched_file

-Eric

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

[Timothy R. Chavez]

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;
> 
> 
> 

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

[Timothy R. Chavez]

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;
> 
> 
>