[LU-15591] ext2fs_open2() missing goto

• Status: Closed
• Project: Lustre
• Type: Bug
• Priority: Minor
• Reporter: John Hammond
• Assignee: WC Triage
• Resolution: Not a Bug
• Severity: 3

Description

In e2fsprogs ext2fs_open2() there is a missing goto cleanup after retval = EXT2_ET_SB_CSUM_INVALID. And probably after retval = EXT2_ET_UNKNOWN_CSUM;.

        if (!(fs->flags & EXT2_FLAG_IGNORE_CSUM_ERRORS)) {
                retval = 0;
                if (!ext2fs_verify_csum_type(fs, fs->super))
                        retval = EXT2_ET_UNKNOWN_CSUM;
                if (!ext2fs_superblock_csum_verify(fs, fs->super)) {
                        if (csum_retries++ < 3)
                                goto retry;
                        retval = EXT2_ET_SB_CSUM_INVALID;
                }
        }

Also I find it strange that we check the super block magic after verifying the checksum.

Issue Links

• is related to: LU-15590 e2fsprogs direct IO raw_read_blk() is...

Comments

[Andreas Dilger]

I was looking at the history of this code. There used to be an "if (retval) goto cleanup;" here, but it was moved:

commit f92c600c09bbda482c6c4a37602db4f0db864a9e
Author:     Darrick J. Wong <[e-mail]>
AuthorDate: Mon Sep 8 16:11:49 2014 -0700
Commit:     Theodore Ts'o <[e-mail]>
CommitDate: Thu Sep 11 12:40:54 2014 -0400

    libext2fs: report bad magic over bad sb checksum
    
    We don't want ext2fs_open2() to report bad sb checksum on something
    that's not even an ext* superblock.  This apparently happens pretty
    easily if we try to open an XFS filesystem.  Thus, make it so that a
    bad magic number code always trumps the sb checksum error code.
    
    Signed-off-by: Darrick J. Wong <[e-mail]>
    Signed-off-by: Theodore Ts'o <[e-mail]>

diff --git a/lib/ext2fs/openfs.c b/lib/ext2fs/openfs.c
index 00320f34..1d6f147c 100644
--- a/lib/ext2fs/openfs.c
+++ b/lib/ext2fs/openfs.c
@@ -221,8 +221,6 @@ errcode_t ext2fs_open2(const char *name, const char *io_opti
ons,
                        retval = EXT2_ET_UNKNOWN_CSUM;
                if (!ext2fs_superblock_csum_verify(fs, fs->super))
                        retval = EXT2_ET_SB_CSUM_INVALID;
-               if (retval)
-                       goto cleanup;
        }
 
 #ifdef WORDS_BIGENDIAN
@@ -235,10 +233,11 @@ errcode_t ext2fs_open2(const char *name, const char *io_options,
        }
 #endif
 
-       if (fs->super->s_magic != EXT2_SUPER_MAGIC) {
+       if (fs->super->s_magic != EXT2_SUPER_MAGIC)
                retval = EXT2_ET_BAD_MAGIC;
+       if (retval)
                goto cleanup;
-       }
+
        if (fs->super->s_rev_level > EXT2_LIB_CURRENT_REV) {
                retval = EXT2_ET_REV_TOO_HIGH;
                goto cleanup;

It isn't clear why the whole checksum validation block wasn't moved below the superblock magic number check. It may be because the checksum needs to be computed on the un-swabbed/little-endian superblock, which happens immediately after the checksum validation?

Is this actually a functional problem, or just an anomaly that you found while looking into this code? Looking at the code it appears it will correctly handle a non-zero checksum retval (i.e. it won't be overwritten by 0), just not immediately. I'm just wondering if a fix is needed for this issue for proper O_DIRECT usage, or if I should tag e2fsprogs-1.46.2.wc5 with only the LU-15590 fix?

[John Hammond]

Maybe it's more confusing that a bug. Looking at the traces, it tried to read a good checksum three times then it returned EXT2_ET_BAD_MAGIC.