linux-ext4 - Re: [PATCH] ext4: handle read only external journal device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 17 Jul 2020 02:10:18 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Lukas Czerner <lczerner@...hat.com>
Cc:     linux-ext4@...r.kernel.org, tytso@....edu
Subject: Re: [PATCH] ext4: handle read only external journal device

On Jul 16, 2020, at 12:39 PM, Lukas Czerner <lczerner@...hat.com> wrote:
> 
> Ext4 uses blkdev_get_by_dev() to get the block_device for journal device
> which does check to see if the read-only block device was opened
> read-only.
> 
> As a result ext4 will hapily proceed mounting the file system with
> external journal on read-only device. This is bad as we would not be
> able to use the journal leading to errors later on.
> 
> Instead of simply failing to mount file system in this case, treat it in
> a similar way we treat internal journal on read-only device. Allow to
> mount with -o noload in read-only mode.
> 
> This can be reproduced easily like this:
> 
> mke2fs -F -O journal_dev $JOURNAL_DEV 100M
> mkfs.$FSTYPE -F -J device=$JOURNAL_DEV $FS_DEV
> blockdev --setro $JOURNAL_DEV
> mount $FS_DEV $MNT
> touch $MNT/file
> umount $MNT
> 
> Signed-off-by: Lukas Czerner <lczerner@...hat.com>
> ---
> fs/ext4/super.c | 55 ++++++++++++++++++++++++++++++-------------------
> 1 file changed, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 330957ed1f05..a15e3c751766 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5088,7 +5089,30 @@ static int ext4_load_journal(struct super_block *sb,
> 	} else
> 		journal_dev = new_decode_dev(le32_to_cpu(es->s_journal_dev));
> 
> -	really_read_only = bdev_read_only(sb->s_bdev);
> +	if (journal_inum && journal_dev) {
> +		ext4_msg(sb, KERN_ERR, "filesystem has both journal "
> +		       "and inode journals!");

(style) keep error string on a single line.  Also, "journal and inode journal"
is not very clear what the problem is.  Maybe something like:

+		ext4_msg(sb, KERN_ERR,
+			 "filesystem has both journal inode and device!");

> +		return -EINVAL;
> +	}
> +
> +	if (journal_inum) {
> +		if (!(journal = ext4_get_journal(sb, journal_inum)))
> +			return -EINVAL;
> +	} else {
> +		if (!(journal = ext4_get_dev_journal(sb, journal_dev)))
> +			return -EINVAL;
> +	}
> +
> +	journal_dev_ro = bdev_read_only(journal->j_dev);
> +	really_read_only = bdev_read_only(sb->s_bdev) | journal_dev_ro;
> +
> +	if (journal_dev_ro && !sb_rdonly(sb)) {
> +		ext4_msg(sb, KERN_ERR, "write access "
> +			"unavailable, cannot proceed "
> +			"(try mounting read-only)");

(style) should keep error strings on a single line.  Also, this isn't very
obvious that that this is because of the read-only journal device.  Maybe:

		ext4_msg(sb, KERN_ERR,
			 "journal device read-only, try mounting with '-o ro'");

> @@ -5141,11 +5152,8 @@ static int ext4_load_journal(struct super_block *sb,
> 		kfree(save);
> 	}
> 
> -	if (err) {
> -		ext4_msg(sb, KERN_ERR, "error loading journal");
> -		jbd2_journal_destroy(journal);
> -		return err;
> -	}
> +	if (err)
> +		goto err_out;
> 
> 	EXT4_SB(sb)->s_journal = journal;
> 	ext4_clear_journal_err(sb, es);
> @@ -5159,6 +5167,11 @@ static int ext4_load_journal(struct super_block *sb,
> 	}
> 
> 	return 0;
> +
> +err_out:
> +	ext4_msg(sb, KERN_ERR, "error loading journal");

Is there any error case that doesn't already print its own error message?
Maybe better to leave the ext4_msg() in the original location, and only
do cleanup here.


Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)