corner cases of open() on procfs symlinks

I'm not sure whether to treat that as a bug or as a weird misfeature
enshrined in userland ABI:

	open("/tmp", O_CREAT, 0) => -EISDIR	// LAST_NORM case
	open("/", O_CREAT, 0) => -EISDIR	// LAST_ROOT
	open(".", O_CREAT, 0) => -EISDIR	// LAST_DOT
	open("..", O_CREAT, 0) => -EISDIR	// LAST_DOTDOT
	open("/proc/self/cwd", O_CREAT, 0) => success	// LAST_BIND
	open("/proc/self/cwd/", O_CREAT, 0) => -EISDIR	// trailing slashes

At the very least, it's inconsistent. OTOH, it's exposed to userland. OTTH, SuS says that O_CREAT without O_RDWR or O_WRONLY is undefined, and either of those two would suffice for -EISDIR in all cases (may_open() takes care of that).

Another unpleasantness:

	open("/proc/self/exe", LOOKUP_DIRECTORY, 0) -> success

That one is clearly a bug. Moreover, getting rid of both of those bugs actually simplifies the mess in do_last(). I would obviously like to do that - do_last() is far too convoluted as it is; the only question is whether we can change the first weirdness... Comments?

FWIW, the simplification of do_last() would look like that:

index 85e40d1..617599c 100644
@@ -2690,28 +2690,10 @@ static int do_last(struct nameidata *nd, struct path *path,
 	nd->flags &= ~LOOKUP_PARENT;
 	nd->flags |= op->intent;
 
-	switch (nd->last_type) {
-	case LAST_DOTDOT:
-	case LAST_DOT:
+	if (nd->last_type != LAST_NORM) {
 		error = handle_dots(nd, nd->last_type);
 		if (error)
 			return error;
-		/* fallthrough */
-	case LAST_ROOT:
-		error = complete_walk(nd);
-		if (error)
-			return error;
-		audit_inode(name, nd->path.dentry, 0);
-		if (open_flag & O_CREAT) {
-			error = -EISDIR;
-			goto out;
-		}
-		goto finish_open;
-	case LAST_BIND:
-		error = complete_walk(nd);
-		if (error)
-			return error;
-		audit_inode(name, dir, 0);
 		goto finish_open;
 	}
 

@@ -2841,6 +2823,7 @@ finish_lookup:
 	}
 	nd->inode = inode;
 	/* Why this, you ask?  _Now_ we might have grown LOOKUP_JUMPED... */
+finish_open:
 	error = complete_walk(nd);
 	if (error) {
 		path_put(&save_parent);

@@ -2853,7 +2836,6 @@ finish_lookup:
 	if ((nd->flags & LOOKUP_DIRECTORY) && !nd->inode->i_op->lookup)
 		goto out;
 	audit_inode(name, nd->path.dentry, 0);
-finish_open:
 	if (!S_ISREG(nd->inode->i_mode))
 		will_truncate = false;
 

Ok, that looks buggy. O_CREAT should definitely return EISDIR for /proc/self/cwd too, since it's a directory. I don't think the O_RDWR/O_WRONLY thing should matter.

Exactly which cases does that change? I have no objections if it's only the "LAST_BIND" case that now starts returning EISDIR. Is there anything else it affects?

That said, obviously if something breaks, we'd have to revert it, and as a cleanup rather than some serious bug (ie this doesn't cause crashes or security issues), I suspect this should wait until 3.11 regardless. No?

Linus

LAST_BIND gets to go through the EISDIR and ENOTDIR checks that way, which fixes these two bugs.

LAST_DOT/LAST_DOTDOT/LAST_ROOT end up checking whether we are at the directory or not; sure, we know that we are, so these tests are redundant, but I really don't think it's worth optimizing for. We are not generating any data misses and arguably we reduce instruction cache footprint a bit, not that it would be noticable with the I$ horror do_last() still is...

What really happens in that switch is that do_last() tries to be too smart and ends up skipping a few things too many.

Probably... procfs symlinks neutering O_DIRECTORY might, in theory, be usable to cook something nasty, but I don't see any obvious ways to exploit that. FWIW, resulting kernel seems to survive the minimal beating, but obviously more is needed.

Let's plan on merging that patch in the 3.11 merge window, and perhaps mark it for stable?

Linus