linux-kernel - Re: [EXT4 set 5][PATCH 1/1] expand inode i_extra_isize to support features in larger inode

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-Id: <1184631876.3836.8.camel@localhost.localdomain>
Date:	Mon, 16 Jul 2007 17:24:36 -0700
From:	Mingming Cao <cmm@...ibm.com>
To:	Andreas Dilger <adilger@...sterfs.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-ext4@...r.kernel.org, Andy Whitcroft <apw@...dowen.org>
Subject: Re: [EXT4 set 5][PATCH 1/1] expand inode i_extra_isize to support
	features in larger inode

On Mon, 2007-07-16 at 18:06 -0600, Andreas Dilger wrote:
> On Jul 16, 2007  16:52 -0700, Mingming Cao wrote:
> > I am not sure why we need GFP_KERNEL flag here. I think we should use
> > GFP_NOFS instead. The following patch use the GFP_NOFS flag, as well as
> > fixing memory leak issue introduced by the ext4 expand inode extra isize
> > patch.
> > 
> > Fixing memory allocation issue with expand inode extra isize patch.
> > 
> > - use GFP_NOFS instead of GFP_KERNEL flag for memory allocation
> > - use kzalloc instead of kmalloc
> 
> This doesn't need kzalloc() for buffer and b_entry_name, since they are
> immediately overwritten by memcpy().
> 
ok.
> > - fix memory leak in the success case, at the end of while loop.
> >  			goto cleanup;
> > @@ -1302,7 +1302,15 @@ retry:
> >  		error = ext4_xattr_block_set(handle, inode, &i, bs);
> >  		if (error)
> >  			goto cleanup;
> > +		kfree(b_entry_name);
> > +		kfree(buffer);
> > +		brelse(is->iloc.bh);
> > +		kfree(is);
> > +		kfree(bs);
> > +		brelse(bh);
> >  	}
> > +	up_write(&EXT4_I(inode)->xattr_sem);
> > +        return 0;
> >  
> >  cleanup:
> >  	kfree(b_entry_name);
> 
> I don't think you should have brelse(bh) inside the loop, since it is
> allocated before the loop starts.
> 
Ahh right. thanks.

Updated fix.

Fixing memory allocation issue with expand inode extra isize patch.

- use GFP_NOFS instead of GFP_KERNEL to prevent fs reenter
- fix memory leak in the success case, at the end of while loop.


Signed-off-by: Mingming Cao <cmm@...ibm.com>
---
 fs/ext4/xattr.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

Index: linux-2.6.22/fs/ext4/xattr.c
===================================================================
--- linux-2.6.22.orig/fs/ext4/xattr.c	2007-07-16 17:21:14.000000000 -0700
+++ linux-2.6.22/fs/ext4/xattr.c	2007-07-16 17:22:25.000000000 -0700
@@ -1204,8 +1204,8 @@ retry:
 		unsigned int shift_bytes; /* No. of bytes to shift EAs by? */
 		unsigned int min_total_size = ~0U;
 
-		is = kzalloc(sizeof(struct ext4_xattr_ibody_find), GFP_KERNEL);
-		bs = kzalloc(sizeof(struct ext4_xattr_block_find), GFP_KERNEL);
+		is = kzalloc(sizeof(struct ext4_xattr_ibody_find), GFP_NOFS);
+		bs = kzalloc(sizeof(struct ext4_xattr_block_find), GFP_NOFS);
 		if (!is || !bs) {
 			error = -ENOMEM;
 			goto cleanup;
@@ -1251,8 +1251,8 @@ retry:
 		size = le32_to_cpu(entry->e_value_size);
 		entry_size = EXT4_XATTR_LEN(entry->e_name_len);
 		i.name_index = entry->e_name_index,
-		buffer = kmalloc(EXT4_XATTR_SIZE(size), GFP_KERNEL);
-		b_entry_name = kmalloc(entry->e_name_len + 1, GFP_KERNEL);
+		buffer = kmalloc(EXT4_XATTR_SIZE(size), GFP_NOFS);
+		b_entry_name = kmalloc(entry->e_name_len + 1, GFP_NOFS);
 		if (!buffer || !b_entry_name) {
 			error = -ENOMEM;
 			goto cleanup;
@@ -1302,7 +1302,15 @@ retry:
 		error = ext4_xattr_block_set(handle, inode, &i, bs);
 		if (error)
 			goto cleanup;
+		kfree(b_entry_name);
+		kfree(buffer);
+		brelse(is->iloc.bh);
+		kfree(is);
+		kfree(bs);
 	}
+	brelse(bh);
+	up_write(&EXT4_I(inode)->xattr_sem);
+       return 0;
 
 cleanup:
 	kfree(b_entry_name);


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/