[PATCH] proc: allow to mark /proc files permanent outside of fs/proc/

[PATCH] proc: allow to mark /proc files permanent outside of fs/proc/

[Alexey Dobriyan]

From 06e2ff406942fef65b9c397a7f44478dd4b61451 Mon Sep 17 00:00:00 2001
From: Alexey Dobriyan <adobriyan@gmail.com>
Date: Sat, 5 Apr 2025 14:50:10 +0300
Subject: [PATCH 1/1] proc: allow to mark /proc files permanent outside of
 fs/proc/

From: Mateusz Guzik <mjguzik@gmail.com>

Add proc_make_permanent() function to mark PDE as permanent to speed up
open/read/close (one alloc/free and lock/unlock less).

Enable it for built-in code and for compiled-in modules.
This function becomes nop magically in modular code.

Use it on /proc/filesystems to add a user.

		Note, note, note!

If built-in code creates and deletes PDEs dynamically (not in init
hook), then proc_make_permanent() must not be used.

It is intended for simple code:

	static int __init xxx_module_init(void)
	{
		g_pde = proc_create_single();
		proc_make_permanent(g_pde);
		return 0;
	}
	static void __exit xxx_module_exit(void)
	{
		remove_proc_entry(g_pde);
	}

If module is built-in then exit hook never executed and PDE is
permanent so it is OK to mark it as such.

If module is module then rmmod will yank PDE, but proc_make_permanent()
is nop and core /proc code will do everything right.

[adobriyan@gmail.com: unexport function (usual exporting is a bug)]
[adobriyan@gmail.com: rewrite changelog]

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
 fs/filesystems.c        |  4 +++-
 fs/proc/generic.c       | 12 ++++++++++++
 fs/proc/internal.h      |  3 +++
 include/linux/proc_fs.h | 10 ++++++++++
 4 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/fs/filesystems.c b/fs/filesystems.c
index 58b9067b2391..81dcd0ddadb6 100644
--- a/fs/filesystems.c
+++ b/fs/filesystems.c
@@ -252,7 +252,9 @@ static int filesystems_proc_show(struct seq_file *m, void *v)
 
 static int __init proc_filesystems_init(void)
 {
-	proc_create_single("filesystems", 0, NULL, filesystems_proc_show);
+	struct proc_dir_entry *pde =
+		proc_create_single("filesystems", 0, NULL, filesystems_proc_show);
+	proc_make_permanent(pde);
 	return 0;
 }
 module_init(proc_filesystems_init);
diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index a3e22803cddf..0342600c0172 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -826,3 +826,15 @@ ssize_t proc_simple_write(struct file *f, const char __user *ubuf, size_t size,
 	kfree(buf);
 	return ret == 0 ? size : ret;
 }
+
+/*
+ * Not exported to modules:
+ * modules' /proc files aren't permanent because modules aren't permanent.
+ */
+void impl_proc_make_permanent(struct proc_dir_entry *pde);
+void impl_proc_make_permanent(struct proc_dir_entry *pde)
+{
+	if (pde) {
+		pde_make_permanent(pde);
+	}
+}
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 96122e91c645..885b1cd38020 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -80,8 +80,11 @@ static inline bool pde_is_permanent(const struct proc_dir_entry *pde)
 	return pde->flags & PROC_ENTRY_PERMANENT;
 }
 
+/* This is for builtin code, not even for modules which are compiled in. */
 static inline void pde_make_permanent(struct proc_dir_entry *pde)
 {
+	/* Ensure magic flag does something. */
+	static_assert(PROC_ENTRY_PERMANENT != 0);
 	pde->flags |= PROC_ENTRY_PERMANENT;
 }
 
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index ea62201c74c4..2d59f29b49eb 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -247,4 +247,14 @@ static inline struct pid_namespace *proc_pid_ns(struct super_block *sb)
 
 bool proc_ns_file(const struct file *file);
 
