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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 27 Nov 2015 09:45:53 +0100
From:	Jan Kara <jack@...e.cz>
To:	Junxiao Bi <junxiao.bi@...cle.com>
Cc:	linux-ext4@...r.kernel.org, ocfs2-devel@....oracle.com,
	jack@...e.com
Subject: Re: [PATCH] jbd2: fix null committed data return in undo_access

On Fri 27-11-15 09:57:21, Junxiao Bi wrote:
> commit de92c8c ("jbd2: speedup jbd2_journal_get_[write|undo]_access()")
> introduced jbd2_write_access_granted() to improve write|undo_access
> speed, but missed to check the status of b_committed_data which caused
> a kernel panic on ocfs2.

Thanks for debugging this! Just a few minor nits:

1) Please use at least 12 characters of the commit hash - i.e. de92c8caf16c
- that is pretty much guaranteed to stay unique for the lifetime of Linux.
Using just 7 characters will become non-unique with high probability rather
soon.

2) Send this patch to Ted Tso as well as he is the one merging jbd2
patches.

3) The fact that OCFS2 hit this problem means that it is mixing
jbd2_journal_get_write_access() and jbd2_journal_get_undo_access() for the
same buffer. That is slightly suspicious to me. Not that it would be
outright bug but you have to account for the fact that someone can be
modifying the buffer data while you are getting undo access...

4) Some other comments below in the code.

> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index 6b8338e..4750bda 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -1009,7 +1009,8 @@ out:
>  }
>  
>  /* Fast check whether buffer is already attached to the required transaction */
> -static bool jbd2_write_access_granted(handle_t *handle, struct buffer_head *bh)
> +static bool jbd2_write_access_granted(handle_t *handle, struct buffer_head *bh,
> +							int undo)
							^^^
Use 'bool' please. The function is already using bools so it is good for
consistency.

>  {
>  	struct journal_head *jh;
>  	bool ret = false;
> @@ -1036,6 +1037,8 @@ static bool jbd2_write_access_granted(handle_t *handle, struct buffer_head *bh)
>  	jh = READ_ONCE(bh->b_private);
>  	if (!jh)
>  		goto out;

Short comment here please. Like:

	/* For undo access buffer must have data copied */

> +	if (undo && !jh->b_committed_data)
> +		goto out;
>  	if (jh->b_transaction != handle->h_transaction &&
>  	    jh->b_next_transaction != handle->h_transaction)
>  		goto out;
> @@ -1073,7 +1076,7 @@ int jbd2_journal_get_write_access(handle_t *handle, struct buffer_head *bh)
>  	struct journal_head *jh;
>  	int rc;
>  
> -	if (jbd2_write_access_granted(handle, bh))
> +	if (jbd2_write_access_granted(handle, bh, 0))

Here 'false' instead of 0.

>  	JBUFFER_TRACE(jh, "entry");
> -	if (jbd2_write_access_granted(handle, bh))
> +	if (jbd2_write_access_granted(handle, bh, 1))

Here 'true' instead of 1.

Thanks.

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR
--
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