Null Pointer when make_indexed_dir returns -ENOSPC

PATCH Superseded Allison Henderson 5 participants 17 messages May 7, 2011 – May 10, 2011 ext4 file system

lore

Share & tools

lore

Allison Henderson

author

2011-05-07 23:54 UTC #0 lore raw

PATCH 1/1 Null Pointer when make_indexed_dir returns -ENOSPC

Fix for a null pointer bug found while running punch hole tests

Signed-off-by Allison Henderson <[email protected]>

---
:100644 100644 3c7a06e... 3302a6c... M fs/ext4/namei.c

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

fs/ext4/namei.c +2 -1

index 3c7a06e..3302a6c 100644
@@ -1422,7 +1422,8 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
 		 */
 		ext4_mark_inode_dirty(handle, dir);
 		ext4_handle_dirty_metadata(handle, dir, frame->bh);
-		ext4_handle_dirty_metadata(handle, dir, bh);
+		if (bh)
+			ext4_handle_dirty_metadata(handle, dir, bh);
 		dx_release(frames);
 		return retval;
 	}

Signature · 1 line

1.7.1

Ted Ts'o

maintainer

2011-05-09 00:38 UTC #1 lore raw

Quoted 3 lines · Allison Henderson

Fix for a null pointer bug found while running punch hole tests
Signed-off-by Allison Henderson <[email protected]>

Thanks, I've added this to the ext4 tree.

- Ted

Jan Kara

reviewer

2011-05-09 11:03 UTC #2 lore raw

In reply to Allison Henderson

Quoted 13 lines · Allison Henderson

Fix for a null pointer bug found while running punch hole tests
Signed-off-by Allison Henderson <[email protected]>
---
:100644 100644 3c7a06e... 3302a6c... M fs/ext4/namei.c
 fs/ext4/namei.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
fs/ext4/namei.c +0 -0
index 3c7a06e..3302a6c 100644

fs/ext4/namei.c static int make_indexed_dir(handle_t *handle, struct dentry *dentry, · line 1426

Quoted 6 lines

fs/ext4/namei.c +2 -1

@@ -1422,7 +1422,8 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
 		 */
 		ext4_mark_inode_dirty(handle, dir);
 		ext4_handle_dirty_metadata(handle, dir, frame->bh);
-		ext4_handle_dirty_metadata(handle, dir, bh);
+		if (bh)
+			ext4_handle_dirty_metadata(handle, dir, bh);

I'm puzzled - bh here is bh2 from the beginning of the function and we
check it for being NULL after we ext4_append() it. So how can this happen?

Honza

Quoted 6 lines

fs/ext4/namei.c +0 -0
 		dx_release(frames);
 		return retval;
 	}
Signature · 1 line
1.7.1

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Yongqiang Yang

2011-05-09 11:18 UTC #3 lore raw

Quoted 9 lines · Jan Kara

Quoted 8 lines · Allison Henderson
Fix for a null pointer bug found while running punch hole tests
Signed-off-by Allison Henderson <[email protected]>
---
:100644 100644 3c7a06e... 3302a6c... M fs/ext4/namei.c fs/ext4/namei.c | 3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Quoted 13 lines

fs/ext4/namei.c static int make_indexed_dir(handle_t *handle, struct dentry *dentry, · line 1426
Quoted 11 lines
fs/ext4/namei.c +2 -1
index 3c7a06e..3302a6c 100644
@@ -1422,7 +1422,8 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
                */
               ext4_mark_inode_dirty(handle, dir);
               ext4_handle_dirty_metadata(handle, dir, frame->bh);
-             ext4_handle_dirty_metadata(handle, dir, bh);
+             if (bh)
+                     ext4_handle_dirty_metadata(handle, dir, bh);
I'm puzzled - bh here is bh2 from the beginning of the function and we check it for being NULL after we ext4_append() it. So how can this happen?

do_split() encounters a journal error and set bh to NULL before returning.

Quoted 16 lines

Honza
Quoted 6 lines
fs/ext4/namei.c +0 -1
               dx_release(frames);
               return retval;
       }
--
1.7.1
--
Jan Kara <[email protected]>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 6 lines

Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Yongqiang Yang