+static inline void proc_make_permanent(struct proc_dir_entry *pde)
+{
+	/* Don't give matches to modules. */
+#if defined CONFIG_PROC_FS && !defined MODULE
+	/* This mess is created by defining "struct proc_dir_entry" elsewhere. */
+	void impl_proc_make_permanent(struct proc_dir_entry *pde);
+	impl_proc_make_permanent(pde);
+#endif
+}
+
 #endif /* _LINUX_PROC_FS_H */
-- 
2.47.0

Re: [PATCH] proc: allow to mark /proc files permanent outside of fs/proc/

[Mateusz Guzik]

On Wed, Apr 9, 2025 at 9:20 PM Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> From 06e2ff406942fef65b9c397a7f44478dd4b61451 Mon Sep 17 00:00:00 2001
> From: Alexey Dobriyan <adobriyan@gmail.com>
> Date: Sat, 5 Apr 2025 14:50:10 +0300
> Subject: [PATCH 1/1] proc: allow to mark /proc files permanent outside of
>  fs/proc/
>
> From: Mateusz Guzik <mjguzik@gmail.com>
>
> Add proc_make_permanent() function to mark PDE as permanent to speed up
> open/read/close (one alloc/free and lock/unlock less).
>
> Enable it for built-in code and for compiled-in modules.
> This function becomes nop magically in modular code.
>
> Use it on /proc/filesystems to add a user.
>
>                 Note, note, note!
>
> If built-in code creates and deletes PDEs dynamically (not in init
> hook), then proc_make_permanent() must not be used.
>
> It is intended for simple code:
>
>         static int __init xxx_module_init(void)
>         {
>                 g_pde = proc_create_single();
>                 proc_make_permanent(g_pde);
>                 return 0;
>         }
>         static void __exit xxx_module_exit(void)
>         {
>                 remove_proc_entry(g_pde);
>         }
>
> If module is built-in then exit hook never executed and PDE is
> permanent so it is OK to mark it as such.
>
> If module is module then rmmod will yank PDE, but proc_make_permanent()
> is nop and core /proc code will do everything right.
>
> [adobriyan@gmail.com: unexport function (usual exporting is a bug)]
> [adobriyan@gmail.com: rewrite changelog]
>
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
>  fs/filesystems.c        |  4 +++-
>  fs/proc/generic.c       | 12 ++++++++++++
>  fs/proc/internal.h      |  3 +++
>  include/linux/proc_fs.h | 10 ++++++++++
>  4 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/fs/filesystems.c b/fs/filesystems.c
> index 58b9067b2391..81dcd0ddadb6 100644
> --- a/fs/filesystems.c
> +++ b/fs/filesystems.c
> @@ -252,7 +252,9 @@ static int filesystems_proc_show(struct seq_file *m, void *v)
>
>  static int __init proc_filesystems_init(void)
>  {
> -       proc_create_single("filesystems", 0, NULL, filesystems_proc_show);
> +       struct proc_dir_entry *pde =
> +               proc_create_single("filesystems", 0, NULL, filesystems_proc_show);
> +       proc_make_permanent(pde);
>         return 0;
>  }
>  module_init(proc_filesystems_init);
> diff --git a/fs/proc/generic.c b/fs/proc/generic.c
> index a3e22803cddf..0342600c0172 100644
> --- a/fs/proc/generic.c
> +++ b/fs/proc/generic.c
> @@ -826,3 +826,15 @@ ssize_t proc_simple_write(struct file *f, const char __user *ubuf, size_t size,
>         kfree(buf);
>         return ret == 0 ? size : ret;
>  }
> +
> +/*
> + * Not exported to modules:
> + * modules' /proc files aren't permanent because modules aren't permanent.
> + */
> +void impl_proc_make_permanent(struct proc_dir_entry *pde);
> +void impl_proc_make_permanent(struct proc_dir_entry *pde)
> +{
> +       if (pde) {
> +               pde_make_permanent(pde);
> +       }
> +}
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index 96122e91c645..885b1cd38020 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -80,8 +80,11 @@ static inline bool pde_is_permanent(const struct proc_dir_entry *pde)
>         return pde->flags & PROC_ENTRY_PERMANENT;
>  }
>
> +/* This is for builtin code, not even for modules which are compiled in. */
>  static inline void pde_make_permanent(struct proc_dir_entry *pde)
>  {
> +       /* Ensure magic flag does something. */
> +       static_assert(PROC_ENTRY_PERMANENT != 0);
>         pde->flags |= PROC_ENTRY_PERMANENT;
>  }
>
> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> index ea62201c74c4..2d59f29b49eb 100644
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -247,4 +247,14 @@ static inline struct pid_namespace *proc_pid_ns(struct super_block *sb)
>
>  bool proc_ns_file(const struct file *file);
>
> +static inline void proc_make_permanent(struct proc_dir_entry *pde)
> +{
> +       /* Don't give matches to modules. */
> +#if defined CONFIG_PROC_FS && !defined MODULE
> +       /* This mess is created by defining "struct proc_dir_entry" elsewhere. */
> +       void impl_proc_make_permanent(struct proc_dir_entry *pde);
> +       impl_proc_make_permanent(pde);
> +#endif
> +}
> +
>  #endif /* _LINUX_PROC_FS_H */

