After the patch I posted last week regarding buffer head ref leaks in
no-journal mode, I looked at all the code that uses buffer heads and
searched for more potential leaks.
The patch below fixes the issues I found; these can occur even when a
journal is present.
The change to inode.c fixes a double release if
ext4_journal_get_create_access() fails.
The changes to namei.c are more complicated. add_dirent_to_buf() will
release the input buffer head EXCEPT when it returns -ENOSPC. There are
some callers of this routine that don't always do the brelse() in the event
that -ENOSPC is returned. Unfortunately, to put this fix into ext4_add_entry()
required capturing the return value of make_indexed_dir() and
add_dirent_to_buf().
I'd appreciate comments on these changes, in particular if I'm just missing
something obvious here.
Signed-off-by: Curt Wohlgemuth <[email protected]>
---
diff -Naur orig/fs/ext4/inode.c new/fs/ext4/inode.c
--- orig/fs/ext4/inode.c 2009-07-14 11:19:01.000000000 -0700
+++ new/fs/ext4/inode.c 2009-07-14 11:51:42.000000000 -0700
@@ -758,8 +758,9 @@
BUFFER_TRACE(bh, "call get_create_access");
err = ext4_journal_get_create_access(handle, bh);
if (err) {
+ /* Don't brelse(bh) here; it's done in journal_forget()
+ * below */
unlock_buffer(bh);
- brelse(bh);
goto failed;
}
diff -Naur orig/fs/ext4/namei.c new/fs/ext4/namei.c
--- orig/fs/ext4/namei.c 2009-07-14 11:19:46.000000000 -0700
+++ new/fs/ext4/namei.c 2009-07-14 11:19:28.000000000 -0700
@@ -1498,12 +1498,14 @@
sb = dir->i_sb;
blocksize = sb->s_blocksize;
- if (!dentry->d_name.len)
- return -EINVAL;
+ if (!dentry->d_name.len) {
+ retval = -EINVAL;
+ goto out;
+ }
if (is_dx(dir)) {
retval = ext4_dx_add_entry(handle, dentry, inode);
if (!retval || (retval != ERR_BAD_DX_DIR))
- return retval;
+ goto out;
EXT4_I(dir)->i_flags &= ~EXT4_INDEX_FL;
dx_fallback++;
ext4_mark_inode_dirty(handle, dir);
@@ -1512,23 +1514,31 @@
for (block = 0; block < blocks; block++) {
bh = ext4_bread(handle, dir, block, 0, &retval);
if(!bh)
- return retval;
+ goto out;
retval = add_dirent_to_buf(handle, dentry, inode, NULL, bh);
if (retval != -ENOSPC)
- return retval;
+ goto out;
if (blocks == 1 && !dx_fallback &&
- EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_DIR_INDEX))
- return make_indexed_dir(handle, dentry, inode, bh);
+ EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_DIR_INDEX)) {
+ retval = make_indexed_dir(handle, dentry, inode, bh);
+ if (retval == -ENOSPC)
+ brelse(bh);
+ goto out;
+ }
brelse(bh);
}
bh = ext4_append(handle, dir, &block, &retval);
if (!bh)
- return retval;
+ goto out;
de = (struct ext4_dir_entry_2 *) bh->b_data;
de->inode = 0;
de->rec_len = ext4_rec_len_to_disk(blocksize, blocksize);
- return add_dirent_to_buf(handle, dentry, inode, de, bh);
+ retval = add_dirent_to_buf(handle, dentry, inode, de, bh);
+ if (retval == -ENOSPC)
+ brelse(bh);
+out:
+ return retval;
}
/*
@@ -1657,7 +1667,8 @@
if (!de)
goto cleanup;
err = add_dirent_to_buf(handle, dentry, inode, de, bh);
- bh = NULL;
+ if (err != -ENOSPC)
+ bh = NULL;
goto cleanup;
journal_error:
On Tue, Jul 14, 2009 at 01:58:29PM -0700, Curt Wohlgemuth wrote:
> After the patch I posted last week regarding buffer head ref leaks in
> no-journal mode, I looked at all the code that uses buffer heads and
> searched for more potential leaks.
>
> The patch below fixes the issues I found; these can occur even when a
> journal is present.
>
> The change to inode.c fixes a double release if
> ext4_journal_get_create_access() fails.
>
> The changes to namei.c are more complicated. add_dirent_to_buf() will
> release the input buffer head EXCEPT when it returns -ENOSPC. There are
> some callers of this routine that don't always do the brelse() in the event
> that -ENOSPC is returned. Unfortunately, to put this fix into ext4_add_entry()
> required capturing the return value of make_indexed_dir() and
> add_dirent_to_buf().
>
> I'd appreciate comments on these changes, in particular if I'm just missing
> something obvious here.
>
> Signed-off-by: Curt Wohlgemuth <[email protected]>
>
> ---
> diff -Naur orig/fs/ext4/inode.c new/fs/ext4/inode.c
> --- orig/fs/ext4/inode.c 2009-07-14 11:19:01.000000000 -0700
> +++ new/fs/ext4/inode.c 2009-07-14 11:51:42.000000000 -0700
> @@ -758,8 +758,9 @@
> BUFFER_TRACE(bh, "call get_create_access");
> err = ext4_journal_get_create_access(handle, bh);
> if (err) {
> + /* Don't brelse(bh) here; it's done in journal_forget()
> + * below */
> unlock_buffer(bh);
> - brelse(bh);
> goto failed;
> }
>
I am not able to find the journal_foget call in the path. brelse is
dropping the buffer_head reference got from sb_getblk right ? Can you
tell me what is that i am missing ?
> diff -Naur orig/fs/ext4/namei.c new/fs/ext4/namei.c
> --- orig/fs/ext4/namei.c 2009-07-14 11:19:46.000000000 -0700
> +++ new/fs/ext4/namei.c 2009-07-14 11:19:28.000000000 -0700
> @@ -1498,12 +1498,14 @@
.. snip..
-aneesh
>
> diff -Naur orig/fs/ext4/namei.c new/fs/ext4/namei.c
> --- orig/fs/ext4/namei.c 2009-07-14 11:19:46.000000000 -0700
> +++ new/fs/ext4/namei.c 2009-07-14 11:19:28.000000000 -0700
> @@ -1498,12 +1498,14 @@
>
> sb = dir->i_sb;
> blocksize = sb->s_blocksize;
> - if (!dentry->d_name.len)
> - return -EINVAL;
> + if (!dentry->d_name.len) {
> + retval = -EINVAL;
> + goto out;
> + }
>
Do we really need those goto changes. We just return even at the label "out".
Seperating that out of the patch makes review easier
-aneesh
On Tue, Jul 14, 2009 at 10:52 PM, Aneesh Kumar
K.V<[email protected]> wrote:
> On Tue, Jul 14, 2009 at 01:58:29PM -0700, Curt Wohlgemuth wrote:
>> After the patch I posted last week regarding buffer head ref leaks in
>> no-journal mode, I looked at all the code that uses buffer heads and
>> searched for more potential leaks.
>>
>> The patch below fixes the issues I found; these can occur even when a
>> journal is present.
>>
>> The change to inode.c fixes a double release if
>> ext4_journal_get_create_access() fails.
>>
>> The changes to namei.c are more complicated. ?add_dirent_to_buf() will
>> release the input buffer head EXCEPT when it returns -ENOSPC. ?There are
>> some callers of this routine that don't always do the brelse() in the event
>> that -ENOSPC is returned. ?Unfortunately, to put this fix into ext4_add_entry()
>> required capturing the return value of make_indexed_dir() and
>> add_dirent_to_buf().
>>
>> I'd appreciate comments on these changes, in particular if I'm just missing
>> something obvious here.
>>
>> ? ? ? ?Signed-off-by: Curt Wohlgemuth <[email protected]>
>>
>> ---
>> diff -Naur orig/fs/ext4/inode.c new/fs/ext4/inode.c
>> --- orig/fs/ext4/inode.c ? ? ?2009-07-14 11:19:01.000000000 -0700
>> +++ new/fs/ext4/inode.c ? ? ? 2009-07-14 11:51:42.000000000 -0700
>> @@ -758,8 +758,9 @@
>> ? ? ? ? ? ? ? BUFFER_TRACE(bh, "call get_create_access");
>> ? ? ? ? ? ? ? err = ext4_journal_get_create_access(handle, bh);
>> ? ? ? ? ? ? ? if (err) {
>> + ? ? ? ? ? ? ? ? ? ? /* Don't brelse(bh) here; it's done in journal_forget()
>> + ? ? ? ? ? ? ? ? ? ? ?* below */
>> ? ? ? ? ? ? ? ? ? ? ? unlock_buffer(bh);
>> - ? ? ? ? ? ? ? ? ? ? brelse(bh);
>> ? ? ? ? ? ? ? ? ? ? ? goto failed;
>> ? ? ? ? ? ? ? }
>>
>
> I am not able to find the journal_foget call in the path. brelse is
> dropping the buffer_head reference got from sb_getblk right ? Can you
> tell me what is that i am missing ?
Look at the code at the "failed" label. For each of the allocated BHs
thus far, there's a call to ext4_journal_forget().
Oops, I should have put "ext4_journal_forget()" in the comment; my apologies.
I'll resend this out, along with a change to ext4_add_entry() to
remove the gotos.
Thanks,
Curt
>
>
>> diff -Naur orig/fs/ext4/namei.c new/fs/ext4/namei.c
>> --- orig/fs/ext4/namei.c ? ? ?2009-07-14 11:19:46.000000000 -0700
>> +++ new/fs/ext4/namei.c ? ? ? 2009-07-14 11:19:28.000000000 -0700
>> @@ -1498,12 +1498,14 @@
>
> .. snip..
>
> -aneesh
>
I've cleaned up the patch to
- fix the comment in ext4_alloc_branch()
- change ext4_add_entry() to remove the gotos and label
Signed-off-by: Curt Wohlgemuth <[email protected]>
---
diff -Naur orig/fs/ext4/inode.c new/fs/ext4/inode.c
--- orig/fs/ext4/inode.c 2009-07-14 11:19:01.000000000 -0700
+++ new/fs/ext4/inode.c 2009-07-15 08:12:15.000000000 -0700
@@ -758,8 +758,9 @@
BUFFER_TRACE(bh, "call get_create_access");
err = ext4_journal_get_create_access(handle, bh);
if (err) {
+ /* Don't brelse(bh) here; it's done in
+ * ext4_journal_forget() below */
unlock_buffer(bh);
- brelse(bh);
goto failed;
}
diff -Naur orig/fs/ext4/namei.c new/fs/ext4/namei.c
--- orig/fs/ext4/namei.c 2009-07-14 11:19:46.000000000 -0700
+++ new/fs/ext4/namei.c 2009-07-15 08:12:09.000000000 -0700
@@ -1518,8 +1518,12 @@
return retval;
if (blocks == 1 && !dx_fallback &&
- EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_DIR_INDEX))
- return make_indexed_dir(handle, dentry, inode, bh);
+ EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_DIR_INDEX)) {
+ retval = make_indexed_dir(handle, dentry, inode, bh);
+ if (retval == -ENOSPC)
+ brelse(bh);
+ return retval;
+ }
brelse(bh);
}
bh = ext4_append(handle, dir, &block, &retval);
@@ -1528,7 +1532,10 @@
de = (struct ext4_dir_entry_2 *) bh->b_data;
de->inode = 0;
de->rec_len = ext4_rec_len_to_disk(blocksize, blocksize);
- return add_dirent_to_buf(handle, dentry, inode, de, bh);
+ retval = add_dirent_to_buf(handle, dentry, inode, de, bh);
+ if (retval == -ENOSPC)
+ brelse(bh);
+ return retval;
}
/*
@@ -1657,7 +1664,8 @@
if (!de)
goto cleanup;
err = add_dirent_to_buf(handle, dentry, inode, de, bh);
- bh = NULL;
+ if (err != -ENOSPC)
+ bh = NULL;
goto cleanup;
journal_error:
Any comment on the patch below? Ted?
Thanks,
Curt
On Wed, Jul 15, 2009 at 8:53 AM, Curt Wohlgemuth<[email protected]> wrote:
> I've cleaned up the patch to
> ? - fix the comment in ext4_alloc_branch()
> ? - change ext4_add_entry() to remove the gotos and label
>
>
> ? ? ? Signed-off-by: Curt Wohlgemuth <[email protected]>
>
> ---
> diff -Naur orig/fs/ext4/inode.c new/fs/ext4/inode.c
> --- orig/fs/ext4/inode.c ? ? ? ?2009-07-14 11:19:01.000000000 -0700
> +++ new/fs/ext4/inode.c 2009-07-15 08:12:15.000000000 -0700
> @@ -758,8 +758,9 @@
> ? ? ? ? ? ? ? ?BUFFER_TRACE(bh, "call get_create_access");
> ? ? ? ? ? ? ? ?err = ext4_journal_get_create_access(handle, bh);
> ? ? ? ? ? ? ? ?if (err) {
> + ? ? ? ? ? ? ? ? ? ? ? /* Don't brelse(bh) here; it's done in
> + ? ? ? ? ? ? ? ? ? ? ? ?* ext4_journal_forget() below */
> ? ? ? ? ? ? ? ? ? ? ? ?unlock_buffer(bh);
> - ? ? ? ? ? ? ? ? ? ? ? brelse(bh);
> ? ? ? ? ? ? ? ? ? ? ? ?goto failed;
> ? ? ? ? ? ? ? ?}
>
> diff -Naur orig/fs/ext4/namei.c new/fs/ext4/namei.c
> --- orig/fs/ext4/namei.c ? ? ? ?2009-07-14 11:19:46.000000000 -0700
> +++ new/fs/ext4/namei.c 2009-07-15 08:12:09.000000000 -0700
> @@ -1518,8 +1518,12 @@
> ? ? ? ? ? ? ? ? ? ? ? ?return retval;
>
> ? ? ? ? ? ? ? ?if (blocks == 1 && !dx_fallback &&
> - ? ? ? ? ? ? ? ? ? EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_DIR_INDEX))
> - ? ? ? ? ? ? ? ? ? ? ? return make_indexed_dir(handle, dentry, inode, bh);
> + ? ? ? ? ? ? ? ? ? EXT4_HAS_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_DIR_INDEX)) {
> + ? ? ? ? ? ? ? ? ? ? ? retval = make_indexed_dir(handle, dentry, inode, bh);
> + ? ? ? ? ? ? ? ? ? ? ? if (retval == -ENOSPC)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? brelse(bh);
> + ? ? ? ? ? ? ? ? ? ? ? return retval;
> + ? ? ? ? ? ? ? }
> ? ? ? ? ? ? ? ?brelse(bh);
> ? ? ? ?}
> ? ? ? ?bh = ext4_append(handle, dir, &block, &retval);
> @@ -1528,7 +1532,10 @@
> ? ? ? ?de = (struct ext4_dir_entry_2 *) bh->b_data;
> ? ? ? ?de->inode = 0;
> ? ? ? ?de->rec_len = ext4_rec_len_to_disk(blocksize, blocksize);
> - ? ? ? return add_dirent_to_buf(handle, dentry, inode, de, bh);
> + ? ? ? retval = add_dirent_to_buf(handle, dentry, inode, de, bh);
> + ? ? ? if (retval == -ENOSPC)
> + ? ? ? ? ? ? ? brelse(bh);
> + ? ? ? return retval;
> ?}
>
> ?/*
> @@ -1657,7 +1664,8 @@
> ? ? ? ?if (!de)
> ? ? ? ? ? ? ? ?goto cleanup;
> ? ? ? ?err = add_dirent_to_buf(handle, dentry, inode, de, bh);
> - ? ? ? bh = NULL;
> + ? ? ? if (err != -ENOSPC)
> + ? ? ? ? ? ? ? bh = NULL;
> ? ? ? ?goto cleanup;
>
> ?journal_error:
>
On Fri, Jul 17, 2009 at 12:55:49PM -0700, Curt Wohlgemuth wrote:
> Any comment on the patch below? Ted?
oops, sorry for send a reply; I added it to the ext4 patch queue this morning.
- Ted