[PATCH] autofs 4.1.4 no-unlink patch

[PATCH] autofs 4.1.4 no-unlink patch

[Chris Feist]

[-- Attachment #1: Type: text/plain, Size: 592 bytes --]

Ian,

We've been hearing some reports of autofs removing entire home directories 
when mounts expire.  It appears that this could be some sort of race condition 
in the walk_tree code, where when the function starts the directory is 
unmounted and then autofs unlinks files in the the directory that is supposed 
to be umounted.  I've attached a patch to the rm_unwanted_fn which adds some 
additional checks before we unlink anything, and it prevents autofs from 
unlinking files (since I'm pretty sure that autofs never creates a regular 
file).  Let me know what you think.

Thanks,
Chris

[-- Attachment #2: autofs-4.1.4-no-unlink-upstream --]
[-- Type: text/plain, Size: 1264 bytes --]

--- autofs-4.1.4/daemon/automount.c.no-unlink	2005-07-11 13:28:57.000000000 -0500
+++ autofs-4.1.4/daemon/automount.c	2005-07-11 13:50:46.000000000 -0500
@@ -216,16 +216,38 @@ static int walk_tree(const char *base, i
 static int rm_unwanted_fn(const char *file, const struct stat *st, int when, void *arg)
 {
 	int rmsymlink = *(int *) arg;
+	struct stat newst;
 
 	if (when == 0) {
 		if (st->st_dev != ap.dev)
 			return 0;
-	} else {
-		info("rm_unwanted: %s\n", file);
-		if (S_ISDIR(st->st_mode))
-			rmdir(file);
-		else if (!S_ISLNK(st->st_mode) || rmsymlink)
-			unlink(file);
+		return 1;
+	}
+
+	if (lstat(file, &newst)) {
+		crit ("rm_unwanted: unable to stat file, possible race "
+		      "condition.");
+		return 0;
+	}
+
+	if (newst.st_dev != ap.dev) {
+		crit ("rm_unwanted: file %s has the wrong device, possible "
+		      "race condition.",file);
+		return 0;
+	}
+
+	if (S_ISDIR(newst.st_mode)) {
+		if (rmdir(file)) {
+			info ("rm_unwanted: unable to remove directory"
+			      " %s", file);
+			return 0;
+		}
+	} else if (S_ISREG(newst.st_mode)) {
+		crit ("rm_unwanted: attempting to remove files from a mounted "
+		      "directory.");
+		return 0;
+	} else if (S_ISLNK(newst.st_mode) && rmsymlink) {
+		unlink(file);
 	}
 
 	return 1;

[-- Attachment #3: Type: text/plain, Size: 140 bytes --]

_______________________________________________
autofs mailing list
autofs@linux.kernel.org
http://linux.kernel.org/mailman/listinfo/autofs

Re: [PATCH] autofs 4.1.4 no-unlink patch

[Ian Kent]

On Mon, 11 Jul 2005, Chris Feist wrote:

> Ian,
> 
> We've been hearing some reports of autofs removing entire home directories
> when mounts expire.  It appears that this could be some sort of race condition
> in the walk_tree code, where when the function starts the directory is
> unmounted and then autofs unlinks files in the the directory that is supposed
> to be umounted.  I've attached a patch to the rm_unwanted_fn which adds some
> additional checks before we unlink anything, and it prevents autofs from
> unlinking files (since I'm pretty sure that autofs never creates a regular
> file).  Let me know what you think.

Yes. There doesn't seem to be enough checking and the !S_ISLNK doesn't 
look quite right. Your right, the only objects in the autofs filesystem 
are directories and symlinks.

A mount happening during an expire might lead to this. But it shouldn't be 
able to happen due to the nature of the state machine.

I'll have to keep looking.

Ian

Re: [PATCH] autofs 4.1.4 no-unlink patch

[Jeff Moyer]

==> Regarding Re: [autofs] [PATCH] autofs 4.1.4 no-unlink patch; Ian Kent <raven@themaw.net> adds:

raven> On Mon, 11 Jul 2005, Chris Feist wrote:
>> Ian,
>> 
>> We've been hearing some reports of autofs removing entire home
>> directories when mounts expire.  It appears that this could be some sort
>> of race condition in the walk_tree code, where when the function starts
>> the directory is unmounted and then autofs unlinks files in the the
>> directory that is supposed to be umounted.  I've attached a patch to the
>> rm_unwanted_fn which adds some additional checks before we unlink
>> anything, and it prevents autofs from unlinking files (since I'm pretty
>> sure that autofs never creates a regular file).  Let me know what you
>> think.

raven> Yes. There doesn't seem to be enough checking and the !S_ISLNK
raven> doesn't look quite right. Your right, the only objects in the autofs
raven> filesystem are directories and symlinks.

raven> A mount happening during an expire might lead to this. But it
raven> shouldn't be able to happen due to the nature of the state machine.

raven> I'll have to keep looking.

Right, it shouldn't be able to happen, I agree.  However, we've seen 2
occurrences of this in the wild.  Even if/when we fix this problem
properly, I would lobby to put in these extra checks.  They certainly don't
hurt anything, and they make the code more robust.  I certainly wouldn't
like it if autofs deleted all of my files!

-Jeff

Re: [PATCH] autofs 4.1.4 no-unlink patch

[Ian Kent]

On Tue, 12 Jul 2005, Jeff Moyer wrote:

> ==> Regarding Re: [autofs] [PATCH] autofs 4.1.4 no-unlink patch; Ian Kent <raven@themaw.net> adds:
> 
> raven> On Mon, 11 Jul 2005, Chris Feist wrote:
> >> Ian,
> >> 
> >> We've been hearing some reports of autofs removing entire home
> >> directories when mounts expire.  It appears that this could be some sort
> >> of race condition in the walk_tree code, where when the function starts
> >> the directory is unmounted and then autofs unlinks files in the the
> >> directory that is supposed to be umounted.  I've attached a patch to the
> >> rm_unwanted_fn which adds some additional checks before we unlink
> >> anything, and it prevents autofs from unlinking files (since I'm pretty
> >> sure that autofs never creates a regular file).  Let me know what you
> >> think.
> 
> raven> Yes. There doesn't seem to be enough checking and the !S_ISLNK
> raven> doesn't look quite right. Your right, the only objects in the autofs
> raven> filesystem are directories and symlinks.
> 
> raven> A mount happening during an expire might lead to this. But it
> raven> shouldn't be able to happen due to the nature of the state machine.
> 
> raven> I'll have to keep looking.
> 
> Right, it shouldn't be able to happen, I agree.  However, we've seen 2
> occurrences of this in the wild.  Even if/when we fix this problem
> properly, I would lobby to put in these extra checks.  They certainly don't
> hurt anything, and they make the code more robust.  I certainly wouldn't
> like it if autofs deleted all of my files!
> 

Of course.

I'll have a look and publish it as an important patch.

It would be good to understand what's happening as well.

I've never seen this happen before. I wonder what changes we've done in 
this area lately. Mind that !S_ISLNK has been there since 4.0.0per?? and 
looks way wrong.

Ian