Re: Aarch64 EXT4FS inode checksum failures - seems to be weak memory ordering issues

On Thu, Jan 07, 2021 at 10:48:05PM +0100, Arnd Bergmann wrote:

On Thu, Jan 7, 2021 at 5:27 PM Theodore Ts'o [off-list ref] wrote:

quoted

On Thu, Jan 07, 2021 at 01:37:47PM +0000, Russell King - ARM Linux admin wrote:

quoted

quoted

The gcc bugzilla mentions backports into gcc-linaro, but I do not see
them in my git history.

So, do we raise the minimum gcc version for the kernel as a whole to 5.1
or just for aarch64?

Russell, Arnd, thanks so much for tracking down the root cause of the
bug!

There is one more thing that I wondered about when looking through
the ext4 code: Should it just call the crc32c_le() function directly
instead of going through the crypto layer? It seems that with Ard's
rework from 2018, that can just call the underlying architecture specific
implementation anyway.

Yes, I've been wondering about that too. To me, it looks like the
ext4 code performs a layering violation by going "under the covers"
- there are accessor functions to set the CRC and retrieve it. ext4
instead just makes the assumption that the CRC value is stored after
struct shash_desc. Especially as the crypto/crc32c code references
the value using:

	struct chksum_desc_ctx *ctx = shash_desc_ctx(desc);

Not even crypto drivers are allowed to assume that desc+1 is where
the CRC is stored.

However, struct shash_desc is already 128 bytes in size on aarch64,
and the proper way of doing it via SHASH_DESC_ON_STACK() is overkill,
being strangely 2 * sizeof(struct shash_desc) + 360 (which looks like
another bug to me!)

#define HASH_MAX_DESCSIZE       (sizeof(struct shash_desc) + 360)
                                 ^^^^^^^^^^^^^^^^^^^^^^^^^
#define SHASH_DESC_ON_STACK(shash, ctx)                           \
        char __##shash##_desc[sizeof(struct shash_desc) +         \
                              ^^^^^^^^^^^^^^^^^^^^^^^^^
                HASH_MAX_DESCSIZE] CRYPTO_MINALIGN_ATTR; \
        struct shash_desc *shash = (struct shash_desc *)__##shash##_desc

So, I agree with you wrt crc32c_le(), especially as it would be more
efficient, and as the use of crc32c is already hard coded in the ext4
code - not only with crypto_alloc_shash("crc32c", 0, 0) but also with
the fixed-size structure in ext4_chksum().

However, it's ultimately up to the ext4 maintainers to decide.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!