vfs: Fix O_NOFOLLOW behavior for paths with trailing slashes
diff mbox series

Message ID 1273747977-4579-1-git-send-email-jack@suse.cz
State New, archived
Headers show
Series
  • vfs: Fix O_NOFOLLOW behavior for paths with trailing slashes
Related show

Commit Message

Jan Kara May 13, 2010, 10:52 a.m. UTC
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
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Al Viro <viro@zeniv.linux.org.uk>
Reported-by: tolzmann@molgen.mpg.de
Acked-by: Miklos Szeredi <mszeredi@suse.cz>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 Since Al doesn't seem to react for a few days and this is kind of nasty bug,
I'm sending it directly...

 fs/namei.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

Comments

Andrew Morton May 13, 2010, 12:43 p.m. UTC | #1
On Thu, 13 May 2010 17:41:35 +0200 Jan Kara <jack@suse.cz> wrote:

> On Thu 13-05-10 08:24:59, Linus Torvalds wrote:
> > 
> > 
> > On Thu, 13 May 2010, Jan Kara wrote:
> > >
> > > 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
> > 
> > Hmm? Is this correct? Isn't the bug introduced in this merge window, and 
> > thus not relevant for stable?
>   Ah, you're right! I've seen dates in the patches around December so I
> automatically thought the series went to 2.6.33 but checking git logs and
> the actual source code of 2.6.33 it went in later. I'm sorry for the
> confusion.

Yes, it's a bit tricky (for me, at least) to work out "which kernel version did
that patch go into" via git.

I just keep lots of kernel trees around and poke about with `patch
--dry-run'.  PITA.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Linus Torvalds May 13, 2010, 3:24 p.m. UTC | #2
On Thu, 13 May 2010, Jan Kara wrote:
>
> 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

Hmm? Is this correct? Isn't the bug introduced in this merge window, and 
thus not relevant for stable?

		Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Jan Kara May 13, 2010, 3:41 p.m. UTC | #3
On Thu 13-05-10 08:24:59, Linus Torvalds wrote:
> 
> 
> On Thu, 13 May 2010, Jan Kara wrote:
> >
> > 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
> 
> Hmm? Is this correct? Isn't the bug introduced in this merge window, and 
> thus not relevant for stable?
  Ah, you're right! I've seen dates in the patches around December so I
automatically thought the series went to 2.6.33 but checking git logs and
the actual source code of 2.6.33 it went in later. I'm sorry for the
confusion.

									Honza
Christoph Hellwig May 13, 2010, 3:47 p.m. UTC | #4
On Thu, May 13, 2010 at 08:43:35AM -0400, Andrew Morton wrote:
> Yes, it's a bit tricky (for me, at least) to work out "which kernel version did
> that patch go into" via git.

git-describe <revid>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Jan Kara May 13, 2010, 3:51 p.m. UTC | #5
On Thu 13-05-10 08:43:35, Andrew Morton wrote:
> On Thu, 13 May 2010 17:41:35 +0200 Jan Kara <jack@suse.cz> wrote:
> 
> > On Thu 13-05-10 08:24:59, Linus Torvalds wrote:
> > > 
> > > 
> > > On Thu, 13 May 2010, Jan Kara wrote:
> > > >
> > > > 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
> > > 
> > > Hmm? Is this correct? Isn't the bug introduced in this merge window, and 
> > > thus not relevant for stable?
> >   Ah, you're right! I've seen dates in the patches around December so I
> > automatically thought the series went to 2.6.33 but checking git logs and
> > the actual source code of 2.6.33 it went in later. I'm sorry for the
> > confusion.
> 
> Yes, it's a bit tricky (for me, at least) to work out "which kernel version did
> that patch go into" via git.
  Well, if you know the commit id, "git describe --contains <commit-id>"
tells what you need. But sometimes I'm too lazy to use "git describe" and
sometimes I forget "--contains" which then returns the kernel version on
which the patch was based - not quite what I'm interested in...

									Honza
James Bottomley May 13, 2010, 3:53 p.m. UTC | #6
On Thu, 2010-05-13 at 11:47 -0400, Christoph Hellwig wrote:
> On Thu, May 13, 2010 at 08:43:35AM -0400, Andrew Morton wrote:
> > Yes, it's a bit tricky (for me, at least) to work out "which kernel version did
> > that patch go into" via git.
> 
> git-describe <revid>

Actually, that will print the version it was applied to.  To find out
the version it was pulled into, you need git-describe --contains <revid>

James


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Linus Torvalds May 13, 2010, 3:58 p.m. UTC | #7
On Thu, 13 May 2010, Andrew Morton wrote:
>
> >   Ah, you're right! I've seen dates in the patches around December so I
> > automatically thought the series went to 2.6.33 but checking git logs and
> > the actual source code of 2.6.33 it went in later. I'm sorry for the
> > confusion.
> 
> Yes, it's a bit tricky (for me, at least) to work out "which kernel version did
> that patch go into" via git.
> 
> I just keep lots of kernel trees around and poke about with `patch
> --dry-run'.  PITA.

What I did to double-check was:

	git log fs/namei.c

to find the commit series by Al (obviously, you can do it other ways too, 
but that was the easy way). Then, when you find the commit  just do

	git name-rev 1f36f774b22a0ceb7dd33eca626746c81a97b6a5

which gives us

	1f36f774b22a0ceb7dd33eca626746c81a97b6a5 tags/v2.6.34-rc1~195^2

ie that commit is reachable from v2.6.34-rc1, not from any stable kernel.

(Or alternatively, use "git describe", and get "v2.6.33-5088-g1f36f77", 
which means that it is v2.6.33 plus 5088 commits).

And as usual, there are other ways. One particularly obscure one is to say

	git log --tags --source --author=viro fs/namei.c

which basically says "show only commits by viro in fs/namei.c, start from 
all tags, and for each commit, show _which_ tag the commit was reached 
from". It's not perfect, but it does it in one go. With "--oneline", you'd 
have gotten a listing like

	3e297b6 v2.6.34-rc3 Restore LOOKUP_DIRECTORY hint handling in final lookup on open()
	781b167 v2.6.34-rc2 Fix a dumb typo - use of & instead of &&
	1f36f77 v2.6.34-rc2 Switch !O_CREAT case to use of do_last()
	def4af3 v2.6.34-rc2 Get rid of symlink body copying
	3866248 v2.6.34-rc2 Finish pulling of -ESTALE handling to upper level in do_filp_open()
	806b681 v2.6.34-rc2 Turn do_link spaghetty into a normal loop
	10fa8e6 v2.6.34-rc2 Unify exits in O_CREAT handling
	9e67f36 v2.6.34-rc2 Kill is_link argument of do_last()
	...

so you see into which -rc the different patches from Al went.

			Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Al Viro May 14, 2010, 5:12 p.m. UTC | #8
On Thu, May 13, 2010 at 12:52:57PM +0200, Jan Kara wrote:
>  Since Al doesn't seem to react for a few days and this is kind of nasty bug,
> I'm sending it directly...

Belated ACK.  Should've noticed it when LOOKUP_DIRECTORY crap had shown up
weeks ago ;-/  Sorry.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch
diff mbox series

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;