[f2fs-dev] [PATCH v5 00/10] Support negative dentries on case-insensitive ext4 and f2fs

[f2fs-dev] [PATCH v5 00/10] Support negative dentries on case-insensitive ext4 and f2fs

[Gabriel Krisman Bertazi]

Hi,

This is the v5 of this patchset.  Thanks Christian and Eric for you
review.

In this version, the patchset grew a bit because it adds:

  - a preparation patch to merge d_revalidate_name and d_revalidate,
  attending Christian's request.  The original patch is now touching
  several filesystems to update the hook signature, so it has grown
  quite a bit. But, all of that was autogenerated with coccinelle and
  tested with allyesconfig only.

  - A new patch to expose a helper from libfs that I use in ecryptfs.

  - Code to prevent ecryptfs from mounting on top of casefolded
  directories. Also following on Christian's review.

Other than these, there are some minor fixes, listed in each patch
changelog, and more clarifications on the locking, thanks to the
excellent feedback from Eric.

Eric, I believe I have covered the cases where instantiation can happen
and I don't think it is possible for a dentry to become positive amidst
the d_revalidation, since we are holding the inode parent lock to
synchronize with creations.  Please let me know if you find anything
else.

Finally, I've dropped the r-b tags from patch "fs: Expose name under
lookup to d_revalidate hooks", because it changed too much since
the time of review.

Regarding testing, I verified it doesn't regress fstests for f2fs and
ext4, verified building all filesystems work fine with allyesconfig,
even with variation of CONFIG_FS_ENCRYPTION and CONFIG_UNICODE, and,
finally, I checked I wasn't able to mount or lookup casefolded
directories with ecryptfs and overlayfs.

Thanks,

Gabriel Krisman Bertazi (10):
  fs: Expose helper to check if a directory needs casefolding
  ecryptfs: Reject casefold directory inodes
  9p: Split ->weak_revalidate from ->revalidate
  fs: Expose name under lookup to d_revalidate hooks
  fs: Add DCACHE_CASEFOLDED_NAME flag
  libfs: Validate negative dentries in case-insensitive directories
  libfs: Chain encryption checks after case-insensitive revalidation
  libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops
  ext4: Enable negative dentries on case-insensitive lookup
  f2fs: Enable negative dentries on case-insensitive lookup

 Documentation/filesystems/locking.rst |   3 +-
 Documentation/filesystems/vfs.rst     |  11 ++-
 fs/9p/vfs_dentry.c                    |  11 ++-
 fs/afs/dir.c                          |   6 +-
 fs/afs/dynroot.c                      |   4 +-
 fs/ceph/dir.c                         |   3 +-
 fs/coda/dir.c                         |   3 +-
 fs/crypto/fname.c                     |   3 +-
 fs/dcache.c                           |   8 ++
 fs/ecryptfs/dentry.c                  |   5 +-
 fs/ecryptfs/inode.c                   |   8 ++
 fs/exfat/namei.c                      |   3 +-
 fs/ext4/namei.c                       |  35 +-------
 fs/f2fs/namei.c                       |  25 +-----
 fs/fat/namei_vfat.c                   |   6 +-
 fs/fuse/dir.c                         |   3 +-
 fs/gfs2/dentry.c                      |   3 +-
 fs/hfs/sysdep.c                       |   3 +-
 fs/jfs/namei.c                        |   3 +-
 fs/kernfs/dir.c                       |   3 +-
 fs/libfs.c                            | 124 +++++++++++++++++---------
 fs/namei.c                            |  18 ++--
 fs/nfs/dir.c                          |   9 +-
 fs/ocfs2/dcache.c                     |   4 +-
 fs/orangefs/dcache.c                  |   3 +-
 fs/overlayfs/super.c                  |  20 +++--
 fs/proc/base.c                        |   6 +-
 fs/proc/fd.c                          |   3 +-
 fs/proc/generic.c                     |   6 +-
 fs/proc/proc_sysctl.c                 |   3 +-
 fs/reiserfs/xattr.c                   |   3 +-
 fs/smb/client/dir.c                   |   3 +-
 fs/vboxsf/dir.c                       |   4 +-
 include/linux/dcache.h                |  10 ++-
 include/linux/fs.h                    |  21 +++++
 include/linux/fscrypt.h               |   4 +-
 36 files changed, 242 insertions(+), 148 deletions(-)

-- 
2.41.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding

[Gabriel Krisman Bertazi]

In preparation to use it in ecryptfs, move needs_casefolding into a
public header and give it a namespaced name.

Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
---
 fs/libfs.c         | 14 ++------------
 include/linux/fs.h | 21 +++++++++++++++++++++
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index 5b851315eeed..8d0b64cfd5da 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1381,16 +1381,6 @@ bool is_empty_dir_inode(struct inode *inode)
 }
 
 #if IS_ENABLED(CONFIG_UNICODE)
-/*
- * Determine if the name of a dentry should be casefolded.
- *
- * Return: if names will need casefolding
- */
-static bool needs_casefold(const struct inode *dir)
-{
-	return IS_CASEFOLDED(dir) && dir->i_sb->s_encoding;
-}
-
 /**
  * generic_ci_d_compare - generic d_compare implementation for casefolding filesystems
  * @dentry:	dentry whose name we are checking against
@@ -1411,7 +1401,7 @@ static int generic_ci_d_compare(const struct dentry *dentry, unsigned int len,
 	char strbuf[DNAME_INLINE_LEN];
 	int ret;
 
-	if (!dir || !needs_casefold(dir))
+	if (!dir || !dir_is_casefolded(dir))
 		goto fallback;
 	/*
 	 * If the dentry name is stored in-line, then it may be concurrently
@@ -1453,7 +1443,7 @@ static int generic_ci_d_hash(const struct dentry *dentry, struct qstr *str)
 	const struct unicode_map *um = sb->s_encoding;
 	int ret = 0;
 
-	if (!dir || !needs_casefold(dir))
+	if (!dir || !dir_is_casefolded(dir))
 		return 0;
 
 	ret = utf8_casefold_hash(um, dentry, str);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 6867512907d6..e3b631c6d24a 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3213,6 +3213,27 @@ static inline bool dir_relax_shared(struct inode *inode)
 	return !IS_DEADDIR(inode);
 }
 
+/**
+ * dir_is_casefolded - Safely determine if filenames inside of a
+ * directory should be casefolded.
+ * @dir: The directory inode to be checked
+ *
+ * Filesystems should usually rely on this instead of checking the
+ * S_CASEFOLD flag directly when handling inodes, to avoid surprises
+ * with corrupted volumes.  Checking i_sb->s_encoding ensures the
+ * filesystem knows how to deal with case-insensitiveness.
+ *
+ * Return: if names will need casefolding
+ */
+static inline bool dir_is_casefolded(const struct inode *dir)
+{
+#if IS_ENABLED(CONFIG_UNICODE)
+	return IS_CASEFOLDED(dir) && dir->i_sb->s_encoding;
+#else
+	return false;
+#endif
+}
+
 extern bool path_noexec(const struct path *path);
 extern void inode_nohighmem(struct inode *inode);
 
-- 
2.41.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH v5 02/10] ecryptfs: Reject casefold directory inodes

[Gabriel Krisman Bertazi]

Even though it seems to be able to resolve some names of
case-insensitive directories, the lack of d_hash and d_compare means we
end up with a broken state in the d_cache.  Considering it was never a
goal to support these two together, and we are preparing to use
d_revalidate in case-insensitive filesystems, which would make the
combination even more broken, reject any attempt to get a casefolded
inode from ecryptfs.

Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
---
 fs/ecryptfs/inode.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 83274915ba6d..1305dc49df78 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -78,6 +78,14 @@ static struct inode *__ecryptfs_get_inode(struct inode *lower_inode,
 
 	if (lower_inode->i_sb != ecryptfs_superblock_to_lower(sb))
 		return ERR_PTR(-EXDEV);
+
+	/* Reject dealing with casefold directories. */
+	if (S_ISDIR(lower_inode->i_mode) && dir_is_casefolded(lower_inode)) {
+		pr_err_ratelimited("%s: Can't handle casefolded directory.\n",
+				   __func__);
+		return ERR_PTR(-EREMOTE);
+	}
+
 	if (!igrab(lower_inode))
 		return ERR_PTR(-ESTALE);
 	inode = iget5_locked(sb, (unsigned long)lower_inode,
-- 
2.41.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH v5 03/10] 9p: Split ->weak_revalidate from ->revalidate

[Gabriel Krisman Bertazi]

In preparation to change the signature of dentry_ops->revalidate, avoid
reusing the handler directly for d_weak_revalidate in 9p.

Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
---
 fs/9p/vfs_dentry.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/9p/vfs_dentry.c b/fs/9p/vfs_dentry.c
index f16f73581634..0c6fa1f53530 100644
--- a/fs/9p/vfs_dentry.c
+++ b/fs/9p/vfs_dentry.c
@@ -94,9 +94,15 @@ static int v9fs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 	return 1;
 }
 
+static int v9fs_lookup_weak_revalidate(struct dentry *dentry,
+				       unsigned int flags)
+{
+	return v9fs_lookup_revalidate(dentry, flags);
+}
+
 const struct dentry_operations v9fs_cached_dentry_operations = {
 	.d_revalidate = v9fs_lookup_revalidate,
-	.d_weak_revalidate = v9fs_lookup_revalidate,
+	.d_weak_revalidate = v9fs_lookup_weak_revalidate,
 	.d_delete = v9fs_cached_dentry_delete,
 	.d_release = v9fs_dentry_release,
 };
-- 
2.41.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH v5 04/10] fs: Expose name under lookup to d_revalidate hooks

[Gabriel Krisman Bertazi]

From: Gabriel Krisman Bertazi <krisman@collabora.com>

Negative dentries support on case-insensitive ext4/f2fs will require
access to the name under lookup to ensure it matches the dentry.  This
adds the information on d_revalidate and updates its implementation.

This was done through a Coccinelle hook and tested by building with
allyesconfig.

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>

---
Changes since v3:
  - Merge d_revalidate_name with d_revalidate (Christian)
  - Drop Ted's r-b since patch changed quite a bit. (Me)
Changes since v2:
  - Document d_revalidate_name hook. (Eric)
---
 Documentation/filesystems/locking.rst |  3 ++-
 Documentation/filesystems/vfs.rst     | 11 ++++++++++-
 fs/9p/vfs_dentry.c                    |  5 +++--
 fs/afs/dir.c                          |  6 ++++--
 fs/afs/dynroot.c                      |  4 +++-
 fs/ceph/dir.c                         |  3 ++-
 fs/coda/dir.c                         |  3 ++-
 fs/crypto/fname.c                     |  3 ++-
 fs/ecryptfs/dentry.c                  |  5 +++--
 fs/exfat/namei.c                      |  3 ++-
 fs/fat/namei_vfat.c                   |  6 ++++--
 fs/fuse/dir.c                         |  3 ++-
 fs/gfs2/dentry.c                      |  3 ++-
 fs/hfs/sysdep.c                       |  3 ++-
 fs/jfs/namei.c                        |  3 ++-
 fs/kernfs/dir.c                       |  3 ++-
 fs/namei.c                            | 18 ++++++++++--------
 fs/nfs/dir.c                          |  9 ++++++---
 fs/ocfs2/dcache.c                     |  4 +++-
 fs/orangefs/dcache.c                  |  3 ++-
 fs/overlayfs/super.c                  | 20 ++++++++++++--------
 fs/proc/base.c                        |  6 ++++--
 fs/proc/fd.c                          |  3 ++-
 fs/proc/generic.c                     |  6 ++++--
 fs/proc/proc_sysctl.c                 |  3 ++-
 fs/reiserfs/xattr.c                   |  3 ++-
 fs/smb/client/dir.c                   |  3 ++-
 fs/vboxsf/dir.c                       |  4 +++-
 include/linux/dcache.h                |  2 +-
 include/linux/fscrypt.h               |  4 +++-
 30 files changed, 103 insertions(+), 52 deletions(-)

diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
index ed148919e11a..1603c53a1688 100644
--- a/Documentation/filesystems/locking.rst
+++ b/Documentation/filesystems/locking.rst
@@ -17,7 +17,8 @@ dentry_operations
 
 prototypes::
 
