Date:   Thu, 17 Nov 2016 19:20:24 -0700
From:   Andreas Dilger <adilger@...ger.ca>
To:     Eric Biggers <ebiggers@...gle.com>
Cc:     linux-ext4 <linux-ext4@...r.kernel.org>,
        Theodore Ts'o <tytso@....edu>
Subject: Re: [PATCH] ext4: fix reading new encrypted symlinks on no-journal filesystems

On Nov 16, 2016, at 10:50 AM, Eric Biggers <ebiggers@...gle.com> wrote:
> 
> On a filesystem with no journal, a symlink longer than about 32
> characters (exact length depending on padding for encryption) could not
> be followed or read immediately after being created in an encrypted
> directory.  This happened because when the symlink data went through the
> delayed allocation path instead of the journaling path, the symlink was
> incorrectly detected as a "fast" symlink rather than a "slow" symlink
> until its data was written out.

IMHO, this again exposes an issue that we've seen with "fast" vs. "slow"
symlink detection several times in the past whenever there is a data block
allocated for a fast symlink (e.g. when xattrs were allowed on symlinks).

int ext4_inode_is_fast_symlink(struct inode *inode)
{
        int ea_blocks = EXT4_I(inode)->i_file_acl ?
                EXT4_CLUSTER_SIZE(inode->i_sb) >> 9 : 0;

        if (ext4_has_inline_data(inode))
                return 0;

        return (S_ISLNK(inode->i_mode) && inode->i_blocks - ea_blocks == 0);
}

Instead of depending on the i_blocks count to detect slow symlinks, we
should just check the i_size < EXT4_N_BLOCKS * 4 (or <=, need to verify).
I believe this has always been true for fast symlinks, so it should be
OK to make this change.  That will isolate us from future changes that
may add block allocations to symlinks.

Cheers, Andreas

