diff mbox

simplify procfs code for seq_file instances V2

Message ID 20180506171948.GA769@avx2 (mailing list archive)
State Not Applicable
Headers show

Commit Message

Alexey Dobriyan May 6, 2018, 5:19 p.m. UTC 
On Wed, Apr 25, 2018 at 05:47:47PM +0200, Christoph Hellwig wrote:
> Changes since V1:
>  - open code proc_create_data to avoid setting not fully initialized
>    entries live
>  - use unsigned int for state_size

Need this to maintain sizeof(struct proc_dir_entry):

Otherwise ACK fs/proc/ part.

Comments

Al Viro May 6, 2018, 5:45 p.m. UTC | #1 
On Sun, May 06, 2018 at 08:19:49PM +0300, Alexey Dobriyan wrote:
> +++ b/fs/proc/internal.h
> @@ -48,8 +48,8 @@ struct proc_dir_entry {
>  		const struct seq_operations *seq_ops;
>  		int (*single_show)(struct seq_file *, void *);
>  	};
> -	unsigned int state_size;
>  	void *data;
> +	unsigned int state_size;
>  	unsigned int low_ino;
>  	nlink_t nlink;
>  	kuid_t uid;

Makes sense

> @@ -62,9 +62,9 @@ struct proc_dir_entry {
>  	umode_t mode;
>  	u8 namelen;
>  #ifdef CONFIG_64BIT
> -#define SIZEOF_PDE_INLINE_NAME	(192-139)
> +#define SIZEOF_PDE_INLINE_NAME	(192-155)
>  #else
> -#define SIZEOF_PDE_INLINE_NAME	(128-87)
> +#define SIZEOF_PDE_INLINE_NAME	(128-95)

>  #endif
>  	char inline_name[SIZEOF_PDE_INLINE_NAME];
>  } __randomize_layout;

*UGH*

Both to the original state and that kind of "adjustments".
Incidentally, with __bugger_layout in there these expressions
are simply wrong.

If nothing else, I would suggest turning the last one into
	char inline_name[];
in hope that layout won't get... randomized that much and
used

#ifdef CONFIG_64BIT
#define PDE_SIZE 192
#else
#define PDE_SIZE 128
#endif

union __proc_dir_entry {
	char pad[PDE_SIZE];
	struct proc_dir_entry real;
};

#define SIZEOF_PDE_INLINE_NAME (PDE_SIZE - offsetof(struct proc_dir_entry, inline_name))

for constants, adjusted sizeof and sizeof_field when creating
proc_dir_entry_cache and turned proc_root into

union __proc_dir_entry __proc_root = { .real = {
        .low_ino        = PROC_ROOT_INO,
        .namelen        = 5,
        .mode           = S_IFDIR | S_IRUGO | S_IXUGO,
        .nlink          = 2,
        .refcnt         = REFCOUNT_INIT(1),
        .proc_iops      = &proc_root_inode_operations,
        .proc_fops      = &proc_root_operations,
        .parent         = &__proc_root.real,
        .subdir         = RB_ROOT,
        .name           = __proc_root.real.inline_name,
        .inline_name    = "/proc",
}};

#define proc_root __proc_root.real
(or actually used __proc_root.real in all of a 6 places where it remains).

> diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c
> index baf1994289ce..7d94fa005b0d 100644
> --- a/fs/proc/proc_net.c
> +++ b/fs/proc/proc_net.c
> @@ -40,7 +40,7 @@ static struct net *get_proc_net(const struct inode *inode)
>  
>  static int seq_open_net(struct inode *inode, struct file *file)
>  {
> -	size_t state_size = PDE(inode)->state_size;
> +	unsigned int state_size = PDE(inode)->state_size;
>  	struct seq_net_private *p;
>  	struct net *net;

<shakes head>
You and your "size_t is evil" crusade...
Alexey Dobriyan May 9, 2018, 4:53 p.m. UTC | #2 
On Sun, May 06, 2018 at 06:45:31PM +0100, Al Viro wrote:
> On Sun, May 06, 2018 at 08:19:49PM +0300, Alexey Dobriyan wrote:

> > @@ -62,9 +62,9 @@ struct proc_dir_entry {
> >  	umode_t mode;
> >  	u8 namelen;
> >  #ifdef CONFIG_64BIT
> > -#define SIZEOF_PDE_INLINE_NAME	(192-139)
> > +#define SIZEOF_PDE_INLINE_NAME	(192-155)
> >  #else
> > -#define SIZEOF_PDE_INLINE_NAME	(128-87)
> > +#define SIZEOF_PDE_INLINE_NAME	(128-95)
> 
> >  #endif
> >  	char inline_name[SIZEOF_PDE_INLINE_NAME];
> >  } __randomize_layout;
> 
> *UGH*

I agree. Maintaining these numbers is a pain point.
Who knew people were going to start adding fields right away.

> Both to the original state and that kind of "adjustments".
> Incidentally, with __bugger_layout in there these expressions
> are simply wrong.

Struct randomization is exempt from maintaining sizeof as they are already
screwing cachelines and everything. But if patch works with lockdep and
randomization -- even better.

> If nothing else, I would suggest turning the last one into
> 	char inline_name[];
> in hope that layout won't get... randomized that much and
> used
> 
> #ifdef CONFIG_64BIT
> #define PDE_SIZE 192
> #else
> #define PDE_SIZE 128
> #endif
> 
> union __proc_dir_entry {
> 	char pad[PDE_SIZE];
> 	struct proc_dir_entry real;
> };
> 
> #define SIZEOF_PDE_INLINE_NAME (PDE_SIZE - offsetof(struct proc_dir_entry, inline_name))
> 
> for constants, adjusted sizeof and sizeof_field when creating
> proc_dir_entry_cache and turned proc_root into
> 
> union __proc_dir_entry __proc_root = { .real = {
>         .low_ino        = PROC_ROOT_INO,
>         .namelen        = 5,
>         .mode           = S_IFDIR | S_IRUGO | S_IXUGO,
>         .nlink          = 2,
>         .refcnt         = REFCOUNT_INIT(1),
>         .proc_iops      = &proc_root_inode_operations,
>         .proc_fops      = &proc_root_operations,
>         .parent         = &__proc_root.real,
>         .subdir         = RB_ROOT,
>         .name           = __proc_root.real.inline_name,
>         .inline_name    = "/proc",
> }};
> 
> #define proc_root __proc_root.real
> (or actually used __proc_root.real in all of a 6 places where it remains).
> 
> > -	size_t state_size = PDE(inode)->state_size;
> > +	unsigned int state_size = PDE(inode)->state_size;
> 
> <shakes head>
> You and your "size_t is evil" crusade...

[nods]

        unsigned long           flags;          /* error bits */
        unsigned long           i_state;
        unsigned long           s_blocksize;
        unsigned long           s_flags;
        unsigned long           s_iflags;       /* internal SB_I_* flags */
Christoph Hellwig May 15, 2018, 2:03 p.m. UTC | #3 
On Sun, May 06, 2018 at 08:19:49PM +0300, Alexey Dobriyan wrote:
> On Wed, Apr 25, 2018 at 05:47:47PM +0200, Christoph Hellwig wrote:
> > Changes since V1:
> >  - open code proc_create_data to avoid setting not fully initialized
> >    entries live
> >  - use unsigned int for state_size
> 
> Need this to maintain sizeof(struct proc_dir_entry):

I'll fold your changes into the relevant patches.

> Otherwise ACK fs/proc/ part.

I'll take this as a formal ACK-ed by for all patches touching
procfs.  If I was wrong please scream.
diff mbox

Patch

diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 6d171485c45b..a318ae5b36b4 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -48,8 +48,8 @@  struct proc_dir_entry {
 		const struct seq_operations *seq_ops;
 		int (*single_show)(struct seq_file *, void *);
 	};
-	unsigned int state_size;
 	void *data;
+	unsigned int state_size;
 	unsigned int low_ino;
 	nlink_t nlink;
 	kuid_t uid;
@@ -62,9 +62,9 @@  struct proc_dir_entry {
 	umode_t mode;
 	u8 namelen;
 #ifdef CONFIG_64BIT
-#define SIZEOF_PDE_INLINE_NAME	(192-139)
+#define SIZEOF_PDE_INLINE_NAME	(192-155)
 #else
-#define SIZEOF_PDE_INLINE_NAME	(128-87)
+#define SIZEOF_PDE_INLINE_NAME	(128-95)
 #endif
 	char inline_name[SIZEOF_PDE_INLINE_NAME];
 } __randomize_layout;
diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c
index baf1994289ce..7d94fa005b0d 100644
--- a/fs/proc/proc_net.c
+++ b/fs/proc/proc_net.c
@@ -40,7 +40,7 @@  static struct net *get_proc_net(const struct inode *inode)
 
 static int seq_open_net(struct inode *inode, struct file *file)
 {
-	size_t state_size = PDE(inode)->state_size;
+	unsigned int state_size = PDE(inode)->state_size;
 	struct seq_net_private *p;
 	struct net *net;