2011-05-09 11:20 UTC #4 lore raw

Quoted 23 lines · Yongqiang Yang

Quoted 9 lines · Jan Kara
Quoted 8 lines · Allison Henderson
Fix for a null pointer bug found while running punch hole tests
Signed-off-by Allison Henderson <[email protected]>
---
:100644 100644 3c7a06e... 3302a6c... M fs/ext4/namei.c fs/ext4/namei.c | 3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Quoted 13 lines
fs/ext4/namei.c static int make_indexed_dir(handle_t *handle, struct dentry *dentry, · line 1426
Quoted 11 lines
fs/ext4/namei.c +2 -1
index 3c7a06e..3302a6c 100644
@@ -1422,7 +1422,8 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
                */
               ext4_mark_inode_dirty(handle, dir);
               ext4_handle_dirty_metadata(handle, dir, frame->bh);
-             ext4_handle_dirty_metadata(handle, dir, bh);
+             if (bh)
+                     ext4_handle_dirty_metadata(handle, dir, bh);
I'm puzzled - bh here is bh2 from the beginning of the function and we check it for being NULL after we ext4_append() it. So how can this happen?

Quoted 1 line

do_split() encounters a journal error and set bh to NULL before returning.

Sorry, it does not make sense.

Quoted 24 lines

Quoted 9 lines
Honza
Quoted 6 lines
fs/ext4/namei.c +0 -1
               dx_release(frames);
               return retval;
       }
--
1.7.1
Quoted 8 lines
--
Jan Kara <[email protected]>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Wishes
Yongqiang Yang

Signature · 6 lines

Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Yongqiang Yang

2011-05-09 11:21 UTC #5 lore raw

Quoted 24 lines · Yongqiang Yang

Quoted 23 lines · Yongqiang Yang
Quoted 22 lines · Jan Kara
Quoted 13 lines · Allison Henderson
Fix for a null pointer bug found while running punch hole tests
Signed-off-by Allison Henderson <[email protected]>
---
:100644 100644 3c7a06e... 3302a6c... M fs/ext4/namei.c fs/ext4/namei.c | 3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
fs/ext4/namei.c +0 -0
index 3c7a06e..3302a6c 100644
fs/ext4/namei.c static int make_indexed_dir(handle_t *handle, struct dentry *dentry, · line 1426
Quoted 6 lines
fs/ext4/namei.c +2 -1
@@ -1422,7 +1422,8 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
                */
               ext4_mark_inode_dirty(handle, dir);
               ext4_handle_dirty_metadata(handle, dir, frame->bh);
-             ext4_handle_dirty_metadata(handle, dir, bh);
+             if (bh)
+                     ext4_handle_dirty_metadata(handle, dir, bh);
I'm puzzled - bh here is bh2 from the beginning of the function and we check it for being NULL after we ext4_append() it. So how can this happen?

Quoted 2 lines

Quoted 1 line
do_split() encounters a journal error and set bh to NULL before returning.
Sorry, it does not make sense.

Sorry for being dense, it makes sense.

Quoted 32 lines

Quoted 21 lines
Quoted 10 lines
Honza
Quoted 6 lines
fs/ext4/namei.c +0 -1
               dx_release(frames);
               return retval;
       }
--
1.7.1
Quoted 8 lines
--
Jan Kara <[email protected]>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 4 lines
--
Best Wishes
Yongqiang Yang
--
Best Wishes
Yongqiang Yang

Signature · 6 lines

Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jan Kara

reviewer

2011-05-09 11:30 UTC #6 lore raw

In reply to Yongqiang Yang

Quoted 23 lines · Yongqiang Yang

