Re: [PATCH] ext4: take i_mutex in ext4_symlink to eliminate a warning from ext4_truncate

Re: [PATCH] ext4: take i_mutex in ext4_symlink to eliminate a warning from ext4_truncate

[Theodore Ts'o]

I looked more closely at the assumption that ext4_write_begin() holds
i_mutex.  This is guaranteed by Documentation/filesystems/Locking,
which notes that write_begin() and write_end() functions hold i_mutex:

			PageLocked(page)	i_mutex
write_begin:		locks the page		yes
write_end:		yes, unlocks		yes

So the bug is that ext4_symlink() calls __page_symlink();
__page_symlink() calls pagecache_write_begin() which calls
write_begin(), without taking i_mutex.

So we can fix this by taking i_mutex in ext4_symlink(), but I think it
would be better to take the i_mutex in __page_symlink(), since it
would then address a violation of the locking rules for all file
systems.

Al, do you agree?

					- Ted

[PATCH] fs: take i_mutex in __page_symlink()

[Theodore Ts'o]

In Documentation/filesystems/Locking, it's documented that
write_begin() is guaranteed to be called with i_mutex locked.  The
function __page_symlink() was not taking i_mutex before calling
pagecache_write_begin(), which will eventually result in the file
system's write_begin()'s function getting called.

Other callers of pagecache_write_begin such as in fs/splice.c, call
pagecache_write_begin() with i_mutex locked, so fix __page_symlink()
to be consistent.

This was discovered by the addition of a new ext4 debugging assertion
which checked to make sure i_mutex was locked before calling
ext4_truncate().

Reported-by: Zheng Liu <gnehzuil.liu@gmail.com>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Cc: linux-fsdevel@vger.kernel.org
Cc: Al Viro <viro@ZenIV.linux.org.uk>
---

Note: I plan to carry the following patch in the ext4 tree, unless
someone objects or Al insists on carrying this in the vfs git tree.

 fs/namei.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/namei.c b/fs/namei.c
index 57ae9c8..548e57b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4035,8 +4035,10 @@ int __page_symlink(struct inode *inode, const char *symname, int len, int nofs)
 		flags |= AOP_FLAG_NOFS;
 
 retry:
+	mutex_lock(&inode->i_mutex);
 	err = pagecache_write_begin(NULL, mapping, 0, len-1,
 				flags, &page, &fsdata);
+	mutex_unlock(&inode->i_mutex);
 	if (err)
 		goto fail;
 
-- 
1.7.12.rc0.22.gcdd159b

Re: [PATCH] fs: take i_mutex in __page_symlink()

[Al Viro]

On Mon, Apr 01, 2013 at 11:23:42AM -0400, Theodore Ts'o wrote:
> In Documentation/filesystems/Locking, it's documented that
> write_begin() is guaranteed to be called with i_mutex locked.  The
> function __page_symlink() was not taking i_mutex before calling
> pagecache_write_begin(), which will eventually result in the file
> system's write_begin()'s function getting called.
> 
> Other callers of pagecache_write_begin such as in fs/splice.c, call
> pagecache_write_begin() with i_mutex locked, so fix __page_symlink()
> to be consistent.
> 
> This was discovered by the addition of a new ext4 debugging assertion
> which checked to make sure i_mutex was locked before calling
> ext4_truncate().

	I doubt that it's worth doing (inode has just been created and
nobody else should have references to it - it's not fully set up, after
all)...

Re: [PATCH] fs: take i_mutex in __page_symlink()

[Theodore Ts'o]

On Mon, Apr 01, 2013 at 05:35:38PM +0100, Al Viro wrote:
> > This was discovered by the addition of a new ext4 debugging assertion
> > which checked to make sure i_mutex was locked before calling
> > ext4_truncate().
> 
> 	I doubt that it's worth doing (inode has just been created and
> nobody else should have references to it - it's not fully set up, after
> all)...

Well, my other option is to drop the assert in ext4_truncate(), which
I thought was a good thing from a perspective of defensive
programming, or to grab the mutex in ext4_symlink() which is what
calles __page_symlink().

Would you prefer that we take the mutex in ext4_symlink() instead?

      	  	      	      	  	- Ted

Re: [PATCH] fs: take i_mutex in __page_symlink()

[Dmitry Monakhov]

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

On Mon,  1 Apr 2013 11:23:42 -0400, Theodore Ts'o <tytso@mit.edu> wrote:
> In Documentation/filesystems/Locking, it's documented that
> write_begin() is guaranteed to be called with i_mutex locked.  The
> function __page_symlink() was not taking i_mutex before calling
> pagecache_write_begin(), which will eventually result in the file
> system's write_begin()'s function getting called.
> 
> Other callers of pagecache_write_begin such as in fs/splice.c, call
> pagecache_write_begin() with i_mutex locked, so fix __page_symlink()
> to be consistent.
> 
> This was discovered by the addition of a new ext4 debugging assertion
> which checked to make sure i_mutex was locked before calling
> ext4_truncate().
> 
> Reported-by: Zheng Liu <gnehzuil.liu@gmail.com>
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> Cc: linux-fsdevel@vger.kernel.org
> Cc: Al Viro <viro@ZenIV.linux.org.uk>
> ---
> 
> Note: I plan to carry the following patch in the ext4 tree, unless
> someone objects or Al insists on carrying this in the vfs git tree.
> 
>  fs/namei.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 57ae9c8..548e57b 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4035,8 +4035,10 @@ int __page_symlink(struct inode *inode, const char *symname, int len, int nofs)
>  		flags |= AOP_FLAG_NOFS;
>  
>  retry:
> +	mutex_lock(&inode->i_mutex);
>  	err = pagecache_write_begin(NULL, mapping, 0, len-1,
>  				flags, &page, &fsdata);
> +	mutex_unlock(&inode->i_mutex);
Noo. Please do no do that. i_mutex should being hold from write_begin() to
write_end() because:
1) both functions contains one logical block (critical section)
2) write_end() may update i_size
3) write_end() may call truncate

And since we know that we want to lock i_mutex here only for
convention purposes (no one can access this inode yet) let's do that
correct. Original Zheng's patch was much better.
I have following patch in my queue:

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-patch-ext4_symlink.patch.patch --]
[-- Type: text/x-patch, Size: 887 bytes --]

>From c147d9ae5f9511f722a97179cd9f661e7e10417e Mon Sep 17 00:00:00 2001
From: Dmitry Monakhov <dmonakhov@openvz.org>
Date: Sun, 31 Mar 2013 17:35:38 +0400
Subject: [PATCH] patch ext4_symlink.patch


Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/namei.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 57ae9c8..9dcdb27 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4034,6 +4034,7 @@ int __page_symlink(struct inode *inode, const char *symname, int len, int nofs)
 	if (nofs)
 		flags |= AOP_FLAG_NOFS;
 
+	mutex_lock(&inode->i_mutex);
 retry:
 	err = pagecache_write_begin(NULL, mapping, 0, len-1,
 				flags, &page, &fsdata);
@@ -4052,8 +4053,10 @@ retry:
 		goto retry;
 
 	mark_inode_dirty(inode);
+	mutex_unlock(&inode->i_mutex);
 	return 0;
 fail:
+	mutex_unlock(&inode->i_mutex);
 	return err;
 }
 
-- 
1.7.1


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


>  	if (err)
>  		goto fail;
>  
> -- 
> 1.7.12.rc0.22.gcdd159b
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html