vfs-scale, nd->inode after __do_follow_link()

vfs-scale, nd->inode after __do_follow_link()

[J. R. Okajima]

Isn't it path.dentry->d_inode instead of nd.inode?

J. R. Okajima

diff --git a/fs/namei.c b/fs/namei.c
index 5bb7588..1df3bee 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2356,8 +2374,9 @@ reval:
 			goto exit_dput;
 		error = __do_follow_link(&path, &nd, &cookie);
 		if (unlikely(error)) {
-			if (!IS_ERR(cookie) && nd.inode->i_op->put_link)
-				nd.inode->i_op->put_link(path.dentry, &nd, cookie);
+			struct dentry *i = path.dentry->d_inode;
+			if (!IS_ERR(cookie) && i->i_op->put_link)
+				i->i_op->put_link(path.dentry, &nd, cookie);
 			/* nd.path had been dropped */
 			nd.path = path;
 			goto out_path;

Re: vfs-scale, nd->inode after __do_follow_link()

[J. R. Okajima]

"J. R. Okajima":
> Isn't it path.dentry->d_inode instead of nd.inode?

There is another one just after.

J. R. Okajima

diff --git a/fs/namei.c b/fs/namei.c
index 5bb7588..03e45ae 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2356,8 +2374,9 @@ reval:
 			goto exit_dput;
 		error = __do_follow_link(&path, &nd, &cookie);
 		if (unlikely(error)) {
-			if (!IS_ERR(cookie) && nd.inode->i_op->put_link)
-				nd.inode->i_op->put_link(path.dentry, &nd, cookie);
+			struct inode *i = path.dentry->d_inode;
+			if (!IS_ERR(cookie) && i->i_op->put_link)
+				i->i_op->put_link(path.dentry, &nd, cookie);
 			/* nd.path had been dropped */
 			nd.path = path;
 			goto out_path;
@@ -2365,8 +2384,11 @@ reval:
 		holder = path;
 		nd.flags &= ~LOOKUP_PARENT;
 		filp = do_last(&nd, &path, open_flag, acc_mode, mode, pathname);
-		if (nd.inode->i_op->put_link)
-			nd.inode->i_op->put_link(holder.dentry, &nd, cookie);
+		{
+			struct inode *i = holder.dentry->d_inode;
+			if (i->i_op->put_link)
+				i->i_op->put_link(holder.dentry, &nd, cookie);
+		}
 		path_put(&holder);
 	}
 out:

Re: vfs-scale, nd->inode after __do_follow_link()

[Nick Piggin]

On Fri, Jan 14, 2011 at 1:10 PM, J. R. Okajima <hooanon05@yahoo.co.jp> wrote:
>
> Isn't it path.dentry->d_inode instead of nd.inode?
>
> J. R. Okajima
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 5bb7588..1df3bee 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2356,8 +2374,9 @@ reval:
>                        goto exit_dput;
>                error = __do_follow_link(&path, &nd, &cookie);
>                if (unlikely(error)) {
> -                       if (!IS_ERR(cookie) && nd.inode->i_op->put_link)
> -                               nd.inode->i_op->put_link(path.dentry, &nd, cookie);
> +                       struct dentry *i = path.dentry->d_inode;
> +                       if (!IS_ERR(cookie) && i->i_op->put_link)
> +                               i->i_op->put_link(path.dentry, &nd, cookie);
>                        /* nd.path had been dropped */
>                        nd.path = path;
>                        goto out_path;

It should be the inode we followed, rather than the inode of the
new path, I think.
--
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

Re: vfs-scale, nd->inode after __do_follow_link()

[J. R. Okajima]

Nick Piggin:
> > Isn't it path.dentry->d_inode instead of nd.inode?
	:::
> It should be the inode we followed, rather than the inode of the
> new path, I think.

In __do_follow_link(), path.dentry is what we followed, isn't it?

__do_follow_link()
{
	struct dentry *dentry = path->dentry;
	;;;
	*p = dentry->d_inode->i_op->follow_link(dentry, nd);
	;;;


J. R. Okajima

Re: vfs-scale, nd->inode after __do_follow_link()

[Al Viro]

On Fri, Jan 14, 2011 at 03:09:10PM +1100, Nick Piggin wrote:

> > + ? ? ? ? ? ? ? ? ? ? ? struct dentry *i = path.dentry->d_inode;
> > + ? ? ? ? ? ? ? ? ? ? ? if (!IS_ERR(cookie) && i->i_op->put_link)
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? i->i_op->put_link(path.dentry, &nd, cookie);
> > ? ? ? ? ? ? ? ? ? ? ? ?/* nd.path had been dropped */
> > ? ? ? ? ? ? ? ? ? ? ? ?nd.path = path;
> > ? ? ? ? ? ? ? ? ? ? ? ?goto out_path;
> 
> It should be the inode we followed, rather than the inode of the
> new path, I think.

And that's what the first argument of __do_follow_link() is.  I'm actually
tempted to rename it from path to symlink and make it const to clarify
the things a bit.

BTW, "i" as a name for local struct inode * is -><- that close to being a
shootable offense.  Please, rename to e.g. struct inode *link (and it's
struct inode, not struct dentry).

Re: vfs-scale, nd->inode after __do_follow_link()

[J. R. Okajima]

Al Viro:
> BTW, "i" as a name for local struct inode * is -><- that close to being a
> shootable offense.  Please, rename to e.g. struct inode *link (and it's
> struct inode, not struct dentry).

I will do so when the patch will be merged.
I meant the code snippet is just showing what I thought. I didn't mean
it should be merged.


J. R. Okajima

Re: vfs-scale, nd->inode after __do_follow_link()

[J. R. Okajima]

"J. R. Okajima":
> Al Viro:
> > BTW, "i" as a name for local struct inode * is -><- that close to being a
> > shootable offense.  Please, rename to e.g. struct inode *link (and it's
> > struct inode, not struct dentry).
>
> I will do so when the patch will be merged.
> I meant the code snippet is just showing what I thought. I didn't mean
> it should be merged.

Forgot to write.
I noticed about the "struct dentry" too and it was fixed in my second code.

J. R. Okajima

Re: vfs-scale, nd->inode after __do_follow_link()

[Nick Piggin]

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

On Fri, Jan 14, 2011 at 4:28 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, Jan 14, 2011 at 03:09:10PM +1100, Nick Piggin wrote:
>
>> > + ? ? ? ? ? ? ? ? ? ? ? struct dentry *i = path.dentry->d_inode;
>> > + ? ? ? ? ? ? ? ? ? ? ? if (!IS_ERR(cookie) && i->i_op->put_link)
>> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? i->i_op->put_link(path.dentry, &nd, cookie);
>> > ? ? ? ? ? ? ? ? ? ? ? ?/* nd.path had been dropped */
>> > ? ? ? ? ? ? ? ? ? ? ? ?nd.path = path;
>> > ? ? ? ? ? ? ? ? ? ? ? ?goto out_path;
>>
>> It should be the inode we followed, rather than the inode of the
>> new path, I think.
>
> And that's what the first argument of __do_follow_link() is.  I'm actually
> tempted to rename it from path to symlink and make it const to clarify
> the things a bit.

Yes I was completely wrong there, thanks again for another good
catch. I'll merge this in the vfs-scale branch, and ask to merge if
there are no objections.

[-- Attachment #2: fs-namei-putlink-fix.patch --]
[-- Type: application/octet-stream, Size: 3262 bytes --]

fs: namei fix ->put_link on wrong inode in do_filp_open

J. R. Okajima noticed that ->put_link is being attempted on the
wrong inode, and suggested the way to fix it. I changed it a bit
according to Al's suggestion to keep an explicit link path around.

Signed-off-by: Nick Piggin <npiggin@kernel.dk>

Index: linux-2.6/fs/namei.c
===================================================================
--- linux-2.6.orig/fs/namei.c	2011-01-14 18:26:02.000000000 +1100
+++ linux-2.6/fs/namei.c	2011-01-14 19:10:02.000000000 +1100
@@ -779,7 +779,8 @@ static void path_put_conditional(struct
 		mntput(path->mnt);
 }
 
-static inline void path_to_nameidata(struct path *path, struct nameidata *nd)
+static inline void path_to_nameidata(const struct path *path,
+					struct nameidata *nd)
 {
 	if (!(nd->flags & LOOKUP_RCU)) {
 		dput(nd->path.dentry);
@@ -791,20 +792,20 @@ static inline void path_to_nameidata(str
 }
 
 static __always_inline int
-__do_follow_link(struct path *path, struct nameidata *nd, void **p)
+__do_follow_link(const struct path *link, struct nameidata *nd, void **p)
 {
 	int error;
-	struct dentry *dentry = path->dentry;
+	struct dentry *dentry = link->dentry;
 
-	touch_atime(path->mnt, dentry);
+	touch_atime(link->mnt, dentry);
 	nd_set_link(nd, NULL);
 
-	if (path->mnt != nd->path.mnt) {
-		path_to_nameidata(path, nd);
+	if (link->mnt != nd->path.mnt) {
+		path_to_nameidata(link, nd);
 		nd->inode = nd->path.dentry->d_inode;
 		dget(dentry);
 	}
-	mntget(path->mnt);
+	mntget(link->mnt);
 
 	nd->last_type = LAST_BIND;
 	*p = dentry->d_inode->i_op->follow_link(dentry, nd);
@@ -2347,11 +2348,12 @@ struct file *do_filp_open(int dfd, const
 	nd.flags = flags;
 	filp = do_last(&nd, &path, open_flag, acc_mode, mode, pathname);
 	while (unlikely(!filp)) { /* trailing symlink */
-		struct path holder;
+		struct path link = path;
+		struct inode *linki = link.dentry->d_inode;
 		void *cookie;
 		error = -ELOOP;
 		/* S_ISDIR part is a temporary automount kludge */
-		if (!(nd.flags & LOOKUP_FOLLOW) && !S_ISDIR(nd.inode->i_mode))
+		if (!(nd.flags & LOOKUP_FOLLOW) && !S_ISDIR(linki->i_mode))
 			goto exit_dput;
 		if (count++ == 32)
 			goto exit_dput;
@@ -2367,23 +2369,22 @@ struct file *do_filp_open(int dfd, const
 		 * just set LAST_BIND.
 		 */
 		nd.flags |= LOOKUP_PARENT;
-		error = security_inode_follow_link(path.dentry, &nd);
+		error = security_inode_follow_link(link.dentry, &nd);
 		if (error)
 			goto exit_dput;
-		error = __do_follow_link(&path, &nd, &cookie);
+		error = __do_follow_link(&link, &nd, &cookie);
 		if (unlikely(error)) {
-			if (!IS_ERR(cookie) && nd.inode->i_op->put_link)
-				nd.inode->i_op->put_link(path.dentry, &nd, cookie);
+			if (!IS_ERR(cookie) && linki->i_op->put_link)
+				linki->i_op->put_link(link.dentry, &nd, cookie);
 			/* nd.path had been dropped */
-			nd.path = path;
+			nd.path = link;
 			goto out_path;
 		}
-		holder = path;
 		nd.flags &= ~LOOKUP_PARENT;
 		filp = do_last(&nd, &path, open_flag, acc_mode, mode, pathname);
-		if (nd.inode->i_op->put_link)
-			nd.inode->i_op->put_link(holder.dentry, &nd, cookie);
-		path_put(&holder);
+		if (linki->i_op->put_link)
+			linki->i_op->put_link(link.dentry, &nd, cookie);
+		path_put(&link);
 	}
 out:
 	if (nd.root.mnt)

Re: vfs-scale, nd->inode after __do_follow_link()

[Sedat Dilek]

On Fri, Jan 14, 2011 at 9:40 AM, Nick Piggin <npiggin@gmail.com> wrote:
> On Fri, Jan 14, 2011 at 4:28 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> On Fri, Jan 14, 2011 at 03:09:10PM +1100, Nick Piggin wrote:
>>
>>> > + ? ? ? ? ? ? ? ? ? ? ? struct dentry *i = path.dentry->d_inode;
>>> > + ? ? ? ? ? ? ? ? ? ? ? if (!IS_ERR(cookie) && i->i_op->put_link)
>>> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? i->i_op->put_link(path.dentry, &nd, cookie);
>>> > ? ? ? ? ? ? ? ? ? ? ? ?/* nd.path had been dropped */
>>> > ? ? ? ? ? ? ? ? ? ? ? ?nd.path = path;
>>> > ? ? ? ? ? ? ? ? ? ? ? ?goto out_path;
>>>
>>> It should be the inode we followed, rather than the inode of the
>>> new path, I think.
>>
>> And that's what the first argument of __do_follow_link() is.  I'm actually
>> tempted to rename it from path to symlink and make it const to clarify
>> the things a bit.
>
> Yes I was completely wrong there, thanks again for another good
> catch. I'll merge this in the vfs-scale branch, and ask to merge if
> there are no objections.
>

[ CCing patchwork ML ]

Can you send your patch separately, please?
Within a (long) thread it is eaten up.

As *.patch are somehow not presented as diff (see [1]), someone can't
catch them from <patchwork.k.o>.

Furthermore, it would be very cool to see linux-fsdevel (patches from
its ML) in <patchwork.d.o>.
I don't know whom to contact from the linux-fsdevel ML, sorry for this.

- Sedat -

[1] http://lkml.org/lkml/2011/1/14/55
--
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

Re: vfs-scale, nd->inode after __do_follow_link()

[J.H.]

> [ CCing patchwork ML ]
> 
> Can you send your patch separately, please?
> Within a (long) thread it is eaten up.
> 
> As *.patch are somehow not presented as diff (see [1]), someone can't
> catch them from <patchwork.k.o>.

On a side note, due to some conversations with the ocfs2 devs yesterday,
I've actually got the vfs-scale code sitting compiled up waiting for
another kernel panic to happen on the dynamic web boxes, since 2.6.37+
was, literally, unusable with ocfs2 running.

> Furthermore, it would be very cool to see linux-fsdevel (patches from
> its ML) in <patchwork.d.o>.
> I don't know whom to contact from the linux-fsdevel ML, sorry for this.

As to getting a mailing list added into patchwork.kernel.org I just need
a request from someone who is responsible for the mailing list, and who
will actually use the resulting entry in patchwork to e-mail
ftpadmin@kernel.org with the mailing list, where I can deal with the
sign-up and the name / username of the individual (in patchwork) to add
as being in charge.

- John 'Warthog9' Hawley

Re: vfs-scale, nd->inode after __do_follow_link()

[J. R. Okajima]

Nick Piggin:
> Yes I was completely wrong there, thanks again for another good
> catch. I'll merge this in the vfs-scale branch, and ask to merge if
> there are no objections.

I don't have any objection.

Thanks
J. R. Okajima