Quoted 9 lines · Jan Kara
Quoted 8 lines · Allison Henderson
Fix for a null pointer bug found while running punch hole tests
Signed-off-by Allison Henderson <[email protected]>
---
:100644 100644 3c7a06e... 3302a6c... M fs/ext4/namei.c fs/ext4/namei.c | 3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Quoted 13 lines
fs/ext4/namei.c static int make_indexed_dir(handle_t *handle, struct dentry *dentry, · line 1426
Quoted 11 lines
fs/ext4/namei.c +2 -1
index 3c7a06e..3302a6c 100644
@@ -1422,7 +1422,8 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
                */
               ext4_mark_inode_dirty(handle, dir);
               ext4_handle_dirty_metadata(handle, dir, frame->bh);
-             ext4_handle_dirty_metadata(handle, dir, bh);
+             if (bh)
+                     ext4_handle_dirty_metadata(handle, dir, bh);
I'm puzzled - bh here is bh2 from the beginning of the function and we check it for being NULL after we ext4_append() it. So how can this happen?

Quoted 1 line

do_split() encounters a journal error and set bh to NULL before returning.

Ah, I see. But then you just reintroduced the bug I was trying to fix. So either do_split() has to do the marking of buffer dirty, or we have to do it before calllig do_split(), or do_split() has to be changed and not release passed buffer (and the two callers have to do it - which they seem to do anyway). I don't mind either way but your fix is wrong.

Honza

Signature · 6 lines

Jan Kara <[email protected]>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Yongqiang Yang

2011-05-09 11:33 UTC #7 lore raw

Quoted 24 lines · Jan Kara

Quoted 23 lines · Yongqiang Yang
Quoted 22 lines · Jan Kara
Quoted 13 lines · Allison Henderson
Fix for a null pointer bug found while running punch hole tests
Signed-off-by Allison Henderson <[email protected]>
---
:100644 100644 3c7a06e... 3302a6c... M fs/ext4/namei.c fs/ext4/namei.c | 3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
fs/ext4/namei.c +0 -0
index 3c7a06e..3302a6c 100644
fs/ext4/namei.c static int make_indexed_dir(handle_t *handle, struct dentry *dentry, · line 1426
Quoted 6 lines
fs/ext4/namei.c +2 -1
@@ -1422,7 +1422,8 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
                */
               ext4_mark_inode_dirty(handle, dir);
               ext4_handle_dirty_metadata(handle, dir, frame->bh);
-             ext4_handle_dirty_metadata(handle, dir, bh);
+             if (bh)
+                     ext4_handle_dirty_metadata(handle, dir, bh);
I'm puzzled - bh here is bh2 from the beginning of the function and we check it for being NULL after we ext4_append() it. So how can this happen?

Quoted 6 lines

Quoted 1 line
do_split() encounters a journal error and set bh to NULL before returning.
Ah, I see. But then you just reintroduced the bug I was trying to fix. So either do_split() has to do the marking of buffer dirty, or we have to do it before calllig do_split(), or do_split() has to be changed and not release passed buffer (and the two callers have to do it - which they seem to do anyway). I don't mind either way but your fix is wrong.

The fix is made by Allison not me, I think Allison will have a look at the thread.

Yongqiang.

Quoted 6 lines

Honza --
Jan Kara <[email protected]>
SUSE Labs, CR

Signature · 6 lines

Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jan Kara

reviewer

2011-05-09 11:36 UTC #8 lore raw

Quoted 25 lines · Yongqiang Yang

Quoted 24 lines · Jan Kara
Quoted 23 lines · Yongqiang Yang
Quoted 22 lines · Jan Kara
Quoted 13 lines · Allison Henderson
Fix for a null pointer bug found while running punch hole tests
Signed-off-by Allison Henderson <[email protected]>
---
:100644 100644 3c7a06e... 3302a6c... M fs/ext4/namei.c fs/ext4/namei.c | 3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
fs/ext4/namei.c +0 -0
index 3c7a06e..3302a6c 100644
fs/ext4/namei.c static int make_indexed_dir(handle_t *handle, struct dentry *dentry, · line 1426
Quoted 6 lines
fs/ext4/namei.c +2 -1
@@ -1422,7 +1422,8 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
                */
               ext4_mark_inode_dirty(handle, dir);
               ext4_handle_dirty_metadata(handle, dir, frame->bh);
-             ext4_handle_dirty_metadata(handle, dir, bh);
+             if (bh)
+                     ext4_handle_dirty_metadata(handle, dir, bh);
I'm puzzled - bh here is bh2 from the beginning of the function and we check it for being NULL after we ext4_append() it. So how can this happen?

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
do_split() encounters a journal error and set bh to NULL before returning.
Ah, I see. But then you just reintroduced the bug I was trying to fix. So either do_split() has to do the marking of buffer dirty, or we have to do it before calllig do_split(), or do_split() has to be changed and not release passed buffer (and the two callers have to do it - which they seem to do anyway). I don't mind either way but your fix is wrong.
The fix is made by Allison not me, I think Allison will have a look at the thread.

Right, sorry. I was addressing Allison, not you of course ;).

Honza

Signature · 6 lines

Jan Kara <[email protected]>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ted Ts'o

maintainer

2011-05-09 13:55 UTC #9 lore raw

In reply to Jan Kara

Quoted 5 lines · Jan Kara

Ah, I see. But then you just reintroduced the bug I was trying to fix. So either do_split() has to do the marking of buffer dirty, or we have to do it before calllig do_split(), or do_split() has to be changed and not release passed buffer (and the two callers have to do it - which they seem to do anyway). I don't mind either way but your fix is wrong.

I think it's OK. We do call ext4_handle_dirty_metadata on frame->bh, which deals with the original version of bh. And the cases where do_split() sets bh to NULL is either (a) a journal error, in which case we will have already aborted the journal, or an I/O error while reading in the block, so bh won't have gotten modified yet.

Is there a case that you're worried about that I'm missing?

- Ted

Jan Kara

reviewer

2011-05-09 14:05 UTC #10 lore raw

Quoted 7 lines · Ted Tso

Quoted 5 lines · Jan Kara
Ah, I see. But then you just reintroduced the bug I was trying to fix. So either do_split() has to do the marking of buffer dirty, or we have to do it before calllig do_split(), or do_split() has to be changed and not release passed buffer (and the two callers have to do it - which they seem to do anyway). I don't mind either way but your fix is wrong.

Quoted 7 lines

I think it's OK. We do call ext4_handle_dirty_metadata on frame->bh, which deals with the original version of bh. And the cases where do_split() sets bh to NULL is either (a) a journal error, in which case we will have already aborted the journal, or an I/O error while reading in the block, so bh won't have gotten modified yet.
Is there a case that you're worried about that I'm missing?

Yes. ext4_append() can return ENOSPC and passed bh will get set to NULL
without being marked dirty. Note that we need to call ext4_handle_dirty_metadata() on the passed bh as well specifically in the make_indexed_dir() case because there we move contents of the first block (in frame->bh) to the second block (passed bh) and create indexed tree root in the first block. Then we call do_split() to further split the second block...

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Ted Ts'o

maintainer

2011-05-09 14:22 UTC #11 lore raw

Quoted 2 lines · Jan Kara

Yes. ext4_append() can return ENOSPC and passed bh will get set to NULL
without being marked dirty.

Ah, so the right fix then is to add to make the cleanup code like this:

		ext4_mark_inode_dirty(handle, dir);
		ext4_handle_dirty_metadata(handle, dir, frame->bh);
+	        ext4_handle_dirty_metadata(handle, dir, bh2);
+		if (bh)
+			ext4_handle_dirty_metadata(handle, dir, bh);
		dx_release(frames);
		return retval;

Agreed?

- Ted

Theodore Ts'o

maintainer

2011-05-09 14:27 UTC #12 lore raw

PATCH 1/1 ext4: don't dereference null pointer when make_indexed_dir() fails

From: Allison Henderson <[email protected]>

Fix for a null pointer bug found while running punch hole tests

Signed-off-by Allison Henderson <[email protected]>

Signed-off-by "Theodore Ts'o" <[email protected]>

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

fs/ext4/namei.c +3 -1

index 3c7a06e..cc97feb 100644
@@ -1422,7 +1422,9 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
 		 */
 		ext4_mark_inode_dirty(handle, dir);
 		ext4_handle_dirty_metadata(handle, dir, frame->bh);
-		ext4_handle_dirty_metadata(handle, dir, bh);
+		ext4_handle_dirty_metadata(handle, dir, bh2);
+		if (bh)
+			ext4_handle_dirty_metadata(handle, dir, bh);
 		dx_release(frames);
 		return retval;
 	}

Signature · 1 line

1.7.3.1

Eric Sandeen

2011-05-09 14:57 UTC #13 lore raw

Re: [PATCH -v2] ext4: don't dereference null pointer when make_indexed_dir() fails

Quoted 1 line · Theodore Ts'o

From: Allison Henderson <[email protected]>

Ted, can we be a little careful about attribution in cases like this?

If I have it straight, this isn't the patch Allison sent, it's based on it, but it is in fact:

Modified-by Theodore Ts'o <[email protected]>

right?

I think it's important to keep track of who is making various changes to submitted patches, especially when they are functional (i.e. not whitespace or spelling, etc) changes.

Ideally, IMHO, the original submitter should be submitting V2 based on feedback, unless they are AWOL; this way patches attributed to a submitter really are their patches, and it is another layer of review if the original submitter can evaluate the proposed changes to their original patch...

Doing it this way should lighten your load too, I'd think.

Thanks,
-Eric

Quoted 23 lines

Fix for a null pointer bug found while running punch hole tests

Signed-off-by Allison Henderson <[email protected]>

Signed-off-by "Theodore Ts'o" <[email protected]>

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

fs/ext4/namei.c +3 -1

index 3c7a06e..cc97feb 100644
@@ -1422,7 +1422,9 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
 		 */
 		ext4_mark_inode_dirty(handle, dir);
 		ext4_handle_dirty_metadata(handle, dir, frame->bh);
-		ext4_handle_dirty_metadata(handle, dir, bh);
+		ext4_handle_dirty_metadata(handle, dir, bh2);
+		if (bh)
+			ext4_handle_dirty_metadata(handle, dir, bh);
 		dx_release(frames);
 		return retval;
 	}

