Skip to contentSkip to conversation

New: open("a/", O_NOFOLLOW) fails with ELOOP if "a" is a symbolic link to a directory.

Bugme-new Andrew Morton 6 participants 11 messages May 6, 2010 – May 17, 2010 2 messages not in the archive VFS

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]/

Andrew Morton

author
#1 lore raw

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).

Quoted 28 lines · On Wed, 5 May 2010 13:01:22 GMT [email protected] wrote:

https://bugzilla.kernel.org/show_bug.cgi?id=15909

Summary: open("a/",O_NOFOLLOW) fails with ELOOP if "a" is a
symbolic link to a directory.
Product: File System
Version: 2.5
Kernel Version: 2.6.34-rc6
Platform: All
OS/Version: Linux
Tree: Mainline
Status: NEW
Severity: high
Priority: P1
Component: Other
AssignedTo: [email protected]
ReportedBy: [email protected]
Regression: No

mkdir c
ln -s c a

f=open("a/",O_RDONLY+O_NOFOLLOW)

fails with ELOOP. However, this open should behave like open("a/.") not like open("a") according to path_resolution(7). In kernel version 2.6.32 the open worked as documented.

Quoted 6 lines

On a higher level this bug makes

find a/

to fail.

It sounds like this 2.6.32->2.6.34-rc6 regression could have pretty serious ramifications for some users. Does anyone know whcih commit might have caused it?

#2 lore raw
Quoted 11 lines · Andrew Morton
Quoted 11 lines

mkdir c
ln -s c a

f=open("a/",O_RDONLY+O_NOFOLLOW)

fails with ELOOP. However, this open should behave like open("a/.") not like open("a") according to path_resolution(7). In kernel version 2.6.32 the open worked as documented.

On a higher level this bug makes

Quoted 8 lines
Quoted 4 lines

find a/

to fail.

It sounds like this 2.6.32->2.6.34-rc6 regression could have pretty serious ramifications for some users. Does anyone know whcih commit might have caused it?

This probably is the part of do_last cleanup series. (I was looking this series, but unfortunately I also didn't notice about this bug..)

And with quick-look, it seems to be in do_last():

	/* trailing slashes? */
	if (nd->last.name[nd->last.len]) {
		if (open_flag & O_CREAT)
			goto exit;
		nd->flags |= LOOKUP_DIRECTORY;
	}

Before (i.e. do_path_lookup()), it was doing with LOOKUP_FOLLOW for "trailing slash". Current one, it isn't.

If Al or someone don't fix, I'll see more later (it may be next weekend or so, sorry).

Thanks.

Signature · 1 line
OGAWA Hirofumi <[email protected]>
reviewer
#3 lore raw
In reply to Andrew Morton
Quoted 37 lines · Andrew Morton

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).

Quoted 30 lines · On Wed, 5 May 2010 13:01:22 GMT [email protected] wrote:

https://bugzilla.kernel.org/show_bug.cgi?id=15909

Summary: open("a/",O_NOFOLLOW) fails with ELOOP if "a" is a
symbolic link to a directory.
Product: File System
Version: 2.5
Kernel Version: 2.6.34-rc6
Platform: All
OS/Version: Linux
Tree: Mainline
Status: NEW
Severity: high
Priority: P1
Component: Other
AssignedTo: [email protected]
ReportedBy: [email protected]
Regression: No

mkdir c
ln -s c a

f=open("a/",O_RDONLY+O_NOFOLLOW)

fails with ELOOP. However, this open should behave like open("a/.") not like open("a") according to path_resolution(7). In kernel version 2.6.32 the open worked as documented.

On a higher level this bug makes

Quoted 8 lines
Quoted 4 lines

find a/

to fail.

It sounds like this 2.6.32->2.6.34-rc6 regression could have pretty serious ramifications for some users. Does anyone know whcih commit might have caused it?

The patch below fixes the issue for me but someone should have a look
at it because I'm not really an expert in that code and the code paths are so twisted that my mind is currently tied into a knot ;).

Honza
---

Quoted 1 line

From d53d3cc6488d9135bb69c3ff7e034b3b624866ed Mon Sep 17 00:00:00 2001

From: Jan Kara <[email protected]>
Date: Tue, 11 May 2010 16:34:25 +0200
Subject: [PATCH] vfs: Fix O_NOFOLLOW behavior for paths with trailing slashes

According to specification
mkdir d; ln -s d a; open("a/", O_NOFOLLOW | O_RDONLY)
should return success but currently it did return ELOOP. Fix the code to ignore O_NOFOLLOW in case the provided path has trailing slashes. This is a regression caused by path lookup cleanup patch series.

CC [email protected]
Signed-off-by Jan Kara <[email protected]>
---
 fs/namei.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
fs/namei.c +5 -3
index a7dce91..4cc7fbc 100644
@@ -1641,7 +1641,7 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
 	if (nd->last.name[nd->last.len]) {
 		if (open_flag & O_CREAT)
 			goto exit;
-		nd->flags |= LOOKUP_DIRECTORY;
+		nd->flags |= LOOKUP_DIRECTORY | LOOKUP_FOLLOW;
 	}
 
 	/* just plain open? */

