Re: [RFC] mess in jbd2_block_tag_csum_verify()

Re: [RFC] mess in jbd2_block_tag_csum_verify()

[Darrick J. Wong]

[add ext4 list to cc]

On Wed, May 08, 2013 at 06:07:52PM +0100, Al Viro wrote:
> On Wed, May 08, 2013 at 09:45:08AM -0700, Darrick J. Wong wrote:
> 
> > The journal block tag checksum is 16 bits long.
> > 
> > > Problem solved?
> > 
> > I wish.  Now we know what I'll be patching today...
> > 
> > Anyhow, thank you for catching this.
> 
> make C=2 fs/jbd2/ is a good idea (with recent enough sparse); to get rid of
> false positives re endianness, apply the patch below - that'll reduce the
> warnings to
> fs/jbd2/commit.c:358:25: warning: incorrect type in assignment (different base types)
> fs/jbd2/commit.c:358:25:    expected restricted __be16 [usertype] t_checksum
> fs/jbd2/commit.c:358:25:    got restricted __be32 [usertype] <noident>
> fs/jbd2/recovery.c:411:20: warning: cast to restricted __be32
> fs/jbd2/recovery.c:411:20: warning: cast from restricted __be16
> fs/jbd2/recovery.c:411:20: warning: cast to restricted __be32
> fs/jbd2/recovery.c:411:20: warning: cast from restricted __be16
> fs/jbd2/recovery.c:411:20: warning: cast to restricted __be32
> fs/jbd2/recovery.c:411:20: warning: cast from restricted __be16
> fs/jbd2/recovery.c:411:20: warning: cast to restricted __be32
> fs/jbd2/recovery.c:411:20: warning: cast from restricted __be16
> fs/jbd2/recovery.c:411:20: warning: cast to restricted __be32
> fs/jbd2/recovery.c:411:20: warning: cast from restricted __be16
> fs/jbd2/recovery.c:411:20: warning: cast to restricted __be32
> fs/jbd2/recovery.c:411:20: warning: cast from restricted __be16
> fs/jbd2/recovery.c:411:18: warning: incorrect type in assignment (different base types)
> fs/jbd2/recovery.c:411:18:    expected restricted __be32 [usertype] provided
> fs/jbd2/recovery.c:411:18:    got unsigned int

Interesting... I have something that calls itself sparse 0.4.3 and I don't see
any endianness warnings at all.  Is endian checking new for 0.4.4?  Or maybe I
simply don't have it configured correctly?

> which is where that problem lives (writing and checking tag->t_checksum resp.)
> FWIW, I think that the contents of that field in all existing fs images should
> be treated as junk - current kernels will puke if they happen to try and
> check them anyway.  Perhaps the right fix would be to store cpu_to_be16(csum)
> on the write side and do return cpu_to_be16(calculated) == tag->t_checksum
> on the check side - that would be independent from host endianness, as well
> as ignoring the right bits...
> 
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 0f53946..f75fbd7 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -339,7 +339,7 @@ static void jbd2_descr_block_csum_set(journal_t *j,
>  }
>  
>  static void jbd2_block_tag_csum_set(journal_t *j, journal_block_tag_t *tag,
> -				    struct buffer_head *bh, __u32 sequence)
> +				    struct buffer_head *bh, __be32 sequence)

Hmm, I thought "__u" implied native endian.  Any reason why we need to force
the parameter to "__be"?  I'd rather leave the call sites alone, but I don't
have any strong opinions either way.

Anyway, patches to fix the kernel and e2fsprogs are on their way.

--D