Jan Kara

reviewer

2011-05-09 14:42 UTC #14 lore raw

In reply to Ted Ts'o

Quoted 14 lines · Ted Tso

Quoted 2 lines · Jan Kara
Yes. ext4_append() can return ENOSPC and passed bh will get set to NULL
without being marked dirty.
Ah, so the right fix then is to add to make the cleanup code like this:
		ext4_mark_inode_dirty(handle, dir);
		ext4_handle_dirty_metadata(handle, dir, frame->bh);
(quoted diff) +3 -0
+	        ext4_handle_dirty_metadata(handle, dir, bh2);
+		if (bh)
+			ext4_handle_dirty_metadata(handle, dir, bh);
		dx_release(frames);
		return retval;

Quoted 1 line

Agreed?

Not quite. make_indexed_dir() does frame->bh = bh and bh = bh2 before
calling do_split(). So bh2 is not really carrying a valid buffer reference at this point - even more so because do_split() does brelse() on the passed bh so it need not be around when are at this point. The code is a real mess. But for example attached patch will work because both callers of do_split() do brelse() anyway.

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 1 line

From 59729e0ab18a763ba36616a1025ce606a8721f1c Mon Sep 17 00:00:00 2001

From: Jan Kara <[email protected]>
Date: Mon, 9 May 2011 16:39:34 +0200
Subject: [PATCH] ext4: Stop releasing passed bh in do_split()