@@ -1722,7 +1722,7 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
 
 	if (__follow_mount(path)) {
 		error = -ELOOP;
-		if (open_flag & O_NOFOLLOW)
+		if (!(nd->flags & LOOKUP_FOLLOW))
 			goto exit_dput;
 	}
 

@@ -1830,6 +1830,8 @@ reval:
 	}
 	if (open_flag & O_DIRECTORY)
 		nd.flags |= LOOKUP_DIRECTORY;
+	if (!(open_flag & O_NOFOLLOW))
+		nd.flags |= LOOKUP_FOLLOW;
 	filp = do_last(&nd, &path, open_flag, acc_mode, mode, pathname);
 	while (unlikely(!filp)) { /* trailing symlink */
 		struct path holder;

@@ -1837,7 +1839,7 @@ reval:
 		void *cookie;
 		error = -ELOOP;
 		/* S_ISDIR part is a temporary automount kludge */
-		if ((open_flag & O_NOFOLLOW) && !S_ISDIR(inode->i_mode))
+		if (!(nd.flags & LOOKUP_FOLLOW) && !S_ISDIR(inode->i_mode))
 			goto exit_dput;
 		if (count++ == 32)
 			goto exit_dput;
Signature · 1 line
1.6.4.2

A message that is not in the archive (sent off-list or not yet received): 20100511154850.GC2832-+0h/O2h83AeN3ZZ/[email protected]

#5 lore raw
Quoted 58 lines · Jan Kara
Quoted 37 lines · Andrew Morton

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).

Quoted 30 lines · On Wed, 5 May 2010 13:01:22 GMT bugzilla-daemon-590EEB7GvNiWaY/ihj7yzEB+6BGkLq7…

https://bugzilla.kernel.org/show_bug.cgi?id=15909

Summary: open("a/",O_NOFOLLOW) fails with ELOOP if "a" is a
symbolic link to a directory.
Product: File System
Version: 2.5
Kernel Version: 2.6.34-rc6
Platform: All
OS/Version: Linux
Tree: Mainline
Status: NEW
Severity: high
Priority: P1
Component: Other
AssignedTo: fs_other-ztI5WcYan/[email protected]
ReportedBy: tolzmann-KUpvgZVWgV9o1qOY/[email protected]
Regression: No

mkdir c
ln -s c a

f=open("a/",O_RDONLY+O_NOFOLLOW)

fails with ELOOP. However, this open should behave like open("a/.") not like open("a") according to path_resolution(7). In kernel version 2.6.32 the open worked as documented.

On a higher level this bug makes

Quoted 8 lines
Quoted 4 lines

find a/

to fail.

It sounds like this 2.6.32->2.6.34-rc6 regression could have pretty serious ramifications for some users. Does anyone know whcih commit might have caused it?

The patch below fixes the issue for me but someone should have a look
at it because I'm not really an expert in that code and the code paths are so twisted that my mind is currently tied into a knot ;).

Honza
---

From d53d3cc6488d9135bb69c3ff7e034b3b624866ed Mon Sep 17 00:00:00 2001
From: Jan Kara <[email protected]>
Date: Tue, 11 May 2010 16:34:25 +0200
Subject: [PATCH] vfs: Fix O_NOFOLLOW behavior for paths with trailing slashes

Quoted 8 lines

According to specification
mkdir d; ln -s d a; open("a/", O_NOFOLLOW | O_RDONLY)
should return success but currently it did return ELOOP. Fix the code to ignore O_NOFOLLOW in case the provided path has trailing slashes. This is a regression caused by path lookup cleanup patch series.

CC [email protected]
Signed-off-by Jan Kara <[email protected]>

...
BTW: It might be worthwhile to add the attached testcase to LTP?

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR

#define _GNU_SOURCE
#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#include <string.h>
#include <sys/stat.h>
#include <sys/types.h>

int do_test(char *p)
{
int err;
char path[16];

strcpy(path, p);
err = open(path, O_NOFOLLOW | O_RDONLY);

  if (err >= 0) {
    fprintf(stderr, "open(\"%s\", O_NOFOLLOW | O_RDONLY) did not fail!\n", path);
    return 1;
  }
  strcat(path, "/");
  err = open(path, O_NOFOLLOW | O_RDONLY);
  if (err < 0) {
    fprintf(stderr, "open(\"%s\", O_NOFOLLOW | O_RDONLY) failed with %d\n", path, errno);
    return 1;
  }
  strcat(path, ".");
  err = open(path, O_NOFOLLOW | O_RDONLY);
  if (err < 0) {
    fprintf(stderr, "open(\"%s\", O_NOFOLLOW | O_RDONLY) failed with %d\n", path, errno);
    return 1;

}
return 0;
}

int main(void)
{

  if (mkdir("d", 0700) < 0) {
    perror("mkdir");
    return 1;
  }
  if (symlink("d", "a") < 0) {
    perror("link");
    return 1;
  }
  if (do_test("a"))
    return 1;

return 0;
}

