linux-ext4 - Re: [PATCH] fs/jbd2: t_updates should increase when start_this_handle() failed in jbd2__journal

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130620181215.GD4982@thunk.org>
Date:	Thu, 20 Jun 2013 14:12:15 -0400
From:	Theodore Ts'o <tytso@....edu>
To:	Josef Bacik <jbacik@...ionio.com>
Cc:	Younger Liu <younger.liu@...wei.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-ext4@...r.kernel.org,
	Ocfs2-Devel <ocfs2-devel@....oracle.com>,
	Li Zefan <lizefan@...wei.com>, jack@...e.cz
Subject: Re: [PATCH] fs/jbd2: t_updates should increase when
 start_this_handle() failed in jbd2__journal_restart()

On Thu, Jun 20, 2013 at 01:26:09PM -0400, Josef Bacik wrote:
> I realize it's been a little bit since I've looked at jbd but I'll offer my
> opinion.  Callers of jbd2_journal_restart() may not be the ones who originated
> the handle, so doing what Jan has done with jbd2_journal_start_reserved() isn't
> going to work because all the guy at the top is going to see is an error and
> have no way to tell if his handle is invalid or not.

Yeah, that's what I meant by "it would require changing all of the
callers".

> What I would suggest is getting a unified way to mark that the handle has
> already been cleaned up and can just be free'd.

The problem though is we need to make sure none of the callers don't
try to do anything else with handle besides calling
jbd2_journal_stop().  In particular, we can't allow a call to
jbd2_journal_get_write_access(), jbd2_journal_revoke() to operate on
the handle, because its transaction pointer is (potentially) invalid.

> Then fix jbd2_journal_start_reserved() and jbd2_journal_restart() to
> set that in the handle and make jbd2_journal_stop() just free up the
> handle and reset current->journal_info but not return an error.
> It's important to not return an error from jbd2_journal_stop() so
> that it doesn't invoke the ext4 error handling stuff and you get a
> read only file system when the error may not be read only file
> system worthy.

The handle->h_aborted bit, which is currently not used, does most of
the right thing, modulo the question of the fact that
jbd2_journal_stop() will return an error.  What's important from my
perspective is that the various callers that operate on a handle check
is_handle_aborted() before trying to use the it.  We'll still need to
audit the callers to make sure there isn't some uncommon-taken code
path where ext4_handle_dirty_metadata() gets called after
ext4_journal_restart() has returned an error.

As a FAST paper once opined, "EIO: Error Handling Is Occasionally correct".  :-)

> This way you have a nice clean way of dealing with handle errors that allow you
> to pass back a real error to the caller and the caller can just do its normal
> jbd2_journal_stop() and cleanup and do its own error handling the way it feels.
> This keeps the yucky details of no longer valid handles all internal to jbd2 and
> ext4/ocfs2 don't have to worry about it.  Thanks,

Yes, that could work, although we'll need to check to make sure all of
the code paths that invoke jbd2_journal_restart() handle errors
appropriately, and don't rely on jbd2_journal_stop() returning an
error.  Thanks for your thoughts!

Regards,

	       	   	      	 		     - Ted
--
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