This diff should not be changing /proc/filesystems, that's for the
other patch to do.

So I think this patch is all you and my name needs to be dropped from
it, along with marking /proc/filesystems as permanent.

Given that you kept the name (proc_make_permanent), the
fs/filesystems.c side of things is identical so my first patch can be
just swapped for this one.

Alternatively I can resend the patchset with this (augmented as
described above).
-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH] proc: allow to mark /proc files permanent outside of fs/proc/

[Andrew Morton]

On Wed, 9 Apr 2025 22:20:13 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> >From 06e2ff406942fef65b9c397a7f44478dd4b61451 Mon Sep 17 00:00:00 2001
> From: Alexey Dobriyan <adobriyan@gmail.com>
> Date: Sat, 5 Apr 2025 14:50:10 +0300
> Subject: [PATCH 1/1] proc: allow to mark /proc files permanent outside of
>  fs/proc/
> 
> From: Mateusz Guzik <mjguzik@gmail.com>
> 
> Add proc_make_permanent() function to mark PDE as permanent to speed up
> open/read/close (one alloc/free and lock/unlock less).

When proposing a speedup it is preferable to provide some benchmarking
results to help others understand the magnitude of that speedup.

> ...
>
> index 58b9067b2391..81dcd0ddadb6 100644
> --- a/fs/filesystems.c
> +++ b/fs/filesystems.c
> @@ -252,7 +252,9 @@ static int filesystems_proc_show(struct seq_file *m, void *v)
>  
>  static int __init proc_filesystems_init(void)
>  {
> -	proc_create_single("filesystems", 0, NULL, filesystems_proc_show);
> +	struct proc_dir_entry *pde =
> +		proc_create_single("filesystems", 0, NULL, filesystems_proc_show);

To avoid the 80-column nasties, this is more pleasing:

	struct proc_dir_entry *pde;

	pde = proc_create_single("filesystems", 0, NULL, filesystems_proc_show);


> +	proc_make_permanent(pde);
>  	return 0;
>  }
>  module_init(proc_filesystems_init);
> diff --git a/fs/proc/generic.c b/fs/proc/generic.c
> index a3e22803cddf..0342600c0172 100644
> --- a/fs/proc/generic.c
> +++ b/fs/proc/generic.c
> @@ -826,3 +826,15 @@ ssize_t proc_simple_write(struct file *f, const char __user *ubuf, size_t size,
>  	kfree(buf);
>  	return ret == 0 ? size : ret;
>  }
> +
> +/*
> + * Not exported to modules:
> + * modules' /proc files aren't permanent because modules aren't permanent.
> + */
> +void impl_proc_make_permanent(struct proc_dir_entry *pde);

This declaration is unneeded, isn't it?