------------------------------------------------------------------------------

_______________________________________________
Ltp-list mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

#6 lore raw

Hi Jan,

Quoted 73 lines · Jan Kara
Quoted 3 lines · Jan Kara
Quoted 2 lines · Andrew Morton

(switched to email. Please respond via emailed reply-to-all, not

via the

Quoted 8 lines
Quoted 8 lines

bugzilla web interface).

Quoted 3 lines · On Wed, 5 May 2010 13:01:22 GMT [email protected] wrote:

https://bugzilla.kernel.org/show_bug.cgi?id=15909

Summary: open("a/",O_NOFOLLOW) fails with ELOOP if

"a" is a

Quoted 16 lines
Quoted 16 lines
Quoted 16 lines

symbolic link to a directory.
Product: File System
Version: 2.5
Kernel Version: 2.6.34-rc6
Platform: All
OS/Version: Linux
Tree: Mainline
Status: NEW
Severity: high
Priority: P1
Component: Other
AssignedTo: [email protected]
ReportedBy: [email protected]
Regression: No

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

mkdir c
ln -s c a

f=open("a/",O_RDONLY+O_NOFOLLOW)

fails with ELOOP. However, this open should behave like

open("a/.") not like

Quoted 1 line
Quoted 1 line
Quoted 1 line

open("a") according to path_resolution(7). In kernel version

2.6.32 the open

Quoted 10 lines
Quoted 10 lines
Quoted 8 lines

worked as documented.

On a higher level this bug makes

find a/

to fail.

It sounds like this 2.6.32->2.6.34-rc6 regression could have

pretty

Quoted 1 line
Quoted 1 line

serious ramifications for some users. Does anyone know whcih

commit

Quoted 2 lines
Quoted 1 line

might have caused it?

The patch below fixes the issue for me but someone should have a

look

Quoted 1 line

at it because I'm not really an expert in that code and the code

paths are so

Quoted 6 lines

twisted that my mind is currently tied into a knot ;).

Honza
---

From d53d3cc6488d9135bb69c3ff7e034b3b624866ed Mon Sep 17 00:00:00

2001

Quoted 3 lines

From: Jan Kara <[email protected]>
Date: Tue, 11 May 2010 16:34:25 +0200
Subject: [PATCH] vfs: Fix O_NOFOLLOW behavior for paths with

trailing slashes

Quoted 4 lines

According to specification
mkdir d; ln -s d a; open("a/", O_NOFOLLOW | O_RDONLY) should return success but currently it did return ELOOP. Fix the

code to ignore

Quoted 8 lines
Quoted 1 line

O_NOFOLLOW in case the provided path has trailing slashes. This is a

regression

Quoted 4 lines

caused by path lookup cleanup patch series.

CC [email protected]
Signed-off-by Jan Kara <[email protected]>

...
BTW: It might be worthwhile to add the attached testcase to LTP?

It would be great if you can shoot it in the form of a patch, mentioning

1. The GPL Lincense,
2. The purpose of the test case,
3. Exact location it goes,
4. Which LTPROOT/runtest/<file> executes it

Regards--
Subrata

:-)

Quoted 68 lines

Honza

Signature · 65 lines
Jan Kara <[email protected]>
SUSE Labs, CR







C++ source code
attachment
(open-follow-test.c)

#define _GNU_SOURCE
#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#include <string.h>
#include <sys/stat.h>
#include <sys/types.h>

int do_test(char *p)
{
  int err;
  char path[16];

  strcpy(path, p);
  err = open(path, O_NOFOLLOW | O_RDONLY);
  if (err >= 0) {
    fprintf(stderr, "open(\"%s\", O_NOFOLLOW | O_RDONLY) did not fail!
\n", path);
    return 1;
  }
  strcat(path, "/");
  err = open(path, O_NOFOLLOW | O_RDONLY);
  if (err < 0) {
    fprintf(stderr, "open(\"%s\", O_NOFOLLOW | O_RDONLY) failed with %
d\n", path, errno);
    return 1;
  }
  strcat(path, ".");
  err = open(path, O_NOFOLLOW | O_RDONLY);
  if (err < 0) {
    fprintf(stderr, "open(\"%s\", O_NOFOLLOW | O_RDONLY) failed with %
d\n", path, errno);
    return 1;
  }
  return 0;
}

int main(void)
{
  if (mkdir("d", 0700) < 0) {
    perror("mkdir");
    return 1;
  }
  if (symlink("d", "a") < 0) {
    perror("link");
    return 1;
  }
  if (do_test("a"))
    return 1;
  return 0;
}

------------------------------------------------------------------------------

_______________________________________________
Ltp-list mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ltp-list

reviewer
#7 lore raw

Hi,

Quoted 6 lines · Subrata Modak

It would be great if you can shoot it in the form of a patch, mentioning

1. The GPL Lincense,
2. The purpose of the test case,
3. Exact location it goes,
4. Which LTPROOT/runtest/<file> executes it

With a help of Cyril I've created the attached patch and verified that
the test fails on 2.6.33 but succeeds on 2.6.32...

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR

Add O_NOFOLLOW open test with trailing slash

In a situation where symlink (say 's') points to a directory, open("s/", O_NOFOLLOW) should succeed. Check for this.

Signed-off-by Jan Kara <[email protected]>
testcases/kernel/syscalls/open/open07.c renamed from testcases/kernel/syscalls/open/open07.c.orig +71 -23
@@ -40,6 +40,9 @@
  * 	4. Create a symbolic link to a symbolically linked directory, and call
  *	   open(O_NOFOLLOW). Check that it returns ELOOP.
  *
+ * 	5. Create a symbolic link to a directory, and call
+ *         open("link/", O_NOFOLLOW). Check that it succeeds.
+ *
  * USAGE:  <for command-line>
  *  open07 [-c n] [-e] [-i n] [-I x] [-P x] [-t]
  *     where,  -c n : Run n copies concurrently.

@@ -70,34 +73,35 @@ void setupfunc_test1();
 void setupfunc_test2();
 void setupfunc_test3();
 void setupfunc_test4();
+void setupfunc_test5();
 
 char *TCID = "open07";
-int TST_TOTAL = 4;
+int TST_TOTAL = 5;
 extern int Tst_count;
 int fd1, fd2;
 
-char file1[100], file2[100], file3[100];
-
 int exp_enos[] = {ELOOP, 0};
 
 struct test_case_t {
 	char *desc;
-	char *filename;
+	char filename[100];
 	int flags;
 	int mode;
 	void (*setupfunc)();
 	int exp_errno;
-    int fileHandle;
+	int fileHandle;
 } TC[] = {
-	{ "Test for ELOOP on f2: f1 -> f2", file2, O_NOFOLLOW, 00700,
+	{ "Test for ELOOP on f2: f1 -> f2", {}, O_NOFOLLOW, 00700,
 		setupfunc_test1, ELOOP, 0},
-	{ "Test for ELOOP on d2: d1 -> d2", file2, O_NOFOLLOW, 00700,
+	{ "Test for ELOOP on d2: d1 -> d2", {}, O_NOFOLLOW, 00700,
 		setupfunc_test2, ELOOP, 0},
-	{ "Test for ELOOP on f3: f1 -> f2 -> f3", file3, O_NOFOLLOW, 00700,
+	{ "Test for ELOOP on f3: f1 -> f2 -> f3", {}, O_NOFOLLOW, 00700,
 		setupfunc_test3, ELOOP, 0},
-	{ "Test for ELOOP on d3: d1 -> d2 -> d3", file3, O_NOFOLLOW, 00700,
+	{ "Test for ELOOP on d3: d1 -> d2 -> d3", {}, O_NOFOLLOW, 00700,
 		setupfunc_test4, ELOOP, 0},
-	{ NULL, NULL, 0, 0, NULL, 0, 0}
+	{ "Test for success on d2: d1 -> d2", {}, O_NOFOLLOW, 00700,
+		setupfunc_test5, 0, 0},
+	{ NULL, {}, 0, 0, NULL, 0, 0}
 };
 
 int main(int ac, char **av)

@@ -132,21 +136,34 @@ int main(int ac, char **av)
 
 			TEST(open(TC[i].filename, TC[i].flags, TC[i].mode));
 
-			if (TEST_RETURN != -1) {
-				tst_resm(TFAIL, "call succeeded unexpectedly");
-			}
-
-			TEST_ERROR_LOG(TEST_ERRNO);
-
-			if (TEST_ERRNO != TC[i].exp_errno) {
-				tst_resm(TFAIL, "open returned unexpected "
-					 "errno, expected: %d, got: %d",
-					 TC[i].exp_errno, TEST_ERRNO);
+			if (TC[i].exp_errno != 0) {
+				if (TEST_RETURN != -1) {
+					tst_resm(TFAIL, "open succeeded "
+						 "unexpectedly");
+				}
+				TEST_ERROR_LOG(TEST_ERRNO);
+
+				if (TEST_ERRNO != TC[i].exp_errno) {
+					tst_resm(TFAIL, "open returned "
+						 "unexpected errno, expected: "
+						 "%d, got: %d",
+						 TC[i].exp_errno, TEST_ERRNO);
+				} else {
+					tst_resm(TPASS, "open returned "
+						 "expected ELOOP error");
+				}
 			} else {
-				tst_resm(TPASS, "open returned expected "
-					 "ELOOP error");
+				if (TEST_RETURN == -1) {
+					tst_resm(TFAIL, "open failed "
+						 "unexpectedly with errno %d",
+						 TEST_ERRNO);
+				} else {
+					tst_resm(TPASS, "open succeeded as "
+						 "expected");
+				}
 			}
-            close(TC[i].fileHandle);
+
+			close(TC[i].fileHandle);
 		}
 	}
 	cleanup();

@@ -157,6 +174,8 @@ int main(int ac, char **av)
 void
 setupfunc_test1()
 {
+	char file1[100], file2[100];
+
 	sprintf(file1, "open03.1.%d", getpid());
 	sprintf(file2, "open03.2.%d", getpid());
 	if ((fd1 = creat(file1, 00700)) < 0) {

@@ -167,11 +186,14 @@ setupfunc_test1()
 		tst_brkm(TBROK, cleanup, "symlink(2) failed: errno: %d", errno);
 		/*NOTREACHED*/
 	}
+	strcpy(TC[0].filename, file2);
 }
 
 void
 setupfunc_test2()
 {
+	char file1[100], file2[100];
+
 	sprintf(file1, "open03.3.%d", getpid());
 	sprintf(file2, "open03.4.%d", getpid());
 	if (mkdir(file1, 00700) < 0) {

@@ -182,11 +204,14 @@ setupfunc_test2()
 		tst_brkm(TBROK, cleanup, "symlink(2) failed: errno: %d", errno);
 		/*NOTREACHED*/
 	}
+	strcpy(TC[1].filename, file2);
 }
 
 void
 setupfunc_test3()
 {
+	char file1[100], file2[100], file3[100];
+
 	sprintf(file1, "open03.5.%d", getpid());
 	sprintf(file2, "open03.6.%d", getpid());
 	sprintf(file3, "open03.7.%d", getpid());

@@ -202,11 +227,14 @@ setupfunc_test3()
 		tst_brkm(TBROK, cleanup, "symlink(2) failed: errno: %d", errno);
 		/*NOTREACHED*/
 	}
+	strcpy(TC[2].filename, file3);
 }
 
 void
 setupfunc_test4()
 {
+	char file1[100], file2[100], file3[100];
+
 	sprintf(file1, "open03.8.%d", getpid());
 	sprintf(file2, "open03.9.%d", getpid());
 	sprintf(file3, "open03.10.%d", getpid());

@@ -222,6 +250,26 @@ setupfunc_test4()
 		tst_brkm(TBROK, cleanup, "symlink(2) failed: errno: %d", errno);
 		/*NOTREACHED*/
 	}
+	strcpy(TC[3].filename, file3);
+}
+
+void
+setupfunc_test5()
+{
+	char file1[100], file2[100];
+
+	sprintf(file1, "open11.3.%d", getpid());
+	sprintf(file2, "open12.4.%d", getpid());
+	if (mkdir(file1, 00700) < 0) {
+		tst_brkm(TBROK, cleanup, "mkdir(2) failed: errno: %d", errno);
+		/*NOTREACHED*/
+	}
+	if (symlink(file1, file2) < 0) {
+		tst_brkm(TBROK, cleanup, "symlink(2) failed: errno: %d", errno);
+		/*NOTREACHED*/
+	}
+	strcpy(TC[4].filename, file2);
+	strcat(TC[4].filename, "/");
 }
 
 /*
#8 lore raw
Quoted 11 lines · Jan Kara

Hi,

Quoted 6 lines · Subrata Modak

It would be great if you can shoot it in the form of a patch, mentioning

1. The GPL Lincense,
2. The purpose of the test case,
3. Exact location it goes,
4. Which LTPROOT/runtest/<file> executes it

With a help of Cyril I've created the attached patch and verified that
the test fails on 2.6.33 but succeeds on 2.6.32...

Great. Thanks for the patch. But, it falters on the latest git. Can you please rebase and resend:

patching file testcases/kernel/syscalls/open/open07.c Hunk #1 succeeded at 40 with fuzz 1.
Hunk #2 FAILED at 73.
Hunk #3 FAILED at 136.
Hunk #4 succeeded at 173 with fuzz 2 (offset -1 lines). Hunk #5 FAILED at 185.
Hunk #6 FAILED at 203.
Hunk #7 FAILED at 226.
Hunk #8 FAILED at 249.
6 out of 8 hunks FAILED -- saving rejects to file testcases/kernel/syscalls/open/open07.c.rej

Regards--
Subrata

Quoted 2 lines

Honza

reviewer
#9 lore raw
Quoted 10 lines · Subrata Modak
Quoted 9 lines · Jan Kara

Hi,

Quoted 6 lines · Subrata Modak

It would be great if you can shoot it in the form of a patch, mentioning

1. The GPL Lincense,
2. The purpose of the test case,
3. Exact location it goes,
4. Which LTPROOT/runtest/<file> executes it

Quoted 5 lines
Quoted 2 lines

With a help of Cyril I've created the attached patch and verified that
the test fails on 2.6.33 but succeeds on 2.6.32...

Great. Thanks for the patch. But, it falters on the latest git. Can you please rebase and resend:

Ah, OK. Sorry for that. Now a patch rediffed against latest git tree of
LTP. Hopefully this time I got it right.

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
Quoted 1 line

From 02340e1c55f2b446a1706ee087559b6e70480990 Mon Sep 17 00:00:00 2001

From: Jan Kara <[email protected]>
Date: Thu, 13 May 2010 13:24:51 +0200
Subject: [PATCH] Add O_NOFOLLOW open test with trailing slash

In a situation where symlink (say 's') points to a directory, open("s/", O_NOFOLLOW) should succeed. Check for this.

Signed-off-by Jan Kara <[email protected]>
---
 testcases/kernel/syscalls/open/open07.c |   98 ++++++++++++++++++++++---------
 1 files changed, 71 insertions(+), 27 deletions(-)
testcases/kernel/syscalls/open/open07.c +71 -27
index 9d2fc04..2322e85 100644
@@ -40,6 +40,9 @@
  *	4. Create a symbolic link to a symbolically linked directory, and call
  *	   open(O_NOFOLLOW). Check that it returns ELOOP.
  *
+ * 	5. Create a symbolic link to a directory, and call
+ *         open("link/", O_NOFOLLOW). Check that it succeeds.
+ *
  * USAGE:  <for command-line>
  *  open07 [-c n] [-e] [-i n] [-I x] [-P x] [-t]
  *     where,  -c n : Run n copies concurrently.

@@ -70,35 +73,35 @@ void setupfunc_test1();
 void setupfunc_test2();
 void setupfunc_test3();
 void setupfunc_test4();
+void setupfunc_test5();
 
 char *TCID = "open07";
-int TST_TOTAL = 4;
+int TST_TOTAL = 5;
 extern int Tst_count;
 int fd1, fd2;
 
-char file1[100], file2[100], file3[100];
-
 int exp_enos[] = { ELOOP, 0 };
 
 struct test_case_t {
 	char *desc;
-	char *filename;
+	char filename[100];
 	int flags;
 	int mode;
 	void (*setupfunc) ();
 	int exp_errno;
-	int fileHandle;
 } TC[] = {
 	{
-	"Test for ELOOP on f2: f1 -> f2", file2, O_NOFOLLOW, 00700,
-		    setupfunc_test1, ELOOP, 0}, {
-	"Test for ELOOP on d2: d1 -> d2", file2, O_NOFOLLOW, 00700,
-		    setupfunc_test2, ELOOP, 0}, {
-	"Test for ELOOP on f3: f1 -> f2 -> f3", file3, O_NOFOLLOW,
-		    00700, setupfunc_test3, ELOOP, 0}, {
-	"Test for ELOOP on d3: d1 -> d2 -> d3", file3, O_NOFOLLOW,
-		    00700, setupfunc_test4, ELOOP, 0}, {
-	NULL, NULL, 0, 0, NULL, 0, 0}
+	"Test for ELOOP on f2: f1 -> f2", {}, O_NOFOLLOW, 00700,
+		    setupfunc_test1, ELOOP}, {
+	"Test for ELOOP on d2: d1 -> d2", {}, O_NOFOLLOW, 00700,
+		    setupfunc_test2, ELOOP}, {
+	"Test for ELOOP on f3: f1 -> f2 -> f3", {}, O_NOFOLLOW,
+		    00700, setupfunc_test3, ELOOP}, {
+	"Test for ELOOP on d3: d1 -> d2 -> d3", {}, O_NOFOLLOW,
+		    00700, setupfunc_test4, ELOOP}, {
+	"Test for success on d2: d1 -> d2", {}, O_NOFOLLOW, 00700,
+		    setupfunc_test5, 0}, {
+	NULL, {}, 0, 0, NULL, 0}
 };
 
 int main(int ac, char **av)

@@ -133,21 +136,34 @@ int main(int ac, char **av)
 
 			TEST(open(TC[i].filename, TC[i].flags, TC[i].mode));
 
-			if (TEST_RETURN != -1) {
-				tst_resm(TFAIL, "call succeeded unexpectedly");
-			}
+			if (TC[i].exp_errno != 0) {
+				if (TEST_RETURN != -1) {
+					tst_resm(TFAIL, "open succeeded "
+						 "unexpectedly");
+				}
+				TEST_ERROR_LOG(TEST_ERRNO);
 
-			TEST_ERROR_LOG(TEST_ERRNO);
-
-			if (TEST_ERRNO != TC[i].exp_errno) {
-				tst_resm(TFAIL, "open returned unexpected "
-					 "errno, expected: %d, got: %d",
-					 TC[i].exp_errno, TEST_ERRNO);
+				if (TEST_ERRNO != TC[i].exp_errno) {
+					tst_resm(TFAIL, "open returned "
+						 "unexpected errno, expected: "
+						 "%d, got: %d",
+						 TC[i].exp_errno, TEST_ERRNO);
+				} else {
+					tst_resm(TPASS, "open returned "
+						 "expected ELOOP error");
+				}
 			} else {
-				tst_resm(TPASS, "open returned expected "
-					 "ELOOP error");
-			}
-			close(TC[i].fileHandle);
+				if (TEST_RETURN == -1) {
+					tst_resm(TFAIL, "open failed "
+						 "unexpectedly with errno %d",
+						 TEST_ERRNO);
+				} else {
+					tst_resm(TPASS, "open succeeded as "
+						 "expected");
+				}
+ 			}
+			if (TEST_RETURN != -1)
+				close(TEST_RETURN);
 		}
 	}
 	cleanup();

@@ -156,6 +172,8 @@ int main(int ac, char **av)
 
 void setupfunc_test1()
 {
+	char file1[100], file2[100];
+
 	sprintf(file1, "open03.1.%d", getpid());
 	sprintf(file2, "open03.2.%d", getpid());
 	if ((fd1 = creat(file1, 00700)) < 0) {

@@ -164,10 +182,13 @@ void setupfunc_test1()
 	if (symlink(file1, file2) < 0) {
 		tst_brkm(TBROK, cleanup, "symlink(2) failed: errno: %d", errno);
 	 /*NOTREACHED*/}
+	strcpy(TC[0].filename, file2);
 }
 
 void setupfunc_test2()
 {
+	char file1[100], file2[100];
+
 	sprintf(file1, "open03.3.%d", getpid());
 	sprintf(file2, "open03.4.%d", getpid());
 	if (mkdir(file1, 00700) < 0) {

@@ -176,10 +197,13 @@ void setupfunc_test2()
 	if (symlink(file1, file2) < 0) {
 		tst_brkm(TBROK, cleanup, "symlink(2) failed: errno: %d", errno);
 	 /*NOTREACHED*/}
+	strcpy(TC[1].filename, file2);
 }
 
 void setupfunc_test3()
 {
+	char file1[100], file2[100], file3[100];
+
 	sprintf(file1, "open03.5.%d", getpid());
 	sprintf(file2, "open03.6.%d", getpid());
 	sprintf(file3, "open03.7.%d", getpid());

@@ -192,10 +216,13 @@ void setupfunc_test3()
 	if (symlink(file2, file3) < 0) {
 		tst_brkm(TBROK, cleanup, "symlink(2) failed: errno: %d", errno);
 	 /*NOTREACHED*/}
+	strcpy(TC[2].filename, file3);
 }
 
 void setupfunc_test4()
 {
+	char file1[100], file2[100], file3[100];
+
 	sprintf(file1, "open03.8.%d", getpid());
 	sprintf(file2, "open03.9.%d", getpid());
 	sprintf(file3, "open03.10.%d", getpid());

@@ -208,6 +235,23 @@ void setupfunc_test4()
 	if (symlink(file2, file3) < 0) {
 		tst_brkm(TBROK, cleanup, "symlink(2) failed: errno: %d", errno);
 	 /*NOTREACHED*/}
+	strcpy(TC[3].filename, file3);
+}
+
+void setupfunc_test5()
+{
+	char file1[100], file2[100];
+
+	sprintf(file1, "open11.3.%d", getpid());
+	sprintf(file2, "open12.4.%d", getpid());
+	if (mkdir(file1, 00700) < 0) {
+		tst_brkm(TBROK, cleanup, "mkdir(2) failed: errno: %d", errno);
+	/*NOTREACHED*/}
+	if (symlink(file1, file2) < 0) {
+		tst_brkm(TBROK, cleanup, "symlink(2) failed: errno: %d", errno);
+	/*NOTREACHED*/}
+	strcpy(TC[4].filename, file2);
+	strcat(TC[4].filename, "/");
 }
 
 /*
Signature · 1 line
1.6.4.2
#10 lore raw
Quoted 11 lines · Jan Kara
Quoted 10 lines · Subrata Modak
Quoted 9 lines · Jan Kara

Hi,

Quoted 6 lines · Subrata Modak

It would be great if you can shoot it in the form of a patch, mentioning

1. The GPL Lincense,
2. The purpose of the test case,
3. Exact location it goes,
4. Which LTPROOT/runtest/<file> executes it

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

With a help of Cyril I've created the attached patch and verified that
the test fails on 2.6.33 but succeeds on 2.6.32...

Great. Thanks for the patch. But, it falters on the latest git. Can you please rebase and resend:

Ah, OK. Sorry for that. Now a patch rediffed against latest git tree of
LTP. Hopefully this time I got it right.

Thanks.

Regards--
Subrata

Quoted 2 lines

Honza

reviewer
#11 lore raw
In reply to Jan Kara
Quoted 21 lines · Jan Kara

From d53d3cc6488d9135bb69c3ff7e034b3b624866ed Mon Sep 17 00:00:00 2001
From: Jan Kara <[email protected]>
Date: Tue, 11 May 2010 16:34:25 +0200
Subject: [PATCH] vfs: Fix O_NOFOLLOW behavior for paths with trailing slashes

According to specification
mkdir d; ln -s d a; open("a/", O_NOFOLLOW | O_RDONLY)
should return success but currently it did return ELOOP. Fix the code to ignore O_NOFOLLOW in case the provided path has trailing slashes. This is a regression caused by path lookup cleanup patch series.

CC [email protected]
Signed-off-by Jan Kara <[email protected]>
---
 fs/namei.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
fs/namei.c +0 -0
index a7dce91..4cc7fbc 100644

...

fs/namei.c static struct file *do_last(struct nameidata *nd, struct path *path, · line 1727

Quoted 8 lines
fs/namei.c +1 -1
@@ -1722,7 +1722,7 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
 
 	if (__follow_mount(path)) {
 		error = -ELOOP;
-		if (open_flag & O_NOFOLLOW)
+		if (!(nd->flags & LOOKUP_FOLLOW))
 			goto exit_dput;
 	}

Sorry for replying to myself... This change is actually unrelated and as
I've checked the same behavior is in older kernels. So although the check currently does not quite make sense to me the fix should probably leave it alone. So below is a patch without the above hunk.
---

Quoted 1 line

From 2d4a9e9cca7c4f147aa29256ee25de593bb41540 Mon Sep 17 00:00:00 2001

From: Jan Kara <[email protected]>
Date: Tue, 11 May 2010 16:34:25 +0200
Subject: [PATCH] vfs: Fix O_NOFOLLOW behavior for paths with trailing slashes

According to specification
mkdir d; ln -s d a; open("a/", O_NOFOLLOW | O_RDONLY)
should return success but currently it did return ELOOP. Fix the code to ignore O_NOFOLLOW in case the provided path has trailing slashes. This is a regression caused by path lookup cleanup patch series.

CC [email protected]
Signed-off-by Jan Kara <[email protected]>
---
 fs/namei.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
fs/namei.c +4 -2
index a7dce91..16df727 100644
@@ -1641,7 +1641,7 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
 	if (nd->last.name[nd->last.len]) {
 		if (open_flag & O_CREAT)
 			goto exit;
-		nd->flags |= LOOKUP_DIRECTORY;
+		nd->flags |= LOOKUP_DIRECTORY | LOOKUP_FOLLOW;
 	}
 
 	/* just plain open? */

@@ -1830,6 +1830,8 @@ reval:
 	}
 	if (open_flag & O_DIRECTORY)
 		nd.flags |= LOOKUP_DIRECTORY;
+	if (!(open_flag & O_NOFOLLOW))
+		nd.flags |= LOOKUP_FOLLOW;
 	filp = do_last(&nd, &path, open_flag, acc_mode, mode, pathname);
 	while (unlikely(!filp)) { /* trailing symlink */
 		struct path holder;

@@ -1837,7 +1839,7 @@ reval:
 		void *cookie;
 		error = -ELOOP;
 		/* S_ISDIR part is a temporary automount kludge */
-		if ((open_flag & O_NOFOLLOW) && !S_ISDIR(inode->i_mode))
+		if (!(nd.flags & LOOKUP_FOLLOW) && !S_ISDIR(inode->i_mode))
 			goto exit_dput;
 		if (count++ == 32)
 			goto exit_dput;
Signature · 1 line
1.6.4.2
Acked-by
#12 lore raw
Quoted 5 lines · Jan Kara
Quoted 1 line

From 2d4a9e9cca7c4f147aa29256ee25de593bb41540 Mon Sep 17 00:00:00 2001

From: Jan Kara <[email protected]>
Date: Tue, 11 May 2010 16:34:25 +0200
Subject: [PATCH] vfs: Fix O_NOFOLLOW behavior for paths with trailing slashes

Quoted 8 lines

According to specification
mkdir d; ln -s d a; open("a/", O_NOFOLLOW | O_RDONLY)
should return success but currently it did return ELOOP. Fix the code to ignore O_NOFOLLOW in case the provided path has trailing slashes. This is a regression caused by path lookup cleanup patch series.

CC [email protected]
Signed-off-by Jan Kara <[email protected]>
Acked-by Miklos Szeredi <[email protected]>
Quoted 43 lines
---
 fs/namei.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
fs/namei.c +4 -2
index a7dce91..16df727 100644
@@ -1641,7 +1641,7 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
 	if (nd->last.name[nd->last.len]) {
 		if (open_flag & O_CREAT)
 			goto exit;
-		nd->flags |= LOOKUP_DIRECTORY;
+		nd->flags |= LOOKUP_DIRECTORY | LOOKUP_FOLLOW;
 	}
 
 	/* just plain open? */

@@ -1830,6 +1830,8 @@ reval:
 	}
 	if (open_flag & O_DIRECTORY)
 		nd.flags |= LOOKUP_DIRECTORY;
+	if (!(open_flag & O_NOFOLLOW))
+		nd.flags |= LOOKUP_FOLLOW;
 	filp = do_last(&nd, &path, open_flag, acc_mode, mode, pathname);
 	while (unlikely(!filp)) { /* trailing symlink */
 		struct path holder;

@@ -1837,7 +1839,7 @@ reval:
 		void *cookie;
 		error = -ELOOP;
 		/* S_ISDIR part is a temporary automount kludge */
-		if ((open_flag & O_NOFOLLOW) && !S_ISDIR(inode->i_mode))
+		if (!(nd.flags & LOOKUP_FOLLOW) && !S_ISDIR(inode->i_mode))
 			goto exit_dput;
 		if (count++ == 32)
 			goto exit_dput;
Signature · 6 lines
1.6.4.2

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html