[20/20] ext4: Implement encoding-aware dcache hooks

Message ID 20180703170700.9306-21-krisman@collabora.co.uk
State Changes Requested
Headers show
Series
  • EXT4 encoding support
Related show

Commit Message

Gabriel Krisman Bertazi July 3, 2018, 5:07 p.m.
d_revalidate to reject negative dentries is not needed, because we
avoided adding those in the first place during lookup, similar to what
xfs does.

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk>
---
 fs/ext4/dir.c   | 30 ++++++++++++++++++++++++++++++
 fs/ext4/ext4.h  |  1 +
 fs/ext4/super.c |  4 ++++
 3 files changed, 35 insertions(+)

Comments

Theodore Ts'o July 11, 2018, 5:41 p.m. | #1
On Tue, Jul 03, 2018 at 01:07:00PM -0400, Gabriel Krisman Bertazi wrote:
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 53db9b6c7e33..f292cc5bacda 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4358,6 +4358,10 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>  		iput(root);
>  		goto failed_mount4;
>  	}
> +
> +	if (sbi->encoding)
> +		sb->s_d_op = &ext4_dentry_ops;
> +

This is going to be potentially problematic for fscrypt (e.g., when
ext4's encryption is enabled, as will be the case in Android's File
Based Encryption).  See the call to d_set_d_op() in
__fscrypt_prepare_lookup in fs/crypto/hooks.c, since d_set_d_op is
going to overwrite sb->s_d_op.

This probably means that the fscrypt code is going to have to be
case-sensitivity aware.  Or we would have to make case folding and fs
encryption to be mutually exclusive, which would be unfortunate, since
Android is going to be a potential user of case folding.

	   	       	 	   	- Ted
Gabriel Krisman Bertazi July 11, 2018, 8:30 p.m. | #2
"Theodore Y. Ts'o" <tytso@mit.edu> writes:

> On Tue, Jul 03, 2018 at 01:07:00PM -0400, Gabriel Krisman Bertazi wrote:
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 53db9b6c7e33..f292cc5bacda 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -4358,6 +4358,10 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>>  		iput(root);
>>  		goto failed_mount4;
>>  	}
>> +
>> +	if (sbi->encoding)
>> +		sb->s_d_op = &ext4_dentry_ops;
>> +

Hello,

Thanks for the review.

>
> This is going to be potentially problematic for fscrypt (e.g., when
> ext4's encryption is enabled, as will be the case in Android's File
> Based Encryption).  See the call to d_set_d_op() in
> __fscrypt_prepare_lookup in fs/crypto/hooks.c, since d_set_d_op is
> going to overwrite sb->s_d_op.
>
> This probably means that the fscrypt code is going to have to be
> case-sensitivity aware.  Or we would have to make case folding and fs
> encryption to be mutually exclusive, which would be unfortunate, since
> Android is going to be a potential user of case folding.

Yes, I am aware of that issue with fscrypt's d_revalidate, the smoke
tests helped me catch it beforehand.  I worked around it on a different
branch by implementing a wrapper around my own d_revalidate, and
removing it from fscrypt.  I didn't send it along with this patchset
because, as you mentioned, fscrypt needs to become case-sensitive aware
(in fact, encoding-aware), which means we need to either also store the
normalized version of the encrypted file name in the dentry, or do a
much more expensive search in the directory, by decrypting each name
before comparison.  I'm doing the second option now, (decrypting the
filename at comparison time), but I considered this non-trivial change
to be a future step, which is not ready for submission.

My approach in the current patchset is make these features mutually
exclusive for now, as you can see in patch 17.  I'm refusing to mount
with encoding any superblock that has the encryption feature enabled.  I
know it is not a final solution for Android and Valve usecases, but I
think this support can be integrated at a later moment without breaking
the abi.

Patch

diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
index e2902d394f1b..c520b9e94778 100644
--- a/fs/ext4/dir.c
+++ b/fs/ext4/dir.c
@@ -26,6 +26,7 @@ 
 #include <linux/buffer_head.h>
 #include <linux/slab.h>
 #include <linux/iversion.h>
+#include <linux/nls.h>
 #include "ext4.h"
 #include "xattr.h"
 
@@ -664,3 +665,32 @@  const struct file_operations ext4_dir_operations = {
 	.open		= ext4_dir_open,
 	.release	= ext4_release_dir,
 };
+
+static int ext4_d_compare(const struct dentry *dentry, unsigned int len,
+			  const char *str, const struct qstr *name)
+{
+	struct nls_table *charset = EXT4_SB(dentry->d_sb)->encoding;
+	size_t nlen = strlen(name->name);
+
+	return nls_strncmp(charset, str, len, name->name, nlen);
+}
+
+static int ext4_d_hash(const struct dentry *dentry, struct qstr *q)
+{
+	const struct nls_table *charset = EXT4_SB(dentry->d_sb)->encoding;
+	unsigned char norm[PATH_MAX];
+	int len;
+
+	len = nls_normalize(charset, q->name, q->len, norm, PATH_MAX);
+	if (len < 0)
+		return -EINVAL;
+
+	q->hash = full_name_hash(dentry, norm, len);
+
+	return 0;
+}
+
+const struct dentry_operations ext4_dentry_ops = {
+	.d_hash = ext4_d_hash,
+	.d_compare = ext4_d_compare,
+};
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index fb0b70d6eb68..2a5c7712967f 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2953,6 +2953,7 @@  static inline void ext4_unlock_group(struct super_block *sb,
 
 /* dir.c */
 extern const struct file_operations ext4_dir_operations;
+extern const struct dentry_operations ext4_dentry_ops;
 
 /* file.c */
 extern const struct inode_operations ext4_file_inode_operations;
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 53db9b6c7e33..f292cc5bacda 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4358,6 +4358,10 @@  static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 		iput(root);
 		goto failed_mount4;
 	}
+
+	if (sbi->encoding)
+		sb->s_d_op = &ext4_dentry_ops;
+
 	sb->s_root = d_make_root(root);
 	if (!sb->s_root) {
 		ext4_msg(sb, KERN_ERR, "get root dentry failed");