RE: [PATCH] Add file permission mode helpers ( [PATCH xxxx/1285] Replace numeric parameter like 0444 with macro)

[Ni, BaoleX]

Resend the below mail just to You, make sure all list members can receive it.

Thanks!
Baole

-----Original Message-----
From: Ni, BaoleX 
Sent: Thursday, August 04, 2016 5:41 PM
To: 'Steven Rostedt'; 'Sergei Shtylyov'; 'Ville Syrjälä'; 'Hans-Christian Noren Egtvedt'; 'Lino Sanfilippo'; 'Borislav Petkov'; 'Jiri Benc'; 'Leon Romanovsky'; 'Jani Nikula'; 'Juergen Gross'; 'Edward Cree'; 'Jes Sorensen'; 'Mauro Carvalho Chehab'; 'Felipe Balbi'; Pavel Machek; 'Ingo Tuchscherer'; 'Corey Minyard'; 'Ricardo Ribalda Delgado'; Weiny, Ira; 'David Laight'; 'Joe Perches'; 'Krzysztof Kozlowski'; 'Mike Nazarewicz'; 'Lee Jones'; 'Michal Kubecek'; 'Rusty Russell'; Koul, Vinod; 'Theodore Ts'o'; 'rjwysocki@xxxxxxxxx'; 'Steve Cotton'; 'Olof Johansson'; 'Arend van Spriel'; 'Tejun Heo'; 'Dave Kleikamp'; 'Jan Engelhardt'; 'Kalle Valo'; 'Dmitry Torokhov'; 'Henrique de Moraes Holschuh'; 'Dmitry Torokhov'; 'Jiri Slaby'; 'Ismael Luceno'; 'Geoff Levand'; 'RSD @ SFI'; 'Peter Meerwald-Stadler'; 'keescook@xxxxxxxxxx'; 'Shanker Wang'; 'Heiko Carstens'; 'Deucher, Alexander'; 'Bart Van Assche'; 'Randy Dunlap'; 'Sagi Grimberg'; 'Andrew F. Davis'; 'Larry Finger'; 'Eric W. Biederman'; 'James Bottomley'; 'Mark Brown'; 'Steve Wise'; 'Austin S. Hemmelgarn'; 'Cornelia Huck'; 'Christian Borntraeger'; 'Marcel Holtmann'; 'Alan Stern'; 'Jean Delvare'; 'Allen Hubbe'; 'Ralf Baechle'; 'Sven Eckelmann'; 'matwey.kornilov@xxxxxxxxx'; 'Takashi Sakamoto'; 'Sagi Grimberg'; 'Takashi Sakamoto'; 'Peter Ujfalusi'; 'Radim Krcmár'; 'Guenter Roeck'; 'matwey.kornilov@xxxxxxxxx'; 'One Thousand Gnomes'; 'Roger Pau Monné'; 'Conor O'Gorman'; 'Laurence Oberman'; 'Wei Liu'; 'Ricardo Ribalda Delgado'
Cc: 'Ingo Molnar'; Linus Torvalds; 'Al Viro'; Greg Kroah-Hartman; Heiko Carstens; Russell King - ARM Linux; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Linux Kernel Mailing List; Liu, Chuansheng; 'Marcel Holtmann'; 'Willy Tarreau'; 'Oliver Neukum'; 'Richard Weinberger'; 'Konstantin Khlebnikov'; 'Rob Landley'; 'Bart Van Assche'; 'Jarod Wilson'; 'Segher Boessenkool'; 'David Miller'; 'Doug Ledford'; 'Steve Wise'; 'Masami Hiramatsu'; 'Rich Felker'; 'Lukas Wunner'; 'Daniel Vetter'; 'Jiri Kosina'; Nikolova, Tatyana E; Luick, Dean; Hefty, Sean; 'Thomas Hellstrom'; Koul, Vinod; 'inki.dae@xxxxxxxxxxx'; Kirsher, Jeffrey T; Hunter, Adrian; 'Liu [EP], Bin'
Subject: RE: [PATCH] Add file permission mode helpers ( [PATCH xxxx/1285] Replace numeric parameter like 0444 with macro)

Dear All,

Thanks a lot for your kind response and great advice.

1. I made this a series of patches by coccinelle.

2. As a new comer, I indeed made some mistakes on producing and submitting patches.
    As some of you kindly pointed out, I didn't give specific subjects for each patch, 
    and I also shouldn't send out 1285 patches in one thread, in addition, some of the macros I chose weren't the most compact ones, etc.

3. After I sent out my patches, Ingo Molnar introduced another set of symbolic helpers which is easier to read for the octal permission numbers.
    He quickly gave a patch for his nice idea, you can see it in this mail.

4.So I need to resolve my problems firstly with a sane thought, then maybe I will try again.

I'm awfully sorry to bother you.
Thank you again!

Baole