> To fix this, use different inode operations for fast and slow encrypted
> symlinks.  This parallels how fast and slow unencrypted symlinks use
> different inode operations.
> 
> I did not fix this by updating ext4_inode_is_fast_symlink() to account
> for delayed allocation because there didn't seem to be a good way to do
> that in a race-free way.  i_data_sem would need to be acquired to avoid
> racing with ext4_da_update_reserve_space() updating
> i_reserved_data_blocks and i_blocks, but i_data_sem did not seem
> appropriate to take from the context of ext4_inode_is_fast_symlink().
> 
> Signed-off-by: Eric Biggers <ebiggers@...gle.com>
> ---
> fs/ext4/ext4.h    |  1 +
> fs/ext4/inode.c   | 28 +++++++++++++++++++---------
> fs/ext4/namei.c   | 10 +++++++---
> fs/ext4/symlink.c | 28 +++++++++++++++++++++++++---
> 4 files changed, 52 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 53d6d46..a0096af 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3087,6 +3087,7 @@ extern int ext4_mpage_readpages(struct address_space *mapping,
> 
> /* symlink.c */
> extern const struct inode_operations ext4_encrypted_symlink_inode_operations;
> +extern const struct inode_operations ext4_encrypted_fast_symlink_inode_operations;
> extern const struct inode_operations ext4_symlink_inode_operations;
> extern const struct inode_operations ext4_fast_symlink_inode_operations;
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index b1b4c85..be9e369 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -144,6 +144,9 @@ static int ext4_meta_trans_blocks(struct inode *inode, int lblocks,
> 
> /*
>  * Test whether an inode is a fast symlink.
> + *
> + * Careful: the result may be incorrect if the symlink was just created and is
> + * pending delayed allocation (only possible on no-journal filesystems).
>  */
> int ext4_inode_is_fast_symlink(struct inode *inode)
> {
> @@ -4646,16 +4649,23 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
> 		inode->i_op = &ext4_dir_inode_operations;
> 		inode->i_fop = &ext4_dir_operations;
> 	} else if (S_ISLNK(inode->i_mode)) {
> -		if (ext4_encrypted_inode(inode)) {
> -			inode->i_op = &ext4_encrypted_symlink_inode_operations;
> -			ext4_set_aops(inode);
> -		} else if (ext4_inode_is_fast_symlink(inode)) {
> -			inode->i_link = (char *)ei->i_data;
> -			inode->i_op = &ext4_fast_symlink_inode_operations;
> -			nd_terminate_link(ei->i_data, inode->i_size,
> -				sizeof(ei->i_data) - 1);
> +		if (ext4_inode_is_fast_symlink(inode)) {
> +			if (ext4_encrypted_inode(inode)) {
> +				inode->i_op =
> +				  &ext4_encrypted_fast_symlink_inode_operations;
> +			} else {
> +				inode->i_op =
> +					&ext4_fast_symlink_inode_operations;
> +				inode->i_link = (char *)ei->i_data;
> +				nd_terminate_link(ei->i_data, inode->i_size,
> +						  sizeof(ei->i_data) - 1);
> +			}
> 		} else {
> -			inode->i_op = &ext4_symlink_inode_operations;
> +			if (ext4_encrypted_inode(inode))
> +				inode->i_op =
> +				       &ext4_encrypted_symlink_inode_operations;
> +			else
> +				inode->i_op = &ext4_symlink_inode_operations;
> 			ext4_set_aops(inode);
> 		}
> 		inode_nohighmem(inode);
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index eadba91..acf1786 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -3148,11 +3148,12 @@ static int ext4_symlink(struct inode *dir,
> 			goto err_drop_inode;
> 		sd->len = cpu_to_le16(ostr.len);
> 		disk_link.name = (char *) sd;
> -		inode->i_op = &ext4_encrypted_symlink_inode_operations;
> 	}
> 
> 	if ((disk_link.len > EXT4_N_BLOCKS * 4)) {
> -		if (!encryption_required)
> +		if (encryption_required)
> +			inode->i_op = &ext4_encrypted_symlink_inode_operations;
> +		else
> 			inode->i_op = &ext4_symlink_inode_operations;
> 		inode_nohighmem(inode);
> 		ext4_set_aops(inode);
> @@ -3194,7 +3195,10 @@ static int ext4_symlink(struct inode *dir,
> 	} else {
> 		/* clear the extent format for fast symlink */
> 		ext4_clear_inode_flag(inode, EXT4_INODE_EXTENTS);
> -		if (!encryption_required) {
> +		if (encryption_required) {
> +			inode->i_op =
> +				&ext4_encrypted_fast_symlink_inode_operations;
> +		} else {
> 			inode->i_op = &ext4_fast_symlink_inode_operations;
> 			inode->i_link = (char *)&EXT4_I(inode)->i_data;
> 		}
> diff --git a/fs/ext4/symlink.c b/fs/ext4/symlink.c
> index 557b3b0..684bb78 100644
> --- a/fs/ext4/symlink.c
> +++ b/fs/ext4/symlink.c
> @@ -24,7 +24,8 @@
> 
> static const char *ext4_encrypted_get_link(struct dentry *dentry,
> 					   struct inode *inode,
> -					   struct delayed_call *done)
> +					   struct delayed_call *done,
> +					   bool fast)
> {
> 	struct page *cpage = NULL;
> 	char *caddr, *paddr = NULL;
> @@ -40,7 +41,7 @@ static const char *ext4_encrypted_get_link(struct dentry *dentry,
> 	if (res)
> 		return ERR_PTR(res);
> 
> -	if (ext4_inode_is_fast_symlink(inode)) {
> +	if (fast) {
> 		caddr = (char *) EXT4_I(inode)->i_data;
> 		max_size = sizeof(EXT4_I(inode)->i_data);
> 	} else {
> @@ -82,9 +83,30 @@ static const char *ext4_encrypted_get_link(struct dentry *dentry,
> 	return ERR_PTR(res);
> }
> 
> +static const char *ext4_encrypted_get_link_slow(struct dentry *dentry,
> +						struct inode *inode,
> +						struct delayed_call *done)
> +{
> +	return ext4_encrypted_get_link(dentry, inode, done, false);
> +}
> +
> +static const char *ext4_encrypted_get_link_fast(struct dentry *dentry,
> +						struct inode *inode,
> +						struct delayed_call *done)
> +{
> +	return ext4_encrypted_get_link(dentry, inode, done, true);
> +}
> +
> const struct inode_operations ext4_encrypted_symlink_inode_operations = {
> 	.readlink	= generic_readlink,
> -	.get_link	= ext4_encrypted_get_link,
> +	.get_link	= ext4_encrypted_get_link_slow,
> +	.setattr	= ext4_setattr,
> +	.listxattr	= ext4_listxattr,
> +};
> +
> +const struct inode_operations ext4_encrypted_fast_symlink_inode_operations = {
> +	.readlink	= generic_readlink,
> +	.get_link	= ext4_encrypted_get_link_fast,
> 	.setattr	= ext4_setattr,
> 	.listxattr	= ext4_listxattr,
> };
> --
> 2.8.0.rc3.226.g39d4020
> 


Cheers, Andreas






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

Powered by blists - more mailing lists