[WTF?] AT_GETATTR_NOSEC checks

Commit Message

Al Viro Nov. 1, 2024, 1:17 a.m. UTC

AFAICS, since the moment it had been introduced it got passed
to *ALL* ->getattr() calls.  Unconditionally.  So why are we checking
it in ecryptfs and overlayfs instances?

Look: all direct calls of instances are from other instances, with
query_flags passed unchanged.  There is only one call via method -
that in vfs_getattr_nosec(), but that caller explicitly adds
AT_GETATTR_NOSEC to query_flags.

So what the hell are the checks in ecryptfs and overlayfs for?
What am I missing here?  What would break if we did the following:

Comments

Christian Brauner Nov. 6, 2024, 10:27 a.m. UTC | #1

On Fri, Nov 01, 2024 at 01:17:24AM +0000, Al Viro wrote:
> 	AFAICS, since the moment it had been introduced it got passed
> to *ALL* ->getattr() calls.  Unconditionally.  So why are we checking
> it in ecryptfs and overlayfs instances?
> 
> Look: all direct calls of instances are from other instances, with
> query_flags passed unchanged.  There is only one call via method -
> that in vfs_getattr_nosec(), but that caller explicitly adds
> AT_GETATTR_NOSEC to query_flags.
> 
> So what the hell are the checks in ecryptfs and overlayfs for?
> What am I missing here?  What would break if we did the following:

Sounds good. I'm confused why that's a WTF moment though.

Anyway, I'm just slowly getting back to the fray. I caught atypical
pneumonia and that has been going on for a few weeks now. So my replies
are delayed.

Patch

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 5ed1e4cf6c0b..255e60bd7dca 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -1008,14 +1008,6 @@  static int ecryptfs_getattr_link(struct mnt_idmap *idmap,
 	return rc;
 }
 
-static int ecryptfs_do_getattr(const struct path *path, struct kstat *stat,
-			       u32 request_mask, unsigned int flags)
-{
-	if (flags & AT_GETATTR_NOSEC)
-		return vfs_getattr_nosec(path, stat, request_mask, flags);
-	return vfs_getattr(path, stat, request_mask, flags);
-}
-
 static int ecryptfs_getattr(struct mnt_idmap *idmap,
 			    const struct path *path, struct kstat *stat,
 			    u32 request_mask, unsigned int flags)
@@ -1024,7 +1016,7 @@  static int ecryptfs_getattr(struct mnt_idmap *idmap,
 	struct kstat lower_stat;
 	int rc;
 
-	rc = ecryptfs_do_getattr(ecryptfs_dentry_to_lower_path(dentry),
+	rc = vfs_getattr_nosec(ecryptfs_dentry_to_lower_path(dentry),
 				 &lower_stat, request_mask, flags);
 	if (!rc) {
 		fsstack_copy_attr_all(d_inode(dentry),
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 0bfe35da4b7b..dde85ec96444 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -415,9 +415,8 @@  static inline bool ovl_open_flags_need_copy_up(int flags)
 static inline int ovl_do_getattr(const struct path *path, struct kstat *stat,
 				 u32 request_mask, unsigned int flags)
 {
-	if (flags & AT_GETATTR_NOSEC)
-		return vfs_getattr_nosec(path, stat, request_mask, flags);
-	return vfs_getattr(path, stat, request_mask, flags);
+	BUG_ON(!(flags & AT_GETATTR_NOSEC));
+	return vfs_getattr_nosec(path, stat, request_mask, flags);
 }
 
 /* util.c */