diff mbox series

[v10,7/9] proc: move hidepid values to uapi as they are user interface to mount

Message ID 20200327172331.418878-8-gladkov.alexey@gmail.com (mailing list archive)
State New, archived
Headers show
Series proc: modernize proc to support multiple private instances | expand

Commit Message

Alexey Gladkov March 27, 2020, 5:23 p.m. UTC 
Suggested-by: Alexey Dobriyan <adobriyan@gmail.com>
Reviewed-by: Alexey Dobriyan <adobriyan@gmail.com>
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
 include/linux/proc_fs.h      |  9 +--------
 include/uapi/linux/proc_fs.h | 13 +++++++++++++
 2 files changed, 14 insertions(+), 8 deletions(-)
 create mode 100644 include/uapi/linux/proc_fs.h

Comments

Kees Cook March 28, 2020, 8:41 p.m. UTC | #1 
On Fri, Mar 27, 2020 at 06:23:29PM +0100, Alexey Gladkov wrote:
> Suggested-by: Alexey Dobriyan <adobriyan@gmail.com>
> Reviewed-by: Alexey Dobriyan <adobriyan@gmail.com>
> Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
> ---
>  include/linux/proc_fs.h      |  9 +--------
>  include/uapi/linux/proc_fs.h | 13 +++++++++++++
>  2 files changed, 14 insertions(+), 8 deletions(-)
>  create mode 100644 include/uapi/linux/proc_fs.h
> 
> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> index afd38cae2339..d259817ec913 100644
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -7,6 +7,7 @@
>  
>  #include <linux/types.h>
>  #include <linux/fs.h>
> +#include <uapi/linux/proc_fs.h>
>  
>  struct proc_dir_entry;
>  struct seq_file;
> @@ -27,14 +28,6 @@ struct proc_ops {
>  	unsigned long (*proc_get_unmapped_area)(struct file *, unsigned long, unsigned long, unsigned long, unsigned long);
>  };
>  
> -/* definitions for hide_pid field */
> -enum {
> -	HIDEPID_OFF	  = 0,
> -	HIDEPID_NO_ACCESS = 1,
> -	HIDEPID_INVISIBLE = 2,
> -	HIDEPID_NOT_PTRACEABLE = 4, /* Limit pids to only ptraceable pids */
> -};
> -
>  /* definitions for proc mount option pidonly */
>  enum {
>  	PROC_PIDONLY_OFF = 0,
> diff --git a/include/uapi/linux/proc_fs.h b/include/uapi/linux/proc_fs.h
> new file mode 100644
> index 000000000000..dc6d717aa6ec
> --- /dev/null
> +++ b/include/uapi/linux/proc_fs.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_PROC_FS_H
> +#define _UAPI_PROC_FS_H
> +
> +/* definitions for hide_pid field */
> +enum {
> +	HIDEPID_OFF            = 0,
> +	HIDEPID_NO_ACCESS      = 1,
> +	HIDEPID_INVISIBLE      = 2,
> +	HIDEPID_NOT_PTRACEABLE = 4,
> +};
> +
> +#endif
> -- 
> 2.25.2
> 

Should the numeric values still be UAPI if there is string parsing now?
Alexey Gladkov March 28, 2020, 9:25 p.m. UTC | #2 
On Sat, Mar 28, 2020 at 01:41:02PM -0700, Kees Cook wrote:
 > diff --git a/include/uapi/linux/proc_fs.h b/include/uapi/linux/proc_fs.h
> > new file mode 100644
> > index 000000000000..dc6d717aa6ec
> > --- /dev/null
> > +++ b/include/uapi/linux/proc_fs.h
> > @@ -0,0 +1,13 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +#ifndef _UAPI_PROC_FS_H
> > +#define _UAPI_PROC_FS_H
> > +
> > +/* definitions for hide_pid field */
> > +enum {
> > +	HIDEPID_OFF            = 0,
> > +	HIDEPID_NO_ACCESS      = 1,
> > +	HIDEPID_INVISIBLE      = 2,
> > +	HIDEPID_NOT_PTRACEABLE = 4,
> > +};
> > +
> > +#endif
> > -- 
> > 2.25.2
> > 
> 
> Should the numeric values still be UAPI if there is string parsing now?

I think yes, because these are still valid hidepid= values.
Kees Cook March 28, 2020, 9:53 p.m. UTC | #3 
On Sat, Mar 28, 2020 at 10:25:47PM +0100, Alexey Gladkov wrote:
> On Sat, Mar 28, 2020 at 01:41:02PM -0700, Kees Cook wrote:
>  > diff --git a/include/uapi/linux/proc_fs.h b/include/uapi/linux/proc_fs.h
> > > new file mode 100644
> > > index 000000000000..dc6d717aa6ec
> > > --- /dev/null
> > > +++ b/include/uapi/linux/proc_fs.h
> > > @@ -0,0 +1,13 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > +#ifndef _UAPI_PROC_FS_H
> > > +#define _UAPI_PROC_FS_H
> > > +
> > > +/* definitions for hide_pid field */
> > > +enum {
> > > +	HIDEPID_OFF            = 0,
> > > +	HIDEPID_NO_ACCESS      = 1,
> > > +	HIDEPID_INVISIBLE      = 2,
> > > +	HIDEPID_NOT_PTRACEABLE = 4,
> > > +};
> > > +
> > > +#endif
> > > -- 
> > > 2.25.2
> > > 
> > 
> > Should the numeric values still be UAPI if there is string parsing now?
> 
> I think yes, because these are still valid hidepid= values.

But if we don't expose the values, we can do whatever we like with
future numbers (e.g. the "is this a value or a bit field?" question).
Alexey Gladkov March 28, 2020, 11 p.m. UTC | #4 
On Sat, Mar 28, 2020 at 02:53:49PM -0700, Kees Cook wrote:
> > > > +/* definitions for hide_pid field */
> > > > +enum {
> > > > +	HIDEPID_OFF            = 0,
> > > > +	HIDEPID_NO_ACCESS      = 1,
> > > > +	HIDEPID_INVISIBLE      = 2,
> > > > +	HIDEPID_NOT_PTRACEABLE = 4,
> > > > +};
> > > Should the numeric values still be UAPI if there is string parsing now?
> > 
> > I think yes, because these are still valid hidepid= values.
> 
> But if we don't expose the values, we can do whatever we like with
> future numbers (e.g. the "is this a value or a bit field?" question).

Alexey Dobriyan suggested to put these parameters into the UAPI and it
makes sense because these are user parameters.
Kees Cook March 29, 2020, 3:17 a.m. UTC | #5 
On Sun, Mar 29, 2020 at 12:00:46AM +0100, Alexey Gladkov wrote:
> On Sat, Mar 28, 2020 at 02:53:49PM -0700, Kees Cook wrote:
> > > > > +/* definitions for hide_pid field */
> > > > > +enum {
> > > > > +	HIDEPID_OFF            = 0,
> > > > > +	HIDEPID_NO_ACCESS      = 1,
> > > > > +	HIDEPID_INVISIBLE      = 2,
> > > > > +	HIDEPID_NOT_PTRACEABLE = 4,
> > > > > +};
> > > > Should the numeric values still be UAPI if there is string parsing now?
> > > 
> > > I think yes, because these are still valid hidepid= values.
> > 
> > But if we don't expose the values, we can do whatever we like with
> > future numbers (e.g. the "is this a value or a bit field?" question).
> 
> Alexey Dobriyan suggested to put these parameters into the UAPI and it
> makes sense because these are user parameters.

Okidokey. :) Anyway, ignore my HIDEPID_MAX idea then, since this could
become a bitfield. Just checking for individual bits is the way to go
for now. Sorry for the noise.
Eric W. Biederman April 2, 2020, 4:58 p.m. UTC | #6 
I will just say that I do not understand exporting this to the uapi
headers.  Why do we want to export the enumeration names?

