| Message ID | 20180703170700.9306-21-krisman@collabora.co.uk |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | EXT4 encoding support | expand |
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
"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.
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");
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(+)