make_indexed_dir() needs to do error recovery on the passed bh when do_split() fails. So do not release it early in do_split().

Signed-off-by Jan Kara <[email protected]>

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

fs/ext4/namei.c +1 -6

index 3c7a06e..1cddab9 100644
@@ -1165,11 +1165,8 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
 	int	err = 0, i;
 
 	bh2 = ext4_append (handle, dir, &newblock, &err);
-	if (!(bh2)) {
-		brelse(*bh);
-		*bh = NULL;
+	if (!bh2)
 		goto errout;
-	}
 
 	BUFFER_TRACE(*bh, "get_write_access");
 	err = ext4_journal_get_write_access(handle, *bh);

@@ -1235,9 +1232,7 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir,
 	return de;
 
 journal_error:
-	brelse(*bh);
 	brelse(bh2);
-	*bh = NULL;
 	ext4_std_error(dir->i_sb, err);
 errout:
 	*error = err;

Signature · 1 line

1.7.1

Allison Henderson

author

2011-05-09 20:39 UTC #15 lore raw

Quoted 23 lines · Jan Kara

Quoted 14 lines · Ted Tso
Quoted 2 lines · Jan Kara
Yes. ext4_append() can return ENOSPC and passed bh will get set to NULL
without being marked dirty.
Ah, so the right fix then is to add to make the cleanup code like this:
		ext4_mark_inode_dirty(handle, dir);
		ext4_handle_dirty_metadata(handle, dir, frame->bh);
