Re: [Bugme-new] [Bug 15909] New: open("a/", O_NOFOLLOW) fails with ELOOP if "a" is a symbolic link to a directory.

Re: [Bugme-new] [Bug 15909] New: open("a/", O_NOFOLLOW) fails with ELOOP if "a" is a symbolic link to a directory.

[Andrew Morton]

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

On Wed, 5 May 2010 13:01:22 GMT
bugzilla-daemon@bugzilla.kernel.org 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: fs_other@kernel-bugs.osdl.org
>         ReportedBy: tolzmann@molgen.mpg.de
>         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
> 
> 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?

Re: [Bugme-new] [Bug 15909] New: open("a/", O_NOFOLLOW) fails with ELOOP if "a" is a symbolic link to a directory.

[OGAWA Hirofumi]

Andrew Morton <akpm@linux-foundation.org> writes:

>> 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
>> 
>> 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.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [Bugme-new] [Bug 15909] New: open("a/", O_NOFOLLOW) fails with ELOOP if "a" is a symbolic link to a directory.

[Jan Kara]

On Thu 06-05-10 14:30:02, Andrew Morton wrote:
> 
> (switched to email.  Please respond via emailed reply-to-all, not via the
> bugzilla web interface).
> 
> On Wed, 5 May 2010 13:01:22 GMT
> bugzilla-daemon@bugzilla.kernel.org 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: fs_other@kernel-bugs.osdl.org
> >         ReportedBy: tolzmann@molgen.mpg.de
> >         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
> > 
> > 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 <jack@suse.cz>
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: stable@kernel.org
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/namei.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index a7dce91..4cc7fbc 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -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;
-- 
1.6.4.2

Re: [Bugme-new] [Bug 15909] New: open("a/", O_NOFOLLOW) fails with ELOOP if "a" is a symbolic link to a directory.

[Jan Kara]

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

On Tue 11-05-10 17:48:51, Jan Kara wrote:
> On Thu 06-05-10 14:30:02, Andrew Morton wrote:
> > 
> > (switched to email.  Please respond via emailed reply-to-all, not via the
> > bugzilla web interface).
> > 
> > On Wed, 5 May 2010 13:01:22 GMT
> > bugzilla-daemon-590EEB7GvNiWaY/ihj7yzEB+6BGkLq7r@public.gmane.org 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: fs_other-ztI5WcYan/vQLgFONoPN62D2FQJk+8+b@public.gmane.org
> > >         ReportedBy: tolzmann-KUpvgZVWgV9o1qOY/usvUg@public.gmane.org
> > >         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
> > > 
> > > 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 <jack-AlSwsSmVLrQ@public.gmane.org>
> 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: stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
> Signed-off-by: Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org>
...
  BTW: It might be worthwhile to add the attached testcase to LTP?

									Honza
-- 
Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org>
SUSE Labs, CR

[-- Attachment #2: open-follow-test.c --]
[-- Type: text/x-c++src, Size: 991 bytes --]

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

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

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


[-- Attachment #4: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Ltp-list mailing list
Ltp-list-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [Bugme-new] [Bug 15909] New: open("a/", O_NOFOLLOW) fails with ELOOP if "a" is a symbolic link to a directory.

[Subrata Modak]

Hi Jan,

On Tue, 2010-05-11 at 18:24 +0200, Jan Kara wrote:
> On Tue 11-05-10 17:48:51, Jan Kara wrote:
> > On Thu 06-05-10 14:30:02, Andrew Morton wrote:
> > > 
> > > (switched to email.  Please respond via emailed reply-to-all, not
> via the
> > > bugzilla web interface).
> > > 
> > > On Wed, 5 May 2010 13:01:22 GMT
> > > bugzilla-daemon@bugzilla.kernel.org 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: fs_other@kernel-bugs.osdl.org
> > > >         ReportedBy: tolzmann@molgen.mpg.de
> > > >         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
> > > > 
> > > > 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 <jack@suse.cz>
> > 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: stable@kernel.org
> > Signed-off-by: Jan Kara <jack@suse.cz>
> ...
>   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

:-)

> 
>                                                                         Honza
> -- 
> Jan Kara <jack@suse.cz>
> 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;
> }

Re: [LTP] [Bugme-new] [Bug 15909] New: open("a/", O_NOFOLLOW) fails with ELOOP if "a" is a symbolic link to a directory.

[Jan Kara]

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

  Hi,