-----Original Message-----
From: Ingo Molnar [mailto:mingo.kernel.org@xxxxxxxxx] On Behalf Of Ingo Molnar
Sent: Wednesday, August 03, 2016 4:12 PM
To: Linus Torvalds
Cc: Pavel Machek; Greg Kroah-Hartman; Heiko Carstens; Ni, BaoleX; Russell King - ARM Linux; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Linux Kernel Mailing List; Liu, Chuansheng
Subject: [PATCH] Add file permission mode helpers


* Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:

> [ So I answered similarly to another patch, but I'll just re-iterate 
> and change the subject line so that it stands out a bit from the 
> millions of actual patches ]
> 
> On Tue, Aug 2, 2016 at 1:42 PM, Pavel Machek <pavel@xxxxxx> wrote:
> >
> > Everyone knows what 0644 is, but noone can read S_IRUSR | S_IWUSR | 
> > S_IRCRP | S_IROTH (*). Please don't do this.
> 
> Absolutely. It's *much* easier to parse and understand the octal 
> numbers, while the symbolic macro names are just random line noise and 
> hard as hell to understand. You really have to think about it.
> 
> So we should rather go the other way: convert existing bad symbolic 
> permission bit macro use to just use the octal numbers.

In addition to that I'd love to have something even easier to read, a few common variants of the permissions field of 'ls -l' pre-defined. I did some quick grepping, and collected the main variants that are in use:

		PERM_r________	0400
		PERM_r__r_____	0440
		PERM_r__r__r__	0444

		PERM_rw_______	0600
		PERM_rw_r_____	0640
		PERM_rw_r__r__	0644
		PERM_rw_rw_r__	0664
		PERM_rw_rw_rw_	0666

		PERM__w_______	0200
		PERM__w__w____	0220
		PERM__w__w__w_	0222

		PERM_r_x______	0500
		PERM_r_xr_x___	0550
		PERM_r_xr_xr_x	0555

		PERM_rwx______	0700
		PERM_rwxr_x___	0750
		PERM_rwxr_xr_x	0755
		PERM_rwxrwxr_x	0775
		PERM_rwxrwxrwx	0777

		PERM__wx______	0300
		PERM__wx_wx___	0330
		PERM__wx_wx_wx	0333

Allowing these would be nice too, because there were cases in the past where people messed up the octal representation or our internal symbolic helpers, but this representation is fundamentally self-describing and pretty 'fool proof'.

An added advantage would be that during review it would stick out like a sore thumb if anyone used a 'weird' permission variant.

For example, if you saw these lines in a driver patch:

+	__ATTR(l1, 0444, driver_show_l4, NULL);
+		__ATTR(l3, 0446, driver_show_l4, NULL);
+			__ATTR(l2, 04444, driver_show_l4, NULL);
+		__ATTR(l4, 0444, driver_show_l4, NULL);

... would you notice it at a glance that it contains two security holes?

While the weird permissions in this:

+		__ATTR(l1, PERM_r__r__r__,  driver_show_l4, NULL);
+		__ATTR(l3, PERM_r__r__rw_,  driver_show_l4, NULL);
+		__ATTR(l2, PERM_sr__r__r__, driver_show_l4, NULL);
+		__ATTR(l4, PERM_r__r__r__,  driver_show_l4, NULL);

Wouln't even build, because the dangerous patterns of PERM_r__r__rw_ or PERM_sr__r__r__ are not defined to begin with.

The patch below adds them to stat.h.

Thanks,

	Ingo

 include/linux/stat.h | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/include/linux/stat.h b/include/linux/stat.h index 075cb0c7eb2a..863d5563427f 100644
--- a/include/linux/stat.h
+++ b/include/linux/stat.h
@@ -5,6 +5,38 @@
 #include <asm/stat.h>
 #include <uapi/linux/stat.h>
 
+/*
+ * Human readable symbolic definitions for common
+ * file permissions:
+ */
+#define PERM_r________	0400
+#define PERM_r__r_____	0440
+#define PERM_r__r__r__	0444
+
+#define PERM_rw_______	0600
+#define PERM_rw_r_____	0640
+#define PERM_rw_r__r__	0644
+#define PERM_rw_rw_r__	0664
+#define PERM_rw_rw_rw_	0666
+
+#define PERM__w_______	0200
+#define PERM__w__w____	0220
+#define PERM__w__w__w_	0222
+
+#define PERM_r_x______	0500
+#define PERM_r_xr_x___	0550
+#define PERM_r_xr_xr_x	0555
+
+#define PERM_rwx______	0700
+#define PERM_rwxr_x___	0750
+#define PERM_rwxr_xr_x	0755
+#define PERM_rwxrwxr_x	0775
+#define PERM_rwxrwxrwx	0777
+
+#define PERM__wx______	0300
+#define PERM__wx_wx___	0330
+#define PERM__wx_wx_wx	0333
+
 #define S_IRWXUGO	(S_IRWXU|S_IRWXG|S_IRWXO)
 #define S_IALLUGO	(S_ISUID|S_ISGID|S_ISVTX|S_IRWXUGO)
 #define S_IRUGO		(S_IRUSR|S_IRGRP|S_IROTH)