Re: [PATCH 2/2] pidfd: add pidfdfs

[Jiri Slaby <jirislaby@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3106 bytes --]

On 17. 05. 24, 22:07, Linus Torvalds wrote:
> On Fri, 17 May 2024 at 00:54, Jiri Slaby <jirislaby@kernel.org> wrote:
>>
>>           inode->i_private = data;
>>           inode->i_flags |= S_PRIVATE;
>> +       inode->i_mode &= ~S_IFREG;
> 
> That is not a sensible operation. S_IFREG isn't a bit mask.

Oh, sure. I just unmasked what libfs' prepare_anon_dentry() set by default.

> But it looks like 'anon_inode' traditionally had *no* type bytes at
> all. That's literally crazy.
> 
> Doing a 'stat -L' on one in /proc/X/fd/Y will correctly say "weird
> file" about them.
> 
> What a crock. That's horrible, and we apparently never noticed how
> broken anon_inodes were because nobody really cared. But then lsof
> seems to have done the *opposite* and just said (for unfathomable
> reasons) "this can't be a normal regular file".
> 
> But I can't actually find that code in lsof. I see
> 
>                   if (rest && rest[0] == '[' && rest[1] == 'p')
>                       fdinfo_mask |= FDINFO_PID;
> 
> which only checks that the name starts with '[p'. Hmm.

lsof just has received a fix in a form of:
    else if (Lf->ntype == N_REGLR && rest && *rest && strcmp(pbuf, 
"pidfd") == 0) {

https://github.com/lsof-org/lsof/pull/319/commits/c1678e3f6e4b4d984cb3078b7bf0c9e24bedb8ca

> [ Time passes, I go looking ]
> 
> Oh Christ. It's process_proc_node:

Yes, didn't I note it? Hmm, apparently not (or maybe it's hidden in all 
those pulls/isuses/bugs). But definitely been there, seen that. Sorry.