(quoted diff) +3 -0
+	        ext4_handle_dirty_metadata(handle, dir, bh2);
+		if (bh)
+			ext4_handle_dirty_metadata(handle, dir, bh);
		dx_release(frames);
		return retval;
Quoted 1 line
Agreed?
Not quite. make_indexed_dir() does frame->bh = bh and bh = bh2 before
calling do_split(). So bh2 is not really carrying a valid buffer reference at this point - even more so because do_split() does brelse() on the passed bh so it need not be around when are at this point. The code is a real mess. But for example attached patch will work because both callers of do_split() do brelse() anyway.

Quoted 1 line

Honza

Hi all,

Oh, I understand the problem now. Sorry for the late response, I had to stop and dig around with this one for a bit. Would people prefer to add the new code before the do_split like this:

+ ext4_handle_dirty_metadata(handle, dir, frame->bh);
+ ext4_handle_dirty_metadata(handle, dir, bh);
+
de = do_split(handle,dir, &bh, frame, &hinfo, &retval);
if (!de) {
/*
@@ -1421,8 +1425,6 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry,
* with corrupted filesystem.
*/
ext4_mark_inode_dirty(handle, dir);
- ext4_handle_dirty_metadata(handle, dir, frame->bh);
- ext4_handle_dirty_metadata(handle, dir, bh);
dx_release(frames);
return retval;
}

I've tested both patches and they both seem to resolve the null pointer.
The only other solution that comes to mind would be to add flags to
the do_split to skip the brelse or to do the mark dirty before the brelse as you suggest.

Allison Henderson

Jan Kara

reviewer

2011-05-10 13:34 UTC #16 lore raw

Quoted 47 lines · Allison Henderson

Quoted 23 lines · Jan Kara
Quoted 14 lines · Ted Tso
Quoted 2 lines · Jan Kara
Yes. ext4_append() can return ENOSPC and passed bh will get set to NULL
without being marked dirty.
Ah, so the right fix then is to add to make the cleanup code like this:
		ext4_mark_inode_dirty(handle, dir);
		ext4_handle_dirty_metadata(handle, dir, frame->bh);
(quoted diff) +3 -0
+	        ext4_handle_dirty_metadata(handle, dir, bh2);
+		if (bh)
+			ext4_handle_dirty_metadata(handle, dir, bh);
		dx_release(frames);
		return retval;
Quoted 1 line
Agreed?
Not quite. make_indexed_dir() does frame->bh = bh and bh = bh2 before
calling do_split(). So bh2 is not really carrying a valid buffer reference at this point - even more so because do_split() does brelse() on the passed bh so it need not be around when are at this point. The code is a real mess. But for example attached patch will work because both callers of do_split() do brelse() anyway.
Quoted 1 line
Honza
Hi all,
Oh, I understand the problem now. Sorry for the late response, I had to stop and dig around with this one for a bit. Would people prefer to add the new code before the do_split like this:
(quoted diff) +3 -2
+	ext4_handle_dirty_metadata(handle, dir, frame->bh);
+	ext4_handle_dirty_metadata(handle, dir, bh);
+
 	de = do_split(handle,dir, &bh, frame, &hinfo, &retval);
 	if (!de) {
 		/*
@@ -1421,8 +1425,6 @@ static int make_indexed_dir(handle_t *handle,
 struct dentry *dentry,
 		 * with corrupted filesystem.
 		 */
 		ext4_mark_inode_dirty(handle, dir);
-		ext4_handle_dirty_metadata(handle, dir, frame->bh);
-		ext4_handle_dirty_metadata(handle, dir, bh);
 		dx_release(frames);
 		return retval;

Quoted 1 line

}

This would be fine with me as well. It has a slight overhead of marking
buffer dirty twice but I don't think it really matters.

Quoted 4 lines

I've tested both patches and they both seem to resolve the null pointer. The only other solution that comes to mind would be to add flags to the do_split to skip the brelse or to do the mark dirty before the brelse as you suggest.

Yes, I don't mind which of them Ted chooses...

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR