linux-ext4 - Re: [PATCH] fs: take i_mutex in __page

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Tue, 02 Apr 2013 12:19:42 +0400
From:	Dmitry Monakhov <dmonakhov@...nvz.org>
To:	Theodore Ts'o <tytso@....edu>,
	Ext4 Developers List <linux-ext4@...r.kernel.org>
Cc:	Theodore Ts'o <tytso@....edu>, linux-fsdevel@...r.kernel.org,
	Al Viro <viro@...IV.linux.org.uk>
Subject: Re: [PATCH] fs: take i_mutex in __page_symlink()

On Mon,  1 Apr 2013 11:23:42 -0400, Theodore Ts'o <tytso@....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@...il.com>
> Signed-off-by: "Theodore Ts'o" <tytso@....edu>
> Cc: linux-fsdevel@...r.kernel.org
> Cc: Al Viro <viro@...IV.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:

View attachment "0001-patch-ext4_symlink.patch.patch" of type "text/x-patch" (887 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@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html