> +void impl_proc_make_permanent(struct proc_dir_entry *pde)
> +{
> +	if (pde) {
> +		pde_make_permanent(pde);
> +	}

Please let's be running checkpatch more often?

> +}
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index 96122e91c645..885b1cd38020 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -80,8 +80,11 @@ static inline bool pde_is_permanent(const struct proc_dir_entry *pde)
>  	return pde->flags & PROC_ENTRY_PERMANENT;
>  }
>  
> +/* This is for builtin code, not even for modules which are compiled in. */
>  static inline void pde_make_permanent(struct proc_dir_entry *pde)
>  {
> +	/* Ensure magic flag does something. */
> +	static_assert(PROC_ENTRY_PERMANENT != 0);

Looks odd.  What is this doing?  The comment does a poor job of
explaining this!

>  	pde->flags |= PROC_ENTRY_PERMANENT;
>  }
>  
> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> index ea62201c74c4..2d59f29b49eb 100644
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -247,4 +247,14 @@ static inline struct pid_namespace *proc_pid_ns(struct super_block *sb)
>  
>  bool proc_ns_file(const struct file *file);
>  
> +static inline void proc_make_permanent(struct proc_dir_entry *pde)
> +{
> +	/* Don't give matches to modules. */

This comment is also mysterious (to me).  Please expand upon it.

> +#if defined CONFIG_PROC_FS && !defined MODULE
> +	/* This mess is created by defining "struct proc_dir_entry" elsewhere. */

Also mysterious.

> +	void impl_proc_make_permanent(struct proc_dir_entry *pde);

Forward-declaring a function within a function in this manner is quite
unusual.  Let's be conventional, please.

> +	impl_proc_make_permanent(pde);
> +#endif
> +}
> +
>  #endif /* _LINUX_PROC_FS_H */
> -- 
> 2.47.0

Re: [PATCH] proc: allow to mark /proc files permanent outside of fs/proc/

[Mateusz Guzik]

On Wed, Apr 9, 2025 at 11:35 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 9 Apr 2025 22:20:13 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> > >From 06e2ff406942fef65b9c397a7f44478dd4b61451 Mon Sep 17 00:00:00 2001
> > From: Alexey Dobriyan <adobriyan@gmail.com>
> > Date: Sat, 5 Apr 2025 14:50:10 +0300
> > Subject: [PATCH 1/1] proc: allow to mark /proc files permanent outside of
> >  fs/proc/
> >
> > From: Mateusz Guzik <mjguzik@gmail.com>
> >
> > Add proc_make_permanent() function to mark PDE as permanent to speed up
> > open/read/close (one alloc/free and lock/unlock less).
>
> When proposing a speedup it is preferable to provide some benchmarking
> results to help others understand the magnitude of that speedup.
>

It's all in the original submission:
https://lore.kernel.org/linux-fsdevel/20250329192821.822253-3-mjguzik@gmail.com/

-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH] proc: allow to mark /proc files permanent outside of fs/proc/

[Alexey Dobriyan]

