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: <20070302090616.GO6573@schatzie.adilger.int>
Date:	Fri, 2 Mar 2007 17:06:16 +0800
From:	Andreas Dilger <adilger@...sterfs.com>
To:	Dmitriy Monakhov <dmonakhov@...ru>
Cc:	linux-ext4@...r.kernel.org
Subject: Re: [PATCH][RFC] ext3: Handle ext[34]_journal_stop() failure

On Feb 28, 2007  19:46 +0300, Dmitriy Monakhov wrote:
> Where are many places where xxxx_journal_stop() return code wasn't
> checked. Off cause xxxx_journal_stop() failed very rarely (and usually
> with fatal consequences), but this does'n meen it should not be checked.

I fully agree with this.  Otherwise an application will believe that the
operation has succeeded when in fact the filesystem has failed.  It would
notice this on the next filesystem operation, but that might be too late
for e.g. an MTA that just confirmed acceptance of an email.

There are also several places in the code where we don't check error
returns from ext3_journal_get_write_access().  I have a patch that I
was starting to work on but never managed to finish/test.  I'd be
happy if you had time to finish it off.

================= ext3-check-jbd-errors-2.6.20.patch =================
--- ./fs/ext3/namei.c.orig	2007-02-08 08:10:20.000000000 +0800
+++ ./fs/ext3/namei.c	2007-02-17 07:56:16.000000000 +0800
@@ -59,9 +59,14 @@ static struct buffer_head *ext3_append(h
 	*block = inode->i_size >> inode->i_sb->s_blocksize_bits;
 
 	if ((bh = ext3_bread(handle, inode, *block, 1, err))) {
-		inode->i_size += inode->i_sb->s_blocksize;
-		EXT3_I(inode)->i_disksize = inode->i_size;
-		ext3_journal_get_write_access(handle,bh);
+		*err = ext3_journal_get_write_access(handle,bh);
+		if (err) {
+			brelse(bh);
+			bh = NULL;
+		} else {
+			inode->i_size += inode->i_sb->s_blocksize;
+			EXT3_I(inode)->i_disksize = inode->i_size;
+		}
 	}
 	return bh;
 }
@@ -1597,8 +1602,12 @@ static int ext3_delete_entry (handle_t *
 		if (!ext3_check_dir_entry("ext3_delete_entry", dir, de, bh, i))
 			return -EIO;
 		if (de == de_del)  {
+			int err;
+
 			BUFFER_TRACE(bh, "get_write_access");
-			ext3_journal_get_write_access(handle, bh);
+			err = ext3_journal_get_write_access(handle, bh);
+			if (err)
+				return err;
 			if (pde)
 				pde->rec_len =
 					cpu_to_le16(le16_to_cpu(pde->rec_len) +
@@ -1758,7 +1767,13 @@ retry:
 		goto out_stop;
 	}
 	BUFFER_TRACE(dir_block, "get_write_access");
-	ext3_journal_get_write_access(handle, dir_block);
+	err = ext3_journal_get_write_access(handle, dir_block);
+	if (err) {
+		drop_nlink(inode); /* is this nlink == 0? */
+		ext3_mark_inode_dirty(handle, inode);
+		iput (inode);
+		goto out_stop;
+	}
 	de = (struct ext3_dir_entry_2 *) dir_block->b_data;
 	de->inode = cpu_to_le32(inode->i_ino);
 	de->name_len = 1;
@@ -2262,6 +2277,11 @@ static int ext3_rename (struct inode * o
 		if (!new_inode) {
 			brelse (new_bh);
 			new_bh = NULL;
+		} else {
+			BUFFER_TRACE(new_bh, "get write access");
+			retval = ext3_journal_get_write_access(handle, new_bh);
+			if (retval)
+				goto end_rename;
 		}
 	}
 	if (S_ISDIR(old_inode->i_mode)) {
@@ -2280,14 +2300,16 @@ static int ext3_rename (struct inode * o
 		if (!new_inode && new_dir!=old_dir &&
 				new_dir->i_nlink >= EXT3_LINK_MAX)
 			goto end_rename;
+		BUFFER_TRACE(dir_bh, "get_write_access");
+		retval = ext3_journal_get_write_access(handle, dir_bh);
+		if (retval)
+			goto end_rename;
 	}
 	if (!new_bh) {
 		retval = ext3_add_entry (handle, new_dentry, old_inode);
 		if (retval)
 			goto end_rename;
 	} else {
-		BUFFER_TRACE(new_bh, "get write access");
-		ext3_journal_get_write_access(handle, new_bh);
 		new_de->inode = cpu_to_le32(old_inode->i_ino);
 		if (EXT3_HAS_INCOMPAT_FEATURE(new_dir->i_sb,
 					      EXT3_FEATURE_INCOMPAT_FILETYPE))
@@ -2341,8 +2363,6 @@ static int ext3_rename (struct inode * o
 	old_dir->i_ctime = old_dir->i_mtime = CURRENT_TIME_SEC;
 	ext3_update_dx_flag(old_dir);
 	if (dir_bh) {
-		BUFFER_TRACE(dir_bh, "get_write_access");
-		ext3_journal_get_write_access(handle, dir_bh);
 		PARENT_INO(dir_bh->b_data) = cpu_to_le32(new_dir->i_ino);
 		BUFFER_TRACE(dir_bh, "call ext3_journal_dirty_metadata");
 		ext3_journal_dirty_metadata(handle, dir_bh);
==============================================================================

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

-
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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