I understand that the values are uapi.  This looks like it will make it
difficult to make changes that rename enumeration values to make
the code more readable.

Given that this patchset goes immediately to using string enumerated
values, I also don't understand the point of exporting
HIDEPID_NOT_PTRACEABLE.  I don't think we need to ever let
people use the numeric value.

My sense is that if we are switching to string values we should
just leave the existing numeric values as backwards compatiblity
and not do anything to make them easier to use.

Eric


Alexey Gladkov <gladkov.alexey@gmail.com> writes:

> Suggested-by: Alexey Dobriyan <adobriyan@gmail.com>
> Reviewed-by: Alexey Dobriyan <adobriyan@gmail.com>
> Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
> ---
>  include/linux/proc_fs.h      |  9 +--------
>  include/uapi/linux/proc_fs.h | 13 +++++++++++++
>  2 files changed, 14 insertions(+), 8 deletions(-)
>  create mode 100644 include/uapi/linux/proc_fs.h
>
> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> index afd38cae2339..d259817ec913 100644
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -7,6 +7,7 @@
>  
>  #include <linux/types.h>
>  #include <linux/fs.h>
> +#include <uapi/linux/proc_fs.h>
>  
>  struct proc_dir_entry;
>  struct seq_file;
> @@ -27,14 +28,6 @@ struct proc_ops {
>  	unsigned long (*proc_get_unmapped_area)(struct file *, unsigned long, unsigned long, unsigned long, unsigned long);
>  };
>  
> -/* definitions for hide_pid field */
> -enum {
> -	HIDEPID_OFF	  = 0,
> -	HIDEPID_NO_ACCESS = 1,
> -	HIDEPID_INVISIBLE = 2,
> -	HIDEPID_NOT_PTRACEABLE = 4, /* Limit pids to only ptraceable pids */
> -};
> -
>  /* definitions for proc mount option pidonly */
>  enum {
>  	PROC_PIDONLY_OFF = 0,
> diff --git a/include/uapi/linux/proc_fs.h b/include/uapi/linux/proc_fs.h
> new file mode 100644
> index 000000000000..dc6d717aa6ec
> --- /dev/null
> +++ b/include/uapi/linux/proc_fs.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_PROC_FS_H
> +#define _UAPI_PROC_FS_H
> +
> +/* definitions for hide_pid field */
> +enum {
> +	HIDEPID_OFF            = 0,
> +	HIDEPID_NO_ACCESS      = 1,
> +	HIDEPID_INVISIBLE      = 2,
> +	HIDEPID_NOT_PTRACEABLE = 4,
> +};
> +
> +#endif
Kees Cook April 3, 2020, 11:59 p.m. UTC | #7 
On Thu, Apr 02, 2020 at 11:58:28AM -0500, Eric W. Biederman wrote:
> 
> I will just say that I do not understand exporting this to the uapi
> headers.  Why do we want to export the enumeration names?
> 
> I understand that the values are uapi.  This looks like it will make it
> difficult to make changes that rename enumeration values to make
> the code more readable.
> 
> Given that this patchset goes immediately to using string enumerated
> values, I also don't understand the point of exporting
> HIDEPID_NOT_PTRACEABLE.  I don't think we need to ever let
> people use the numeric value.
> 
> My sense is that if we are switching to string values we should
> just leave the existing numeric values as backwards compatiblity
> and not do anything to make them easier to use.

Yeah, that's what I had suggested too. Let's not export this to UAPI.

-Kees

> 
> Eric
> 
> 
> Alexey Gladkov <gladkov.alexey@gmail.com> writes:
> 
> > Suggested-by: Alexey Dobriyan <adobriyan@gmail.com>
> > Reviewed-by: Alexey Dobriyan <adobriyan@gmail.com>
> > Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
> > ---
> >  include/linux/proc_fs.h      |  9 +--------
> >  include/uapi/linux/proc_fs.h | 13 +++++++++++++
> >  2 files changed, 14 insertions(+), 8 deletions(-)
> >  create mode 100644 include/uapi/linux/proc_fs.h
> >
> > diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> > index afd38cae2339..d259817ec913 100644
> > --- a/include/linux/proc_fs.h
> > +++ b/include/linux/proc_fs.h
> > @@ -7,6 +7,7 @@
> >  
> >  #include <linux/types.h>
> >  #include <linux/fs.h>
> > +#include <uapi/linux/proc_fs.h>
> >  
> >  struct proc_dir_entry;
> >  struct seq_file;
> > @@ -27,14 +28,6 @@ struct proc_ops {
> >  	unsigned long (*proc_get_unmapped_area)(struct file *, unsigned long, unsigned long, unsigned long, unsigned long);
> >  };
> >  
> > -/* definitions for hide_pid field */
> > -enum {
> > -	HIDEPID_OFF	  = 0,
> > -	HIDEPID_NO_ACCESS = 1,
> > -	HIDEPID_INVISIBLE = 2,
> > -	HIDEPID_NOT_PTRACEABLE = 4, /* Limit pids to only ptraceable pids */
> > -};
> > -
> >  /* definitions for proc mount option pidonly */
> >  enum {
> >  	PROC_PIDONLY_OFF = 0,
> > diff --git a/include/uapi/linux/proc_fs.h b/include/uapi/linux/proc_fs.h
> > new file mode 100644
> > index 000000000000..dc6d717aa6ec
> > --- /dev/null
> > +++ b/include/uapi/linux/proc_fs.h
> > @@ -0,0 +1,13 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +#ifndef _UAPI_PROC_FS_H
> > +#define _UAPI_PROC_FS_H
> > +
> > +/* definitions for hide_pid field */
> > +enum {
> > +	HIDEPID_OFF            = 0,
> > +	HIDEPID_NO_ACCESS      = 1,
> > +	HIDEPID_INVISIBLE      = 2,
> > +	HIDEPID_NOT_PTRACEABLE = 4,
> > +};
> > +
> > +#endif
diff mbox series

Patch

diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index afd38cae2339..d259817ec913 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -7,6 +7,7 @@ 
 
 #include <linux/types.h>
 #include <linux/fs.h>
+#include <uapi/linux/proc_fs.h>
 
 struct proc_dir_entry;
 struct seq_file;
@@ -27,14 +28,6 @@  struct proc_ops {
 	unsigned long (*proc_get_unmapped_area)(struct file *, unsigned long, unsigned long, unsigned long, unsigned long);
 };
 
-/* definitions for hide_pid field */
-enum {
-	HIDEPID_OFF	  = 0,
-	HIDEPID_NO_ACCESS = 1,
-	HIDEPID_INVISIBLE = 2,
-	HIDEPID_NOT_PTRACEABLE = 4, /* Limit pids to only ptraceable pids */
-};
-
 /* definitions for proc mount option pidonly */
 enum {
 	PROC_PIDONLY_OFF = 0,
diff --git a/include/uapi/linux/proc_fs.h b/include/uapi/linux/proc_fs.h
new file mode 100644
index 000000000000..dc6d717aa6ec
--- /dev/null
+++ b/include/uapi/linux/proc_fs.h
@@ -0,0 +1,13 @@ 
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_PROC_FS_H
+#define _UAPI_PROC_FS_H
+
+/* definitions for hide_pid field */
+enum {
+	HIDEPID_OFF            = 0,
+	HIDEPID_NO_ACCESS      = 1,
+	HIDEPID_INVISIBLE      = 2,
+	HIDEPID_NOT_PTRACEABLE = 4,
+};
+
+#endif