>          type = s->st_mode & S_IFMT;
>          switch (type) {
>          ...
>          case 0:
>              if (!strcmp(p, "anon_inode"))
>                  Lf->ntype = Ntype = N_ANON_INODE;
>              break;
> 
> so yes, process_proc_node() really seems to have intentionally noticed
> that our anon inodes forgot to put a file type in the st_mode, and
> together with the path from readlink matching 'anon_inode' is how lsof
> determines it's one of the special inodes.
> 
> So yeah, we made a mistake, and then lsof decided that mistake was a feature.

Yes, but we can schedule a removal of this compat handling after some 
years...

> But that does mean that we probably just have to live in the bed we made.
> 
> But that
> 
>> +       inode->i_mode &= ~S_IFREG;
> 
> is still very very wrong. It should use the proper bit mask: S_IFMT.

Either, I don't like removing that WARN_ON_ONCE() from libfs' 
prepare_anon_dentry(). Is it OK to remove this S_IFREG after 
path_from_stashed() in pidfs' pidfs_alloc_file(). I.e. after 
d_alloc_anon(), d_instantiate(), stash_dentry(), but before dentry_open()?

That looks weird.

Instead, add a sort of LEGACY_DONT_WARN_ABOUT_IFMT to path_from_stashed()?

Dirty, I think.

So what about LEGACY_NO_MODE which would set "i_mode = 0" and mangle the 
WARN_ON appropriately. Like in the patch attached? It works (when 
applied together with the anon_inode name fix).

> And we'd have to add a big comment about our historical stupidity that
> we are perpetuating.

And immediately add it to Documentation/ABI/obsolete/?

thanks,
-- 
js
suse labs

[-- Attachment #2: 0001-add-LEGACY_NO_MODE.patch --]
[-- Type: text/x-patch, Size: 4723 bytes --]

From b005cd96c97684adfabf07c56bd91fabe45c8cb7 Mon Sep 17 00:00:00 2001
From: "Jiri Slaby (SUSE)" <jirislaby@kernel.org>
Date: Mon, 20 May 2024 10:13:44 +0200
Subject: [PATCH] add LEGACY_NO_MODE

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 Documentation/ABI/obsolete/libfs-LEGACY_NO_MODE |  8 ++++++++
 fs/internal.h                                   |  5 ++++-
 fs/libfs.c                                      | 11 ++++++-----
 fs/nsfs.c                                       |  4 ++--
 fs/pidfs.c                                      |  3 ++-
 5 files changed, 22 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/ABI/obsolete/libfs-LEGACY_NO_MODE

diff --git a/Documentation/ABI/obsolete/libfs-LEGACY_NO_MODE b/Documentation/ABI/obsolete/libfs-LEGACY_NO_MODE
new file mode 100644
index 000000000000..37ad036b18b2
--- /dev/null
+++ b/Documentation/ABI/obsolete/libfs-LEGACY_NO_MODE
@@ -0,0 +1,8 @@
+What:		libfs' LEGACY_NO_MODE
+Date:		May 2024
+KernelVersion:	6.10
+Contact:	linux-fsdevel@vger.kernel.org
+Description:	LEGACY_NO_MODE was added to mimic the old (wrong) i_mode (zero)
+		of anon_inode when pidfs was moved away from anon_inode to
+		libfs helpers.
+Users:		lsof
diff --git a/fs/internal.h b/fs/internal.h
index 7ca738904e34..a7cd0eecc266 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -315,6 +315,9 @@ struct stashed_operations {
 	void (*put_data)(void *data);
 	int (*init_inode)(struct inode *inode, void *data);
 };
+
+#define LEGACY_NO_MODE		BIT(0)
+
 int path_from_stashed(struct dentry **stashed, struct vfsmount *mnt, void *data,
-		      struct path *path);
+		      struct path *path, unsigned int flags);
 void stashed_dentry_prune(struct dentry *dentry);
diff --git a/fs/libfs.c b/fs/libfs.c
index b635ee5adbcc..c047aa4f4dac 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -2045,11 +2045,12 @@ static inline struct dentry *get_stashed_dentry(struct dentry *stashed)
 
 static struct dentry *prepare_anon_dentry(struct dentry **stashed,
 					  struct super_block *sb,
-					  void *data)
+					  void *data, unsigned int flags)
 {
 	struct dentry *dentry;
 	struct inode *inode;
 	const struct stashed_operations *sops = sb->s_fs_info;
+	umode_t i_mode;
 	int ret;
 
 	inode = new_inode_pseudo(sb);
@@ -2059,7 +2060,7 @@ static struct dentry *prepare_anon_dentry(struct dentry **stashed,
 	}
 
 	inode->i_flags |= S_IMMUTABLE;
-	inode->i_mode = S_IFREG;
+	inode->i_mode = i_mode = (flags & LEGACY_NO_MODE) ? 0 : S_IFREG;
 	simple_inode_init_ts(inode);
 
 	ret = sops->init_inode(inode, data);
@@ -2069,7 +2070,7 @@ static struct dentry *prepare_anon_dentry(struct dentry **stashed,
 	}
 
 	/* Notice when this is changed. */
-	WARN_ON_ONCE(!S_ISREG(inode->i_mode));
+	WARN_ON_ONCE((inode->i_mode & S_IFMT) != i_mode);
 	WARN_ON_ONCE(!IS_IMMUTABLE(inode));
 
 	dentry = d_alloc_anon(sb);
@@ -2126,7 +2127,7 @@ static struct dentry *stash_dentry(struct dentry **stashed,
  * Return: On success zero and on failure a negative error is returned.
  */
 int path_from_stashed(struct dentry **stashed, struct vfsmount *mnt, void *data,
-		      struct path *path)
+		      struct path *path, unsigned int flags)
 {
 	struct dentry *dentry;
 	const struct stashed_operations *sops = mnt->mnt_sb->s_fs_info;
@@ -2139,7 +2140,7 @@ int path_from_stashed(struct dentry **stashed, struct vfsmount *mnt, void *data,
 	}
 
 	/* Allocate a new dentry. */
-	dentry = prepare_anon_dentry(stashed, mnt->mnt_sb, data);
+	dentry = prepare_anon_dentry(stashed, mnt->mnt_sb, data, flags);
 	if (IS_ERR(dentry))
 		return PTR_ERR(dentry);
 
diff --git a/fs/nsfs.c b/fs/nsfs.c
index 07e22a15ef02..11b8ef2aeaed 100644
--- a/fs/nsfs.c
+++ b/fs/nsfs.c
@@ -56,7 +56,7 @@ int ns_get_path_cb(struct path *path, ns_get_path_helper_t *ns_get_cb,
 	if (!ns)
 		return -ENOENT;
 
-	return path_from_stashed(&ns->stashed, nsfs_mnt, ns, path);
+	return path_from_stashed(&ns->stashed, nsfs_mnt, ns, path, 0);
 }
 
 struct ns_get_path_task_args {
@@ -101,7 +101,7 @@ int open_related_ns(struct ns_common *ns,
 		return PTR_ERR(relative);
 	}
 
-	err = path_from_stashed(&relative->stashed, nsfs_mnt, relative, &path);
+	err = path_from_stashed(&relative->stashed, nsfs_mnt, relative, &path, 0);
 	if (err < 0) {
 		put_unused_fd(fd);
 		return err;
diff --git a/fs/pidfs.c b/fs/pidfs.c
index a63d5d24aa02..cb894e9024ed 100644
--- a/fs/pidfs.c
+++ b/fs/pidfs.c
@@ -266,7 +266,8 @@ struct file *pidfs_alloc_file(struct pid *pid, unsigned int flags)
 	struct path path;
 	int ret;
 
-	ret = path_from_stashed(&pid->stashed, pidfs_mnt, get_pid(pid), &path);
+	ret = path_from_stashed(&pid->stashed, pidfs_mnt, get_pid(pid), &path,
+				LEGACY_NO_MODE);
 	if (ret < 0)
 		return ERR_PTR(ret);
 
-- 
2.45.1