>  {
>  	struct page *page = bh->b_page;
>  	__u8 *addr;
> @@ -348,7 +348,6 @@ static void jbd2_block_tag_csum_set(journal_t *j, journal_block_tag_t *tag,
>  	if (!JBD2_HAS_INCOMPAT_FEATURE(j, JBD2_FEATURE_INCOMPAT_CSUM_V2))
>  		return;
>  
> -	sequence = cpu_to_be32(sequence);
>  	addr = kmap_atomic(page);
>  	csum = jbd2_chksum(j, j->j_csum_seed, (__u8 *)&sequence,
>  			  sizeof(sequence));
> @@ -695,7 +694,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  		write_tag_block(tag_bytes, tag, jh2bh(jh)->b_blocknr);
>  		tag->t_flags = cpu_to_be16(tag_flag);
>  		jbd2_block_tag_csum_set(journal, tag, jh2bh(new_jh),
> -					commit_transaction->t_tid);
> +					cpu_to_be32(commit_transaction->t_tid));
>  		tagp += tag_bytes;
>  		space_left -= tag_bytes;
>  
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index 626846b..cbbea8c 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -178,7 +178,8 @@ static int jbd2_descr_block_csum_verify(journal_t *j,
>  					void *buf)
>  {
>  	struct jbd2_journal_block_tail *tail;
> -	__u32 provided, calculated;
> +	__be32 provided;
> +	__u32 calculated;
>  
>  	if (!JBD2_HAS_INCOMPAT_FEATURE(j, JBD2_FEATURE_INCOMPAT_CSUM_V2))
>  		return 1;
> @@ -190,8 +191,7 @@ static int jbd2_descr_block_csum_verify(journal_t *j,
>  	calculated = jbd2_chksum(j, j->j_csum_seed, buf, j->j_blocksize);
>  	tail->t_checksum = provided;
>  
> -	provided = be32_to_cpu(provided);
> -	return provided == calculated;
> +	return provided == cpu_to_be32(calculated);
>  }
>  
>  /*
> @@ -381,7 +381,8 @@ static int calc_chksums(journal_t *journal, struct buffer_head *bh,
>  static int jbd2_commit_block_csum_verify(journal_t *j, void *buf)
>  {
>  	struct commit_header *h;
> -	__u32 provided, calculated;
> +	__be32 provided;
> +	__u32 calculated;
>  
>  	if (!JBD2_HAS_INCOMPAT_FEATURE(j, JBD2_FEATURE_INCOMPAT_CSUM_V2))
>  		return 1;
> @@ -392,19 +393,18 @@ static int jbd2_commit_block_csum_verify(journal_t *j, void *buf)
>  	calculated = jbd2_chksum(j, j->j_csum_seed, buf, j->j_blocksize);
>  	h->h_chksum[0] = provided;
>  
> -	provided = be32_to_cpu(provided);
> -	return provided == calculated;
> +	return provided == cpu_to_be32(calculated);
>  }
>  
>  static int jbd2_block_tag_csum_verify(journal_t *j, journal_block_tag_t *tag,
> -				      void *buf, __u32 sequence)
> +				      void *buf, __be32 sequence)
>  {
> -	__u32 provided, calculated;
> +	__be32 provided;
> +	__u32 calculated;
>  
>  	if (!JBD2_HAS_INCOMPAT_FEATURE(j, JBD2_FEATURE_INCOMPAT_CSUM_V2))
>  		return 1;
>  
> -	sequence = cpu_to_be32(sequence);
>  	calculated = jbd2_chksum(j, j->j_csum_seed, (__u8 *)&sequence,
>  				 sizeof(sequence));
>  	calculated = jbd2_chksum(j, calculated, buf, j->j_blocksize);
> @@ -592,7 +592,7 @@ static int do_one_pass(journal_t *journal,
>  					/* Look for block corruption */
>  					if (!jbd2_block_tag_csum_verify(
>  						journal, tag, obh->b_data,
> -						be32_to_cpu(tmp->h_sequence))) {
> +						tmp->h_sequence)) {
>  						brelse(obh);
>  						success = -EIO;
>  						printk(KERN_ERR "JBD: Invalid "
> @@ -809,7 +809,8 @@ static int jbd2_revoke_block_csum_verify(journal_t *j,
>  					 void *buf)
>  {
>  	struct jbd2_journal_revoke_tail *tail;
> -	__u32 provided, calculated;
> +	__be32 provided;
> +	__u32 calculated;
>  
>  	if (!JBD2_HAS_INCOMPAT_FEATURE(j, JBD2_FEATURE_INCOMPAT_CSUM_V2))
>  		return 1;
> @@ -821,8 +822,7 @@ static int jbd2_revoke_block_csum_verify(journal_t *j,
>  	calculated = jbd2_chksum(j, j->j_csum_seed, buf, j->j_blocksize);
>  	tail->r_checksum = provided;
>  
> -	provided = be32_to_cpu(provided);
> -	return provided == calculated;
> +	return provided == cpu_to_be32(calculated);
>  }
>  
>  /* Scan a revoke record, marking all blocks mentioned as revoked. */

Re: [RFC] mess in jbd2_block_tag_csum_verify()

[Al Viro]

On Wed, May 08, 2013 at 02:55:41PM -0700, Darrick J. Wong wrote:

> Interesting... I have something that calls itself sparse 0.4.3 and I don't see
> any endianness warnings at all.  Is endian checking new for 0.4.4?  Or maybe I
> simply don't have it configured correctly?

make C=2 CF=-D__CHECK_ENDIAN__ fs/jbd2/, actually.  Not sure if 0.4.3 will
be recent enough - the endianness checks per se are not a problem, but it
needs to recognize __builting_bswap32() as builtin instead of spewing
an error on undefined identifier...

> Hmm, I thought "__u" implied native endian.  Any reason why we need to force
> the parameter to "__be"?  I'd rather leave the call sites alone, but I don't
> have any strong opinions either way.

*shrug*

We can do
f(..., __u32 n,...)
{
	__be32 n1 = cpu_to_be32(n);
	...
}

or we can simply pass the result of conversion to the function and be
done with that.  IMO the latter is easier, especially when there is only
one caller to start with.  Per se passing around host-endian values
is neither better nor worse than passing big-endian ones - depends on which
variant ends up being more readable.

What we really shouldn't do is use of the same variable to hold host-endian
value at one point and big-endian at another; if compiler sees
	__be32 n1 = cpu_to_be32(n);
notices that this is the last use of n and reuses the same memory location -
let it, it's a valid optimization, but doing it manually and saying something
like
	n = cpu_to_be32(n);
is a Damn Bad Idea(tm).  Because sooner or later you'll lose track and end
up with e.g. a variable containing a host-endian some instruction if you take
one path leading to it and big-endian if you take another.  I've seen enough
bugs of that kind; it's a very bad habit.  Sane rules for endianness
stuff are very simple:
	* treat big-endian, little-endian and host-endian as separate types
	* use whatever makes for more readable code
	* do arithmetics only on host-endian
	* do bitwise operations only on the values of the same type and
treat result of such operation as value of the same type
	* use explicit conversions; the fact that cpu_to_le32 and le32_to_cpu
shuffle the bits in the same way is an accident.  The former should be
used only on host-endian, the latter - only on __le32.  Sure, they'll produce
the same instructions; let compiler take care of that.
	* don't mix these types; yes, __be32 and __u32 have the same size and
compiler might very well decide to use the same memory location for values
of those types; let it take care of that.

sparse can enforce that discipline; if you really need to violate it (e.g.
you want to use a big-endian value as search key in binary tree) add explicit
(__force __u32) in such places.  gcc won't care (__be32 ends up typedefed to
unsigned int as far as gcc is aware, so the cast is simply a no-op), sparse
will know that you really mean it and readers of the code will have a visible
warning that something subtle is going on.  You want to have as few places of
that kind as possible, of course.

You can add more types of that sort, BTW - grep for __bitwise in include/
or fs/*/ for examples.