On Tue 11-05-10 22:05:52, Subrata Modak wrote:
> 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
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[-- Attachment #2: ltp-open-test.diff --]
[-- Type: text/x-patch, Size: 5333 bytes --]

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 <jack@suse.cz>

--- ltp/testcases/kernel/syscalls/open/open07.c.orig	2010-05-12 15:26:03.000000000 +0200
+++ ltp/testcases/kernel/syscalls/open/open07.c	2010-05-12 17:50:54.000000000 +0200
@@ -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, "/");
 }
 
 /*

Re: [LTP] [Bugme-new] [Bug 15909] New: open("a/", O_NOFOLLOW) fails with ELOOP if "a" is a symbolic link to a directory.

[Subrata Modak]

On Wed, 2010-05-12 at 17:59 +0200, Jan Kara wrote:
> Hi,
> 
> On Tue 11-05-10 22:05:52, Subrata Modak wrote:
> > 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

> 
> 								Honza

Re: [LTP] [Bugme-new] [Bug 15909] New: open("a/", O_NOFOLLOW) fails with ELOOP if "a" is a symbolic link to a directory.

[Jan Kara]

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

On Wed 12-05-10 22:16:03, Subrata Modak wrote:
> On Wed, 2010-05-12 at 17:59 +0200, Jan Kara wrote:
> > Hi,
> > 
> > On Tue 11-05-10 22:05:52, Subrata Modak wrote:
> > > 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:
  Ah, OK. Sorry for that. Now a patch rediffed against latest git tree of
LTP. Hopefully this time I got it right.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[-- Attachment #2: ltp-open-test.diff --]
[-- Type: text/x-patch, Size: 5960 bytes --]

>From 02340e1c55f2b446a1706ee087559b6e70480990 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
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 <jack@suse.cz>
---
 testcases/kernel/syscalls/open/open07.c |   98 ++++++++++++++++++++++---------
 1 files changed, 71 insertions(+), 27 deletions(-)

diff --git a/testcases/kernel/syscalls/open/open07.c b/testcases/kernel/syscalls/open/open07.c
index 9d2fc04..2322e85 100644
--- a/testcases/kernel/syscalls/open/open07.c
+++ b/testcases/kernel/syscalls/open/open07.c
@@ -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, "/");
 }
 
 /*
-- 
1.6.4.2

Re: [LTP] [Bugme-new] [Bug 15909] New: open("a/", O_NOFOLLOW) fails with ELOOP if "a" is a symbolic link to a directory.

[Subrata Modak]

On Thu, 2010-05-13 at 13:29 +0200, Jan Kara wrote: 
> On Wed 12-05-10 22:16:03, Subrata Modak wrote:
> > On Wed, 2010-05-12 at 17:59 +0200, Jan Kara wrote:
> > > Hi,
> > > 
> > > On Tue 11-05-10 22:05:52, Subrata Modak wrote:
> > > > 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:
>   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

> 
> 								Honza

Re: [Bugme-new] [Bug 15909] New: open("a/", O_NOFOLLOW) fails with ELOOP if "a" is a symbolic link to a directory.

[Jan Kara]

On Tue 11-05-10 17:48:51, Jan Kara wrote:
> From d53d3cc6488d9135bb69c3ff7e034b3b624866ed Mon Sep 17 00:00:00 2001
> From: Jan Kara <jack@suse.cz>
> 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: stable@kernel.org
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/namei.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index a7dce91..4cc7fbc 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
...
> @@ -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.
---

>From 2d4a9e9cca7c4f147aa29256ee25de593bb41540 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
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: stable@kernel.org
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/namei.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index a7dce91..16df727 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -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;
-- 
1.6.4.2

Re: [Bugme-new] [Bug 15909] New: open("a/", O_NOFOLLOW) fails with ELOOP if "a" is a symbolic link to a directory.

[Miklos Szeredi]

On Tue, 11 May 2010, Jan Kara wrote:
> >From 2d4a9e9cca7c4f147aa29256ee25de593bb41540 Mon Sep 17 00:00:00 2001
> From: Jan Kara <jack@suse.cz>
> 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: stable@kernel.org
> Signed-off-by: Jan Kara <jack@suse.cz>

Acked-by: Miklos Szeredi <mszeredi@suse.cz>

> ---
>  fs/namei.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index a7dce91..16df727 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -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;
> -- 
> 1.6.4.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>