[PATCH 1/5] ext4: ext4_dx_add_entry should dirty directory metadata with the directory inode

[PATCH 1/5] ext4: ext4_dx_add_entry should dirty directory metadata with the directory inode

[Darrick J. Wong]

ext4_dx_add_entry manipulates bh2 and frames[0].bh, which are two buffer_heads
that point to directory blocks assigned to the directory inode.  However, the
function calls ext4_handle_dirty_metadata with the inode of the file that's
being added to the directory, not the directory inode itself.  Therefore,
correct the code to dirty the directory buffers with the directory inode, not
the file inode.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---
 fs/ext4/namei.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)


diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 1c924fa..310b356 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1586,7 +1586,7 @@ static int ext4_dx_add_entry(handle_t *handle, struct dentry *dentry,
 			dxtrace(dx_show_index("node", frames[1].entries));
 			dxtrace(dx_show_index("node",
 			       ((struct dx_node *) bh2->b_data)->entries));
-			err = ext4_handle_dirty_metadata(handle, inode, bh2);
+			err = ext4_handle_dirty_metadata(handle, dir, bh2);
 			if (err)
 				goto journal_error;
 			brelse (bh2);
@@ -1612,7 +1612,7 @@ static int ext4_dx_add_entry(handle_t *handle, struct dentry *dentry,
 			if (err)
 				goto journal_error;
 		}
-		err = ext4_handle_dirty_metadata(handle, inode, frames[0].bh);
+		err = ext4_handle_dirty_metadata(handle, dir, frames[0].bh);
 		if (err) {
 			ext4_std_error(inode->i_sb, err);
 			goto cleanup;

[PATCH 2/5] ext4: ext4_rename should dirty dir_bh with the correct directory

[Darrick J. Wong]

When ext4_rename performs a directory rename (move), dir_bh is a buffer that is
modified to update the '..' link in the directory being moved (old_inode).
However, ext4_handle_dirty_metadata is called with the old parent directory
inode (old_dir) and dir_bh, which is incorrect because dir_bh does not belong
to the parent inode.  Fix this error.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---
 fs/ext4/namei.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)


diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 310b356..6d3fab4 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -2530,7 +2530,7 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry,
 		PARENT_INO(dir_bh->b_data, new_dir->i_sb->s_blocksize) =
 						cpu_to_le32(new_dir->i_ino);
 		BUFFER_TRACE(dir_bh, "call ext4_handle_dirty_metadata");
-		retval = ext4_handle_dirty_metadata(handle, old_dir, dir_bh);
+		retval = ext4_handle_dirty_metadata(handle, old_inode, dir_bh);
 		if (retval) {
 			ext4_std_error(old_dir->i_sb, retval);
 			goto end_rename;

[PATCH 3/5] ext4: ext4_mkdir should dirty dir_block with the parent inode

[Darrick J. Wong]

ext4_mkdir calls ext4_handle_dirty_metadata with dir_block and the inode "dir".
Unfortunately, dir_block belongs to the newly created directory (which is
"inode"), not the parent directory (which is "dir").  Fix the incorrect
association.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---
 fs/ext4/namei.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)


diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 6d3fab4..50c7294 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1863,7 +1863,7 @@ retry:
 	ext4_set_de_type(dir->i_sb, de, S_IFDIR);
 	inode->i_nlink = 2;
 	BUFFER_TRACE(dir_block, "call ext4_handle_dirty_metadata");
-	err = ext4_handle_dirty_metadata(handle, dir, dir_block);
+	err = ext4_handle_dirty_metadata(handle, inode, dir_block);
 	if (err)
 		goto out_clear_inode;
 	err = ext4_mark_inode_dirty(handle, inode);

[PATCH 4/5] ext4: Prevent stack overrun in ext4_file_open when recording last known mountpoint

[Darrick J. Wong]

In ext4_file_open, the filesystem records the mountpoint of the first file that
is opened after mounting the filesystem.  It does this by allocating a 64-byte
stack buffer, calling d_path() to grab the mount point through which this file
was accessed, and then memcpy()ing 64 bytes into the superblock's
s_last_mounted field, starting from the return value of d_path(), which is
stored as "cp".  However, if cp > buf (which it frequently is since path
components are prepended starting at the end of buf) then we can end up copying
stack data into the superblock.

Writing stack variables into the superblock doesn't sound like a great idea, so
use strlcpy instead.  Andi Kleen suggested using strlcpy instead of strncpy.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---
 fs/ext4/file.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)


diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index e4095e9..9781099 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -181,8 +181,8 @@ static int ext4_file_open(struct inode * inode, struct file * filp)
 		path.dentry = mnt->mnt_root;
 		cp = d_path(&path, buf, sizeof(buf));
 		if (!IS_ERR(cp)) {
-			memcpy(sbi->s_es->s_last_mounted, cp,
-			       sizeof(sbi->s_es->s_last_mounted));
+			strlcpy(sbi->s_es->s_last_mounted, cp,
+				sizeof(sbi->s_es->s_last_mounted));
 			ext4_mark_super_dirty(sb);
 		}
 	}

[PATCH 5/5] ext4: Fix endian problem in MMP initialization

[Darrick J. Wong]

As part of startup, the MMP initialization code does this:

mmp->mmp_seq = seq = cpu_to_le32(mmp_new_seq());

Next, mmp->mmp_seq is written out to disk, a delay happens, and then the MMP
block is read back in and the sequence value is tested:

if (seq != le32_to_cpu(mmp->mmp_seq)) {
	/* fail the mount */

On a LE system such as x86, the *le32* functions do nothing and this works.
Unfortunately, on a BE system such as ppc64, this comparison becomes:

if (cpu_to_le32(new_seq) != le32_to_cpu(cpu_to_le32(new_seq)) {
	/* fail the mount */

Except for a few palindromic sequence numbers, this test always causes the
mount to fail, which makes MMP filesystems generally unmountable on ppc64.  The
attached patch fixes this situation.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---
 fs/ext4/mmp.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)


diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 9bdef3f..a7a4986 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -295,7 +295,8 @@ skip:
 	/*
 	 * write a new random sequence number.
 	 */
-	mmp->mmp_seq = seq = cpu_to_le32(mmp_new_seq());
+	seq = mmp_new_seq();
+	mmp->mmp_seq = cpu_to_le32(seq);
 
 	retval = write_mmp_block(bh);
 	if (retval)

Re: [PATCH 5/5] ext4: Fix endian problem in MMP initialization

[Andreas Dilger]

You can add my

 Acked-by: Andreas Dilger <adilger@dilger.ca>

to this. 

Cheers, Andreas

On 2011-10-21, at 3:18 PM, "Darrick J. Wong" <djwong@us.ibm.com> wrote:

> As part of startup, the MMP initialization code does this:
> 
> mmp->mmp_seq = seq = cpu_to_le32(mmp_new_seq());
> 
> Next, mmp->mmp_seq is written out to disk, a delay happens, and then the MMP
> block is read back in and the sequence value is tested:
> 
> if (seq != le32_to_cpu(mmp->mmp_seq)) {
>    /* fail the mount */
> 
> On a LE system such as x86, the *le32* functions do nothing and this works.
> Unfortunately, on a BE system such as ppc64, this comparison becomes:
> 
> if (cpu_to_le32(new_seq) != le32_to_cpu(cpu_to_le32(new_seq)) {
>    /* fail the mount */
> 
> Except for a few palindromic sequence numbers, this test always causes the
> mount to fail, which makes MMP filesystems generally unmountable on ppc64.  The
> attached patch fixes this situation.
> 
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> ---
> fs/ext4/mmp.c |    3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
> 
> 
> diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
> index 9bdef3f..a7a4986 100644
> --- a/fs/ext4/mmp.c
> +++ b/fs/ext4/mmp.c
> @@ -295,7 +295,8 @@ skip:
>    /*
>     * write a new random sequence number.
>     */
> -    mmp->mmp_seq = seq = cpu_to_le32(mmp_new_seq());
> +    seq = mmp_new_seq();
> +    mmp->mmp_seq = cpu_to_le32(seq);
> 
>    retval = write_mmp_block(bh);
>    if (retval)
> 
> --
> 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

Re: [PATCH 2/5] ext4: ext4_rename should dirty dir_bh with the correct directory

[Ted Ts'o]

On Fri, Oct 21, 2011 at 02:18:06PM -0700, Darrick J. Wong wrote:
> When ext4_rename performs a directory rename (move), dir_bh is a buffer that is
> modified to update the '..' link in the directory being moved (old_inode).
> However, ext4_handle_dirty_metadata is called with the old parent directory
> inode (old_dir) and dir_bh, which is incorrect because dir_bh does not belong
> to the parent inode.  Fix this error.
> 
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>

This was already merged (git commit: bcaa9929750 in the master branch).

     	 	 	     	     		 - Ted

Re: [PATCH 3/5] ext4: ext4_mkdir should dirty dir_block with the parent inode

[Ted Ts'o]

On Fri, Oct 21, 2011 at 02:18:12PM -0700, Darrick J. Wong wrote:
> ext4_mkdir calls ext4_handle_dirty_metadata with dir_block and the inode "dir".
> Unfortunately, dir_block belongs to the newly created directory (which is
> "inode"), not the parent directory (which is "dir").  Fix the incorrect
> association.
> 
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>

This is already checked in (git commit: f9287c1f2d3 in the master branch).

     			   		- Ted

Re: [PATCH 1/5] ext4: ext4_dx_add_entry should dirty directory metadata with the directory inode

[Ted Ts'o]

On Fri, Oct 21, 2011 at 02:17:59PM -0700, Darrick J. Wong wrote:
> ext4_dx_add_entry manipulates bh2 and frames[0].bh, which are two buffer_heads
> that point to directory blocks assigned to the directory inode.  However, the
> function calls ext4_handle_dirty_metadata with the inode of the file that's
> being added to the directory, not the directory inode itself.  Therefore,
> correct the code to dirty the directory buffers with the directory inode, not
> the file inode.
> 
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>

This has been accpeted into the ext4 tree already (git commit:
5930ea643 in the master branch)

						- Ted

Re: [PATCH 4/5] ext4: Prevent stack overrun in ext4_file_open when recording last known mountpoint

[Ted Ts'o]

On Fri, Oct 21, 2011 at 02:18:18PM -0700, Darrick J. Wong wrote:
> In ext4_file_open, the filesystem records the mountpoint of the first file that
> is opened after mounting the filesystem.  It does this by allocating a 64-byte
> stack buffer, calling d_path() to grab the mount point through which this file
> was accessed, and then memcpy()ing 64 bytes into the superblock's
> s_last_mounted field, starting from the return value of d_path(), which is
> stored as "cp".  However, if cp > buf (which it frequently is since path
> components are prepended starting at the end of buf) then we can end up copying
> stack data into the superblock.
> 
> Writing stack variables into the superblock doesn't sound like a great idea, so
> use strlcpy instead.  Andi Kleen suggested using strlcpy instead of strncpy.
> 
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>

Thanks, applied.

						- Ted

Re: [PATCH 5/5] ext4: Fix endian problem in MMP initialization

[Ted Ts'o]

On Fri, Oct 21, 2011 at 02:18:25PM -0700, Darrick J. Wong wrote:
> As part of startup, the MMP initialization code does this:
> 
> mmp->mmp_seq = seq = cpu_to_le32(mmp_new_seq());
> 
> Next, mmp->mmp_seq is written out to disk, a delay happens, and then the MMP
> block is read back in and the sequence value is tested:
> 
> if (seq != le32_to_cpu(mmp->mmp_seq)) {
> 	/* fail the mount */
> 
> On a LE system such as x86, the *le32* functions do nothing and this works.
> Unfortunately, on a BE system such as ppc64, this comparison becomes:
> 
> if (cpu_to_le32(new_seq) != le32_to_cpu(cpu_to_le32(new_seq)) {
> 	/* fail the mount */
> 
> Except for a few palindromic sequence numbers, this test always
> causes the mount to fail, which makes MMP filesystems generally
> unmountable on ppc64.  The attached patch fixes this situation.
> 
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>

Thanks, this is already in the ext4 tree, although not yet in the
master branch (just the dev branch).

					- Ted