Hello,
I am trying to respin the series here :
https://www.spinics.net/lists/linux-ext4/msg85081.html
I resent some of the v9 patches and got some reviews from Gabriel,
I did changes as requested and here is v13.
Changes in v13:
- removed stray wrong line in 2/8
- removed old R-b as it's too long since they were given
- removed check for null buff in 2/8
- added new patch `f2fs: Log error when lookup of encoded dentry fails` as suggested
- rebased on unicode.git for-next branch
Changes in v12:
- revert to v10 comparison with propagating the error code from utf comparison
Changes in v11:
- revert to the original v9 implementation for the comparison helper.
Changes in v10:
- reworked a bit the comparison helper to improve performance by
first performing the exact lookup.
* Original commit letter
The case-insensitive implementations in f2fs and ext4 have quite a bit
of duplicated code. This series simplifies the ext4 version, with the
goal of extracting ext4_ci_compare into a helper library that can be
used by both filesystems. It also reduces the clutter from many
codeguards for CONFIG_UNICODE; as requested by Linus, they are part of
the codeflow now.
While there, I noticed we can leverage the utf8 functions to detect
encoded names that are corrupted in the filesystem. Therefore, it also
adds an ext4 error on that scenario, to mark the filesystem as
corrupted.
This series survived passes of xfstests -g quick.
Eugen Hristev (1):
f2fs: Log error when lookup of encoded dentry fails
Gabriel Krisman Bertazi (8):
ext4: Simplify the handling of cached insensitive names
f2fs: Simplify the handling of cached insensitive names
libfs: Introduce case-insensitive string comparison helper
ext4: Reuse generic_ci_match for ci comparisons
f2fs: Reuse generic_ci_match for ci comparisons
ext4: Log error when lookup of encoded dentry fails
ext4: Move CONFIG_UNICODE defguards into the code flow
f2fs: Move CONFIG_UNICODE defguards into the code flow
fs/ext4/crypto.c | 19 ++-----
fs/ext4/ext4.h | 35 +++++++-----
fs/ext4/namei.c | 129 ++++++++++++++++-----------------------------
fs/ext4/super.c | 4 +-
fs/f2fs/dir.c | 112 ++++++++++++++-------------------------
fs/f2fs/f2fs.h | 16 +++++-
fs/f2fs/namei.c | 10 ++--
fs/f2fs/recovery.c | 5 +-
fs/f2fs/super.c | 8 +--
fs/libfs.c | 81 ++++++++++++++++++++++++++++
include/linux/fs.h | 4 ++
11 files changed, 219 insertions(+), 204 deletions(-)
--
2.34.1
From: Gabriel Krisman Bertazi <[email protected]>
Keeping it as qstr avoids the unnecessary conversion in ext4_match
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
[[email protected]: port to 6.8-rc3]
Signed-off-by: Eugen Hristev <[email protected]>
---
fs/ext4/ext4.h | 2 +-
fs/ext4/namei.c | 23 +++++++++++------------
2 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index a5d784872303..4061d11b9763 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2506,7 +2506,7 @@ struct ext4_filename {
struct fscrypt_str crypto_buf;
#endif
#if IS_ENABLED(CONFIG_UNICODE)
- struct fscrypt_str cf_name;
+ struct qstr cf_name;
#endif
};
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 5e4f65c14dfb..b96983a4c185 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1445,7 +1445,8 @@ static int ext4_ci_compare(const struct inode *parent, const struct qstr *name,
int ext4_fname_setup_ci_filename(struct inode *dir, const struct qstr *iname,
struct ext4_filename *name)
{
- struct fscrypt_str *cf_name = &name->cf_name;
+ struct qstr *cf_name = &name->cf_name;
+ unsigned char *buf;
struct dx_hash_info *hinfo = &name->hinfo;
int len;
@@ -1455,18 +1456,18 @@ int ext4_fname_setup_ci_filename(struct inode *dir, const struct qstr *iname,
return 0;
}
- cf_name->name = kmalloc(EXT4_NAME_LEN, GFP_NOFS);
- if (!cf_name->name)
+ buf = kmalloc(EXT4_NAME_LEN, GFP_NOFS);
+ if (!buf)
return -ENOMEM;
- len = utf8_casefold(dir->i_sb->s_encoding,
- iname, cf_name->name,
- EXT4_NAME_LEN);
+ len = utf8_casefold(dir->i_sb->s_encoding, iname, buf, EXT4_NAME_LEN);
if (len <= 0) {
- kfree(cf_name->name);
- cf_name->name = NULL;
+ kfree(buf);
+ buf = NULL;
}
+ cf_name->name = buf;
cf_name->len = (unsigned) len;
+
if (!IS_ENCRYPTED(dir))
return 0;
@@ -1503,8 +1504,6 @@ static bool ext4_match(struct inode *parent,
if (IS_CASEFOLDED(parent) &&
(!IS_ENCRYPTED(parent) || fscrypt_has_encryption_key(parent))) {
if (fname->cf_name.name) {
- struct qstr cf = {.name = fname->cf_name.name,
- .len = fname->cf_name.len};
if (IS_ENCRYPTED(parent)) {
if (fname->hinfo.hash != EXT4_DIRENT_HASH(de) ||
fname->hinfo.minor_hash !=
@@ -1513,8 +1512,8 @@ static bool ext4_match(struct inode *parent,
return false;
}
}
- return !ext4_ci_compare(parent, &cf, de->name,
- de->name_len, true);
+ return !ext4_ci_compare(parent, &fname->cf_name,
+ de->name, de->name_len, true);
}
return !ext4_ci_compare(parent, fname->usr_fname, de->name,
de->name_len, false);
--
2.34.1
From: Gabriel Krisman Bertazi <[email protected]>
Keeping it as qstr avoids the unnecessary conversion in f2fs_match
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
[[email protected]: port to 6.8-rc3 and minor changes]
Signed-off-by: Eugen Hristev <[email protected]>
---
fs/f2fs/dir.c | 51 +++++++++++++++++++++++++---------------------
fs/f2fs/f2fs.h | 16 ++++++++++++++-
fs/f2fs/recovery.c | 5 +----
3 files changed, 44 insertions(+), 28 deletions(-)
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 042593aed1ec..266279b82afc 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -42,35 +42,47 @@ static unsigned int bucket_blocks(unsigned int level)
return 4;
}
+#if IS_ENABLED(CONFIG_UNICODE)
/* If @dir is casefolded, initialize @fname->cf_name from @fname->usr_fname. */
int f2fs_init_casefolded_name(const struct inode *dir,
struct f2fs_filename *fname)
{
-#if IS_ENABLED(CONFIG_UNICODE)
struct super_block *sb = dir->i_sb;
+ unsigned char *buf;
+ int len;
if (IS_CASEFOLDED(dir) &&
!is_dot_dotdot(fname->usr_fname->name, fname->usr_fname->len)) {
- fname->cf_name.name = f2fs_kmem_cache_alloc(f2fs_cf_name_slab,
- GFP_NOFS, false, F2FS_SB(sb));
- if (!fname->cf_name.name)
+ buf = f2fs_kmem_cache_alloc(f2fs_cf_name_slab,
+ GFP_NOFS, false, F2FS_SB(sb));
+ if (!buf)
return -ENOMEM;
- fname->cf_name.len = utf8_casefold(sb->s_encoding,
- fname->usr_fname,
- fname->cf_name.name,
- F2FS_NAME_LEN);
- if ((int)fname->cf_name.len <= 0) {
- kmem_cache_free(f2fs_cf_name_slab, fname->cf_name.name);
- fname->cf_name.name = NULL;
+
+ len = utf8_casefold(sb->s_encoding, fname->usr_fname,
+ buf, F2FS_NAME_LEN);
+ if (len <= 0) {
+ kmem_cache_free(f2fs_cf_name_slab, buf);
if (sb_has_strict_encoding(sb))
return -EINVAL;
/* fall back to treating name as opaque byte sequence */
+ return 0;
}
+ fname->cf_name.name = buf;
+ fname->cf_name.len = len;
}
-#endif
+
return 0;
}
+void f2fs_free_casefolded_name(struct f2fs_filename *fname)
+{
+ unsigned char *buf = (unsigned char *)fname->cf_name.name;
+
+ kmem_cache_free(f2fs_cf_name_slab, buf);
+ fname->cf_name.name = NULL;
+}
+#endif /* CONFIG_UNICODE */
+
static int __f2fs_setup_filename(const struct inode *dir,
const struct fscrypt_name *crypt_name,
struct f2fs_filename *fname)
@@ -142,12 +154,7 @@ void f2fs_free_filename(struct f2fs_filename *fname)
kfree(fname->crypto_buf.name);
fname->crypto_buf.name = NULL;
#endif
-#if IS_ENABLED(CONFIG_UNICODE)
- if (fname->cf_name.name) {
- kmem_cache_free(f2fs_cf_name_slab, fname->cf_name.name);
- fname->cf_name.name = NULL;
- }
-#endif
+ f2fs_free_casefolded_name(fname);
}
static unsigned long dir_block_index(unsigned int level,
@@ -235,11 +242,9 @@ static inline int f2fs_match_name(const struct inode *dir,
struct fscrypt_name f;
#if IS_ENABLED(CONFIG_UNICODE)
- if (fname->cf_name.name) {
- struct qstr cf = FSTR_TO_QSTR(&fname->cf_name);
-
- return f2fs_match_ci_name(dir, &cf, de_name, de_name_len);
- }
+ if (fname->cf_name.name)
+ return f2fs_match_ci_name(dir, &fname->cf_name,
+ de_name, de_name_len);
#endif
f.usr_fname = fname->usr_fname;
f.disk_name = fname->disk_name;
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 65294e3b0bef..4f68c58718fb 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -524,7 +524,7 @@ struct f2fs_filename {
* internal operation where usr_fname is also NULL. In all these cases
* we fall back to treating the name as an opaque byte sequence.
*/
- struct fscrypt_str cf_name;
+ struct qstr cf_name;
#endif
};
@@ -3528,8 +3528,22 @@ int f2fs_get_tmpfile(struct mnt_idmap *idmap, struct inode *dir,
/*
* dir.c
*/
+#if IS_ENABLED(CONFIG_UNICODE)
int f2fs_init_casefolded_name(const struct inode *dir,
struct f2fs_filename *fname);
+void f2fs_free_casefolded_name(struct f2fs_filename *fname);
+#else
+static inline int f2fs_init_casefolded_name(const struct inode *dir,
+ struct f2fs_filename *fname)
+{
+ return 0;
+}
+
+static inline void f2fs_free_casefolded_name(struct f2fs_filename *fname)
+{
+}
+#endif /* CONFIG_UNICODE */
+
int f2fs_setup_filename(struct inode *dir, const struct qstr *iname,
int lookup, struct f2fs_filename *fname);
int f2fs_prepare_lookup(struct inode *dir, struct dentry *dentry,
diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index d0f24ccbd1ac..7e53abf6d216 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -153,11 +153,8 @@ static int init_recovered_filename(const struct inode *dir,
if (err)
return err;
f2fs_hash_filename(dir, fname);
-#if IS_ENABLED(CONFIG_UNICODE)
/* Case-sensitive match is fine for recovery */
- kmem_cache_free(f2fs_cf_name_slab, fname->cf_name.name);
- fname->cf_name.name = NULL;
-#endif
+ f2fs_free_casefolded_name(fname);
} else {
f2fs_hash_filename(dir, fname);
}
--
2.34.1
From: Gabriel Krisman Bertazi <[email protected]>
generic_ci_match can be used by case-insensitive filesystems to compare
strings under lookup with dirents in a case-insensitive way. This
function is currently reimplemented by each filesystem supporting
casefolding, so this reduces code duplication in filesystem-specific
code.
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
[[email protected]: rework to first test the exact match]
Signed-off-by: Eugen Hristev <[email protected]>
---
fs/libfs.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++
include/linux/fs.h | 4 +++
2 files changed, 85 insertions(+)
diff --git a/fs/libfs.c b/fs/libfs.c
index c297953db948..c107c24f33b9 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1776,6 +1776,87 @@ static const struct dentry_operations generic_ci_dentry_ops = {
.d_revalidate = fscrypt_d_revalidate,
#endif
};
+
+/**
+ * generic_ci_match() - Match a name (case-insensitively) with a dirent.
+ * This is a filesystem helper for comparison with directory entries.
+ * generic_ci_d_compare should be used in VFS' ->d_compare instead.
+ *
+ * @parent: Inode of the parent of the dirent under comparison
+ * @name: name under lookup.
+ * @folded_name: Optional pre-folded name under lookup
+ * @de_name: Dirent name.
+ * @de_name_len: dirent name length.
+ *
+ * Test whether a case-insensitive directory entry matches the filename
+ * being searched. If @folded_name is provided, it is used instead of
+ * recalculating the casefold of @name.
+ *
+ * Return: > 0 if the directory entry matches, 0 if it doesn't match, or
+ * < 0 on error.
+ */
+int generic_ci_match(const struct inode *parent,
+ const struct qstr *name,
+ const struct qstr *folded_name,
+ const u8 *de_name, u32 de_name_len)
+{
+ const struct super_block *sb = parent->i_sb;
+ const struct unicode_map *um = sb->s_encoding;
+ struct fscrypt_str decrypted_name = FSTR_INIT(NULL, de_name_len);
+ struct qstr dirent = QSTR_INIT(de_name, de_name_len);
+ int res, match = 0;
+
+ if (IS_ENCRYPTED(parent)) {
+ const struct fscrypt_str encrypted_name =
+ FSTR_INIT((u8 *) de_name, de_name_len);
+
+ if (WARN_ON_ONCE(!fscrypt_has_encryption_key(parent)))
+ return -EINVAL;
+
+ decrypted_name.name = kmalloc(de_name_len, GFP_KERNEL);
+ if (!decrypted_name.name)
+ return -ENOMEM;
+ res = fscrypt_fname_disk_to_usr(parent, 0, 0, &encrypted_name,
+ &decrypted_name);
+ if (res < 0)
+ goto out;
+ dirent.name = decrypted_name.name;
+ dirent.len = decrypted_name.len;
+ }
+
+ /*
+ * Attempt a case-sensitive match first. It is cheaper and
+ * should cover most lookups, including all the sane
+ * applications that expect a case-sensitive filesystem.
+ */
+ if (folded_name->name) {
+ if (dirent.len == folded_name->len &&
+ !memcmp(folded_name->name, dirent.name, dirent.len)) {
+ match = 1;
+ goto out;
+ }
+ res = utf8_strncasecmp_folded(um, folded_name, &dirent);
+ } else {
+ if (dirent.len == name->len &&
+ !memcmp(name->name, dirent.name, dirent.len) &&
+ (!sb_has_strict_encoding(sb) || !utf8_validate(um, name))) {
+ match = 1;
+ goto out;
+ }
+ res = utf8_strncasecmp(um, name, &dirent);
+ }
+
+out:
+ kfree(decrypted_name.name);
+ if (match) /* matched by direct comparison */
+ return 1;
+ else if (!res) /* matched by utf8 comparison */
+ return 1;
+ else if (res < 0) /* error on utf8 comparison */
+ return res;
+ return 0; /* no match */
+}
+EXPORT_SYMBOL(generic_ci_match);
#endif
#ifdef CONFIG_FS_ENCRYPTION
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ff1338109b54..690b37e1db95 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3281,6 +3281,10 @@ extern int generic_file_fsync(struct file *, loff_t, loff_t, int);
extern int generic_check_addressable(unsigned, u64);
extern void generic_set_sb_d_ops(struct super_block *sb);
+extern int generic_ci_match(const struct inode *parent,
+ const struct qstr *name,
+ const struct qstr *folded_name,
+ const u8 *de_name, u32 de_name_len);
static inline bool sb_has_encoding(const struct super_block *sb)
{
--
2.34.1
From: Gabriel Krisman Bertazi <[email protected]>
Instead of reimplementing ext4_match_ci, use the new libfs helper.
It also adds a comment explaining why fname->cf_name.name must be
checked prior to the encryption hash optimization, because that tripped
me before.
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
Signed-off-by: Eugen Hristev <[email protected]>
---
fs/ext4/namei.c | 91 +++++++++++++++----------------------------------
1 file changed, 27 insertions(+), 64 deletions(-)
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index b96983a4c185..2d0ee232fbe7 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1390,58 +1390,6 @@ static void dx_insert_block(struct dx_frame *frame, u32 hash, ext4_lblk_t block)
}
#if IS_ENABLED(CONFIG_UNICODE)
-/*
- * Test whether a case-insensitive directory entry matches the filename
- * being searched for. If quick is set, assume the name being looked up
- * is already in the casefolded form.
- *
- * Returns: 0 if the directory entry matches, more than 0 if it
- * doesn't match or less than zero on error.
- */
-static int ext4_ci_compare(const struct inode *parent, const struct qstr *name,
- u8 *de_name, size_t de_name_len, bool quick)
-{
- const struct super_block *sb = parent->i_sb;
- const struct unicode_map *um = sb->s_encoding;
- struct fscrypt_str decrypted_name = FSTR_INIT(NULL, de_name_len);
- struct qstr entry = QSTR_INIT(de_name, de_name_len);
- int ret;
-
- if (IS_ENCRYPTED(parent)) {
- const struct fscrypt_str encrypted_name =
- FSTR_INIT(de_name, de_name_len);
-
- decrypted_name.name = kmalloc(de_name_len, GFP_KERNEL);
- if (!decrypted_name.name)
- return -ENOMEM;
- ret = fscrypt_fname_disk_to_usr(parent, 0, 0, &encrypted_name,
- &decrypted_name);
- if (ret < 0)
- goto out;
- entry.name = decrypted_name.name;
- entry.len = decrypted_name.len;
- }
-
- if (quick)
- ret = utf8_strncasecmp_folded(um, name, &entry);
- else
- ret = utf8_strncasecmp(um, name, &entry);
- if (ret < 0) {
- /* Handle invalid character sequence as either an error
- * or as an opaque byte sequence.
- */
- if (sb_has_strict_encoding(sb))
- ret = -EINVAL;
- else if (name->len != entry.len)
- ret = 1;
- else
- ret = !!memcmp(name->name, entry.name, entry.len);
- }
-out:
- kfree(decrypted_name.name);
- return ret;
-}
-
int ext4_fname_setup_ci_filename(struct inode *dir, const struct qstr *iname,
struct ext4_filename *name)
{
@@ -1503,20 +1451,35 @@ static bool ext4_match(struct inode *parent,
#if IS_ENABLED(CONFIG_UNICODE)
if (IS_CASEFOLDED(parent) &&
(!IS_ENCRYPTED(parent) || fscrypt_has_encryption_key(parent))) {
- if (fname->cf_name.name) {
- if (IS_ENCRYPTED(parent)) {
- if (fname->hinfo.hash != EXT4_DIRENT_HASH(de) ||
- fname->hinfo.minor_hash !=
- EXT4_DIRENT_MINOR_HASH(de)) {
+ int ret;
- return false;
- }
- }
- return !ext4_ci_compare(parent, &fname->cf_name,
- de->name, de->name_len, true);
+ /*
+ * Just checking IS_ENCRYPTED(parent) below is not
+ * sufficient to decide whether one can use the hash for
+ * skipping the string comparison, because the key might
+ * have been added right after
+ * ext4_fname_setup_ci_filename(). In this case, a hash
+ * mismatch will be a false negative. Therefore, make
+ * sure cf_name was properly initialized before
+ * considering the calculated hash.
+ */
+ if (IS_ENCRYPTED(parent) && fname->cf_name.name &&
+ (fname->hinfo.hash != EXT4_DIRENT_HASH(de) ||
+ fname->hinfo.minor_hash != EXT4_DIRENT_MINOR_HASH(de)))
+ return false;
+
+ ret = generic_ci_match(parent, fname->usr_fname,
+ &fname->cf_name, de->name,
+ de->name_len);
+ if (ret < 0) {
+ /*
+ * Treat comparison errors as not a match. The
+ * only case where it happens is on a disk
+ * corruption or ENOMEM.
+ */
+ return false;
}
- return !ext4_ci_compare(parent, fname->usr_fname, de->name,
- de->name_len, false);
+ return ret;
}
#endif
--
2.34.1
From: Gabriel Krisman Bertazi <[email protected]>
Now that ci_match is part of libfs, make f2fs reuse it instead of having
a different implementation.
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
Signed-off-by: Eugen Hristev <[email protected]>
---
fs/f2fs/dir.c | 58 ++++-----------------------------------------------
1 file changed, 4 insertions(+), 54 deletions(-)
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 266279b82afc..0601b4c8bacc 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -183,58 +183,6 @@ static struct f2fs_dir_entry *find_in_block(struct inode *dir,
return f2fs_find_target_dentry(&d, fname, max_slots);
}
-#if IS_ENABLED(CONFIG_UNICODE)
-/*
- * Test whether a case-insensitive directory entry matches the filename
- * being searched for.
- *
- * Returns 1 for a match, 0 for no match, and -errno on an error.
- */
-static int f2fs_match_ci_name(const struct inode *dir, const struct qstr *name,
- const u8 *de_name, u32 de_name_len)
-{
- const struct super_block *sb = dir->i_sb;
- const struct unicode_map *um = sb->s_encoding;
- struct fscrypt_str decrypted_name = FSTR_INIT(NULL, de_name_len);
- struct qstr entry = QSTR_INIT(de_name, de_name_len);
- int res;
-
- if (IS_ENCRYPTED(dir)) {
- const struct fscrypt_str encrypted_name =
- FSTR_INIT((u8 *)de_name, de_name_len);
-
- if (WARN_ON_ONCE(!fscrypt_has_encryption_key(dir)))
- return -EINVAL;
-
- decrypted_name.name = kmalloc(de_name_len, GFP_KERNEL);
- if (!decrypted_name.name)
- return -ENOMEM;
- res = fscrypt_fname_disk_to_usr(dir, 0, 0, &encrypted_name,
- &decrypted_name);
- if (res < 0)
- goto out;
- entry.name = decrypted_name.name;
- entry.len = decrypted_name.len;
- }
-
- res = utf8_strncasecmp_folded(um, name, &entry);
- /*
- * In strict mode, ignore invalid names. In non-strict mode,
- * fall back to treating them as opaque byte sequences.
- */
- if (res < 0 && !sb_has_strict_encoding(sb)) {
- res = name->len == entry.len &&
- memcmp(name->name, entry.name, name->len) == 0;
- } else {
- /* utf8_strncasecmp_folded returns 0 on match */
- res = (res == 0);
- }
-out:
- kfree(decrypted_name.name);
- return res;
-}
-#endif /* CONFIG_UNICODE */
-
static inline int f2fs_match_name(const struct inode *dir,
const struct f2fs_filename *fname,
const u8 *de_name, u32 de_name_len)
@@ -243,8 +191,10 @@ static inline int f2fs_match_name(const struct inode *dir,
#if IS_ENABLED(CONFIG_UNICODE)
if (fname->cf_name.name)
- return f2fs_match_ci_name(dir, &fname->cf_name,
- de_name, de_name_len);
+ return generic_ci_match(dir, fname->usr_fname,
+ &fname->cf_name,
+ de_name, de_name_len);
+
#endif
f.usr_fname = fname->usr_fname;
f.disk_name = fname->disk_name;
--
2.34.1
From: Gabriel Krisman Bertazi <[email protected]>
If the volume is in strict mode, ext4_ci_compare can report a broken
encoding name. This will not trigger on a bad lookup, which is caught
earlier, only if the actual disk name is bad.
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
Signed-off-by: Eugen Hristev <[email protected]>
---
fs/ext4/namei.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 2d0ee232fbe7..3268cf45d9db 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1477,6 +1477,9 @@ static bool ext4_match(struct inode *parent,
* only case where it happens is on a disk
* corruption or ENOMEM.
*/
+ if (ret == -EINVAL)
+ EXT4_ERROR_INODE(parent,
+ "Directory contains filename that is invalid UTF-8");
return false;
}
return ret;
--
2.34.1
If the volume is in strict mode, generi c_ci_compare can report a broken
encoding name. This will not trigger on a bad lookup, which is caught
earlier, only if the actual disk name is bad.
Suggested-by: Gabriel Krisman Bertazi <[email protected]>
Signed-off-by: Eugen Hristev <[email protected]>
---
fs/f2fs/dir.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 0601b4c8bacc..1bb98970a56a 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -190,11 +190,22 @@ static inline int f2fs_match_name(const struct inode *dir,
struct fscrypt_name f;
#if IS_ENABLED(CONFIG_UNICODE)
- if (fname->cf_name.name)
- return generic_ci_match(dir, fname->usr_fname,
- &fname->cf_name,
- de_name, de_name_len);
-
+ if (fname->cf_name.name) {
+ int ret = generic_ci_match(dir, fname->usr_fname,
+ &fname->cf_name,
+ de_name, de_name_len);
+ if (ret < 0) {
+ /*
+ * Treat comparison errors as not a match. The
+ * only case where it happens is on a disk
+ * corruption or ENOMEM.
+ */
+ if (ret == -EINVAL)
+ f2fs_warn(F2FS_SB(dir->i_sb),
+ "Directory contains filename that is invalid UTF-8");
+ }
+ return ret;
+ }
#endif
f.usr_fname = fname->usr_fname;
f.disk_name = fname->disk_name;
--
2.34.1
From: Gabriel Krisman Bertazi <[email protected]>
Instead of a bunch of ifdefs, make the unicode built checks part of the
code flow where possible, as requested by Torvalds.
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
[[email protected]: port to 6.8-rc3]
Signed-off-by: Eugen Hristev <[email protected]>
---
fs/ext4/crypto.c | 19 +++----------------
fs/ext4/ext4.h | 33 +++++++++++++++++++++------------
fs/ext4/namei.c | 14 +++++---------
fs/ext4/super.c | 4 +---
4 files changed, 30 insertions(+), 40 deletions(-)
diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
index 7ae0b61258a7..1d2f8b79529c 100644
--- a/fs/ext4/crypto.c
+++ b/fs/ext4/crypto.c
@@ -31,12 +31,7 @@ int ext4_fname_setup_filename(struct inode *dir, const struct qstr *iname,
ext4_fname_from_fscrypt_name(fname, &name);
-#if IS_ENABLED(CONFIG_UNICODE)
- err = ext4_fname_setup_ci_filename(dir, iname, fname);
- if (err)
- ext4_fname_free_filename(fname);
-#endif
- return err;
+ return ext4_fname_setup_ci_filename(dir, iname, fname);
}
int ext4_fname_prepare_lookup(struct inode *dir, struct dentry *dentry,
@@ -51,12 +46,7 @@ int ext4_fname_prepare_lookup(struct inode *dir, struct dentry *dentry,
ext4_fname_from_fscrypt_name(fname, &name);
-#if IS_ENABLED(CONFIG_UNICODE)
- err = ext4_fname_setup_ci_filename(dir, &dentry->d_name, fname);
- if (err)
- ext4_fname_free_filename(fname);
-#endif
- return err;
+ return ext4_fname_setup_ci_filename(dir, &dentry->d_name, fname);
}
void ext4_fname_free_filename(struct ext4_filename *fname)
@@ -70,10 +60,7 @@ void ext4_fname_free_filename(struct ext4_filename *fname)
fname->usr_fname = NULL;
fname->disk_name.name = NULL;
-#if IS_ENABLED(CONFIG_UNICODE)
- kfree(fname->cf_name.name);
- fname->cf_name.name = NULL;
-#endif
+ ext4_fname_free_ci_filename(fname);
}
static bool uuid_is_zero(__u8 u[16])
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 4061d11b9763..c68f48f706cd 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2740,8 +2740,25 @@ ext4_fsblk_t ext4_inode_to_goal_block(struct inode *);
#if IS_ENABLED(CONFIG_UNICODE)
extern int ext4_fname_setup_ci_filename(struct inode *dir,
- const struct qstr *iname,
- struct ext4_filename *fname);
+ const struct qstr *iname,
+ struct ext4_filename *fname);
+
+static inline void ext4_fname_free_ci_filename(struct ext4_filename *fname)
+{
+ kfree(fname->cf_name.name);
+ fname->cf_name.name = NULL;
+}
+#else
+static inline int ext4_fname_setup_ci_filename(struct inode *dir,
+ const struct qstr *iname,
+ struct ext4_filename *fname)
+{
+ return 0;
+}
+
+static inline void ext4_fname_free_ci_filename(struct ext4_filename *fname)
+{
+}
#endif
/* ext4 encryption related stuff goes here crypto.c */
@@ -2764,16 +2781,11 @@ static inline int ext4_fname_setup_filename(struct inode *dir,
int lookup,
struct ext4_filename *fname)
{
- int err = 0;
fname->usr_fname = iname;
fname->disk_name.name = (unsigned char *) iname->name;
fname->disk_name.len = iname->len;
-#if IS_ENABLED(CONFIG_UNICODE)
- err = ext4_fname_setup_ci_filename(dir, iname, fname);
-#endif
-
- return err;
+ return ext4_fname_setup_ci_filename(dir, iname, fname);
}
static inline int ext4_fname_prepare_lookup(struct inode *dir,
@@ -2785,10 +2797,7 @@ static inline int ext4_fname_prepare_lookup(struct inode *dir,
static inline void ext4_fname_free_filename(struct ext4_filename *fname)
{
-#if IS_ENABLED(CONFIG_UNICODE)
- kfree(fname->cf_name.name);
- fname->cf_name.name = NULL;
-#endif
+ ext4_fname_free_ci_filename(fname);
}
static inline int ext4_ioctl_get_encryption_pwsalt(struct file *filp,
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 3268cf45d9db..a5d9e5b01015 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1834,8 +1834,7 @@ static struct dentry *ext4_lookup(struct inode *dir, struct dentry *dentry, unsi
}
}
-#if IS_ENABLED(CONFIG_UNICODE)
- if (!inode && IS_CASEFOLDED(dir)) {
+ if (IS_ENABLED(CONFIG_UNICODE) && !inode && IS_CASEFOLDED(dir)) {
/* Eventually we want to call d_add_ci(dentry, NULL)
* for negative dentries in the encoding case as
* well. For now, prevent the negative dentry
@@ -1843,7 +1842,7 @@ static struct dentry *ext4_lookup(struct inode *dir, struct dentry *dentry, unsi
*/
return NULL;
}
-#endif
+
return d_splice_alias(inode, dentry);
}
@@ -3173,16 +3172,14 @@ static int ext4_rmdir(struct inode *dir, struct dentry *dentry)
ext4_fc_track_unlink(handle, dentry);
retval = ext4_mark_inode_dirty(handle, dir);
-#if IS_ENABLED(CONFIG_UNICODE)
/* VFS negative dentries are incompatible with Encoding and
* Case-insensitiveness. Eventually we'll want avoid
* invalidating the dentries here, alongside with returning the
* negative dentries at ext4_lookup(), when it is better
* supported by the VFS for the CI case.
*/
- if (IS_CASEFOLDED(dir))
+ if (IS_ENABLED(CONFIG_UNICODE) && IS_CASEFOLDED(dir))
d_invalidate(dentry);
-#endif
end_rmdir:
brelse(bh);
@@ -3284,16 +3281,15 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry)
goto out_trace;
retval = __ext4_unlink(dir, &dentry->d_name, d_inode(dentry), dentry);
-#if IS_ENABLED(CONFIG_UNICODE)
+
/* VFS negative dentries are incompatible with Encoding and
* Case-insensitiveness. Eventually we'll want avoid
* invalidating the dentries here, alongside with returning the
* negative dentries at ext4_lookup(), when it is better
* supported by the VFS for the CI case.
*/
- if (IS_CASEFOLDED(dir))
+ if (IS_ENABLED(CONFIG_UNICODE) && IS_CASEFOLDED(dir))
d_invalidate(dentry);
-#endif
out_trace:
trace_ext4_unlink_exit(dentry, retval);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 215b4614eb15..179083728b4b 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3609,14 +3609,12 @@ int ext4_feature_set_ok(struct super_block *sb, int readonly)
return 0;
}
-#if !IS_ENABLED(CONFIG_UNICODE)
- if (ext4_has_feature_casefold(sb)) {
+ if (!IS_ENABLED(CONFIG_UNICODE) && ext4_has_feature_casefold(sb)) {
ext4_msg(sb, KERN_ERR,
"Filesystem with casefold feature cannot be "
"mounted without CONFIG_UNICODE");
return 0;
}
-#endif
if (readonly)
return 1;
--
2.34.1
Eugen Hristev <[email protected]> writes:
> From: Gabriel Krisman Bertazi <[email protected]>
>
> generic_ci_match can be used by case-insensitive filesystems to compare
> strings under lookup with dirents in a case-insensitive way. This
> function is currently reimplemented by each filesystem supporting
> casefolding, so this reduces code duplication in filesystem-specific
> code.
>
> Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
> [[email protected]: rework to first test the exact match]
> Signed-off-by: Eugen Hristev <[email protected]>
> ---
> fs/libfs.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/fs.h | 4 +++
> 2 files changed, 85 insertions(+)
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index c297953db948..c107c24f33b9 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -1776,6 +1776,87 @@ static const struct dentry_operations generic_ci_dentry_ops = {
> .d_revalidate = fscrypt_d_revalidate,
> #endif
> };
> +
> +/**
> + * generic_ci_match() - Match a name (case-insensitively) with a dirent.
> + * This is a filesystem helper for comparison with directory entries.
> + * generic_ci_d_compare should be used in VFS' ->d_compare instead.
> + *
> + * @parent: Inode of the parent of the dirent under comparison
> + * @name: name under lookup.
> + * @folded_name: Optional pre-folded name under lookup
> + * @de_name: Dirent name.
> + * @de_name_len: dirent name length.
> + *
> + * Test whether a case-insensitive directory entry matches the filename
> + * being searched. If @folded_name is provided, it is used instead of
> + * recalculating the casefold of @name.
> + *
> + * Return: > 0 if the directory entry matches, 0 if it doesn't match, or
> + * < 0 on error.
> + */
> +int generic_ci_match(const struct inode *parent,
> + const struct qstr *name,
> + const struct qstr *folded_name,
> + const u8 *de_name, u32 de_name_len)
> +{
> + const struct super_block *sb = parent->i_sb;
> + const struct unicode_map *um = sb->s_encoding;
> + struct fscrypt_str decrypted_name = FSTR_INIT(NULL, de_name_len);
> + struct qstr dirent = QSTR_INIT(de_name, de_name_len);
> + int res, match = 0;
> +
> + if (IS_ENCRYPTED(parent)) {
> + const struct fscrypt_str encrypted_name =
> + FSTR_INIT((u8 *) de_name, de_name_len);
> +
> + if (WARN_ON_ONCE(!fscrypt_has_encryption_key(parent)))
> + return -EINVAL;
> +
> + decrypted_name.name = kmalloc(de_name_len, GFP_KERNEL);
> + if (!decrypted_name.name)
> + return -ENOMEM;
> + res = fscrypt_fname_disk_to_usr(parent, 0, 0, &encrypted_name,
> + &decrypted_name);
> + if (res < 0)
> + goto out;
> + dirent.name = decrypted_name.name;
> + dirent.len = decrypted_name.len;
> + }
> +
> + /*
> + * Attempt a case-sensitive match first. It is cheaper and
> + * should cover most lookups, including all the sane
> + * applications that expect a case-sensitive filesystem.
> + */
> + if (folded_name->name) {
> + if (dirent.len == folded_name->len &&
> + !memcmp(folded_name->name, dirent.name, dirent.len)) {
> + match = 1;
> + goto out;
> + }
> + res = utf8_strncasecmp_folded(um, folded_name, &dirent);
> + } else {
> + if (dirent.len == name->len &&
> + !memcmp(name->name, dirent.name, dirent.len) &&
> + (!sb_has_strict_encoding(sb) || !utf8_validate(um, name))) {
> + match = 1;
> + goto out;
> + }
> + res = utf8_strncasecmp(um, name, &dirent);
> + }
> +
> +out:
> + kfree(decrypted_name.name);
> + if (match) /* matched by direct comparison */
> + return 1;
> + else if (!res) /* matched by utf8 comparison */
> + return 1;
> + else if (res < 0) /* error on utf8 comparison */
> + return res;
> + return 0; /* no match */
> +}
I think I've made this comment before, but I'd prefer this to be written
in a much simpler way
if (res < 0)
return res;
return (match || !res);
Other than that, this looks good to me.
--
Gabriel Krisman Bertazi
On 3/14/24 01:36, Gabriel Krisman Bertazi wrote:
> Eugen Hristev <[email protected]> writes:
>
>> +void f2fs_free_casefolded_name(struct f2fs_filename *fname)
>> +{
>> + unsigned char *buf = (unsigned char *)fname->cf_name.name;
>> +
>> + kmem_cache_free(f2fs_cf_name_slab, buf);
>> + fname->cf_name.name = NULL;
>
> In my previous review, I mentioned you could drop the "if (buf)" check
> here *if and only if* you used kfree. By doing an unchecked kmem_cache_free
> like this, you will immediately hit an Oops in the first lookup (see below).
>
> Please, make sure you actually stress test this patchset with fstests
> against both f2fs and ext4 before sending each new version.
I did run the xfstests, however, maybe I did not run the full suite, or maybe I am
running it in a wrong way ?
How are you running the kvm-xfstests with qemu ? Can you share your command
arguments please ?
Thanks
>
> Thanks,
>
>
> [ 74.202044] F2FS-fs (loop0): Using encoding defined by superblock: utf8-12.1.0 with flags 0x0
> [ 74.206592] F2FS-fs (loop0): Found nat_bits in checkpoint
> [ 74.221467] F2FS-fs (loop0): Mounted with checkpoint version = 3e684111
> FSTYP -- f2fs
> PLATFORM -- Linux/x86_64 sle15sp5 6.7.0-gf27274eae416 #8 SMP PREEMPT_DYNAMIC Thu Mar 14 00:22:47 CET 2024
> MKFS_OPTIONS -- -O encrypt /dev/loop1
> MOUNT_OPTIONS -- -o acl,user_xattr /dev/loop1 /root/work/scratch
>
> [ 75.038385] F2FS-fs (loop1): Found nat_bits in checkpoint
> [ 75.054311] F2FS-fs (loop1): Mounted with checkpoint version = 6b9fbccb
> [ 75.176328] F2FS-fs (loop0): Using encoding defined by superblock: utf8-12.1.0 with flags 0x0
> [ 75.179261] F2FS-fs (loop0): Found nat_bits in checkpoint
> [ 75.194264] F2FS-fs (loop0): Mounted with checkpoint version = 3e684114
> f2fs/001 1s ... [ 75.570867] run fstests f2fs/001 at 2024-03-14 00:24:33
> [ 75.753604] BUG: unable to handle page fault for address: fffff14ad2000008
> [ 75.754209] #PF: supervisor read access in kernel mode
> [ 75.754647] #PF: error_code(0x0000) - not-present page
> [ 75.755077] PGD 0 P4D 0
> [ 75.755300] Oops: 0000 [#1] PREEMPT SMP NOPTI
> [ 75.755683] CPU: 0 PID: 2740 Comm: xfs_io Not tainted 6.7.0-gf27274eae416 #8
> [ 75.756266] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS unknown 2/2/2022
> [ 75.756911] RIP: 0010:kmem_cache_free+0x6a/0x320
> [ 75.757309] Code: 80 48 01 d8 0f 82 b4 02 00 00 48 c7 c2 00 00 00 80 48 2b 15 f8 c2 18 01 48 01 d0 48 c1 e8 0c 48 c1 e0 06 48 03 05 d6 c2 18 01 <48> 8b 50 08 49 89 c6 f6 c2 01 0f 85 ea 01 00 00 0f 1f 44 00 00 49
> [ 75.758834] RSP: 0018:ffffa59bc231bb10 EFLAGS: 00010286
> [ 75.759270] RAX: fffff14ad2000000 RBX: 0000000000000000 RCX: 0000000000000000
> [ 75.759860] RDX: 0000620400000000 RSI: 0000000000000000 RDI: ffff9dfc80043600
> [ 75.760450] RBP: ffffa59bc231bb30 R08: ffffa59bc231b9a0 R09: 00000000000003fa
> [ 75.761037] R10: 00000000000fd024 R11: 0000000000000107 R12: ffff9dfc80043600
> [ 75.761626] R13: ffffffff8404dc7a R14: 0000000000000000 R15: ffff9dfc8f1aa000
> [ 75.762221] FS: 00007f9601efb780(0000) GS:ffff9dfcfbc00000(0000) knlGS:0000000000000000
> [ 75.762888] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 75.763372] CR2: fffff14ad2000008 CR3: 0000000111750000 CR4: 0000000000750ef0
> [ 75.763962] PKRU: 55555554
> [ 75.764194] Call Trace:
> [ 75.764435] <TASK>
> [ 75.764677] ? __die_body+0x1a/0x60
> [ 75.764982] ? page_fault_oops+0x154/0x440
> [ 75.765335] ? srso_alias_return_thunk+0x5/0xfbef5
> [ 75.765760] ? search_module_extables+0x46/0x70
> [ 75.766149] ? srso_alias_return_thunk+0x5/0xfbef5
> [ 75.766548] ? fixup_exception+0x22/0x300
> [ 75.766892] ? srso_alias_return_thunk+0x5/0xfbef5
> [ 75.767292] ? exc_page_fault+0xa6/0x140
> [ 75.767633] ? asm_exc_page_fault+0x22/0x30
> [ 75.767995] ? f2fs_free_filename+0x2a/0x40
> [ 75.768362] ? kmem_cache_free+0x6a/0x320
> [ 75.768703] ? f2fs_free_filename+0x2a/0x40
> [ 75.769061] f2fs_free_filename+0x2a/0x40
> [ 75.769403] f2fs_lookup+0x19f/0x380
> [ 75.769712] __lookup_slow+0x8b/0x130
> [ 75.770034] walk_component+0xfc/0x170
> [ 75.770353] path_lookupat+0x69/0x140
> [ 75.770664] filename_lookup+0xe1/0x1c0
> [ 75.770991] ? srso_alias_return_thunk+0x5/0xfbef5
> [ 75.771393] ? srso_alias_return_thunk+0x5/0xfbef5
> [ 75.771792] ? do_wp_page+0x3f6/0xbf0
> [ 75.772109] ? srso_alias_return_thunk+0x5/0xfbef5
> [ 75.772523] ? preempt_count_add+0x70/0xa0
> [ 75.772902] ? vfs_statx+0x89/0x180
> [ 75.773224] vfs_statx+0x89/0x180
> [ 75.773530] ? srso_alias_return_thunk+0x5/0xfbef5
> [ 75.773939] vfs_fstatat+0x80/0xa0
> [ 75.774237] __do_sys_newfstatat+0x26/0x60
> [ 75.774595] ? srso_alias_return_thunk+0x5/0xfbef5
> [ 75.775021] ? srso_alias_return_thunk+0x5/0xfbef5
> [ 75.775448] ? srso_alias_return_thunk+0x5/0xfbef5
> [ 75.775878] ? do_user_addr_fault+0x563/0x7c0
> [ 75.776273] ? srso_alias_return_thunk+0x5/0xfbef5
> [ 75.776699] do_syscall_64+0x50/0x110
> [ 75.777028] entry_SYSCALL_64_after_hwframe+0x6e/0x76
> [ 75.777479] RIP: 0033:0x7f9601b07aea
> [ 75.777793] Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 41 89 ca b8 06 01 00 00 0f 05 <3d> 00 f0 ff ff 77 07 31 c0 c3 0f 1f 40 00 48 8b 15 01 23 0e 00 f7
> [ 75.779391] RSP: 002b:00007ffc160eaae8 EFLAGS: 00000246 ORIG_RAX: 0000000000000106
> [ 75.780050] RAX: ffffffffffffffda RBX: 0000000000000042 RCX: 00007f9601b07aea
> [ 75.780663] RDX: 00007ffc160eab80 RSI: 00007ffc160ecb88 RDI: 00000000ffffff9c
> [ 75.781278] RBP: 00007ffc160ead20 R08: 00007ffc160ead20 R09: 0000000000000000
> [ 75.781902] R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffc160eae70
> [ 75.782532] R13: 00007ffc160ecb88 R14: 00007ffc160eae70 R15: 0000000000000020
> [ 75.783150] </TASK>
> [ 75.783349] Modules linked in:
> [ 75.783628] CR2: fffff14ad2000008
> [ 75.783918] ---[ end trace 0000000000000000 ]---
> [ 75.784315] RIP: 0010:kmem_cache_free+0x6a/0x320
> [ 75.784718] Code: 80 48 01 d8 0f 82 b4 02 00 00 48 c7 c2 00 00 00 80 48 2b 15 f8 c2 18 01 48 01 d0 48 c1 e8 0c 48 c1 e0 06 48 03 05 d6 c2 18 01 <48> 8b 50 08 49 89 c6 f6 c2 01 0f 85 ea 01 00 00 0f 1f 44 00 00 49
> [ 75.786294] RSP: 0018:ffffa59bc231bb10 EFLAGS: 00010286
> [ 75.786747] RAX: fffff14ad2000000 RBX: 0000000000000000 RCX: 0000000000000000
> [ 75.787369] RDX: 0000620400000000 RSI: 0000000000000000 RDI: ffff9dfc80043600
> [ 75.788016] RBP: ffffa59bc231bb30 R08: ffffa59bc231b9a0 R09: 00000000000003fa
> [ 75.788672] R10: 00000000000fd024 R11: 0000000000000107 R12: ffff9dfc80043600
> [ 75.789296] R13: ffffffff8404dc7a R14: 0000000000000000 R15: ffff9dfc8f1aa000
> [ 75.789938] FS: 00007f9601efb780(0000) GS:ffff9dfcfbc00000(0000) knlGS:0000000000000000
> [ 75.790677] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 75.791212] CR2: fffff14ad2000008 CR3: 0000000111750000 CR4: 0000000000750ef0
> [ 75.791862] PKRU: 55555554
> [ 75.792112] Kernel panic - not syncing: Fatal exception
> [ 75.792797] Kernel Offset: 0x2a00000 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffffbfffffff)
>
>
Eugen Hristev <[email protected]> writes:
>> Please, make sure you actually stress test this patchset with fstests
>> against both f2fs and ext4 before sending each new version.
>
> I did run the xfstests, however, maybe I did not run the full suite, or maybe I am
> running it in a wrong way ?
No worries. Did you manage to reproduce it?
> How are you running the kvm-xfstests with qemu ? Can you share your command
> arguments please ?
I don't use kvm-xfstests. I run ./check directly:
export SCRATCH_DEV=/dev/loop1
export SCRATCH_MNT=$BASEMNT/scratch
export TEST_DEV=/dev/loop0
export TEST_DIR=$BASEMNT/test
export RESULT_BASE=${BASEMNT}/results
export REPORT_DIR=${BASEMNT}/report
export FSTYP=f2fs
mkfs.f2fs -f -C utf8 -O casefold ${TEST_DEV}
./check -g encrypt,quick
--
Gabriel Krisman Bertazi
On 3/14/24 16:41, Gabriel Krisman Bertazi wrote:
> Eugen Hristev <[email protected]> writes:
>
>>> Please, make sure you actually stress test this patchset with fstests
>>> against both f2fs and ext4 before sending each new version.
>>
>> I did run the xfstests, however, maybe I did not run the full suite, or maybe I am
>> running it in a wrong way ?
>
> No worries. Did you manage to reproduce it?
Yes, thank you, using qemu on the x86_64 with your commands below.
While the oops was caused by that wrong kfree call, fixing it and moving further
got me into further problems.
I am unsure though how these patches cause it.
Here is a snippet of the problem that occurs now :
generic/417 12s ... [ 616.265444] run fstests generic/417 at 2024-03-18 09:22:48
[ 616.768435] ------------[ cut here ]------------
[ 616.769493] WARNING: CPU: 4 PID: 133 at block/blk-merge.c:580
__blk_rq_map_sg+0x46a/0x480
[ 616.771253] Modules linked in:
[ 616.771873] CPU: 4 PID: 133 Comm: kworker/4:1H Not tainted
6.7.0-09941-g554c4640dff5 #18
[ 616.773660] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1
04/01/2014
[ 616.775573] Workqueue: kblockd blk_mq_run_work_fn
[ 616.776570] RIP: 0010:__blk_rq_map_sg+0x46a/0x480
[ 616.777547] Code: 17 fe ff ff 44 89 58 0c 48 8b 01 e9 ec fc ff ff 43 8d 3c 06 48
8b 14 24 81 ff 00 10 00 00 0f 86 af fc ff ff e9 02 fe ff ff 90 <0f> 0b 90 e9 76 fd
ff ff 90 0f 0b 90 0f 0b 0f 1f 84 00 00 00 00 00
[ 616.781245] RSP: 0018:ffff97e4804f3b98 EFLAGS: 00010216
[ 616.782322] RAX: 000000000000005e RBX: 0000000000000f10 RCX: ffff8f5701eed000
[ 616.783929] RDX: ffffdc0c4052df82 RSI: 0000000000001000 RDI: 00000000fffffffc
[ 616.785426] RBP: 000000000000005e R08: 0000000000000000 R09: ffff8f5702120000
[ 616.787065] R10: ffffdc0c4052df80 R11: 0000000000000000 R12: ffff8f5702118000
[ 616.788650] R13: 0000000000000000 R14: 0000000000001000 R15: ffffdc0c4052df80
[ 616.790129] FS: 0000000000000000(0000) GS:ffff8f577db00000(0000)
knlGS:0000000000000000
[ 616.791826] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 616.793069] CR2: 00007fbe596adc98 CR3: 000000001162e000 CR4: 0000000000350ef0
[ 616.794528] Call Trace:
[ 616.795093] <TASK>
[ 616.795541] ? __warn+0x7f/0x130
[ 616.796242] ? __blk_rq_map_sg+0x46a/0x480
[ 616.797101] ? report_bug+0x199/0x1b0
[ 616.797843] ? handle_bug+0x3d/0x70
[ 616.798656] ? exc_invalid_op+0x18/0x70
[ 616.799461] ? asm_exc_invalid_op+0x1a/0x20
[ 616.800313] ? __blk_rq_map_sg+0x46a/0x480
[ 616.801207] ? __blk_rq_map_sg+0xfc/0x480
[ 616.802213] scsi_alloc_sgtables+0xae/0x2b0
[ 616.803258] sd_init_command+0x181/0x860
[ 616.804111] scsi_queue_rq+0x7c3/0xae0
[ 616.804910] blk_mq_dispatch_rq_list+0x2bf/0x7c0
[ 616.805962] __blk_mq_sched_dispatch_requests+0x40a/0x5c0
[ 616.807226] blk_mq_sched_dispatch_requests+0x34/0x60
[ 616.808389] blk_mq_run_work_fn+0x5f/0x70
[ 616.809332] process_one_work+0x136/0x2f0
[ 616.810268] ? __pfx_worker_thread+0x10/0x10
[ 616.811320] worker_thread+0x2ef/0x400
[ 616.812215] ? __pfx_worker_thread+0x10/0x10
[ 616.813205] kthread+0xd5/0x100
[ 616.813907] ? __pfx_kthread+0x10/0x10
[ 616.814787] ret_from_fork+0x2f/0x50
[ 616.815598] ? __pfx_kthread+0x10/0x10
[ 616.816394] ret_from_fork_asm+0x1b/0x30
[ 616.817210] </TASK>
[ 616.817658] ---[ end trace 0000000000000000 ]---
[ 616.818687] ------------[ cut here ]------------
[ 616.819697] kernel BUG at drivers/scsi/scsi_lib.c:1068!
Do you have any ideas ?
Thanks !
Eugen
>
>> How are you running the kvm-xfstests with qemu ? Can you share your command
>> arguments please ?
>
> I don't use kvm-xfstests. I run ./check directly:
>
> export SCRATCH_DEV=/dev/loop1
> export SCRATCH_MNT=$BASEMNT/scratch
> export TEST_DEV=/dev/loop0
> export TEST_DIR=$BASEMNT/test
> export RESULT_BASE=${BASEMNT}/results
> export REPORT_DIR=${BASEMNT}/report
> export FSTYP=f2fs
>
> mkfs.f2fs -f -C utf8 -O casefold ${TEST_DEV}
>
> ./check -g encrypt,quick
>