linux-ext4 - Re: [PATCH v2] e2fsprogs: add ext2fs_is_fast

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-Id: <4005AF3A-2FFA-4285-B4B8-9952633782FD@dilger.ca>
Date:   Wed, 5 Jul 2017 10:26:16 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Tahsin Erdogan <tahsin@...gle.com>
Cc:     "Darrick J . Wong" <darrick.wong@...cle.com>,
        Theodore Ts'o <tytso@....edu>, linux-ext4@...r.kernel.org
Subject: Re: [PATCH v2] e2fsprogs: add ext2fs_is_fast_symlink() function

On Jun 29, 2017, at 7:36 PM, Tahsin Erdogan <tahsin@...gle.com> wrote:
> 
> Current way of determining whether a symlink is in fast symlink
> format is to call ext2fs_inode_data_blocks2(). If number of data
> blocks is zero and EXT4_INLINE_DATA_FL flag is not set, then symlink
> data must be in inode->i_block.
> 
> This heuristic is becoming increasingly hard to maintain because
> inode->i_blocks count can also be incremented for blocks used by
> extended attributes. Before ea_inode feature, extra block could come
> from xattr block, now more blocks can be added because of xattr
> inodes.
> 
> To address the issue, add a ext2fs_is_fast_symlink() function that
> gives a direct answer based on inode->i_size field. This is
> equivalent to kernel's ext4_inode_is_fast_symlink() function.
> 
> This patch also fixes a few issues related to fast symlink handling:
> 
>  - Both rdump_symlink() and follow_link() interpreted symlinks with
>    0 data blocks to always mean fast symlinks. This is incorrect
>    because symlinks that are stored as inline data also have
>    0 data blocks. Thus, they try to read everything from
>    inode->i_block and miss the symlink suffix in inode extra area.
> 
>  - e2fsck_pass1_check_symlink() had code to handle inode with
>    EXT4_INLINE_DATA_FL flag twice. The first if block always returns
>    from the function so the second one is unreachable code.

This looks mostly good, one question below.

> Suggested-by: Andreas Dilger <adilger@...ger.ca>
> Signed-off-by: Tahsin Erdogan <tahsin@...gle.com>
> ---
> v2: Added Suggested-by to commit message
> 
> debugfs/debugfs.c    | 55 +++++++++++++++++++++++-----------------------------
> debugfs/dump.c       |  4 +---
> e2fsck/pass1.c       | 42 ++++++++-------------------------------
> lib/ext2fs/alloc.c   |  9 +++++----
> lib/ext2fs/ext2fs.h  |  1 +
> lib/ext2fs/namei.c   | 20 ++++++++++++++++---
> lib/ext2fs/swapfs.c  | 46 +++++++++++++++++++++----------------------
> lib/ext2fs/symlink.c | 11 +++++++++++
> misc/fuse2fs.c       |  9 ++++-----
> 9 files changed, 94 insertions(+), 103 deletions(-)
> 
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index 422a3d699111..dd650427795c 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> 
> @@ -221,12 +220,15 @@ int e2fsck_pass1_check_symlink(ext2_filsys fs, ext2_ino_t ino,
> 		return 1;
> 	}
> 
> -	blocks = ext2fs_inode_data_blocks2(fs, inode);
> -	if (blocks) {
> -		if (inode->i_flags & EXT4_INLINE_DATA_FL)
> +	if (ext2fs_is_fast_symlink(inode)) {
> +		if (inode->i_size >= sizeof(inode->i_block))
> +			return 0;

If this is a fast symlink, which is now determined by the file size, I don't see
how this i_size check can ever be true?

> +
> +		len = strnlen((char *)inode->i_block, sizeof(inode->i_block));
> +		if (len == sizeof(inode->i_block))
> 			return 0;
> +	} else {
> 		if ((inode->i_size >= fs->blocksize) ||
> 		    (inode->i_block[0] < fs->super->s_first_data_block) ||
> 		    (inode->i_block[0] >= ext2fs_blocks_count(fs->super)))
> 			return 0;
> diff --git a/lib/ext2fs/symlink.c b/lib/ext2fs/symlink.c
> index 0e6f9a9fd14e..271aa1ccc134 100644
> --- a/lib/ext2fs/symlink.c
> +++ b/lib/ext2fs/symlink.c
> @@ -174,3 +174,14 @@ cleanup:
> 		ext2fs_free_mem(&block_buf);
> 	return retval;
> }
> +
> +/*
> + * Test whether an inode is a fast symlink.
> + *
> + * A fast symlink has its symlink data stored in inode->i_block.
> + */
> +int ext2fs_is_fast_symlink(struct ext2_inode *inode)
> +{
> +	return LINUX_S_ISLNK(inode->i_mode) && EXT2_I_SIZE(inode) &&
> +	       EXT2_I_SIZE(inode) < sizeof(inode->i_block);
> +}
> \ No newline at end of file

Need to add newline here?


Cheers, Andreas






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