[RFC] mess in jbd2_block_tag_csum_verify()

[RFC] mess in jbd2_block_tag_csum_verify()

[Al Viro]

You have
	calculated = jbd2_chksum(j, calculated, buf, j->j_blocksize);
	provided = be32_to_cpu(tag->t_checksum);

	return provided == cpu_to_be32(calculated);

in there, which makes no sense whatsoever.  First of all, you are
converting big-endian to native, then another native to big-endian
and compare results.  The bogosity aside, it's equivalent to simply
comparing tag->t_checksum with calculated - cpu_to_be32() is the
same mapping as be32_to_cpu() on all architectures and it's a one-to-one
mapping, at that.

Bogosity, of course, is that tag->t_checksum is apparently big-endian
and definitely a 16bit value.  How in hell is that check going to
yield true?  Note that you are asking for 16 bits out of crc32c result
to be zero, _NOT_ to be ignored.

Producer of that value shoves lower 16 bits of cpu_to_be32(crc) into the
on-disk structure.  Also a bloody bad idea, since the values on little-endian
and big-endian hosts will be different; move the disk from one box to
another and watch the mismatches...

What the hell is going on there?

Re: [RFC] mess in jbd2_block_tag_csum_verify()

[Andreas Dilger]

On 2013-05-08, at 9:51, Al Viro <viro@ZenIV.linux.org.uk> wrote:

> You have
>    calculated = jbd2_chksum(j, calculated, buf, j->j_blocksize);
>    provided = be32_to_cpu(tag->t_checksum);
> 
>    return provided == cpu_to_be32(calculated);
> 
> in there, which makes no sense whatsoever.  First of all, you are
> converting big-endian to native, then another native to big-endian
> and compare results.  The bogosity aside, it's equivalent to simply
> comparing tag->t_checksum with calculated - cpu_to_be32() is the
> same mapping as be32_to_cpu() on all architectures and it's a one-to-one
> mapping, at that.

I agree this is a bit of extra swabbing that isn't needed. 

> Bogosity, of course, is that tag->t_checksum is apparently big-endian
> and definitely a 16bit value.  How in hell is that check going to
> yield true?  Note that you are asking for 16 bits out of crc32c result
> to be zero, _NOT_ to be ignored.

I think you're mixing up the jbd2 transaction block checksum, which actually has up to 128 bits of space (in case we want to move to a better checksum in the future) with the ext4 group descriptor checksum (which is only 16 bits for compatibility reasons). 

Problem solved?

Cheers, Andreas

> Producer of that value shoves lower 16 bits of cpu_to_be32(crc) into the
> on-disk structure.  Also a bloody bad idea, since the values on little-endian
> and big-endian hosts will be different; move the disk from one box to
> another and watch the mismatches...
> 
> What the hell is going on there?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] mess in jbd2_block_tag_csum_verify()

[Andreas Dilger]

On 2013-05-08, at 10:04, Andreas Dilger <adilger@dilger.ca> wrote:

> On 2013-05-08, at 9:51, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
>> You have
>>   calculated = jbd2_chksum(j, calculated, buf, j->j_blocksize);
>>   provided = be32_to_cpu(tag->t_checksum);
>> 
>>   return provided == cpu_to_be32(calculated);
>> 
>> in there, which makes no sense whatsoever.  First of all, you are
>> converting big-endian to native, then another native to big-endian
>> and compare results.  The bogosity aside, it's equivalent to simply
>> comparing tag->t_checksum with calculated - cpu_to_be32() is the
>> same mapping as be32_to_cpu() on all architectures and it's a one-to-one
>> mapping, at that.
> 
> I agree this is a bit of extra swabbing that isn't needed. 
> 
>> Bogosity, of course, is that tag->t_checksum is apparently big-endian
>> and definitely a 16bit value.  How in hell is that check going to
>> yield true?  Note that you are asking for 16 bits out of crc32c result
>> to be zero, _NOT_ to be ignored.
> 
> I think you're mixing up the jbd2 transaction block checksum, which actually has up to 128 bits of space (in case we want to move to a better checksum in the future) with the ext4 group descriptor checksum (which is only 16 bits for compatibility reasons). 
> 
> Problem solved?

Doh, my bad. You are looking at t_checksum and not h_chksum. The t_checksum is definitely a 16-bit value, and I totally agree this code seems bogus. 

be32_to_cpu() on a 16-bit value is bad, as is the lack of truncation to 16 bits for comparison. I'm not sure how this code could work on either BE or LE systems. 

Thanks for finding this. 

Cheers, Andreas

>> Producer of that value shoves lower 16 bits of cpu_to_be32(crc) into the
>> on-disk structure.  Also a bloody bad idea, since the values on little-endian
>> and big-endian hosts will be different; move the disk from one box to
>> another and watch the mismatches...
>> 
>> What the hell is going on there?
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] mess in jbd2_block_tag_csum_verify()

[Darrick J. Wong]

On Wed, May 08, 2013 at 10:04:31AM -0600, Andreas Dilger wrote:
> On 2013-05-08, at 9:51, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> > You have
> >    calculated = jbd2_chksum(j, calculated, buf, j->j_blocksize);
> >    provided = be32_to_cpu(tag->t_checksum);
> > 
> >    return provided == cpu_to_be32(calculated);
> > 
> > in there, which makes no sense whatsoever.  First of all, you are
> > converting big-endian to native, then another native to big-endian
> > and compare results.  The bogosity aside, it's equivalent to simply
> > comparing tag->t_checksum with calculated - cpu_to_be32() is the
> > same mapping as be32_to_cpu() on all architectures and it's a one-to-one
> > mapping, at that.
> 
> I agree this is a bit of extra swabbing that isn't needed. 
> 
> > Bogosity, of course, is that tag->t_checksum is apparently big-endian
> > and definitely a 16bit value.  How in hell is that check going to
> > yield true?  Note that you are asking for 16 bits out of crc32c result
> > to be zero, _NOT_ to be ignored.

Yes, that is effing awful.  It looks like I missed that detail at some point,
probably when tweaking the endian handling or something.

> I think you're mixing up the jbd2 transaction block checksum, which actually
> has up to 128 bits of space (in case we want to move to a better checksum in
> the future) with the ext4 group descriptor checksum (which is only 16 bits
> for compatibility reasons). 

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.

--D
> 
> Cheers, Andreas
> 
> > Producer of that value shoves lower 16 bits of cpu_to_be32(crc) into the
> > on-disk structure.  Also a bloody bad idea, since the values on little-endian
> > and big-endian hosts will be different; move the disk from one box to
> > another and watch the mismatches...
> > 
> > What the hell is going on there?
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] mess in jbd2_block_tag_csum_verify()

[Al Viro]

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
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)
 {
 	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()

[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.