On Wed, Apr 09, 2025 at 02:35:46PM -0700, Andrew Morton wrote:
> On Wed, 9 Apr 2025 22:20:13 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:
> 
> > >From 06e2ff406942fef65b9c397a7f44478dd4b61451 Mon Sep 17 00:00:00 2001
> > From: Alexey Dobriyan <adobriyan@gmail.com>
> > Date: Sat, 5 Apr 2025 14:50:10 +0300
> > Subject: [PATCH 1/1] proc: allow to mark /proc files permanent outside of
> >  fs/proc/
> > 
> > From: Mateusz Guzik <mjguzik@gmail.com>
> > 
> > Add proc_make_permanent() function to mark PDE as permanent to speed up
> > open/read/close (one alloc/free and lock/unlock less).
> 
> When proposing a speedup it is preferable to provide some benchmarking
> results to help others understand the magnitude of that speedup.
> 
> > ...
> >
> > index 58b9067b2391..81dcd0ddadb6 100644
> > --- a/fs/filesystems.c
> > +++ b/fs/filesystems.c
> > @@ -252,7 +252,9 @@ static int filesystems_proc_show(struct seq_file *m, void *v)
> >  
> >  static int __init proc_filesystems_init(void)
> >  {
> > -	proc_create_single("filesystems", 0, NULL, filesystems_proc_show);
> > +	struct proc_dir_entry *pde =
> > +		proc_create_single("filesystems", 0, NULL, filesystems_proc_show);
> 
> To avoid the 80-column nasties, this is more pleasing:

It is inferior style, see below. BTW, how the kernel is still on 80 columns?

> 	struct proc_dir_entry *pde;
> 
> 	pde = proc_create_single("filesystems", 0, NULL, filesystems_proc_show);
> 
> 
> > +	proc_make_permanent(pde);
> >  	return 0;
> >  }
> >  module_init(proc_filesystems_init);
> > diff --git a/fs/proc/generic.c b/fs/proc/generic.c
> > index a3e22803cddf..0342600c0172 100644
> > --- a/fs/proc/generic.c
> > +++ b/fs/proc/generic.c
> > @@ -826,3 +826,15 @@ ssize_t proc_simple_write(struct file *f, const char __user *ubuf, size_t size,
> >  	kfree(buf);
> >  	return ret == 0 ? size : ret;
> >  }
> > +
> > +/*
> > + * Not exported to modules:
> > + * modules' /proc files aren't permanent because modules aren't permanent.
> > + */
> > +void impl_proc_make_permanent(struct proc_dir_entry *pde);
> 
> This declaration is unneeded, isn't it?

It is necessary, but I need to make a comment, yes.

> > +void impl_proc_make_permanent(struct proc_dir_entry *pde)
> > +{
> > +	if (pde) {
> > +		pde_make_permanent(pde);
> > +	}
> 
> Please let's be running checkpatch more often?

No! I'd rather change kernel coding style.

> > --- a/fs/proc/internal.h
> > +++ b/fs/proc/internal.h
> > @@ -80,8 +80,11 @@ static inline bool pde_is_permanent(const struct proc_dir_entry *pde)
> >  	return pde->flags & PROC_ENTRY_PERMANENT;
> >  }
> >  
> > +/* This is for builtin code, not even for modules which are compiled in. */
> >  static inline void pde_make_permanent(struct proc_dir_entry *pde)
> >  {
> > +	/* Ensure magic flag does something. */
> > +	static_assert(PROC_ENTRY_PERMANENT != 0);
> 
> Looks odd.  What is this doing?  The comment does a poor job of
> explaining this!
> 
> >  	pde->flags |= PROC_ENTRY_PERMANENT;
> >  }
> >  
> > diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> > index ea62201c74c4..2d59f29b49eb 100644
> > --- a/include/linux/proc_fs.h
> > +++ b/include/linux/proc_fs.h
> > @@ -247,4 +247,14 @@ static inline struct pid_namespace *proc_pid_ns(struct super_block *sb)
> >  
> >  bool proc_ns_file(const struct file *file);
> >  
> > +static inline void proc_make_permanent(struct proc_dir_entry *pde)
> > +{
> > +	/* Don't give matches to modules. */
> 
> This comment is also mysterious (to me).  Please expand upon it.
> 
> > +#if defined CONFIG_PROC_FS && !defined MODULE
> > +	/* This mess is created by defining "struct proc_dir_entry" elsewhere. */
> 
> Also mysterious.
> 
> > +	void impl_proc_make_permanent(struct proc_dir_entry *pde);
> 
> Forward-declaring a function within a function in this manner is quite
> unusual.  Let's be conventional, please.
> 
> > +	impl_proc_make_permanent(pde);
> > +#endif
> > +}

This patch tries to create semi-exported interface which does nothing
if module built as module is using it it but does something it built-in or
module built-in does. This is why all complications and prorotypes.

But let me change coding style first.