iSCSI fix endieness of digest to be network byte order

Am I dreaming (or just sleep deprived ;), or crc32c swabs the crc one too many times on big-endian machines when CRC_LE_BITS != 1?

If I understand the api design correctly crypto_hash-{digest,final} are supposed to prepare the digest as a byte string in network order rather than a number in cpu order but the internal state can be kept in cpu order. It seems like crc32c_le was intended to be called directly in the past (in the CRC_LE_BITS != 1 case) and therefore it returns the result in network order. The question is if this property should be maintained or not, and if not, then chksum_final() in crypto/crc32c.c should not swab the result

- *(__le32 *)out = ~cpu_to_le32(mctx->crc);
+ *(__le32 *)out = ~mctx->crc;

(plus crc32c_[lb]e implementation for CRC_BE_BITS == 1 should also swab the crc before and after doing their dance)

The other approach is demonstrated in the patch below which always keeps the intermediate crc state in cpu order and swabs it only when returned to the caller as a digest.

Benny

Tomo, looking at libcrc32c, when CRC_LE_BITS == 8
crc32c_le keeps its state in network order unlike CRC_LE_BITS == 1 for every update it will swab the state on BE machine do the calculation and return the swabbed result that chksum_update stores in mctx->crc. Finally chksum_final swabs the result again. Note that since the initial state is ~0 the first swab in crc32c_le (where CRC_LE_BITS == 8) the first swab is a no-op.

If I'm not wrong the correct fix would be

index 60f4680..b8d379b 100644
@@ -163,13 +163,13 @@ static const u32 crc32c_table[256] = {
 u32 __attribute_pure__
 crc32c_le(u32 seed, unsigned char const *data, size_t length)
 {
-       u32 crc = __cpu_to_le32(seed);
+       u32 crc = seed;
 
        while (length--)
                crc =
                    crc32c_table[(crc ^ *data++) & 0xFFL] ^ (crc >> 8);
 
-       return __le32_to_cpu(crc);
+       return crc;
 }
 
 #endif /* CRC_LE_BITS == 8 */

rather than not doing the swab in chksum_final in order to be compatible with the CRC_LE_BITS == 1 case and overall this results in fewer swabs for multiple crypto_hash_updates.

Your patch looks good to me since nobody in the kernel actually uses the crc32c functions directly.

Please resend the patch with a change description + sign-off.

BTW you can get rid of the byteorder.h inclusion with the patch.

Thanks,

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

I wonder why crc32c isn't used directly while crc32 is used directly since they vary by only the polynomial.

Using crc32c directly might enable us to kill net/sctp/crc32c.c and merge lib/crc32.c and lib/libcrc32c?

It's because iSCSI uses it with scatterlists. The crypto layer provides a nice interface for that while using it directly would require the user to do the scatterlist walk.

I'm pretty sure we can kill net/sctp/crc32c.c today by converting it over to use the crypto interface.

Cheers,

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Funny, I looked for crc32c in the lib just the other week and didn't spot it. Need to look harder :)

-vlad