-	int (*d_revalidate)(struct dentry *, unsigned int);
+	int (*d_revalidate)(struct dentry *, const struct qstr *,
+			    unsigned int);
 	int (*d_weak_revalidate)(struct dentry *, unsigned int);
 	int (*d_hash)(const struct dentry *, struct qstr *);
 	int (*d_compare)(const struct dentry *,
diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst
index cb2a97e49872..ddd542c2a722 100644
--- a/Documentation/filesystems/vfs.rst
+++ b/Documentation/filesystems/vfs.rst
@@ -1251,7 +1251,8 @@ defined:
 .. code-block:: c
 
 	struct dentry_operations {
-		int (*d_revalidate)(struct dentry *, unsigned int);
+		int (*d_revalidate)(struct dentry *, const struct qstr *,
+				    unsigned int);
 		int (*d_weak_revalidate)(struct dentry *, unsigned int);
 		int (*d_hash)(const struct dentry *, struct qstr *);
 		int (*d_compare)(const struct dentry *,
@@ -1284,6 +1285,14 @@ defined:
 	they can change and, in d_inode case, even become NULL under
 	us).
 
+	Filesystems shouldn't rely on the name under lookup, unless
+	there are particular filename encoding semantics to be handled
+	during revalidation.  Note the name under lookup can change from
+	under d_revalidate, so it must be protected with ->d_lock before
+	accessing.  The exception is when revalidating negative dentries
+	for creation, in which case the parent inode prevents it from
+	changing.
+
 	If a situation is encountered that rcu-walk cannot handle,
 	return
 	-ECHILD and it will be called again in ref-walk mode.
diff --git a/fs/9p/vfs_dentry.c b/fs/9p/vfs_dentry.c
index 0c6fa1f53530..de679d9505db 100644
--- a/fs/9p/vfs_dentry.c
+++ b/fs/9p/vfs_dentry.c
@@ -56,7 +56,8 @@ static void v9fs_dentry_release(struct dentry *dentry)
 	dentry->d_fsdata = NULL;
 }
 
-static int v9fs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
+static int v9fs_lookup_revalidate(struct dentry *dentry,
+				  const struct qstr *name, unsigned int flags)
 {
 	struct p9_fid *fid;
 	struct inode *inode;
@@ -97,7 +98,7 @@ static int v9fs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 static int v9fs_lookup_weak_revalidate(struct dentry *dentry,
 				       unsigned int flags)
 {
-	return v9fs_lookup_revalidate(dentry, flags);
+	return v9fs_lookup_revalidate(dentry, NULL, flags);
 }
 
 const struct dentry_operations v9fs_cached_dentry_operations = {
diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index 5219182e52e1..e3ba14512715 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -21,7 +21,8 @@ static struct dentry *afs_lookup(struct inode *dir, struct dentry *dentry,
 				 unsigned int flags);
 static int afs_dir_open(struct inode *inode, struct file *file);
 static int afs_readdir(struct file *file, struct dir_context *ctx);
-static int afs_d_revalidate(struct dentry *dentry, unsigned int flags);
+static int afs_d_revalidate(struct dentry *dentry, const struct qstr *name,
+			    unsigned int flags);
 static int afs_d_delete(const struct dentry *dentry);
 static void afs_d_iput(struct dentry *dentry, struct inode *inode);
 static bool afs_lookup_one_filldir(struct dir_context *ctx, const char *name, int nlen,
@@ -1084,7 +1085,8 @@ static int afs_d_revalidate_rcu(struct dentry *dentry)
  * - NOTE! the hit can be a negative hit too, so we can't assume we have an
  *   inode
  */
-static int afs_d_revalidate(struct dentry *dentry, unsigned int flags)
+static int afs_d_revalidate(struct dentry *dentry, const struct qstr *name,
+			    unsigned int flags)
 {
 	struct afs_vnode *vnode, *dir;
 	struct afs_fid fid;
diff --git a/fs/afs/dynroot.c b/fs/afs/dynroot.c
index d7d9402ff718..44a8f736eaf8 100644
--- a/fs/afs/dynroot.c
+++ b/fs/afs/dynroot.c
@@ -247,7 +247,9 @@ const struct inode_operations afs_dynroot_inode_operations = {
 /*
  * Dirs in the dynamic root don't need revalidation.
  */
-static int afs_dynroot_d_revalidate(struct dentry *dentry, unsigned int flags)
+static int afs_dynroot_d_revalidate(struct dentry *dentry,
+				    const struct qstr *name,
+				    unsigned int flags)
 {
 	return 1;
 }
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 4a2b39d9a61a..dffc115adae0 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1758,7 +1758,8 @@ static int dir_lease_is_valid(struct inode *dir, struct dentry *dentry,
 /*
  * Check if cached dentry can be trusted.
  */
-static int ceph_d_revalidate(struct dentry *dentry, unsigned int flags)
+static int ceph_d_revalidate(struct dentry *dentry, const struct qstr *name,
+			     unsigned int flags)
 {
 	int valid = 0;
 	struct dentry *parent;
diff --git a/fs/coda/dir.c b/fs/coda/dir.c
index 8450b1bd354b..bb2ecac4a7e7 100644
--- a/fs/coda/dir.c
+++ b/fs/coda/dir.c
@@ -452,7 +452,8 @@ static int coda_readdir(struct file *coda_file, struct dir_context *ctx)
 }
 
 /* called when a cache lookup succeeds */
-static int coda_dentry_revalidate(struct dentry *de, unsigned int flags)
+static int coda_dentry_revalidate(struct dentry *de, const struct qstr *name,
+				  unsigned int flags)
 {
 	struct inode *inode;
 	struct coda_inode_info *cii;
diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 6eae3f12ad50..d543e4648a0f 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -580,7 +580,8 @@ EXPORT_SYMBOL_GPL(fscrypt_fname_siphash);
  * Validate dentries in encrypted directories to make sure we aren't potentially
  * caching stale dentries after a key has been added.
  */
-int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
+int fscrypt_d_revalidate(struct dentry *dentry, const struct qstr *name,
+			 unsigned int flags)
 {
 	struct dentry *dir;
 	int err;
diff --git a/fs/ecryptfs/dentry.c b/fs/ecryptfs/dentry.c
index acaa0825e9bb..56093648d838 100644
--- a/fs/ecryptfs/dentry.c
+++ b/fs/ecryptfs/dentry.c
@@ -28,7 +28,8 @@
  * Returns 1 if valid, 0 otherwise.
  *
  */
-static int ecryptfs_d_revalidate(struct dentry *dentry, unsigned int flags)
+static int ecryptfs_d_revalidate(struct dentry *dentry,
+				 const struct qstr *name, unsigned int flags)
 {
 	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
 	int rc = 1;
@@ -37,7 +38,7 @@ static int ecryptfs_d_revalidate(struct dentry *dentry, unsigned int flags)
 		return -ECHILD;
 
 	if (lower_dentry->d_flags & DCACHE_OP_REVALIDATE)
-		rc = lower_dentry->d_op->d_revalidate(lower_dentry, flags);
+		rc = lower_dentry->d_op->d_revalidate(lower_dentry, name, flags);
 
 	if (d_really_is_positive(dentry)) {
 		struct inode *inode = d_inode(dentry);
diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
index e0ff9d156f6f..6220046a687b 100644
--- a/fs/exfat/namei.c
+++ b/fs/exfat/namei.c
@@ -31,7 +31,8 @@ static inline void exfat_d_version_set(struct dentry *dentry,
  * If it happened, the negative dentry isn't actually negative anymore.  So,
  * drop it.
  */
-static int exfat_d_revalidate(struct dentry *dentry, unsigned int flags)
+static int exfat_d_revalidate(struct dentry *dentry, const struct qstr *name,
+			      unsigned int flags)
 {
 	int ret;
 
diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index c4d00999a433..73981b0e4ea7 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -53,7 +53,8 @@ static int vfat_revalidate_shortname(struct dentry *dentry)
 	return ret;
 }
 
-static int vfat_revalidate(struct dentry *dentry, unsigned int flags)
+static int vfat_revalidate(struct dentry *dentry, const struct qstr *name,
+			   unsigned int flags)
 {
 	if (flags & LOOKUP_RCU)
 		return -ECHILD;
@@ -64,7 +65,8 @@ static int vfat_revalidate(struct dentry *dentry, unsigned int flags)
 	return vfat_revalidate_shortname(dentry);
 }
 
-static int vfat_revalidate_ci(struct dentry *dentry, unsigned int flags)
+static int vfat_revalidate_ci(struct dentry *dentry, const struct qstr *name,
+			      unsigned int flags)
 {
 	if (flags & LOOKUP_RCU)
 		return -ECHILD;
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 35bc174f9ba2..948bbfc1aae4 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -202,7 +202,8 @@ static void fuse_lookup_init(struct fuse_conn *fc, struct fuse_args *args,
  * the lookup once more.  If the lookup results in the same inode,
  * then refresh the attributes, timeouts and mark the dentry valid.
  */
-static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
+static int fuse_dentry_revalidate(struct dentry *entry,
+				  const struct qstr *name, unsigned int flags)
 {
 	struct inode *inode;
 	struct dentry *parent;
diff --git a/fs/gfs2/dentry.c b/fs/gfs2/dentry.c
index 2e215e8c3c88..3dd93d36aaf2 100644
--- a/fs/gfs2/dentry.c
+++ b/fs/gfs2/dentry.c
@@ -30,7 +30,8 @@
  * Returns: 1 if the dentry is ok, 0 if it isn't
  */
 
-static int gfs2_drevalidate(struct dentry *dentry, unsigned int flags)
+static int gfs2_drevalidate(struct dentry *dentry, const struct qstr *name,
+			    unsigned int flags)
 {
 	struct dentry *parent;
 	struct gfs2_sbd *sdp;
diff --git a/fs/hfs/sysdep.c b/fs/hfs/sysdep.c
index 2875961fdc10..68fb32f4fbb8 100644
--- a/fs/hfs/sysdep.c
+++ b/fs/hfs/sysdep.c
@@ -13,7 +13,8 @@
 
 /* dentry case-handling: just lowercase everything */
 
-static int hfs_revalidate_dentry(struct dentry *dentry, unsigned int flags)
+static int hfs_revalidate_dentry(struct dentry *dentry,
+				 const struct qstr *name, unsigned int flags)
 {
 	struct inode *inode;
 	int diff;
diff --git a/fs/jfs/namei.c b/fs/jfs/namei.c
index 9b030297aa64..0d2b5b54e2d8 100644
--- a/fs/jfs/namei.c
+++ b/fs/jfs/namei.c
@@ -1573,7 +1573,8 @@ static int jfs_ci_compare(const struct dentry *dentry,
 	return result;
 }
 
-static int jfs_ci_revalidate(struct dentry *dentry, unsigned int flags)
+static int jfs_ci_revalidate(struct dentry *dentry, const struct qstr *name,
+			     unsigned int flags)
 {
 	/*
 	 * This is not negative dentry. Always valid.
diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 5a1a4af9d3d2..820988710ce5 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -1084,7 +1084,8 @@ struct kernfs_node *kernfs_create_empty_dir(struct kernfs_node *parent,
 	return ERR_PTR(rc);
 }
 
-static int kernfs_dop_revalidate(struct dentry *dentry, unsigned int flags)
+static int kernfs_dop_revalidate(struct dentry *dentry,
+				 const struct qstr *name, unsigned int flags)
 {
 	struct kernfs_node *kn;
 	struct kernfs_root *root;
diff --git a/fs/namei.c b/fs/namei.c
index e56ff39a79bc..7631c762217a 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -853,10 +853,12 @@ static bool try_to_unlazy_next(struct nameidata *nd, struct dentry *dentry)
 	return false;
 }
 
-static inline int d_revalidate(struct dentry *dentry, unsigned int flags)
+static inline int d_revalidate(struct dentry *dentry,
+			       const struct qstr *name,
+			       unsigned int flags)
 {
 	if (unlikely(dentry->d_flags & DCACHE_OP_REVALIDATE))
-		return dentry->d_op->d_revalidate(dentry, flags);
+		return dentry->d_op->d_revalidate(dentry, name, flags);
 	else
 		return 1;
 }
@@ -1565,7 +1567,7 @@ static struct dentry *lookup_dcache(const struct qstr *name,
 {
 	struct dentry *dentry = d_lookup(dir, name);
 	if (dentry) {
-		int error = d_revalidate(dentry, flags);
+		int error = d_revalidate(dentry, name, flags);
 		if (unlikely(error <= 0)) {
 			if (!error)
 				d_invalidate(dentry);
@@ -1636,19 +1638,19 @@ static struct dentry *lookup_fast(struct nameidata *nd)
 		if (read_seqcount_retry(&parent->d_seq, nd->seq))
 			return ERR_PTR(-ECHILD);
 
-		status = d_revalidate(dentry, nd->flags);
+		status = d_revalidate(dentry, &nd->last, nd->flags);
 		if (likely(status > 0))
 			return dentry;
 		if (!try_to_unlazy_next(nd, dentry))
 			return ERR_PTR(-ECHILD);
 		if (status == -ECHILD)
 			/* we'd been told to redo it in non-rcu mode */
-			status = d_revalidate(dentry, nd->flags);
+			status = d_revalidate(dentry, &nd->last, nd->flags);
 	} else {
 		dentry = __d_lookup(parent, &nd->last);
 		if (unlikely(!dentry))
 			return NULL;
-		status = d_revalidate(dentry, nd->flags);
+		status = d_revalidate(dentry, &nd->last, nd->flags);
 	}
 	if (unlikely(status <= 0)) {
 		if (!status)
@@ -1676,7 +1678,7 @@ static struct dentry *__lookup_slow(const struct qstr *name,
 	if (IS_ERR(dentry))
 		return dentry;
 	if (unlikely(!d_in_lookup(dentry))) {
-		int error = d_revalidate(dentry, flags);
+		int error = d_revalidate(dentry, name, flags);
 		if (unlikely(error <= 0)) {
 			if (!error) {
 				d_invalidate(dentry);
@@ -3421,7 +3423,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 		if (d_in_lookup(dentry))
 			break;
 
-		error = d_revalidate(dentry, nd->flags);
+		error = d_revalidate(dentry, &nd->last, nd->flags);
 		if (likely(error > 0))
 			break;
 		if (error)
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 8f3112e71a6a..be162ef6a24e 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1805,7 +1805,8 @@ __nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags,
 	return ret;
 }
 
-static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
+static int nfs_lookup_revalidate(struct dentry *dentry,
+				 const struct qstr *name, unsigned int flags)
 {
 	return __nfs_lookup_revalidate(dentry, flags, nfs_do_lookup_revalidate);
 }
@@ -1993,7 +1994,8 @@ void nfs_d_prune_case_insensitive_aliases(struct inode *inode)
 EXPORT_SYMBOL_GPL(nfs_d_prune_case_insensitive_aliases);
 
 #if IS_ENABLED(CONFIG_NFS_V4)
-static int nfs4_lookup_revalidate(struct dentry *, unsigned int);
+static int nfs4_lookup_revalidate(struct dentry *, const struct qstr *name,
+				  unsigned int);
 
 const struct dentry_operations nfs4_dentry_operations = {
 	.d_revalidate	= nfs4_lookup_revalidate,
@@ -2226,7 +2228,8 @@ nfs4_do_lookup_revalidate(struct inode *dir, struct dentry *dentry,
 	return nfs_do_lookup_revalidate(dir, dentry, flags);
 }
 
-static int nfs4_lookup_revalidate(struct dentry *dentry, unsigned int flags)
+static int nfs4_lookup_revalidate(struct dentry *dentry,
+				  const struct qstr *name, unsigned int flags)
 {
 	return __nfs_lookup_revalidate(dentry, flags,
 			nfs4_do_lookup_revalidate);
diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c
index 04fc8344063a..277757f4fd2d 100644
--- a/fs/ocfs2/dcache.c
+++ b/fs/ocfs2/dcache.c
@@ -32,7 +32,9 @@ void ocfs2_dentry_attach_gen(struct dentry *dentry)
 }
 
 
-static int ocfs2_dentry_revalidate(struct dentry *dentry, unsigned int flags)
+static int ocfs2_dentry_revalidate(struct dentry *dentry,
+				   const struct qstr *name,
+				   unsigned int flags)
 {
 	struct inode *inode;
 	int ret = 0;    /* if all else fails, just return false */
diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c
index 8bbe9486e3a6..435b88007809 100644
--- a/fs/orangefs/dcache.c
+++ b/fs/orangefs/dcache.c
@@ -94,7 +94,8 @@ static int orangefs_revalidate_lookup(struct dentry *dentry)
  *
  * Should return 1 if dentry can still be trusted, else 0.
  */
-static int orangefs_d_revalidate(struct dentry *dentry, unsigned int flags)
+static int orangefs_d_revalidate(struct dentry *dentry,
+				 const struct qstr *name, unsigned int flags)
 {
 	int ret;
 	unsigned long time = (unsigned long) dentry->d_fsdata;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 5b069f1a1e44..1233e38d029d 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -77,7 +77,8 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
 	return dentry;
 }
 
-static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak)
+static int ovl_revalidate_real(struct dentry *d, const struct qstr *name,
+			       unsigned int flags, bool weak)
 {
 	int ret = 1;
 
@@ -88,7 +89,7 @@ static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak)
 		if (d->d_flags & DCACHE_OP_WEAK_REVALIDATE)
 			ret =  d->d_op->d_weak_revalidate(d, flags);
 	} else if (d->d_flags & DCACHE_OP_REVALIDATE) {
-		ret = d->d_op->d_revalidate(d, flags);
+		ret = d->d_op->d_revalidate(d, name, flags);
 		if (!ret) {
 			if (!(flags & LOOKUP_RCU))
 				d_invalidate(d);
@@ -99,6 +100,7 @@ static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak)
 }
 
 static int ovl_dentry_revalidate_common(struct dentry *dentry,
+					const struct qstr *name,
 					unsigned int flags, bool weak)
 {
 	struct ovl_entry *oe = OVL_E(dentry);
@@ -114,22 +116,24 @@ static int ovl_dentry_revalidate_common(struct dentry *dentry,
 
 	upper = ovl_i_dentry_upper(inode);
 	if (upper)
-		ret = ovl_revalidate_real(upper, flags, weak);
+		ret = ovl_revalidate_real(upper, name, flags, weak);
 
 	for (i = 0; ret > 0 && i < ovl_numlower(oe); i++)
-		ret = ovl_revalidate_real(lowerstack[i].dentry, flags, weak);
+		ret = ovl_revalidate_real(lowerstack[i].dentry, name, flags, weak);
 
 	return ret;
 }
 
-static int ovl_dentry_revalidate(struct dentry *dentry, unsigned int flags)
+static int ovl_dentry_revalidate(struct dentry *dentry,
+				 const struct qstr *name, unsigned int flags)
 {
-	return ovl_dentry_revalidate_common(dentry, flags, false);
+	return ovl_dentry_revalidate_common(dentry, name, flags, false);
 }
 
-static int ovl_dentry_weak_revalidate(struct dentry *dentry, unsigned int flags)
+static int ovl_dentry_weak_revalidate(struct dentry *dentry,
+				      unsigned int flags)
 {
-	return ovl_dentry_revalidate_common(dentry, flags, true);
+	return ovl_dentry_revalidate_common(dentry, NULL, flags, true);
 }
 
 static const struct dentry_operations ovl_dentry_operations = {
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 05452c3b9872..bdf212c52c8f 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2005,7 +2005,8 @@ void pid_update_inode(struct task_struct *task, struct inode *inode)
  * performed a setuid(), etc.
  *
  */
-static int pid_revalidate(struct dentry *dentry, unsigned int flags)
+static int pid_revalidate(struct dentry *dentry, const struct qstr *name,
+			  unsigned int flags)
 {
 	struct inode *inode;
 	struct task_struct *task;
@@ -2138,7 +2139,8 @@ static int dname_to_vma_addr(struct dentry *dentry,
 	return 0;
 }
 
-static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags)
+static int map_files_d_revalidate(struct dentry *dentry,
+				  const struct qstr *name, unsigned int flags)
 {
 	unsigned long vm_start, vm_end;
 	bool exact_vma_exists = false;
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index b3140deebbbf..efd604fe8d82 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -136,7 +136,8 @@ static void tid_fd_update_inode(struct task_struct *task, struct inode *inode,
 	security_task_to_inode(task, inode);
 }
 
-static int tid_fd_revalidate(struct dentry *dentry, unsigned int flags)
+static int tid_fd_revalidate(struct dentry *dentry, const struct qstr *name,
+			     unsigned int flags)
 {
 	struct task_struct *task;
 	struct inode *inode;
diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index 42ae38ff6e7e..7cb15ab01a5a 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -216,7 +216,8 @@ void proc_free_inum(unsigned int inum)
 	ida_simple_remove(&proc_inum_ida, inum - PROC_DYNAMIC_FIRST);
 }
 
-static int proc_misc_d_revalidate(struct dentry *dentry, unsigned int flags)
+static int proc_misc_d_revalidate(struct dentry *dentry,
+				  const struct qstr *name, unsigned int flags)
 {
 	if (flags & LOOKUP_RCU)
 		return -ECHILD;
@@ -343,7 +344,8 @@ static const struct file_operations proc_dir_operations = {
 	.iterate_shared		= proc_readdir,
 };
 
-static int proc_net_d_revalidate(struct dentry *dentry, unsigned int flags)
+static int proc_net_d_revalidate(struct dentry *dentry,
+				 const struct qstr *name, unsigned int flags)
 {
 	return 0;
 }
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 5ea42653126e..d067ebff1c74 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -886,7 +886,8 @@ static const struct inode_operations proc_sys_dir_operations = {
 	.getattr	= proc_sys_getattr,
 };
 
-static int proc_sys_revalidate(struct dentry *dentry, unsigned int flags)
+static int proc_sys_revalidate(struct dentry *dentry, const struct qstr *name,
+			       unsigned int flags)
 {
 	if (flags & LOOKUP_RCU)
 		return -ECHILD;
diff --git a/fs/reiserfs/xattr.c b/fs/reiserfs/xattr.c
index 651027967159..2d09e4cdedd1 100644
--- a/fs/reiserfs/xattr.c
+++ b/fs/reiserfs/xattr.c
@@ -957,7 +957,8 @@ int reiserfs_permission(struct mnt_idmap *idmap, struct inode *inode,
 	return generic_permission(&nop_mnt_idmap, inode, mask);
 }
 
-static int xattr_hide_revalidate(struct dentry *dentry, unsigned int flags)
+static int xattr_hide_revalidate(struct dentry *dentry,
+				 const struct qstr *name, unsigned int flags)
 {
 	return -EPERM;
 }
diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c
index 30b1e1bfd204..0ced1a98de9f 100644
--- a/fs/smb/client/dir.c
+++ b/fs/smb/client/dir.c
@@ -714,7 +714,8 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 }
 
 static int
-cifs_d_revalidate(struct dentry *direntry, unsigned int flags)
+cifs_d_revalidate(struct dentry *direntry, const struct qstr *name,
+		  unsigned int flags)
 {
 	struct inode *inode;
 	int rc;
diff --git a/fs/vboxsf/dir.c b/fs/vboxsf/dir.c
index 075f15c43c78..81a03a7331a4 100644
--- a/fs/vboxsf/dir.c
+++ b/fs/vboxsf/dir.c
@@ -191,7 +191,9 @@ const struct file_operations vboxsf_dir_fops = {
  * This is called during name resolution/lookup to check if the @dentry in
  * the cache is still valid. the job is handled by vboxsf_inode_revalidate.
  */
-static int vboxsf_dentry_revalidate(struct dentry *dentry, unsigned int flags)
+static int vboxsf_dentry_revalidate(struct dentry *dentry,
+				    const struct qstr *name,
+				    unsigned int flags)
 {
 	if (flags & LOOKUP_RCU)
 		return -ECHILD;
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 6b351e009f59..9362e4ef0bad 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -126,7 +126,7 @@ enum dentry_d_lock_class
 };
 
 struct dentry_operations {
-	int (*d_revalidate)(struct dentry *, unsigned int);
+	int (*d_revalidate)(struct dentry *, const struct qstr *, unsigned int);
 	int (*d_weak_revalidate)(struct dentry *, unsigned int);
 	int (*d_hash)(const struct dentry *, struct qstr *);
 	int (*d_compare)(const struct dentry *,
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index c895b12737a1..d8c68a366a2b 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -353,7 +353,8 @@ int fscrypt_fname_disk_to_usr(const struct inode *inode,
 bool fscrypt_match_name(const struct fscrypt_name *fname,
 			const u8 *de_name, u32 de_name_len);
 u64 fscrypt_fname_siphash(const struct inode *dir, const struct qstr *name);
-int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags);
+int fscrypt_d_revalidate(struct dentry *dentry, const struct qstr *name,
+			 unsigned int flags);
 
 /* bio.c */
 bool fscrypt_decrypt_bio(struct bio *bio);
@@ -647,6 +648,7 @@ static inline u64 fscrypt_fname_siphash(const struct inode *dir,
 }
 
 static inline int fscrypt_d_revalidate(struct dentry *dentry,
+				       const struct qstr *name,
 				       unsigned int flags)
 {
 	return 1;
-- 
2.41.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH v5 05/10] fs: Add DCACHE_CASEFOLDED_NAME flag

[Gabriel Krisman Bertazi]

From: Gabriel Krisman Bertazi <krisman@collabora.com>

This flag marks a negative or positive dentry as being created after a
case-insensitive lookup operation.  It is useful to differentiate
dentries this way to detect whether the negative dentry can be trusted
during a case-insensitive lookup.

Reviewed-by: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>

---
Changes since v4:
  - Fixup names of functions to reflect flag name change (Eric)

Changes since v2:
  -  Rename DCACHE_CASEFOLD_LOOKUP -> DCACHE_CASEFOLDED_NAME (Eric)
---
 fs/dcache.c            | 8 ++++++++
 include/linux/dcache.h | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/fs/dcache.c b/fs/dcache.c
index 52e6d5fdab6b..269367c1a86c 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1958,6 +1958,14 @@ void d_set_fallthru(struct dentry *dentry)
 }
 EXPORT_SYMBOL(d_set_fallthru);
 
+void d_set_casefolded_name(struct dentry *dentry)
+{
+	spin_lock(&dentry->d_lock);
+	dentry->d_flags |= DCACHE_CASEFOLDED_NAME;
+	spin_unlock(&dentry->d_lock);
+}
+EXPORT_SYMBOL(d_set_casefold_lookup);
+
 static unsigned d_flags_for_inode(struct inode *inode)
 {
 	unsigned add_flags = DCACHE_REGULAR_TYPE;
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 9362e4ef0bad..ccbb5c4db7ce 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -208,6 +208,7 @@ struct dentry_operations {
 #define DCACHE_FALLTHRU			0x01000000 /* Fall through to lower layer */
 #define DCACHE_NOKEY_NAME		0x02000000 /* Encrypted name encoded without key */
 #define DCACHE_OP_REAL			0x04000000
+#define DCACHE_CASEFOLDED_NAME		0x08000000 /* Dentry comes from a casefold directory */
 
 #define DCACHE_PAR_LOOKUP		0x10000000 /* being looked up (with parent locked shared) */
 #define DCACHE_DENTRY_CURSOR		0x20000000
@@ -496,6 +497,13 @@ static inline bool d_is_fallthru(const struct dentry *dentry)
 	return dentry->d_flags & DCACHE_FALLTHRU;
 }
 
+extern void d_set_casefolded_name(struct dentry *dentry);
+
+static inline bool d_is_casefolded_name(const struct dentry *dentry)
+{
+	return dentry->d_flags & DCACHE_CASEFOLDED_NAME;
+}
+
 
 extern int sysctl_vfs_cache_pressure;
 
-- 
2.41.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH v5 06/10] libfs: Validate negative dentries in case-insensitive directories

[Gabriel Krisman Bertazi]

From: Gabriel Krisman Bertazi <krisman@collabora.com>

Introduce a dentry revalidation helper to be used by case-insensitive
filesystems to check if it is safe to reuse a negative dentry.

A negative dentry is safe to be reused on a case-insensitive lookup if
it was created during a case-insensitive lookup and this is not a lookup
that will instantiate a dentry. If this is a creation lookup, we also
need to make sure the name matches sensitively the name under lookup in
order to assure the name preserving semantics.

dentry->d_name is only checked by the case-insensitive d_revalidate hook
in the LOOKUP_CREATE/LOOKUP_RENAME_TARGET case since, for these cases,
d_revalidate is always called with the parent inode at least
read-locked, and therefore the name cannot change from under us.

d_revalidate is only called in 4 places: lookup_dcache, __lookup_slow,
lookup_open and lookup_fast:

  - lookup_dcache always calls it with zeroed flags, with the exception
    of when coming from __lookup_hash, which needs the parent locked
    already, for instance in the open/creation path, which is locked in
    open_last_lookups.

  - In __lookup_slow, either the parent inode is read-locked by the
    caller (lookup_slow), or it is called with no flags (lookup_one*).
    The read lock suffices to prevent ->d_name modifications, with the
    exception of one case: __d_unalias, will call __d_move to fix a
    directory accessible from multiple dentries, which effectively swaps
    ->d_name while holding only the shared read lock.  This happens
    through this flow:

    lookup_slow()  //LOOKUP_CREATE
      d_lookup()
        ->d_lookup()
          d_splice_alias()
            __d_unalias()
              __d_move()

    Nevertheless, this case is not a problem because negative dentries
    are not allowed to be moved with __d_move.  In addition,
    d_instantiate shouldn't race with this case because it's callers
    also acquire the parent inode lock, preventing it from racing with
    lookup creation, so the dentry cannot become positive and be moved
    while inside d_revalidate, which would be a problem if a parallel
    lookup did d_splice_alias.

  - lookup_open also requires the parent to be locked in the creation
    case, which is done in open_last_lookups.

  - lookup_fast will indeed be called with the parent unlocked, but it
    shouldn't be called with LOOKUP_CREATE.  Either it is called in the
    link_path_walk, where nd->flags doesn't have LOOKUP_CREATE yet or in
    open_last_lookups. But, in this case, it also never has LOOKUP_CREATE,
    because it is only called on the !O_CREAT case, which means op->intent
    doesn't have LOOKUP_CREAT (set in build_open_flags only if O_CREAT is
    set).

Finally, for the LOOKUP_RENAME_TARGET, we are doing a rename, so the
parents inodes are also locked.

Reviewed-by: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>

---
Changes since v4:
  - Drop useless inline declaration (eric)
  - Refactor to drop extra identation (Christian)
  - Discuss d_instantiate
Changes since v3:
  - Add comment regarding creation (Eric)
  - Reorder checks to clarify !flags meaning (Eric)
  - Add commit message explanaton of the inode read lock wrt.
    __d_move. (Eric)
Changes since v2:
  - Add comments to all rejection cases (Eric)
  - safeguard against filesystem creating dentries without LOOKUP flags
---
 fs/libfs.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/fs/libfs.c b/fs/libfs.c
index 8d0b64cfd5da..cb98c4721327 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1452,9 +1452,66 @@ static int generic_ci_d_hash(const struct dentry *dentry, struct qstr *str)
 	return 0;
 }
 
+static int generic_ci_d_revalidate(struct dentry *dentry,
+				   const struct qstr *name,
+				   unsigned int flags)
+{
+	const struct dentry *parent;
+	const struct inode *dir;
+
+	if (!d_is_negative(dentry))
+		return 1;
+
+	parent = READ_ONCE(dentry->d_parent);
+	dir = READ_ONCE(parent->d_inode);
+
+	if (!dir || !dir_is_casefolded(dir))
+		return 1;
+
+	/*
+	 * Negative dentries created prior to turning the directory
+	 * case-insensitive cannot be trusted, since they don't ensure
+	 * any possible case version of the filename doesn't exist.
+	 */
+	if (!d_is_casefolded_name(dentry))
+		return 0;
+
+	/*
+	 * Filesystems will call into d_revalidate without setting
+	 * LOOKUP_ flags even for file creation (see lookup_one*
+	 * variants).  Reject negative dentries in this case, since we
+	 * can't know for sure it won't be used for creation.
+	 */
+	if (!flags)
+		return 0;
+
+	/*
+	 * If the lookup is for creation, then a negative dentry can
+	 * only be reused if it's a case-sensitive match, not just a
+	 * case-insensitive one.  This is needed to make the new file be
+	 * created with the name the user specified, preserving case.
+	 */
+	if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) {
+		/*
+		 * ->d_name won't change from under us in the creation
+		 * path only, since d_revalidate during creation and
+		 * renames is always called with the parent inode
+		 * locked.  It isn't the case for all lookup callpaths,
+		 * so ->d_name must not be touched outside
+		 * (LOOKUP_CREATE|LOOKUP_RENAME_TARGET) context.
+		 */
+		if (dentry->d_name.len != name->len ||
+		    memcmp(dentry->d_name.name, name->name, name->len))
+			return 0;
+	}
+
+	return 1;
+}
+
 static const struct dentry_operations generic_ci_dentry_ops = {
 	.d_hash = generic_ci_d_hash,
 	.d_compare = generic_ci_d_compare,
+	.d_revalidate = generic_ci_d_revalidate,
 };
 #endif
 
-- 
2.41.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH v5 07/10] libfs: Chain encryption checks after case-insensitive revalidation

[Gabriel Krisman Bertazi]

From: Gabriel Krisman Bertazi <krisman@collabora.com>

Support encrypted dentries in generic_ci_d_revalidate by chaining
fscrypt_d_revalidate at the tail of the d_revalidate.  This allows
filesystem to just call generic_ci_d_revalidate and let it handle any
case-insensitive dentry (encrypted or not).

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>

---
Changes since v2:
  - Enable negative dentries of encrypted filesystems (Eric)
---
 fs/libfs.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index cb98c4721327..efb245118d10 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1452,9 +1452,8 @@ static int generic_ci_d_hash(const struct dentry *dentry, struct qstr *str)
 	return 0;
 }
 
-static int generic_ci_d_revalidate(struct dentry *dentry,
-				   const struct qstr *name,
-				   unsigned int flags)
+static int ci_d_revalidate(struct dentry *dentry, const struct qstr *name,
+			   unsigned int flags)
 {
 	const struct dentry *parent;
 	const struct inode *dir;
@@ -1508,6 +1507,15 @@ static int generic_ci_d_revalidate(struct dentry *dentry,
 	return 1;
 }
 
+static int generic_ci_d_revalidate(struct dentry *dentry,
+				   const struct qstr *name,
+				   unsigned int flags)
+{
+	if (!ci_d_revalidate(dentry, name, flags))
+		return 0;
+	return fscrypt_d_revalidate(dentry, name, flags);
+}
+
 static const struct dentry_operations generic_ci_dentry_ops = {
 	.d_hash = generic_ci_d_hash,
 	.d_compare = generic_ci_d_compare,
@@ -1525,7 +1533,7 @@ static const struct dentry_operations generic_encrypted_dentry_ops = {
 static const struct dentry_operations generic_encrypted_ci_dentry_ops = {
 	.d_hash = generic_ci_d_hash,
 	.d_compare = generic_ci_d_compare,
-	.d_revalidate = fscrypt_d_revalidate,
+	.d_revalidate = generic_ci_d_revalidate,
 };
 #endif
 
-- 
2.41.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH v5 08/10] libfs: Merge encrypted_ci_dentry_ops and ci_dentry_ops

[Gabriel Krisman Bertazi]

From: Gabriel Krisman Bertazi <krisman@collabora.com>

Now that casefold needs d_revalidate and calls fscrypt_d_revalidate
itself, generic_encrypt_ci_dentry_ops and generic_ci_dentry_ops are now
equivalent.  Merge them together and simplify the setup code.

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>

---
changes since v2:
  - reword comment for clarity (Eric)
---
 fs/libfs.c | 45 +++++++++++++--------------------------------
 1 file changed, 13 insertions(+), 32 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index efb245118d10..6b15a4f0312f 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1516,7 +1516,7 @@ static int generic_ci_d_revalidate(struct dentry *dentry,
 	return fscrypt_d_revalidate(dentry, name, flags);
 }
 
-static const struct dentry_operations generic_ci_dentry_ops = {
+static const struct dentry_operations generic_encrypted_ci_dentry_ops = {
 	.d_hash = generic_ci_d_hash,
 	.d_compare = generic_ci_d_compare,
 	.d_revalidate = generic_ci_d_revalidate,
@@ -1529,26 +1529,19 @@ static const struct dentry_operations generic_encrypted_dentry_ops = {
 };
 #endif
 
-#if defined(CONFIG_FS_ENCRYPTION) && IS_ENABLED(CONFIG_UNICODE)
-static const struct dentry_operations generic_encrypted_ci_dentry_ops = {
-	.d_hash = generic_ci_d_hash,
-	.d_compare = generic_ci_d_compare,
-	.d_revalidate = generic_ci_d_revalidate,
-};
-#endif
-
 /**
  * generic_set_encrypted_ci_d_ops - helper for setting d_ops for given dentry
  * @dentry:	dentry to set ops on
  *
- * Casefolded directories need d_hash and d_compare set, so that the dentries
- * contained in them are handled case-insensitively.  Note that these operations
- * are needed on the parent directory rather than on the dentries in it, and
- * while the casefolding flag can be toggled on and off on an empty directory,
- * dentry_operations can't be changed later.  As a result, if the filesystem has
- * casefolding support enabled at all, we have to give all dentries the
- * casefolding operations even if their inode doesn't have the casefolding flag
- * currently (and thus the casefolding ops would be no-ops for now).
+ * Casefolded directories need some dentry_operations set, so that the dentries
+ * contained in them are handled case-insensitively.  Note that d_hash and
+ * d_compare are needed on the parent directory rather than on the dentries in
+ * it, and while the casefolding flag can be toggled on and off on an empty
+ * directory, dentry_operations can't be changed later.  As a result, if the
+ * filesystem has casefolding support enabled at all, we have to give all
+ * dentries the casefolding operations even if their inode doesn't have the
+ * casefolding flag currently (and thus the casefolding ops would be no-ops for
+ * now).
  *
  * Encryption works differently in that the only dentry operation it needs is
  * d_revalidate, which it only needs on dentries that have the no-key name flag.
@@ -1557,34 +1550,22 @@ static const struct dentry_operations generic_encrypted_ci_dentry_ops = {
  * Finally, to maximize compatibility with overlayfs (which isn't compatible
  * with certain dentry operations) and to avoid taking an unnecessary
  * performance hit, we use custom dentry_operations for each possible
- * combination rather than always installing all operations.
+ * combination of operations rather than always installing them.
  */
 void generic_set_encrypted_ci_d_ops(struct dentry *dentry)
 {
-#ifdef CONFIG_FS_ENCRYPTION
-	bool needs_encrypt_ops = dentry->d_flags & DCACHE_NOKEY_NAME;
-#endif
 #if IS_ENABLED(CONFIG_UNICODE)
-	bool needs_ci_ops = dentry->d_sb->s_encoding;
-#endif
-#if defined(CONFIG_FS_ENCRYPTION) && IS_ENABLED(CONFIG_UNICODE)
-	if (needs_encrypt_ops && needs_ci_ops) {
+	if (dentry->d_sb->s_encoding) {
 		d_set_d_op(dentry, &generic_encrypted_ci_dentry_ops);
 		return;
 	}
 #endif
 #ifdef CONFIG_FS_ENCRYPTION
-	if (needs_encrypt_ops) {
+	if (dentry->d_flags & DCACHE_NOKEY_NAME) {
 		d_set_d_op(dentry, &generic_encrypted_dentry_ops);
 		return;
 	}
 #endif
-#if IS_ENABLED(CONFIG_UNICODE)
-	if (needs_ci_ops) {
-		d_set_d_op(dentry, &generic_ci_dentry_ops);
-		return;
-	}
-#endif
 }
 EXPORT_SYMBOL(generic_set_encrypted_ci_d_ops);
 
-- 
2.41.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH v5 09/10] ext4: Enable negative dentries on case-insensitive lookup

[Gabriel Krisman Bertazi]

From: Gabriel Krisman Bertazi <krisman@collabora.com>

Instead of invalidating negative dentries during case-insensitive
lookups, mark them as such and let them be added to the dcache.
d_ci_revalidate is able to properly filter them out if necessary based
on the dentry casefold flag.

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>

---
Changes since v4:
  - Use helper to decide if should set dentry flag.
Changes since v2:

  - Move dentry flag set closer to fscrypt code (Eric)
---
 fs/ext4/namei.c | 35 ++++-------------------------------
 1 file changed, 4 insertions(+), 31 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 0caf6c730ce3..8d33a74bcc95 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1759,6 +1759,10 @@ static struct buffer_head *ext4_lookup_entry(struct inode *dir,
 
 	err = ext4_fname_prepare_lookup(dir, dentry, &fname);
 	generic_set_encrypted_ci_d_ops(dentry);
+
+	if (dir_is_casefolded(dir))
+		d_set_casefolded_name(dentry);
+
 	if (err == -ENOENT)
 		return NULL;
 	if (err)
@@ -1866,16 +1870,6 @@ static struct dentry *ext4_lookup(struct inode *dir, struct dentry *dentry, unsi
 		}
 	}
 
-#if IS_ENABLED(CONFIG_UNICODE)
-	if (!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
-		 * from being cached.
-		 */
-		return NULL;
-	}
-#endif
 	return d_splice_alias(inode, dentry);
 }
 
@@ -3206,17 +3200,6 @@ 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))
-		d_invalidate(dentry);
-#endif
-
 end_rmdir:
 	brelse(bh);
 	if (handle)
@@ -3317,16 +3300,6 @@ 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))
-		d_invalidate(dentry);
-#endif
 
 out_trace:
 	trace_ext4_unlink_exit(dentry, retval);
-- 
2.41.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH v5 10/10] f2fs: Enable negative dentries on case-insensitive lookup

[Gabriel Krisman Bertazi]

From: Gabriel Krisman Bertazi <krisman@collabora.com>

Instead of invalidating negative dentries during case-insensitive
lookups, mark them as such and let them be added to the dcache.
d_ci_revalidate is able to properly filter them out if necessary based
on the dentry casefold flag.

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>

---
Changes since v4:
  - Use helper to decide if should set dentry flag.

Changes since v2:
  - Move dentry flag set closer to fscrypt code (Eric)
---
 fs/f2fs/namei.c | 25 ++++---------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index bee0568888da..8b8fd4cdf62d 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -533,6 +533,10 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
 
 	err = f2fs_prepare_lookup(dir, dentry, &fname);
 	generic_set_encrypted_ci_d_ops(dentry);
+
+	if (dir_is_casefolded(dir))
+		d_set_casefolded_name(dentry);
+
 	if (err == -ENOENT)
 		goto out_splice;
 	if (err)
@@ -578,17 +582,6 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
 		goto out_iput;
 	}
 out_splice:
-#if IS_ENABLED(CONFIG_UNICODE)
-	if (!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
-		 * from being cached.
-		 */
-		trace_f2fs_lookup_end(dir, dentry, ino, err);
-		return NULL;
-	}
-#endif
 	new = d_splice_alias(inode, dentry);
 	trace_f2fs_lookup_end(dir, !IS_ERR_OR_NULL(new) ? new : dentry,
 				ino, IS_ERR(new) ? PTR_ERR(new) : err);
@@ -641,16 +634,6 @@ static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
 	f2fs_delete_entry(de, page, dir, inode);
 	f2fs_unlock_op(sbi);
 
-#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 f2fs_lookup(), when it is better
-	 * supported by the VFS for the CI case.
-	 */
-	if (IS_CASEFOLDED(dir))
-		d_invalidate(dentry);
-#endif
 	if (IS_DIRSYNC(dir))
 		f2fs_sync_fs(sbi->sb, 1);
 fail:
-- 
2.41.0



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding

[Eric Biggers]

On Fri, Aug 11, 2023 at 08:41:37PM -0400, Gabriel Krisman Bertazi wrote:
> In preparation to use it in ecryptfs, move needs_casefolding into a
> public header and give it a namespaced name.
> 
> Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
> ---
>  fs/libfs.c         | 14 ++------------
>  include/linux/fs.h | 21 +++++++++++++++++++++
>  2 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 5b851315eeed..8d0b64cfd5da 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -1381,16 +1381,6 @@ bool is_empty_dir_inode(struct inode *inode)
>  }
>  
>  #if IS_ENABLED(CONFIG_UNICODE)
> -/*
> - * Determine if the name of a dentry should be casefolded.
> - *
> - * Return: if names will need casefolding
> - */
> -static bool needs_casefold(const struct inode *dir)
> -{
> -	return IS_CASEFOLDED(dir) && dir->i_sb->s_encoding;
> -}
> -
>  /**
>   * generic_ci_d_compare - generic d_compare implementation for casefolding filesystems
>   * @dentry:	dentry whose name we are checking against
> @@ -1411,7 +1401,7 @@ static int generic_ci_d_compare(const struct dentry *dentry, unsigned int len,
>  	char strbuf[DNAME_INLINE_LEN];
>  	int ret;
>  
> -	if (!dir || !needs_casefold(dir))
> +	if (!dir || !dir_is_casefolded(dir))
>  		goto fallback;
>  	/*
>  	 * If the dentry name is stored in-line, then it may be concurrently
> @@ -1453,7 +1443,7 @@ static int generic_ci_d_hash(const struct dentry *dentry, struct qstr *str)
>  	const struct unicode_map *um = sb->s_encoding;
>  	int ret = 0;
>  
> -	if (!dir || !needs_casefold(dir))
> +	if (!dir || !dir_is_casefolded(dir))
>  		return 0;
>  
>  	ret = utf8_casefold_hash(um, dentry, str);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 6867512907d6..e3b631c6d24a 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3213,6 +3213,27 @@ static inline bool dir_relax_shared(struct inode *inode)
>  	return !IS_DEADDIR(inode);
>  }
>  
> +/**
> + * dir_is_casefolded - Safely determine if filenames inside of a
> + * directory should be casefolded.
> + * @dir: The directory inode to be checked
> + *
> + * Filesystems should usually rely on this instead of checking the
> + * S_CASEFOLD flag directly when handling inodes, to avoid surprises
> + * with corrupted volumes.  Checking i_sb->s_encoding ensures the
> + * filesystem knows how to deal with case-insensitiveness.
> + *
> + * Return: if names will need casefolding
> + */
> +static inline bool dir_is_casefolded(const struct inode *dir)
> +{
> +#if IS_ENABLED(CONFIG_UNICODE)
> +	return IS_CASEFOLDED(dir) && dir->i_sb->s_encoding;
> +#else
> +	return false;
> +#endif
> +}

To be honest I've always been confused about why the ->s_encoding check is
there.  It looks like Ted added it in 6456ca6520ab ("ext4: fix kernel oops
caused by spurious casefold flag") to address a fuzzing report for a filesystem
that had a casefolded directory but didn't have the casefold feature flag set.
It seems like an unnecessarily complex fix, though.  The filesystem should just
reject the inode earlier, in __ext4_iget().  And likewise for f2fs.  Then no
other code has to worry about this problem.

Actually, f2fs already does it, as I added it in commit f6322f3f1212:

        if ((fi->i_flags & F2FS_CASEFOLD_FL) && !f2fs_sb_has_casefold(sbi)) {
                set_sbi_flag(sbi, SBI_NEED_FSCK);
                f2fs_warn(sbi, "%s: inode (ino=%lx) has casefold flag, but casefold feature is off",
                          __func__, inode->i_ino);
                return false;
        }

So just __ext4_iget() needs to be fixed.  I think we should consider doing that
before further entrenching all the extra ->s_encoding checks.

Also I don't understand why this needs to be part of your patch series anyway.
Shouldn't eCryptfs check IS_CASEFOLDED() anyway?

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 04/10] fs: Expose name under lookup to d_revalidate hooks

[Eric Biggers]

On Fri, Aug 11, 2023 at 08:41:40PM -0400, Gabriel Krisman Bertazi wrote:
> diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst
> index cb2a97e49872..ddd542c2a722 100644
> --- a/Documentation/filesystems/vfs.rst
> +++ b/Documentation/filesystems/vfs.rst
> @@ -1251,7 +1251,8 @@ defined:
>  .. code-block:: c
>  
>  	struct dentry_operations {
> -		int (*d_revalidate)(struct dentry *, unsigned int);
> +		int (*d_revalidate)(struct dentry *, const struct qstr *,
> +				    unsigned int);
>  		int (*d_weak_revalidate)(struct dentry *, unsigned int);
>  		int (*d_hash)(const struct dentry *, struct qstr *);
>  		int (*d_compare)(const struct dentry *,
> @@ -1284,6 +1285,14 @@ defined:
>  	they can change and, in d_inode case, even become NULL under
>  	us).
>  
> +	Filesystems shouldn't rely on the name under lookup, unless
> +	there are particular filename encoding semantics to be handled
> +	during revalidation.  Note the name under lookup can change from
> +	under d_revalidate, so it must be protected with ->d_lock before
> +	accessing.  The exception is when revalidating negative dentries
> +	for creation, in which case the parent inode prevents it from
> +	changing.

Actually, the "name under lookup" can never change.  It's passed as the 'name'
argument, newly added by this patch.  What this paragraph is actually about is
the ->d_name of the dentry being revalidated.  The documentation should make it
clear when it means ->d_name and when it means name, how they differ from each
other, and what the purpose of each is.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 05/10] fs: Add DCACHE_CASEFOLDED_NAME flag

[Eric Biggers]

On Fri, Aug 11, 2023 at 08:41:41PM -0400, Gabriel Krisman Bertazi wrote:
> +void d_set_casefolded_name(struct dentry *dentry)
> +{
> +	spin_lock(&dentry->d_lock);
> +	dentry->d_flags |= DCACHE_CASEFOLDED_NAME;
> +	spin_unlock(&dentry->d_lock);
> +}
> +EXPORT_SYMBOL(d_set_casefold_lookup);

s/d_set_casefold_lookup/d_set_casefolded_name/

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 06/10] libfs: Validate negative dentries in case-insensitive directories

[Eric Biggers]

On Fri, Aug 11, 2023 at 08:41:42PM -0400, Gabriel Krisman Bertazi wrote:
> +	/*
> +	 * Filesystems will call into d_revalidate without setting
> +	 * LOOKUP_ flags even for file creation (see lookup_one*
> +	 * variants).  Reject negative dentries in this case, since we
> +	 * can't know for sure it won't be used for creation.
> +	 */
> +	if (!flags)
> +		return 0;
> +
> +	/*
> +	 * If the lookup is for creation, then a negative dentry can
> +	 * only be reused if it's a case-sensitive match, not just a
> +	 * case-insensitive one.  This is needed to make the new file be
> +	 * created with the name the user specified, preserving case.
> +	 */
> +	if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) {
> +		/*
> +		 * ->d_name won't change from under us in the creation
> +		 * path only, since d_revalidate during creation and
> +		 * renames is always called with the parent inode
> +		 * locked.  It isn't the case for all lookup callpaths,
> +		 * so ->d_name must not be touched outside
> +		 * (LOOKUP_CREATE|LOOKUP_RENAME_TARGET) context.
> +		 */
> +		if (dentry->d_name.len != name->len ||
> +		    memcmp(dentry->d_name.name, name->name, name->len))
> +			return 0;
> +	}

This is still really confusing to me.  Can you consider the below?  The code is
the same except for the reordering, but the explanation is reworked to be much
clearer (IMO).  Anything I am misunderstanding?

	/*
	 * If the lookup is for creation, then a negative dentry can only be
	 * reused if it's a case-sensitive match, not just a case-insensitive
	 * one.  This is needed to make the new file be created with the name
	 * the user specified, preserving case.
	 *
	 * LOOKUP_CREATE or LOOKUP_RENAME_TARGET cover most creations.  In these
	 * cases, ->d_name is stable and can be compared to 'name' without
	 * taking ->d_lock because the caller holds dir->i_rwsem for write.
	 * (This is because the directory lock blocks the dentry from being
	 * concurrently instantiated, and negative dentries are never moved.)
	 *
	 * All other creations actually use flags==0.  These come from the edge
	 * case of filesystems calling functions like lookup_one() that do a
	 * lookup without setting the lookup flags at all.  Such lookups might
	 * or might not be for creation, and if not don't guarantee stable
	 * ->d_name.  Therefore, invalidate all negative dentries when flags==0.
	 */
	if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) {
		if (dentry->d_name.len != name->len ||
		    memcmp(dentry->d_name.name, name->name, name->len))
			return 0;
	}
	if (!flags)
		return 0;


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding

[Theodore Ts'o]

On Fri, Aug 11, 2023 at 06:59:15PM -0700, Eric Biggers wrote:
> 
> To be honest I've always been confused about why the ->s_encoding check is
> there.  It looks like Ted added it in 6456ca6520ab ("ext4: fix kernel oops
> caused by spurious casefold flag") to address a fuzzing report for a filesystem
> that had a casefolded directory but didn't have the casefold feature flag set.
> It seems like an unnecessarily complex fix, though.  The filesystem should just
> reject the inode earlier, in __ext4_iget().  And likewise for f2fs.  Then no
> other code has to worry about this problem.

It's not enough to check it in ext4_iget, since the casefold flag can
get set *after* the inode has been fetched, but before you try to use
it.  This can happen because syzbot has opened the block device for
writing, and edits the superblock while it is mounted.

One could say that this is an insane threat model, but the syzbot team
thinks that this can be used to break out of a kernel lockdown after a
UEFI secure boot.  Which is fine, except I don't think I've been able
to get any company (including Google) to pay for headcount to fix
problems like this, and the unremitting stream of these sorts of
syzbot reports have already caused one major file system developer to
burn out and step down.

So problems like this get fixed on my own time, and when I have some
free time.  And if we "simplify" the code, it will inevitably cause
more syzbot reports, which I will then have to ignore, and the syzbot
team will write more "kernel security disaster" slide deck
presentations to senior VP's, although I'll note this has never
resulted in my getting any additional SWE's to help me fix the
problem...

> So just __ext4_iget() needs to be fixed.  I think we should consider doing that
> before further entrenching all the extra ->s_encoding checks.

If we can get an upstream kernel consensus that syzbot reports caused
by writing to a mounted file system aren't important, and we can
publish this somewhere where hopefully the syzbot team will pay
attention to it, sure...


						- Ted


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding

[Eric Biggers]

On Sat, Aug 12, 2023 at 07:06:47PM -0400, Theodore Ts'o wrote:
> On Fri, Aug 11, 2023 at 06:59:15PM -0700, Eric Biggers wrote:
> > 
> > To be honest I've always been confused about why the ->s_encoding check is
> > there.  It looks like Ted added it in 6456ca6520ab ("ext4: fix kernel oops
> > caused by spurious casefold flag") to address a fuzzing report for a filesystem
> > that had a casefolded directory but didn't have the casefold feature flag set.
> > It seems like an unnecessarily complex fix, though.  The filesystem should just
> > reject the inode earlier, in __ext4_iget().  And likewise for f2fs.  Then no
> > other code has to worry about this problem.
> 
> the casefold flag can get set *after* the inode has been fetched, but before
> you try to use it.  This can happen because syzbot has opened the block device
> for writing, and edits the superblock while it is mounted.

I don't see how that is relevant here.

I think the actual problem you're hinting at is that checking the casefold
feature after the filesystem has been mounted is not guaranteed to work
properly, as ->s_encoding will be NULL if the casefold feature was not present
at mount time.  If we'd like to be robust in the event of the casefold feature
being concurrently enabled by a write to the block device, then all we need to
do is avoid checking the casefold feature after mount time, and instead check
->s_encoding.  I believe __ext4_iget() is still the only place it's needed.

> One could say that this is an insane threat model, but the syzbot team
> thinks that this can be used to break out of a kernel lockdown after a
> UEFI secure boot.  Which is fine, except I don't think I've been able
> to get any company (including Google) to pay for headcount to fix
> problems like this, and the unremitting stream of these sorts of
> syzbot reports have already caused one major file system developer to
> burn out and step down.
> 
> So problems like this get fixed on my own time, and when I have some
> free time.  And if we "simplify" the code, it will inevitably cause
> more syzbot reports, which I will then have to ignore, and the syzbot
> team will write more "kernel security disaster" slide deck
> presentations to senior VP's, although I'll note this has never
> resulted in my getting any additional SWE's to help me fix the
> problem...
> 
> > So just __ext4_iget() needs to be fixed.  I think we should consider doing that
> > before further entrenching all the extra ->s_encoding checks.
> 
> If we can get an upstream kernel consensus that syzbot reports caused
> by writing to a mounted file system aren't important, and we can
> publish this somewhere where hopefully the syzbot team will pay
> attention to it, sure...

But, more generally, I think it's clear that concurrent writes to the block
device's page cache is not something that filesystems can be robust against.  I
think this needs to be solved by providing an option to forbid this, as Jan
Kara's patchset "block: Add config option to not allow writing to mounted
devices" does, and then transitioning legacy use cases to new APIs.

Yes, "transitioning legacy use cases" will be a lot of work.  And if The Linux
Filesystem Maintainers(TM) do not have time for it, that's the way it is.
Someone who cares about it (such as someone who actually cares about the
potential impact on the Lockdown feature) will need to come along and do it.

But I think that should be the plan, and The Linux Filesystem Maintainers(TM) do
not need to try to play whack-a-mole with "fixing" filesystem code to be
consistently revalidating already-validated cached metadata.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding

[Matthew Wilcox]

On Sat, Aug 12, 2023 at 07:06:47PM -0400, Theodore Ts'o wrote:
> One could say that this is an insane threat model, but the syzbot team
> thinks that this can be used to break out of a kernel lockdown after a
> UEFI secure boot.  Which is fine, except I don't think I've been able
> to get any company (including Google) to pay for headcount to fix
> problems like this, and the unremitting stream of these sorts of
> syzbot reports have already caused one major file system developer to
> burn out and step down.
> 
> So problems like this get fixed on my own time, and when I have some
> free time.  And if we "simplify" the code, it will inevitably cause
> more syzbot reports, which I will then have to ignore, and the syzbot
> team will write more "kernel security disaster" slide deck
> presentations to senior VP's, although I'll note this has never
> resulted in my getting any additional SWE's to help me fix the
> problem...
> 
> > So just __ext4_iget() needs to be fixed.  I think we should consider doing that
> > before further entrenching all the extra ->s_encoding checks.
> 
> If we can get an upstream kernel consensus that syzbot reports caused
> by writing to a mounted file system aren't important, and we can
> publish this somewhere where hopefully the syzbot team will pay
> attention to it, sure...

What the syzbot team don't seem to understand is that more bug reports
aren't better.  I used to investigate one most days, but the onslaught is
relentless and I just ignore syzbot reports now.  I appreciate maintainers
don't necessarily get that privilege.

They seem motivated to find new and exciting ways to break the kernel
without realising that they're sapping the will to work on the bugs that
we already have.

Hm.  Maybe this is a topic for kernel-summit?


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding

[Eric Biggers]

On Sun, Aug 13, 2023 at 04:08:58AM +0100, Matthew Wilcox wrote:
> On Sat, Aug 12, 2023 at 07:06:47PM -0400, Theodore Ts'o wrote:
> > One could say that this is an insane threat model, but the syzbot team
> > thinks that this can be used to break out of a kernel lockdown after a
> > UEFI secure boot.  Which is fine, except I don't think I've been able
> > to get any company (including Google) to pay for headcount to fix
> > problems like this, and the unremitting stream of these sorts of
> > syzbot reports have already caused one major file system developer to
> > burn out and step down.
> > 
> > So problems like this get fixed on my own time, and when I have some
> > free time.  And if we "simplify" the code, it will inevitably cause
> > more syzbot reports, which I will then have to ignore, and the syzbot
> > team will write more "kernel security disaster" slide deck
> > presentations to senior VP's, although I'll note this has never
> > resulted in my getting any additional SWE's to help me fix the
> > problem...
> > 
> > > So just __ext4_iget() needs to be fixed.  I think we should consider doing that
> > > before further entrenching all the extra ->s_encoding checks.
> > 
> > If we can get an upstream kernel consensus that syzbot reports caused
> > by writing to a mounted file system aren't important, and we can
> > publish this somewhere where hopefully the syzbot team will pay
> > attention to it, sure...
> 
> What the syzbot team don't seem to understand is that more bug reports
> aren't better.  I used to investigate one most days, but the onslaught is
> relentless and I just ignore syzbot reports now.  I appreciate maintainers
> don't necessarily get that privilege.
> 
> They seem motivated to find new and exciting ways to break the kernel
> without realising that they're sapping the will to work on the bugs that
> we already have.
> 

Well, one thing that the kernel community can do to make things better is
identify when a large number of bug reports are caused by a single issue
("userspace can write to mounted block devices"), and do something about that
underlying issue (https://lore.kernel.org/r/20230704122727.17096-1-jack@suse.cz)
instead of trying to "fix" large numbers of individual "bugs".  We can have 1000
bugs or 1 bug, it is actually our choice in this case.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding

[Theodore Ts'o]

On Sat, Aug 12, 2023 at 09:30:22PM -0700, Eric Biggers wrote:
> Well, one thing that the kernel community can do to make things better is
> identify when a large number of bug reports are caused by a single issue
> ("userspace can write to mounted block devices"), and do something about that
> underlying issue (https://lore.kernel.org/r/20230704122727.17096-1-jack@suse.cz)
> instead of trying to "fix" large numbers of individual "bugs".  We can have 1000
> bugs or 1 bug, it is actually our choice in this case.

That's assuming the syzbot folks are willing to enable the config in
Jan's patch.  The syzbot folks refused to enable it, unless the config
was gated on CONFIG_INSECURE, which I object to, because that's
presuming a threat model that we have not all agreed is valid.

Or rather, if it *is* valid some community members (or cough, cough,
**companies**) need to step up and supply patches.  As the saying
goes, "patches gratefully accepted".  It is *not* the maintainer's
responsibility to grant every single person whining about a feature
request, or even a bug fix.

       	       	       	       		   	  - Ted


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 06/10] libfs: Validate negative dentries in case-insensitive directories

[Gabriel Krisman Bertazi]

Eric Biggers <ebiggers@kernel.org> writes:

> On Fri, Aug 11, 2023 at 08:41:42PM -0400, Gabriel Krisman Bertazi wrote:
>> +	/*
>> +	 * Filesystems will call into d_revalidate without setting
>> +	 * LOOKUP_ flags even for file creation (see lookup_one*
>> +	 * variants).  Reject negative dentries in this case, since we
>> +	 * can't know for sure it won't be used for creation.
>> +	 */
>> +	if (!flags)
>> +		return 0;
>> +
>> +	/*
>> +	 * If the lookup is for creation, then a negative dentry can
>> +	 * only be reused if it's a case-sensitive match, not just a
>> +	 * case-insensitive one.  This is needed to make the new file be
>> +	 * created with the name the user specified, preserving case.
>> +	 */
>> +	if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) {
>> +		/*
>> +		 * ->d_name won't change from under us in the creation
>> +		 * path only, since d_revalidate during creation and
>> +		 * renames is always called with the parent inode
>> +		 * locked.  It isn't the case for all lookup callpaths,
>> +		 * so ->d_name must not be touched outside
>> +		 * (LOOKUP_CREATE|LOOKUP_RENAME_TARGET) context.
>> +		 */
>> +		if (dentry->d_name.len != name->len ||
>> +		    memcmp(dentry->d_name.name, name->name, name->len))
>> +			return 0;
>> +	}
>
> This is still really confusing to me.  Can you consider the below?  The code is
> the same except for the reordering, but the explanation is reworked to be much
> clearer (IMO).  Anything I am misunderstanding?
>
> 	/*
> 	 * If the lookup is for creation, then a negative dentry can only be
> 	 * reused if it's a case-sensitive match, not just a case-insensitive
> 	 * one.  This is needed to make the new file be created with the name
> 	 * the user specified, preserving case.
> 	 *
> 	 * LOOKUP_CREATE or LOOKUP_RENAME_TARGET cover most creations.  In these
> 	 * cases, ->d_name is stable and can be compared to 'name' without
> 	 * taking ->d_lock because the caller holds dir->i_rwsem for write.
> 	 * (This is because the directory lock blocks the dentry from being
> 	 * concurrently instantiated, and negative dentries are never moved.)
> 	 *
> 	 * All other creations actually use flags==0.  These come from the edge
> 	 * case of filesystems calling functions like lookup_one() that do a
> 	 * lookup without setting the lookup flags at all.  Such lookups might
> 	 * or might not be for creation, and if not don't guarantee stable
> 	 * ->d_name.  Therefore, invalidate all negative dentries when flags==0.
> 	 */
> 	if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) {
> 		if (dentry->d_name.len != name->len ||
> 		    memcmp(dentry->d_name.name, name->name, name->len))
> 			return 0;
> 	}
> 	if (!flags)
> 		return 0;

I don't see it as particularly better or less confusing than the
original. but I also don't mind taking it into the next iteration.

-- 
Gabriel Krisman Bertazi


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding

[Gabriel Krisman Bertazi]

Eric Biggers <ebiggers@kernel.org> writes:

> On Fri, Aug 11, 2023 at 08:41:37PM -0400, Gabriel Krisman Bertazi wrote:
>> In preparation to use it in ecryptfs, move needs_casefolding into a
>> public header and give it a namespaced name.
>> 
>> Signed-off-by: Gabriel Krisman Bertazi <krisman@suse.de>
>> ---
>>  fs/libfs.c         | 14 ++------------
>>  include/linux/fs.h | 21 +++++++++++++++++++++
>>  2 files changed, 23 insertions(+), 12 deletions(-)
>> 
>> diff --git a/fs/libfs.c b/fs/libfs.c
>> index 5b851315eeed..8d0b64cfd5da 100644
>> --- a/fs/libfs.c
>> +++ b/fs/libfs.c
>> @@ -1381,16 +1381,6 @@ bool is_empty_dir_inode(struct inode *inode)
>>  }
>>  
>>  #if IS_ENABLED(CONFIG_UNICODE)
>> -/*
>> - * Determine if the name of a dentry should be casefolded.
>> - *
>> - * Return: if names will need casefolding
>> - */
>> -static bool needs_casefold(const struct inode *dir)
>> -{
>> -	return IS_CASEFOLDED(dir) && dir->i_sb->s_encoding;
>> -}
>> -
>>  /**
>>   * generic_ci_d_compare - generic d_compare implementation for casefolding filesystems
>>   * @dentry:	dentry whose name we are checking against
>> @@ -1411,7 +1401,7 @@ static int generic_ci_d_compare(const struct dentry *dentry, unsigned int len,
>>  	char strbuf[DNAME_INLINE_LEN];
>>  	int ret;
>>  
>> -	if (!dir || !needs_casefold(dir))
>> +	if (!dir || !dir_is_casefolded(dir))
>>  		goto fallback;
>>  	/*
>>  	 * If the dentry name is stored in-line, then it may be concurrently
>> @@ -1453,7 +1443,7 @@ static int generic_ci_d_hash(const struct dentry *dentry, struct qstr *str)
>>  	const struct unicode_map *um = sb->s_encoding;
>>  	int ret = 0;
>>  
>> -	if (!dir || !needs_casefold(dir))
>> +	if (!dir || !dir_is_casefolded(dir))
>>  		return 0;
>>  
>>  	ret = utf8_casefold_hash(um, dentry, str);
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index 6867512907d6..e3b631c6d24a 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -3213,6 +3213,27 @@ static inline bool dir_relax_shared(struct inode *inode)
>>  	return !IS_DEADDIR(inode);
>>  }
>>  
>> +/**
>> + * dir_is_casefolded - Safely determine if filenames inside of a
>> + * directory should be casefolded.
>> + * @dir: The directory inode to be checked
>> + *
>> + * Filesystems should usually rely on this instead of checking the
>> + * S_CASEFOLD flag directly when handling inodes, to avoid surprises
>> + * with corrupted volumes.  Checking i_sb->s_encoding ensures the
>> + * filesystem knows how to deal with case-insensitiveness.
>> + *
>> + * Return: if names will need casefolding
>> + */
>> +static inline bool dir_is_casefolded(const struct inode *dir)
>> +{
>> +#if IS_ENABLED(CONFIG_UNICODE)
>> +	return IS_CASEFOLDED(dir) && dir->i_sb->s_encoding;
>> +#else
>> +	return false;
>> +#endif
>> +}
>
> To be honest I've always been confused about why the ->s_encoding check is
> there.  It looks like Ted added it in 6456ca6520ab ("ext4: fix kernel oops
> caused by spurious casefold flag") to address a fuzzing report for a filesystem
> that had a casefolded directory but didn't have the casefold feature flag set.
> It seems like an unnecessarily complex fix, though.  The filesystem should just
> reject the inode earlier, in __ext4_iget().  And likewise for f2fs.  Then no
> other code has to worry about this problem.
>
> Actually, f2fs already does it, as I added it in commit f6322f3f1212:
>
>         if ((fi->i_flags & F2FS_CASEFOLD_FL) && !f2fs_sb_has_casefold(sbi)) {
>                 set_sbi_flag(sbi, SBI_NEED_FSCK);
>                 f2fs_warn(sbi, "%s: inode (ino=%lx) has casefold flag, but casefold feature is off",
>                           __func__, inode->i_ino);
>                 return false;
>         }
>
> So just __ext4_iget() needs to be fixed.  I think we should consider doing that
> before further entrenching all the extra ->s_encoding checks.
>
> Also I don't understand why this needs to be part of your patch series anyway.
> Shouldn't eCryptfs check IS_CASEFOLDED() anyway?

While I agree with Ted's point about how this change is tiny compared to
the benefit of preventing casefold-related superblock corruptions on
runtime (and we want to keep it being done in ext4),  I also agree with
you that we don't need to check it also in ecryptfs.  But, I'll preserve
it in d_revalidate, since it is what we are currently doing in
d_compare/d_hash.

Also, this patchset has been sitting for years before the latest
discussions, and I'm tired of it, so I'm happy to keep this
discussion for another time.  Will drop this patch and just check
IS_CASEFOLDED in ecryptfs for the next iteration.

I'll follow up with another case-insensitive cleanup patchset I've been
siting on forever, which includes this patch and will restart this
discussion, among others.

-- 
Gabriel Krisman Bertazi


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 05/10] fs: Add DCACHE_CASEFOLDED_NAME flag

[Gabriel Krisman Bertazi]

Eric Biggers <ebiggers@kernel.org> writes:

> On Fri, Aug 11, 2023 at 08:41:41PM -0400, Gabriel Krisman Bertazi wrote:
>> +void d_set_casefolded_name(struct dentry *dentry)
>> +{
>> +	spin_lock(&dentry->d_lock);
>> +	dentry->d_flags |= DCACHE_CASEFOLDED_NAME;
>> +	spin_unlock(&dentry->d_lock);
>> +}
>> +EXPORT_SYMBOL(d_set_casefold_lookup);
>
> s/d_set_casefold_lookup/d_set_casefolded_name/

My apologies for this error again.  It sucks there is no compile-time
warning for EXPORT_SYMBOL, but I should have caught it in the past two
iterations.  Will fix.

-- 
Gabriel Krisman Bertazi


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding

[Eric Biggers]

On Mon, Aug 14, 2023 at 07:38:52AM -0400, Theodore Ts'o wrote:
> On Sat, Aug 12, 2023 at 09:30:22PM -0700, Eric Biggers wrote:
> > Well, one thing that the kernel community can do to make things better is
> > identify when a large number of bug reports are caused by a single issue
> > ("userspace can write to mounted block devices"), and do something about that
> > underlying issue (https://lore.kernel.org/r/20230704122727.17096-1-jack@suse.cz)
> > instead of trying to "fix" large numbers of individual "bugs".  We can have 1000
> > bugs or 1 bug, it is actually our choice in this case.
> 
> That's assuming the syzbot folks are willing to enable the config in
> Jan's patch.  The syzbot folks refused to enable it, unless the config
> was gated on CONFIG_INSECURE, which I object to, because that's
> presuming a threat model that we have not all agreed is valid.
> 
> Or rather, if it *is* valid some community members (or cough, cough,
> **companies**) need to step up and supply patches.  As the saying
> goes, "patches gratefully accepted".  It is *not* the maintainer's
> responsibility to grant every single person whining about a feature
> request, or even a bug fix.

They did disable CONFIG_XFS_SUPPORT_V4.  Yes, there was some pushback, but they
are very understandably (and correctly) concerned about ending up in a situation
where buggy code is disabled for syzkaller but enabled for everyone else.  They
eventually did accept the proposal to disable CONFIG_XFS_SUPPORT_V4, for reasons
including the fact that there is a concrete deprecation plan.  (FWIW, the XFS
maintainers had also pushed back strongly when I suggested adding a kconfig
option for XFS v4 support back in 2018.  Sometimes these things just take time.)

Anyway, syzkaller is just the messenger that is reminding us of a problem.  The
actual problem here is that "make filesystems robust against concurrent changes
to block device's page cache" is effectively unsolvable.  *Every* memory access
to the cache would need to use READ_ONCE/WRITE_ONCE, and *every* re-read of
every field would need to be fully re-validated.  PageChecked would need to go
away for metadata, as it would be invalid to cache the checked status at all.
There's basically zero chance of getting this correct; filesystems struggle to
validate even the metadata read from disk correctly, so how are they going to
successfully continually revalidate all cached metadata in memory?

I don't understand why we would waste time trying to do that instead of focusing
on an actual solution.  Sure, probably The Linux Filesystem Maintainers(TM)
don't have time to help with migrating legacy use cases of writing to mounted
block devices, but that just means that someone needs to step up to do it.  It
doesn't mean that they need to instead waste time on pointless "fixes".

Keep in mind, the syzkaller team isn't asking for these pointless "fixes"
either.  They'd very much prefer 1 fix to 1000 fixes.  I think some confusion
might be arising from the very different types of problems that syzkaller finds.
Sometimes 1 syzkaller report == 1 bug == 1 high-priority "must fix" bug == 1
vulnerability == 1 fix needed.  But in general syzkaller is just letting kernel
developers know about a problem, and it is up to them to decide what to do about
it.  In this case there is one underlying issue that needs to be fixed, and the
individual syzkaller reports that result from that issue are not important.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 06/10] libfs: Validate negative dentries in case-insensitive directories

[Eric Biggers]

On Mon, Aug 14, 2023 at 10:50:13AM -0400, Gabriel Krisman Bertazi wrote:
> Eric Biggers <ebiggers@kernel.org> writes:
> 
> > On Fri, Aug 11, 2023 at 08:41:42PM -0400, Gabriel Krisman Bertazi wrote:
> >> +	/*
> >> +	 * Filesystems will call into d_revalidate without setting
> >> +	 * LOOKUP_ flags even for file creation (see lookup_one*
> >> +	 * variants).  Reject negative dentries in this case, since we
> >> +	 * can't know for sure it won't be used for creation.
> >> +	 */
> >> +	if (!flags)
> >> +		return 0;
> >> +
> >> +	/*
> >> +	 * If the lookup is for creation, then a negative dentry can
> >> +	 * only be reused if it's a case-sensitive match, not just a
> >> +	 * case-insensitive one.  This is needed to make the new file be
> >> +	 * created with the name the user specified, preserving case.
> >> +	 */
> >> +	if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) {
> >> +		/*
> >> +		 * ->d_name won't change from under us in the creation
> >> +		 * path only, since d_revalidate during creation and
> >> +		 * renames is always called with the parent inode
> >> +		 * locked.  It isn't the case for all lookup callpaths,
> >> +		 * so ->d_name must not be touched outside
> >> +		 * (LOOKUP_CREATE|LOOKUP_RENAME_TARGET) context.
> >> +		 */
> >> +		if (dentry->d_name.len != name->len ||
> >> +		    memcmp(dentry->d_name.name, name->name, name->len))
> >> +			return 0;
> >> +	}
> >
> > This is still really confusing to me.  Can you consider the below?  The code is
> > the same except for the reordering, but the explanation is reworked to be much
> > clearer (IMO).  Anything I am misunderstanding?
> >
> > 	/*
> > 	 * If the lookup is for creation, then a negative dentry can only be
> > 	 * reused if it's a case-sensitive match, not just a case-insensitive
> > 	 * one.  This is needed to make the new file be created with the name
> > 	 * the user specified, preserving case.
> > 	 *
> > 	 * LOOKUP_CREATE or LOOKUP_RENAME_TARGET cover most creations.  In these
> > 	 * cases, ->d_name is stable and can be compared to 'name' without
> > 	 * taking ->d_lock because the caller holds dir->i_rwsem for write.
> > 	 * (This is because the directory lock blocks the dentry from being
> > 	 * concurrently instantiated, and negative dentries are never moved.)
> > 	 *
> > 	 * All other creations actually use flags==0.  These come from the edge
> > 	 * case of filesystems calling functions like lookup_one() that do a
> > 	 * lookup without setting the lookup flags at all.  Such lookups might
> > 	 * or might not be for creation, and if not don't guarantee stable
> > 	 * ->d_name.  Therefore, invalidate all negative dentries when flags==0.
> > 	 */
> > 	if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) {
> > 		if (dentry->d_name.len != name->len ||
> > 		    memcmp(dentry->d_name.name, name->name, name->len))
> > 			return 0;
> > 	}
> > 	if (!flags)
> > 		return 0;
> 
> I don't see it as particularly better or less confusing than the
> original. but I also don't mind taking it into the next iteration.
> 

Your commit message is still much longer and covers some quite different details
which seem irrelevant to me.  So if you don't see my explanation as being much
different, I think we're still not on the same page.  I hope that I'm not
misunderstanding anything, in which I believe that what I wrote above is a good
explanation and your commit message should be substantially simplified.
Remember, longer != better.  Keep things as simple as possible.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding

[Eric Biggers]

On Mon, Aug 14, 2023 at 11:02:38AM -0400, Gabriel Krisman Bertazi wrote:
> 
> Also, this patchset has been sitting for years before the latest
> discussions, and I'm tired of it, so I'm happy to keep this
> discussion for another time.  Will drop this patch and just check
> IS_CASEFOLDED in ecryptfs for the next iteration.
> 
> I'll follow up with another case-insensitive cleanup patchset I've been
> siting on forever, which includes this patch and will restart this
> discussion, among others.
> 

Well, as we know unfortunately filesystem developers are in short supply, and
doing proper reviews (identifying issues and working closely with the patchset
author over multiple iterations to address them, as opposed to just slapping on
a Reviewed-by) is very time consuming.  Earlier this year I tried to get the
Android Systems team, which is ostensibly responsible for Android's use of
casefolding, to take a look, but their entire feedback was just "looks good to
me".  Also, the fact that this patchset originally excluded the casefold+encrypt
case technically made it not applicable to Android, and discouraged me from
taking a look since encryption is my focus.  Sorry for not taking a look sooner.

Anyway, thanks for doing this, and I think it's near the finish line now.  Once
you address the latest feedback and get a couple acks, I think that Christian
should take this through the VFS tree.  BTW, in my opinion, as the maintainer of
the "Unicode subsystem" you are also authorized to send a pull request for this
to Linus yourself.  But VFS does seem ideal in this case, given the diffstat,
and Christian has been fairly active with taking patches.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 06/10] libfs: Validate negative dentries in case-insensitive directories

[Gabriel Krisman Bertazi]

Eric Biggers <ebiggers@kernel.org> writes:

> On Mon, Aug 14, 2023 at 10:50:13AM -0400, Gabriel Krisman Bertazi wrote:
>> Eric Biggers <ebiggers@kernel.org> writes:
>> 
>> > On Fri, Aug 11, 2023 at 08:41:42PM -0400, Gabriel Krisman Bertazi wrote:
>> >> +	/*
>> >> +	 * Filesystems will call into d_revalidate without setting
>> >> +	 * LOOKUP_ flags even for file creation (see lookup_one*
>> >> +	 * variants).  Reject negative dentries in this case, since we
>> >> +	 * can't know for sure it won't be used for creation.
>> >> +	 */
>> >> +	if (!flags)
>> >> +		return 0;
>> >> +
>> >> +	/*
>> >> +	 * If the lookup is for creation, then a negative dentry can
>> >> +	 * only be reused if it's a case-sensitive match, not just a
>> >> +	 * case-insensitive one.  This is needed to make the new file be
>> >> +	 * created with the name the user specified, preserving case.
>> >> +	 */
>> >> +	if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) {
>> >> +		/*
>> >> +		 * ->d_name won't change from under us in the creation
>> >> +		 * path only, since d_revalidate during creation and
>> >> +		 * renames is always called with the parent inode
>> >> +		 * locked.  It isn't the case for all lookup callpaths,
>> >> +		 * so ->d_name must not be touched outside
>> >> +		 * (LOOKUP_CREATE|LOOKUP_RENAME_TARGET) context.
>> >> +		 */
>> >> +		if (dentry->d_name.len != name->len ||
>> >> +		    memcmp(dentry->d_name.name, name->name, name->len))
>> >> +			return 0;
>> >> +	}
>> >
>> > This is still really confusing to me.  Can you consider the below?  The code is
>> > the same except for the reordering, but the explanation is reworked to be much
>> > clearer (IMO).  Anything I am misunderstanding?
>> >
>> > 	/*
>> > 	 * If the lookup is for creation, then a negative dentry can only be
>> > 	 * reused if it's a case-sensitive match, not just a case-insensitive
>> > 	 * one.  This is needed to make the new file be created with the name
>> > 	 * the user specified, preserving case.
>> > 	 *
>> > 	 * LOOKUP_CREATE or LOOKUP_RENAME_TARGET cover most creations.  In these
>> > 	 * cases, ->d_name is stable and can be compared to 'name' without
>> > 	 * taking ->d_lock because the caller holds dir->i_rwsem for write.
>> > 	 * (This is because the directory lock blocks the dentry from being
>> > 	 * concurrently instantiated, and negative dentries are never moved.)
>> > 	 *
>> > 	 * All other creations actually use flags==0.  These come from the edge
>> > 	 * case of filesystems calling functions like lookup_one() that do a
>> > 	 * lookup without setting the lookup flags at all.  Such lookups might
>> > 	 * or might not be for creation, and if not don't guarantee stable
>> > 	 * ->d_name.  Therefore, invalidate all negative dentries when flags==0.
>> > 	 */
>> > 	if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) {
>> > 		if (dentry->d_name.len != name->len ||
>> > 		    memcmp(dentry->d_name.name, name->name, name->len))
>> > 			return 0;
>> > 	}
>> > 	if (!flags)
>> > 		return 0;
>> 
>> I don't see it as particularly better or less confusing than the
>> original. but I also don't mind taking it into the next iteration.
>> 
>
> Your commit message is still much longer and covers some quite different details
> which seem irrelevant to me.  So if you don't see my explanation as being much
> different, I think we're still not on the same page.  I hope that I'm not
> misunderstanding anything, in which I believe that what I wrote above is a good
> explanation and your commit message should be substantially simplified.
> Remember, longer != better.  Keep things as simple as possible.

I think we are on the same page regarding the code.  I was under the
impression your suggestion was regarding the *code comment* you proposed
to replace, and not the commit message.  I don't see your code comment
to be much different than the original.

The commit message has information accumulated on previous discussions,
including the conclusions from the locking discussion Viro requested.
I'll reword it too for the next iteration to see if I can make it more
concise.

Thx

-- 
Gabriel Krisman Bertazi


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding

[Gabriel Krisman Bertazi]

Eric Biggers <ebiggers@kernel.org> writes:

> On Mon, Aug 14, 2023 at 11:02:38AM -0400, Gabriel Krisman Bertazi wrote:
>> 
>> Also, this patchset has been sitting for years before the latest
>> discussions, and I'm tired of it, so I'm happy to keep this
>> discussion for another time.  Will drop this patch and just check
>> IS_CASEFOLDED in ecryptfs for the next iteration.
>> 
>> I'll follow up with another case-insensitive cleanup patchset I've been
>> siting on forever, which includes this patch and will restart this
>> discussion, among others.
>> 
>
> Well, as we know unfortunately filesystem developers are in short supply, and
> doing proper reviews (identifying issues and working closely with the patchset
> author over multiple iterations to address them, as opposed to just slapping on
> a Reviewed-by) is very time consuming.  Earlier this year I tried to get the
> Android Systems team, which is ostensibly responsible for Android's use of
> casefolding, to take a look, but their entire feedback was just "looks good to
> me".  Also, the fact that this patchset originally excluded the casefold+encrypt
> case technically made it not applicable to Android, and discouraged me from
> taking a look since encryption is my focus.  Sorry for not taking a look sooner.
>
> Anyway, thanks for doing this, and I think it's near the finish line now.  Once

On the contrary, thank *you* for your review. I always appreciate your
feedback, particularly since you are always able to find the corner
cases I missed. That, and your responsiveness, are the reasons I always
put you in my --cc list since v1 for anything related to unicode/fs :)



-- 
Gabriel Krisman Bertazi


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 06/10] libfs: Validate negative dentries in case-insensitive directories

[Eric Biggers]

On Mon, Aug 14, 2023 at 03:21:33PM -0400, Gabriel Krisman Bertazi wrote:
> Eric Biggers <ebiggers@kernel.org> writes:
> 
> > On Mon, Aug 14, 2023 at 10:50:13AM -0400, Gabriel Krisman Bertazi wrote:
> >> Eric Biggers <ebiggers@kernel.org> writes:
> >> 
> >> > On Fri, Aug 11, 2023 at 08:41:42PM -0400, Gabriel Krisman Bertazi wrote:
> >> >> +	/*
> >> >> +	 * Filesystems will call into d_revalidate without setting
> >> >> +	 * LOOKUP_ flags even for file creation (see lookup_one*
> >> >> +	 * variants).  Reject negative dentries in this case, since we
> >> >> +	 * can't know for sure it won't be used for creation.
> >> >> +	 */
> >> >> +	if (!flags)
> >> >> +		return 0;
> >> >> +
> >> >> +	/*
> >> >> +	 * If the lookup is for creation, then a negative dentry can
> >> >> +	 * only be reused if it's a case-sensitive match, not just a
> >> >> +	 * case-insensitive one.  This is needed to make the new file be
> >> >> +	 * created with the name the user specified, preserving case.
> >> >> +	 */
> >> >> +	if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) {
> >> >> +		/*
> >> >> +		 * ->d_name won't change from under us in the creation
> >> >> +		 * path only, since d_revalidate during creation and
> >> >> +		 * renames is always called with the parent inode
> >> >> +		 * locked.  It isn't the case for all lookup callpaths,
> >> >> +		 * so ->d_name must not be touched outside
> >> >> +		 * (LOOKUP_CREATE|LOOKUP_RENAME_TARGET) context.
> >> >> +		 */
> >> >> +		if (dentry->d_name.len != name->len ||
> >> >> +		    memcmp(dentry->d_name.name, name->name, name->len))
> >> >> +			return 0;
> >> >> +	}
> >> >
> >> > This is still really confusing to me.  Can you consider the below?  The code is
> >> > the same except for the reordering, but the explanation is reworked to be much
> >> > clearer (IMO).  Anything I am misunderstanding?
> >> >
> >> > 	/*
> >> > 	 * If the lookup is for creation, then a negative dentry can only be
> >> > 	 * reused if it's a case-sensitive match, not just a case-insensitive
> >> > 	 * one.  This is needed to make the new file be created with the name
> >> > 	 * the user specified, preserving case.
> >> > 	 *
> >> > 	 * LOOKUP_CREATE or LOOKUP_RENAME_TARGET cover most creations.  In these
> >> > 	 * cases, ->d_name is stable and can be compared to 'name' without
> >> > 	 * taking ->d_lock because the caller holds dir->i_rwsem for write.
> >> > 	 * (This is because the directory lock blocks the dentry from being
> >> > 	 * concurrently instantiated, and negative dentries are never moved.)
> >> > 	 *
> >> > 	 * All other creations actually use flags==0.  These come from the edge
> >> > 	 * case of filesystems calling functions like lookup_one() that do a
> >> > 	 * lookup without setting the lookup flags at all.  Such lookups might
> >> > 	 * or might not be for creation, and if not don't guarantee stable
> >> > 	 * ->d_name.  Therefore, invalidate all negative dentries when flags==0.
> >> > 	 */
> >> > 	if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) {
> >> > 		if (dentry->d_name.len != name->len ||
> >> > 		    memcmp(dentry->d_name.name, name->name, name->len))
> >> > 			return 0;
> >> > 	}
> >> > 	if (!flags)
> >> > 		return 0;
> >> 
> >> I don't see it as particularly better or less confusing than the
> >> original. but I also don't mind taking it into the next iteration.
> >> 
> >
> > Your commit message is still much longer and covers some quite different details
> > which seem irrelevant to me.  So if you don't see my explanation as being much
> > different, I think we're still not on the same page.  I hope that I'm not
> > misunderstanding anything, in which I believe that what I wrote above is a good
> > explanation and your commit message should be substantially simplified.
> > Remember, longer != better.  Keep things as simple as possible.
> 
> I think we are on the same page regarding the code.  I was under the
> impression your suggestion was regarding the *code comment* you proposed
> to replace, and not the commit message.  I don't see your code comment
> to be much different than the original.
> 
> The commit message has information accumulated on previous discussions,
> including the conclusions from the locking discussion Viro requested.
> I'll reword it too for the next iteration to see if I can make it more
> concise.
> 

Yes, I was talking about the code comment, but the commit message is explaining
the same thing so it needs to be consistent (or have the commit message just
reference the code).  As-is they seem to be in contradiction.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 01/10] fs: Expose helper to check if a directory needs casefolding

[Theodore Ts'o]

On Mon, Aug 14, 2023 at 10:22:44AM -0700, Eric Biggers wrote:
> 
> Keep in mind, the syzkaller team isn't asking for these pointless "fixes"
> either.  They'd very much prefer 1 fix to 1000 fixes.  I think some confusion
> might be arising from the very different types of problems that syzkaller finds.
> Sometimes 1 syzkaller report == 1 bug == 1 high-priority "must fix" bug == 1
> vulnerability == 1 fix needed.  But in general syzkaller is just letting kernel
> developers know about a problem, and it is up to them to decide what to do about
> it.  In this case there is one underlying issue that needs to be fixed, and the
> individual syzkaller reports that result from that issue are not important.

... except that the Syzkaller folks have created slide decks talking
about "Linux kernel security disaster", blaming the entire community,
where they quote the number unresolved syzkaller reports, without the
kind of nuance that you are referring to.

There is also not a great way of categorizing syzkaller reports as
"requires maliciously fuzzed file system image", or "writing to
mounted file system" --- either manually, or (ideally) automatically,
since the syzbot test generators knows what they are doing.

And finally, the reality is even if someone where to fix the "one
underlying issue", the reality is that it will be ten years or so
before said fixed can be rolled out, since it requires changes in
userspace utilities, as well as rolled out kernels, and enterprise
distros are around for a decade; even community distros need to be
supported for at least 3-5 years.

Finally, it's not just "one underlying issue"; there are also
"maliciously fuzzed file systems", and working around those syzbot
reports can be quite painful, especially the ones that lead to lockdep
deadlock reports.  Many of these are spurious, caused by an inode
being used in two contexts, that can only happen in a badly corrupted
file system, and for which we've already signalled that the file
system is corrupted, so if you panic on error, it wouldn't deadlock.
(And if you deadlock, it's not _that_ much worse than panicing on a
maliciously fuzzed file system.)  And all of these bugs get counted,
one for each lockdep report variation (so there can be 3-4 per root
cause) as a "security bug" in the "Linux kernel security disaster"
statistics.

I might not mind the hyperbole if said slide decks asked for more
headcount.  But instead, they blame the "Linux upstream community" for
not fixing bugs, or treating the bugs seriously.   Sigh....

  	    	    	       	      	       - Ted


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 04/10] fs: Expose name under lookup to d_revalidate hooks

[kernel test robot]

Hi Gabriel,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tytso-ext4/dev]
[also build test WARNING on linus/master]
[cannot apply to tyhicks-ecryptfs/next ericvh-v9fs/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Gabriel-Krisman-Bertazi/fs-Expose-helper-to-check-if-a-directory-needs-casefolding/20230812-084506
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
patch link:    https://lore.kernel.org/r/20230812004146.30980-5-krisman%40suse.de
patch subject: [PATCH v5 04/10] fs: Expose name under lookup to d_revalidate hooks
config: mips-randconfig-r002-20230817 (https://download.01.org/0day-ci/archive/20230817/202308171453.3HpCqtib-lkp@intel.com/config)
compiler: mipsel-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230817/202308171453.3HpCqtib-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308171453.3HpCqtib-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> fs/ecryptfs/dentry.c:33: warning: Function parameter or member 'name' not described in 'ecryptfs_d_revalidate'


vim +33 fs/ecryptfs/dentry.c

237fead619984c Michael Halcrow         2006-10-04  17  
237fead619984c Michael Halcrow         2006-10-04  18  /**
237fead619984c Michael Halcrow         2006-10-04  19   * ecryptfs_d_revalidate - revalidate an ecryptfs dentry
237fead619984c Michael Halcrow         2006-10-04  20   * @dentry: The ecryptfs dentry
0b728e1911cbe6 Al Viro                 2012-06-10  21   * @flags: lookup flags
237fead619984c Michael Halcrow         2006-10-04  22   *
237fead619984c Michael Halcrow         2006-10-04  23   * Called when the VFS needs to revalidate a dentry. This
237fead619984c Michael Halcrow         2006-10-04  24   * is called whenever a name lookup finds a dentry in the
237fead619984c Michael Halcrow         2006-10-04  25   * dcache. Most filesystems leave this as NULL, because all their
237fead619984c Michael Halcrow         2006-10-04  26   * dentries in the dcache are valid.
237fead619984c Michael Halcrow         2006-10-04  27   *
237fead619984c Michael Halcrow         2006-10-04  28   * Returns 1 if valid, 0 otherwise.
237fead619984c Michael Halcrow         2006-10-04  29   *
237fead619984c Michael Halcrow         2006-10-04  30   */
0838ae103beaf6 Gabriel Krisman Bertazi 2023-08-11  31  static int ecryptfs_d_revalidate(struct dentry *dentry,
0838ae103beaf6 Gabriel Krisman Bertazi 2023-08-11  32  				 const struct qstr *name, unsigned int flags)
237fead619984c Michael Halcrow         2006-10-04 @33  {
2edbfbf1c1ab0a Al Viro                 2013-09-15  34  	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
5556e7e6d30e8e Tyler Hicks             2015-08-05  35  	int rc = 1;
237fead619984c Michael Halcrow         2006-10-04  36  
0b728e1911cbe6 Al Viro                 2012-06-10  37  	if (flags & LOOKUP_RCU)
34286d6662308d Nicholas Piggin         2011-01-07  38  		return -ECHILD;
34286d6662308d Nicholas Piggin         2011-01-07  39  
5556e7e6d30e8e Tyler Hicks             2015-08-05  40  	if (lower_dentry->d_flags & DCACHE_OP_REVALIDATE)
0838ae103beaf6 Gabriel Krisman Bertazi 2023-08-11  41  		rc = lower_dentry->d_op->d_revalidate(lower_dentry, name, flags);
5556e7e6d30e8e Tyler Hicks             2015-08-05  42  
2b0143b5c986be David Howells           2015-03-17  43  	if (d_really_is_positive(dentry)) {
5556e7e6d30e8e Tyler Hicks             2015-08-05  44  		struct inode *inode = d_inode(dentry);
ae56fb16337c88 Michael Halcrow         2006-11-16  45  
5556e7e6d30e8e Tyler Hicks             2015-08-05  46  		fsstack_copy_attr_all(inode, ecryptfs_inode_to_lower(inode));
5556e7e6d30e8e Tyler Hicks             2015-08-05  47  		if (!inode->i_nlink)
5556e7e6d30e8e Tyler Hicks             2015-08-05  48  			return 0;
ae56fb16337c88 Michael Halcrow         2006-11-16  49  	}
237fead619984c Michael Halcrow         2006-10-04  50  	return rc;
237fead619984c Michael Halcrow         2006-10-04  51  }
237fead619984c Michael Halcrow         2006-10-04  52  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 04/10] fs: Expose name under lookup to d_revalidate hooks

[kernel test robot]

Hi Gabriel,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tytso-ext4/dev]
[also build test WARNING on linus/master]
[cannot apply to tyhicks-ecryptfs/next ericvh-v9fs/for-next viro-vfs/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Gabriel-Krisman-Bertazi/fs-Expose-helper-to-check-if-a-directory-needs-casefolding/20230812-084506
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
patch link:    https://lore.kernel.org/r/20230812004146.30980-5-krisman%40suse.de
patch subject: [PATCH v5 04/10] fs: Expose name under lookup to d_revalidate hooks
config: x86_64-randconfig-x012-20230817 (https://download.01.org/0day-ci/archive/20230817/202308171740.0u9DuWtr-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce: (https://download.01.org/0day-ci/archive/20230817/202308171740.0u9DuWtr-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308171740.0u9DuWtr-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> fs/ecryptfs/dentry.c:33: warning: Function parameter or member 'name' not described in 'ecryptfs_d_revalidate'


vim +33 fs/ecryptfs/dentry.c

237fead619984c Michael Halcrow         2006-10-04  17  
237fead619984c Michael Halcrow         2006-10-04  18  /**
237fead619984c Michael Halcrow         2006-10-04  19   * ecryptfs_d_revalidate - revalidate an ecryptfs dentry
237fead619984c Michael Halcrow         2006-10-04  20   * @dentry: The ecryptfs dentry
0b728e1911cbe6 Al Viro                 2012-06-10  21   * @flags: lookup flags
237fead619984c Michael Halcrow         2006-10-04  22   *
237fead619984c Michael Halcrow         2006-10-04  23   * Called when the VFS needs to revalidate a dentry. This
237fead619984c Michael Halcrow         2006-10-04  24   * is called whenever a name lookup finds a dentry in the
237fead619984c Michael Halcrow         2006-10-04  25   * dcache. Most filesystems leave this as NULL, because all their
237fead619984c Michael Halcrow         2006-10-04  26   * dentries in the dcache are valid.
237fead619984c Michael Halcrow         2006-10-04  27   *
237fead619984c Michael Halcrow         2006-10-04  28   * Returns 1 if valid, 0 otherwise.
237fead619984c Michael Halcrow         2006-10-04  29   *
237fead619984c Michael Halcrow         2006-10-04  30   */
0838ae103beaf6 Gabriel Krisman Bertazi 2023-08-11  31  static int ecryptfs_d_revalidate(struct dentry *dentry,
0838ae103beaf6 Gabriel Krisman Bertazi 2023-08-11  32  				 const struct qstr *name, unsigned int flags)
237fead619984c Michael Halcrow         2006-10-04 @33  {
2edbfbf1c1ab0a Al Viro                 2013-09-15  34  	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
5556e7e6d30e8e Tyler Hicks             2015-08-05  35  	int rc = 1;
237fead619984c Michael Halcrow         2006-10-04  36  
0b728e1911cbe6 Al Viro                 2012-06-10  37  	if (flags & LOOKUP_RCU)
34286d6662308d Nicholas Piggin         2011-01-07  38  		return -ECHILD;
34286d6662308d Nicholas Piggin         2011-01-07  39  
5556e7e6d30e8e Tyler Hicks             2015-08-05  40  	if (lower_dentry->d_flags & DCACHE_OP_REVALIDATE)
0838ae103beaf6 Gabriel Krisman Bertazi 2023-08-11  41  		rc = lower_dentry->d_op->d_revalidate(lower_dentry, name, flags);
5556e7e6d30e8e Tyler Hicks             2015-08-05  42  
2b0143b5c986be David Howells           2015-03-17  43  	if (d_really_is_positive(dentry)) {
5556e7e6d30e8e Tyler Hicks             2015-08-05  44  		struct inode *inode = d_inode(dentry);
ae56fb16337c88 Michael Halcrow         2006-11-16  45  
5556e7e6d30e8e Tyler Hicks             2015-08-05  46  		fsstack_copy_attr_all(inode, ecryptfs_inode_to_lower(inode));
5556e7e6d30e8e Tyler Hicks             2015-08-05  47  		if (!inode->i_nlink)
5556e7e6d30e8e Tyler Hicks             2015-08-05  48  			return 0;
ae56fb16337c88 Michael Halcrow         2006-11-16  49  	}
237fead619984c Michael Halcrow         2006-10-04  50  	return rc;
237fead619984c Michael Halcrow         2006-10-04  51  }
237fead619984c Michael Halcrow         2006-10-04  52  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel