* Please don't replace numeric parameter like 0444 with macro
@ 2016-08-02 20:58 ` Linus Torvalds
0 siblings, 0 replies; 28+ messages in thread
From: Linus Torvalds @ 2016-08-02 20:58 UTC (permalink / raw)
To: Pavel Machek, Greg Kroah-Hartman, Heiko Carstens
Cc: Baole Ni, Russell King - ARM Linux, linux-arm-kernel,
Linux Kernel Mailing List, chuansheng.liu
[ 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@ucw.cz> 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.
The symbolic names are good for the *other* bits (ie sticky bit, and
the inode mode _type_ numbers etc), but for the permission bits, the
symbolic names are just insane crap. Nobody sane should ever use them.
Not in the kernel, not in user space.
Linus
^ permalink raw reply [flat|nested] 28+ messages in thread
* Please don't replace numeric parameter like 0444 with macro @ 2016-08-02 20:58 ` Linus Torvalds 0 siblings, 0 replies; 28+ messages in thread From: Linus Torvalds @ 2016-08-02 20:58 UTC (permalink / raw) To: linux-arm-kernel [ 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@ucw.cz> 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. The symbolic names are good for the *other* bits (ie sticky bit, and the inode mode _type_ numbers etc), but for the permission bits, the symbolic names are just insane crap. Nobody sane should ever use them. Not in the kernel, not in user space. Linus ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Please don't replace numeric parameter like 0444 with macro 2016-08-02 20:58 ` Linus Torvalds @ 2016-08-02 21:53 ` Rob Landley -1 siblings, 0 replies; 28+ messages in thread From: Rob Landley @ 2016-08-02 21:53 UTC (permalink / raw) To: Linus Torvalds, Pavel Machek, Greg Kroah-Hartman, Heiko Carstens Cc: Baole Ni, Russell King - ARM Linux, linux-arm-kernel, Linux Kernel Mailing List, chuansheng.liu On 08/02/2016 03:58 PM, Linus Torvalds 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@ucw.cz> 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. Posix even specifies the numeric value for each macro in the chmod command's extended description: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/chmod.html Rob ^ permalink raw reply [flat|nested] 28+ messages in thread
* Please don't replace numeric parameter like 0444 with macro @ 2016-08-02 21:53 ` Rob Landley 0 siblings, 0 replies; 28+ messages in thread From: Rob Landley @ 2016-08-02 21:53 UTC (permalink / raw) To: linux-arm-kernel On 08/02/2016 03:58 PM, Linus Torvalds 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@ucw.cz> 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. Posix even specifies the numeric value for each macro in the chmod command's extended description: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/chmod.html Rob ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH] checkpatch: Look for symbolic permissions and suggest octal instead 2016-08-02 20:58 ` Linus Torvalds (?) (?) @ 2016-08-02 23:39 ` Joe Perches 2016-08-03 0:15 ` Al Viro 2016-08-15 16:38 ` Joe Perches -1 siblings, 2 replies; 28+ messages in thread From: Joe Perches @ 2016-08-02 23:39 UTC (permalink / raw) To: Andrew Morton, Andy Whitcroft; +Cc: Linus Torvalds, linux-kernel S_<FOO> uses should be avoided where octal is more intelligible. Signed-off-by: Joe Perches <joe@perches.com> --- scripts/checkpatch.pl | 49 +++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 43 insertions(+), 6 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 1d5b09d..1140940 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -541,6 +541,32 @@ our $mode_perms_world_writable = qr{ 0[0-7][0-7][2367] }x; +our %mode_permission_string_types = ( + "S_IRWXU" => 0700, + "S_IRUSR" => 0400, + "S_IWUSR" => 0200, + "S_IXUSR" => 0100, + "S_IRWXG" => 0070, + "S_IRGRP" => 0040, + "S_IWGRP" => 0020, + "S_IXGRP" => 0010, + "S_IRWXO" => 0007, + "S_IROTH" => 0004, + "S_IWOTH" => 0002, + "S_IXOTH" => 0001, + "S_IRWXUGO" => 0777, + "S_IRUGO" => 0444, + "S_IWUGO" => 0222, + "S_IXUGO" => 0111, +); + +#Create a search pattern for all these strings to speed up a loop below +our $mode_perms_string_search = ""; +foreach my $entry (keys %mode_permission_string_types) { + $mode_perms_string_search .= '|' if ($mode_perms_string_search ne ""); + $mode_perms_string_search .= $entry; +} + our $allowed_asm_includes = qr{(?x: irq| memory| @@ -5996,20 +6022,31 @@ sub process { $arg_pos--; $skip_args = "(?:\\s*$FuncArg\\s*,\\s*){$arg_pos,$arg_pos}"; } - my $test = "\\b$func\\s*\\(${skip_args}([\\d]+)\\s*[,\\)]"; + my $test = "\\b$func\\s*\\(${skip_args}($FuncArg(?:\\|\\s*$FuncArg)*)\\s*[,\\)]"; if ($line =~ /$test/) { my $val = $1; $val = $6 if ($skip_args ne ""); - - if ($val !~ /^0$/ && - (($val =~ /^$Int$/ && $val !~ /^$Octal$/) || - length($val) ne 4)) { + if (($val =~ /^$Int$/ && $val !~ /^$Octal$/) || + ($val =~ /^$Octal$/ && length($val) ne 4)) { ERROR("NON_OCTAL_PERMISSIONS", "Use 4 digit octal (0777) not decimal permissions\n" . $herecurr); - } elsif ($val =~ /^$Octal$/ && (oct($val) & 02)) { + } + if ($val =~ /^$Octal$/ && (oct($val) & 02)) { ERROR("EXPORTED_WORLD_WRITABLE", "Exporting writable files is usually an error. Consider more restrictive permissions.\n" . $herecurr); } + if ($val =~ /\b$mode_perms_string_search\b/) { + my $to = 0; + while ($val =~ /\b($mode_perms_string_search)\b(?:\s*\|\s*)?\s*/g) { + $to |= $mode_permission_string_types{$1}; + } + my $new = sprintf("%04o", $to); + if (WARN("SYMBOLIC_PERMS", + "Symbolic permissions are not preferred. Consider using octal permissions $new.\n" . $herecurr) && + $fix) { + $fixed[$fixlinenr] =~ s/\Q$val\E/$new/; + } + } } } } -- 2.8.0.rc4.16.g56331f8 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Look for symbolic permissions and suggest octal instead 2016-08-02 23:39 ` [PATCH] checkpatch: Look for symbolic permissions and suggest octal instead Joe Perches @ 2016-08-03 0:15 ` Al Viro 2016-08-03 0:30 ` Joe Perches 2016-08-15 16:38 ` Joe Perches 1 sibling, 1 reply; 28+ messages in thread From: Al Viro @ 2016-08-03 0:15 UTC (permalink / raw) To: Joe Perches; +Cc: Andrew Morton, Andy Whitcroft, Linus Torvalds, linux-kernel On Tue, Aug 02, 2016 at 04:39:24PM -0700, Joe Perches wrote: > S_<FOO> uses should be avoided where octal is more intelligible. Oh, for Cthulhu sake! So not only we had been dribbled upon with 1200-odd piles of pointless crap, now we'll be getting yet another set of equally pointless garbage each time a bored wanker gets to run checkpatch.pl? Leave that alone. In either direction. Dipshits on quest for commit count have enough tools already, no need to add another one... ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Look for symbolic permissions and suggest octal instead 2016-08-03 0:15 ` Al Viro @ 2016-08-03 0:30 ` Joe Perches 0 siblings, 0 replies; 28+ messages in thread From: Joe Perches @ 2016-08-03 0:30 UTC (permalink / raw) To: Al Viro; +Cc: Andrew Morton, Andy Whitcroft, Linus Torvalds, linux-kernel On Wed, 2016-08-03 at 01:15 +0100, Al Viro wrote: > On Tue, Aug 02, 2016 at 04:39:24PM -0700, Joe Perches wrote: > > > > S_ uses should be avoided where octal is more intelligible. > Oh, for Cthulhu sake! So not only we had been dribbled upon with 1200-odd > piles of pointless crap, now we'll be getting yet another set of equally > pointless garbage each time a bored wanker gets to run checkpatch.pl? > > Leave that alone. In either direction. Dipshits on quest for commit count > have enough tools already, no need to add another one... Well, we disagree. Use like DEVICE_ATTR are a mix of S_<FOO> vs octal (~3:1) and a single style can be easier to grep for misuses. Crud like this doesn't need to be cleaned up all at once and checkpatch can be useful to encourage whatever preferred style is desired. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] checkpatch: Look for symbolic permissions and suggest octal instead 2016-08-02 23:39 ` [PATCH] checkpatch: Look for symbolic permissions and suggest octal instead Joe Perches 2016-08-03 0:15 ` Al Viro @ 2016-08-15 16:38 ` Joe Perches 1 sibling, 0 replies; 28+ messages in thread From: Joe Perches @ 2016-08-15 16:38 UTC (permalink / raw) To: Andrew Morton, Andy Whitcroft; +Cc: Linus Torvalds, linux-kernel On Tue, 2016-08-02 at 16:39 -0700, Joe Perches wrote: > S_ uses should be avoided where octal is more intelligible. ping? Should CodingStyle and Documentation/filesystems change too? > Signed-off-by: Joe Perches <joe@perches.com> > --- > scripts/checkpatch.pl | 49 +++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 43 insertions(+), 6 deletions(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 1d5b09d..1140940 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -541,6 +541,32 @@ our $mode_perms_world_writable = qr{ > 0[0-7][0-7][2367] > }x; > > +our %mode_permission_string_types = ( > + "S_IRWXU" => 0700, > + "S_IRUSR" => 0400, > + "S_IWUSR" => 0200, > + "S_IXUSR" => 0100, > + "S_IRWXG" => 0070, > + "S_IRGRP" => 0040, > + "S_IWGRP" => 0020, > + "S_IXGRP" => 0010, > + "S_IRWXO" => 0007, > + "S_IROTH" => 0004, > + "S_IWOTH" => 0002, > + "S_IXOTH" => 0001, > + "S_IRWXUGO" => 0777, > + "S_IRUGO" => 0444, > + "S_IWUGO" => 0222, > + "S_IXUGO" => 0111, > +); > + > +#Create a search pattern for all these strings to speed up a loop below > +our $mode_perms_string_search = ""; > +foreach my $entry (keys %mode_permission_string_types) { > + $mode_perms_string_search .= '|' if ($mode_perms_string_search ne ""); > + $mode_perms_string_search .= $entry; > +} > + > our $allowed_asm_includes = qr{(?x: > irq| > memory| > @@ -5996,20 +6022,31 @@ sub process { > $arg_pos--; > $skip_args = "(?:\\s*$FuncArg\\s*,\\s*){$arg_pos,$arg_pos}"; > } > - my $test = "\\b$func\\s*\\(${skip_args}([\\d]+)\\s*[,\\)]"; > + my $test = "\\b$func\\s*\\(${skip_args}($FuncArg(?:\\|\\s*$FuncArg)*)\\s*[,\\)]"; > if ($line =~ /$test/) { > my $val = $1; > $val = $6 if ($skip_args ne ""); > - > - if ($val !~ /^0$/ && > - (($val =~ /^$Int$/ && $val !~ /^$Octal$/) || > - length($val) ne 4)) { > + if (($val =~ /^$Int$/ && $val !~ /^$Octal$/) || > + ($val =~ /^$Octal$/ && length($val) ne 4)) { > ERROR("NON_OCTAL_PERMISSIONS", > "Use 4 digit octal (0777) not decimal permissions\n" . $herecurr); > - } elsif ($val =~ /^$Octal$/ && (oct($val) & 02)) { > + } > + if ($val =~ /^$Octal$/ && (oct($val) & 02)) { > ERROR("EXPORTED_WORLD_WRITABLE", > "Exporting writable files is usually an error. Consider more restrictive permissions.\n" . $herecurr); > } > + if ($val =~ /\b$mode_perms_string_search\b/) { > + my $to = 0; > + while ($val =~ /\b($mode_perms_string_search)\b(?:\s*\|\s*)?\s*/g) { > + $to |= $mode_permission_string_types{$1}; > + } > + my $new = sprintf("%04o", $to); > + if (WARN("SYMBOLIC_PERMS", > + "Symbolic permissions are not preferred. Consider using octal permissions $new.\n" . $herecurr) && > + $fix) { > + $fixed[$fixlinenr] =~ s/\Q$val\E/$new/; > + } > + } > } > } > } ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Please don't replace numeric parameter like 0444 with macro 2016-08-02 20:58 ` Linus Torvalds @ 2016-08-03 0:42 ` Al Viro -1 siblings, 0 replies; 28+ messages in thread From: Al Viro @ 2016-08-03 0:42 UTC (permalink / raw) To: Linus Torvalds Cc: Pavel Machek, Greg Kroah-Hartman, Heiko Carstens, Baole Ni, Russell King - ARM Linux, linux-arm-kernel, Linux Kernel Mailing List, chuansheng.liu On Tue, Aug 02, 2016 at 04:58:29PM -0400, Linus Torvalds 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@ucw.cz> 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. > > The symbolic names are good for the *other* bits (ie sticky bit, and > the inode mode _type_ numbers etc), but for the permission bits, the > symbolic names are just insane crap. Nobody sane should ever use them. > Not in the kernel, not in user space. Except that you are inviting the mixes like S_IFDIR | 17 /* oops, should've been 017, or do we spell it 0017? */ that way. I certainly agree that this patch series had been a huge pile of manure, but "let's convert it in other direction" is inviting pretty much the same thing, with lovely potential for typos, etc. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Please don't replace numeric parameter like 0444 with macro @ 2016-08-03 0:42 ` Al Viro 0 siblings, 0 replies; 28+ messages in thread From: Al Viro @ 2016-08-03 0:42 UTC (permalink / raw) To: linux-arm-kernel On Tue, Aug 02, 2016 at 04:58:29PM -0400, Linus Torvalds 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@ucw.cz> 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. > > The symbolic names are good for the *other* bits (ie sticky bit, and > the inode mode _type_ numbers etc), but for the permission bits, the > symbolic names are just insane crap. Nobody sane should ever use them. > Not in the kernel, not in user space. Except that you are inviting the mixes like S_IFDIR | 17 /* oops, should've been 017, or do we spell it 0017? */ that way. I certainly agree that this patch series had been a huge pile of manure, but "let's convert it in other direction" is inviting pretty much the same thing, with lovely potential for typos, etc. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Please don't replace numeric parameter like 0444 with macro 2016-08-03 0:42 ` Al Viro @ 2016-08-03 8:07 ` Konstantin Khlebnikov -1 siblings, 0 replies; 28+ messages in thread From: Konstantin Khlebnikov @ 2016-08-03 8:07 UTC (permalink / raw) To: Al Viro Cc: Linus Torvalds, Greg Kroah-Hartman, Heiko Carstens, Russell King - ARM Linux, Linux Kernel Mailing List, Baole Ni, Pavel Machek, chuansheng.liu, linux-arm-kernel On Wed, Aug 3, 2016 at 3:42 AM, Al Viro <viro@zeniv.linux.org.uk> wrote: > On Tue, Aug 02, 2016 at 04:58:29PM -0400, Linus Torvalds 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@ucw.cz> 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. >> >> The symbolic names are good for the *other* bits (ie sticky bit, and >> the inode mode _type_ numbers etc), but for the permission bits, the >> symbolic names are just insane crap. Nobody sane should ever use them. >> Not in the kernel, not in user space. > > Except that you are inviting the mixes like S_IFDIR | 17 /* oops, should've > been 017, or do we spell it 0017? */ that way. I certainly agree that this > patch series had been a huge pile of manure, but "let's convert it in other > direction" is inviting pretty much the same thing, with lovely potential for > typos, etc. We could add several macro with readable names for really used rwx combinations, like: #define KERN_SECRET_RO 0400 #define KERN_SECRET_RW 0600 #define KERN_SECRET_WO 0200 #define KERN_SECRET_DIR 0500 #define KERN_PUBLIC_RO 0444 #define KERN_PUBLIC_RW 0644 #define KERN_PUBLIC_DIR 0555 #define KERN_UNSAFE_RW 0666 #define KERN_UNSAFE_WO 0222 #define KERN_UNSAFE_DIR 0777 > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 28+ messages in thread
* Please don't replace numeric parameter like 0444 with macro @ 2016-08-03 8:07 ` Konstantin Khlebnikov 0 siblings, 0 replies; 28+ messages in thread From: Konstantin Khlebnikov @ 2016-08-03 8:07 UTC (permalink / raw) To: linux-arm-kernel On Wed, Aug 3, 2016 at 3:42 AM, Al Viro <viro@zeniv.linux.org.uk> wrote: > On Tue, Aug 02, 2016 at 04:58:29PM -0400, Linus Torvalds 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@ucw.cz> 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. >> >> The symbolic names are good for the *other* bits (ie sticky bit, and >> the inode mode _type_ numbers etc), but for the permission bits, the >> symbolic names are just insane crap. Nobody sane should ever use them. >> Not in the kernel, not in user space. > > Except that you are inviting the mixes like S_IFDIR | 17 /* oops, should've > been 017, or do we spell it 0017? */ that way. I certainly agree that this > patch series had been a huge pile of manure, but "let's convert it in other > direction" is inviting pretty much the same thing, with lovely potential for > typos, etc. We could add several macro with readable names for really used rwx combinations, like: #define KERN_SECRET_RO 0400 #define KERN_SECRET_RW 0600 #define KERN_SECRET_WO 0200 #define KERN_SECRET_DIR 0500 #define KERN_PUBLIC_RO 0444 #define KERN_PUBLIC_RW 0644 #define KERN_PUBLIC_DIR 0555 #define KERN_UNSAFE_RW 0666 #define KERN_UNSAFE_WO 0222 #define KERN_UNSAFE_DIR 0777 > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: Please don't replace numeric parameter like 0444 with macro 2016-08-03 8:07 ` Konstantin Khlebnikov @ 2016-08-03 8:30 ` Richard Weinberger -1 siblings, 0 replies; 28+ messages in thread From: Richard Weinberger @ 2016-08-03 8:30 UTC (permalink / raw) To: Konstantin Khlebnikov Cc: Al Viro, Linus Torvalds, Greg Kroah-Hartman, Heiko Carstens, Russell King - ARM Linux, Linux Kernel Mailing List, Baole Ni, Pavel Machek, chuansheng.liu, linux-arm-kernel On Wed, Aug 3, 2016 at 10:07 AM, Konstantin Khlebnikov <koct9i@gmail.com> wrote: > On Wed, Aug 3, 2016 at 3:42 AM, Al Viro <viro@zeniv.linux.org.uk> wrote: >> On Tue, Aug 02, 2016 at 04:58:29PM -0400, Linus Torvalds 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@ucw.cz> 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. >>> >>> The symbolic names are good for the *other* bits (ie sticky bit, and >>> the inode mode _type_ numbers etc), but for the permission bits, the >>> symbolic names are just insane crap. Nobody sane should ever use them. >>> Not in the kernel, not in user space. >> >> Except that you are inviting the mixes like S_IFDIR | 17 /* oops, should've >> been 017, or do we spell it 0017? */ that way. I certainly agree that this >> patch series had been a huge pile of manure, but "let's convert it in other >> direction" is inviting pretty much the same thing, with lovely potential for >> typos, etc. > > We could add several macro with readable names for really used rwx > combinations, like: > > #define KERN_SECRET_RO 0400 > #define KERN_SECRET_RW 0600 > #define KERN_SECRET_WO 0200 > #define KERN_SECRET_DIR 0500 > > #define KERN_PUBLIC_RO 0444 > #define KERN_PUBLIC_RW 0644 > #define KERN_PUBLIC_DIR 0555 > > #define KERN_UNSAFE_RW 0666 > #define KERN_UNSAFE_WO 0222 > #define KERN_UNSAFE_DIR 0777 Or just keep it as-is, everybody can read them. Why do we have to hide all kind of numbers behind CPP macros? Is this a thing these days? -- Thanks, //richard ^ permalink raw reply [flat|nested] 28+ messages in thread
* Please don't replace numeric parameter like 0444 with macro @ 2016-08-03 8:30 ` Richard Weinberger 0 siblings, 0 replies; 28+ messages in thread From: Richard Weinberger @ 2016-08-03 8:30 UTC (permalink / raw) To: linux-arm-kernel On Wed, Aug 3, 2016 at 10:07 AM, Konstantin Khlebnikov <koct9i@gmail.com> wrote: > On Wed, Aug 3, 2016 at 3:42 AM, Al Viro <viro@zeniv.linux.org.uk> wrote: >> On Tue, Aug 02, 2016 at 04:58:29PM -0400, Linus Torvalds 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@ucw.cz> 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. >>> >>> The symbolic names are good for the *other* bits (ie sticky bit, and >>> the inode mode _type_ numbers etc), but for the permission bits, the >>> symbolic names are just insane crap. Nobody sane should ever use them. >>> Not in the kernel, not in user space. >> >> Except that you are inviting the mixes like S_IFDIR | 17 /* oops, should've >> been 017, or do we spell it 0017? */ that way. I certainly agree that this >> patch series had been a huge pile of manure, but "let's convert it in other >> direction" is inviting pretty much the same thing, with lovely potential for >> typos, etc. > > We could add several macro with readable names for really used rwx > combinations, like: > > #define KERN_SECRET_RO 0400 > #define KERN_SECRET_RW 0600 > #define KERN_SECRET_WO 0200 > #define KERN_SECRET_DIR 0500 > > #define KERN_PUBLIC_RO 0444 > #define KERN_PUBLIC_RW 0644 > #define KERN_PUBLIC_DIR 0555 > > #define KERN_UNSAFE_RW 0666 > #define KERN_UNSAFE_WO 0222 > #define KERN_UNSAFE_DIR 0777 Or just keep it as-is, everybody can read them. Why do we have to hide all kind of numbers behind CPP macros? Is this a thing these days? -- Thanks, //richard ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH] Add file permission mode helpers 2016-08-02 20:58 ` Linus Torvalds @ 2016-08-03 8:11 ` Ingo Molnar -1 siblings, 0 replies; 28+ messages in thread From: Ingo Molnar @ 2016-08-03 8:11 UTC (permalink / raw) To: Linus Torvalds Cc: Pavel Machek, Greg Kroah-Hartman, Heiko Carstens, Baole Ni, Russell King - ARM Linux, linux-arm-kernel, Linux Kernel Mailing List, chuansheng.liu * Linus Torvalds <torvalds@linux-foundation.org> 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@ucw.cz> 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) ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH] Add file permission mode helpers @ 2016-08-03 8:11 ` Ingo Molnar 0 siblings, 0 replies; 28+ messages in thread From: Ingo Molnar @ 2016-08-03 8:11 UTC (permalink / raw) To: linux-arm-kernel * Linus Torvalds <torvalds@linux-foundation.org> 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@ucw.cz> 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) ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH] Add file permission mode helpers 2016-08-03 8:11 ` Ingo Molnar @ 2016-08-03 8:28 ` Greg Kroah-Hartman -1 siblings, 0 replies; 28+ messages in thread From: Greg Kroah-Hartman @ 2016-08-03 8:28 UTC (permalink / raw) To: Ingo Molnar Cc: Linus Torvalds, Pavel Machek, Heiko Carstens, Baole Ni, Russell King - ARM Linux, linux-arm-kernel, Linux Kernel Mailing List, chuansheng.liu On Wed, Aug 03, 2016 at 10:11:40AM +0200, Ingo Molnar wrote: > 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? I've tried to deal with that in the past with the __ATTR_RW() and __ATTR_RO() and __ATTR_WO() macros that more should be using. I swept the tree a few years ago to try to fix up most of them, but I know I didn't catch them all, and more files have been added since then. > 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. Because of that, odds are people will just stick to the octal numbers, because they think they want something other than the ones you defined for foolish reasons :) That being said, I do like them much better than the macros we have today, which I always have to go and look up every time I see them... thanks, greg k-h ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH] Add file permission mode helpers @ 2016-08-03 8:28 ` Greg Kroah-Hartman 0 siblings, 0 replies; 28+ messages in thread From: Greg Kroah-Hartman @ 2016-08-03 8:28 UTC (permalink / raw) To: linux-arm-kernel On Wed, Aug 03, 2016 at 10:11:40AM +0200, Ingo Molnar wrote: > 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? I've tried to deal with that in the past with the __ATTR_RW() and __ATTR_RO() and __ATTR_WO() macros that more should be using. I swept the tree a few years ago to try to fix up most of them, but I know I didn't catch them all, and more files have been added since then. > 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. Because of that, odds are people will just stick to the octal numbers, because they think they want something other than the ones you defined for foolish reasons :) That being said, I do like them much better than the macros we have today, which I always have to go and look up every time I see them... thanks, greg k-h ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Add file permission mode helpers 2016-08-03 8:28 ` Greg Kroah-Hartman @ 2016-08-03 8:39 ` Ingo Molnar -1 siblings, 0 replies; 28+ messages in thread From: Ingo Molnar @ 2016-08-03 8:39 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Linus Torvalds, Pavel Machek, Heiko Carstens, Baole Ni, Russell King - ARM Linux, linux-arm-kernel, Linux Kernel Mailing List, chuansheng.liu * Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Wed, Aug 03, 2016 at 10:11:40AM +0200, Ingo Molnar wrote: > > 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? > > I've tried to deal with that in the past with the __ATTR_RW() and > __ATTR_RO() and __ATTR_WO() macros that more should be using. I swept > the tree a few years ago to try to fix up most of them, but I know I > didn't catch them all, and more files have been added since then. > > > 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. > > Because of that, odds are people will just stick to the octal numbers, > because they think they want something other than the ones you defined > for foolish reasons :) For code I maintain I'd insist on contributors using the human readable versions, because in the past I've mixed up octals (and the symbolic helpers we have today) myself and I find the 'ls -l' format much easier to read because that's the primary file permission format I see every day working on code. > That being said, I do like them much better than the macros we have today, which > I always have to go and look up every time I see them... Same here! I'm sure core VFS developers know all of the octals and the helpers by heart, but the set of maintainers accepting debugfs and sysfs file permission patches is much wider than that, so every little bit of clarity helps. Thanks, Ingo ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH] Add file permission mode helpers @ 2016-08-03 8:39 ` Ingo Molnar 0 siblings, 0 replies; 28+ messages in thread From: Ingo Molnar @ 2016-08-03 8:39 UTC (permalink / raw) To: linux-arm-kernel * Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Wed, Aug 03, 2016 at 10:11:40AM +0200, Ingo Molnar wrote: > > 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? > > I've tried to deal with that in the past with the __ATTR_RW() and > __ATTR_RO() and __ATTR_WO() macros that more should be using. I swept > the tree a few years ago to try to fix up most of them, but I know I > didn't catch them all, and more files have been added since then. > > > 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. > > Because of that, odds are people will just stick to the octal numbers, > because they think they want something other than the ones you defined > for foolish reasons :) For code I maintain I'd insist on contributors using the human readable versions, because in the past I've mixed up octals (and the symbolic helpers we have today) myself and I find the 'ls -l' format much easier to read because that's the primary file permission format I see every day working on code. > That being said, I do like them much better than the macros we have today, which > I always have to go and look up every time I see them... Same here! I'm sure core VFS developers know all of the octals and the helpers by heart, but the set of maintainers accepting debugfs and sysfs file permission patches is much wider than that, so every little bit of clarity helps. Thanks, Ingo ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Add file permission mode helpers 2016-08-03 8:39 ` Ingo Molnar @ 2016-08-03 9:21 ` Willy Tarreau -1 siblings, 0 replies; 28+ messages in thread From: Willy Tarreau @ 2016-08-03 9:21 UTC (permalink / raw) To: Ingo Molnar Cc: Greg Kroah-Hartman, Heiko Carstens, Russell King - ARM Linux, Linux Kernel Mailing List, Baole Ni, Pavel Machek, chuansheng.liu, Linus Torvalds, linux-arm-kernel On Wed, Aug 03, 2016 at 10:39:03AM +0200, Ingo Molnar wrote: > > > 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. > > > > Because of that, odds are people will just stick to the octal numbers, > > because they think they want something other than the ones you defined > > for foolish reasons :) > > For code I maintain I'd insist on contributors using the human readable versions, > because in the past I've mixed up octals (and the symbolic helpers we have today) > myself and I find the 'ls -l' format much easier to read because that's the > primary file permission format I see every day working on code. FWIW, the only "human readable" ones for me are the octal ones, which are also the same as those I'm using every day with "chmod" or "find" and that I find hard to get wrong. But I agree that the PERM_* idea above are a nice alternative since they match the "ls -l" output, and you can even add the directory flag there with "d" like "ls" does. You could also have PERM_0444 and similar for those who are more at ease with the octal numers without defining the few ones that are definitely wrong, as a safety belt. > > That being said, I do like them much better than the macros we have today, which > > I always have to go and look up every time I see them... > > Same here! Same for me. I never use S_I* and never know where to look for their definitions when I see them. Willy ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH] Add file permission mode helpers @ 2016-08-03 9:21 ` Willy Tarreau 0 siblings, 0 replies; 28+ messages in thread From: Willy Tarreau @ 2016-08-03 9:21 UTC (permalink / raw) To: linux-arm-kernel On Wed, Aug 03, 2016 at 10:39:03AM +0200, Ingo Molnar wrote: > > > 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. > > > > Because of that, odds are people will just stick to the octal numbers, > > because they think they want something other than the ones you defined > > for foolish reasons :) > > For code I maintain I'd insist on contributors using the human readable versions, > because in the past I've mixed up octals (and the symbolic helpers we have today) > myself and I find the 'ls -l' format much easier to read because that's the > primary file permission format I see every day working on code. FWIW, the only "human readable" ones for me are the octal ones, which are also the same as those I'm using every day with "chmod" or "find" and that I find hard to get wrong. But I agree that the PERM_* idea above are a nice alternative since they match the "ls -l" output, and you can even add the directory flag there with "d" like "ls" does. You could also have PERM_0444 and similar for those who are more at ease with the octal numers without defining the few ones that are definitely wrong, as a safety belt. > > That being said, I do like them much better than the macros we have today, which > > I always have to go and look up every time I see them... > > Same here! Same for me. I never use S_I* and never know where to look for their definitions when I see them. Willy ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Add file permission mode helpers 2016-08-03 8:28 ` Greg Kroah-Hartman @ 2016-08-03 9:53 ` Marcel Holtmann -1 siblings, 0 replies; 28+ messages in thread From: Marcel Holtmann @ 2016-08-03 9:53 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Ingo Molnar, Linus Torvalds, Pavel Machek, Heiko Carstens, Baole Ni, Russell King - ARM Linux, linux-arm-kernel, Linux Kernel Mailing List, chuansheng.liu Hi Greg, >> 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? > > I've tried to deal with that in the past with the __ATTR_RW() and > __ATTR_RO() and __ATTR_WO() macros that more should be using. I swept > the tree a few years ago to try to fix up most of them, but I know I > didn't catch them all, and more files have been added since then. I said in another response that maybe module_param_rw and module_param_ro will make some sense. Not sure if they are easier to read or not. I mean for each usage, we could look at the tree and see what values are actually used. My bet is that for module_param only a few ones are used. I have the feeling it is 0444 or 0644 and nothing else. Maybe some outlaws with 0400 and 0600 that don't even need to be that secretive. Regards Marcel ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH] Add file permission mode helpers @ 2016-08-03 9:53 ` Marcel Holtmann 0 siblings, 0 replies; 28+ messages in thread From: Marcel Holtmann @ 2016-08-03 9:53 UTC (permalink / raw) To: linux-arm-kernel Hi Greg, >> 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? > > I've tried to deal with that in the past with the __ATTR_RW() and > __ATTR_RO() and __ATTR_WO() macros that more should be using. I swept > the tree a few years ago to try to fix up most of them, but I know I > didn't catch them all, and more files have been added since then. I said in another response that maybe module_param_rw and module_param_ro will make some sense. Not sure if they are easier to read or not. I mean for each usage, we could look at the tree and see what values are actually used. My bet is that for module_param only a few ones are used. I have the feeling it is 0444 or 0644 and nothing else. Maybe some outlaws with 0400 and 0600 that don't even need to be that secretive. Regards Marcel ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Add file permission mode helpers 2016-08-03 8:11 ` Ingo Molnar @ 2016-08-03 15:49 ` Joe Perches -1 siblings, 0 replies; 28+ messages in thread From: Joe Perches @ 2016-08-03 15:49 UTC (permalink / raw) To: Ingo Molnar, Linus Torvalds Cc: Pavel Machek, Greg Kroah-Hartman, Heiko Carstens, Baole Ni, Russell King - ARM Linux, linux-arm-kernel, Linux Kernel Mailing List, chuansheng.liu On Wed, 2016-08-03 at 10:11 +0200, Ingo Molnar wrote: > * Linus Torvalds <torvalds@linux-foundation.org> 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@ucw.cz> 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 [etc] While the proposed PERM_ variants are easily read, using a single style instead of 2+ incompatible symbolic styles makes treewide misuse identification via grep style tools easier. ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH] Add file permission mode helpers @ 2016-08-03 15:49 ` Joe Perches 0 siblings, 0 replies; 28+ messages in thread From: Joe Perches @ 2016-08-03 15:49 UTC (permalink / raw) To: linux-arm-kernel On Wed, 2016-08-03 at 10:11 +0200, Ingo Molnar wrote: > * Linus Torvalds <torvalds@linux-foundation.org> 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@ucw.cz> 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 [etc] While the proposed PERM_ variants are easily read, using a single style instead of 2+ incompatible symbolic styles makes treewide misuse identification via grep style tools easier. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Add file permission mode helpers 2016-08-03 8:11 ` Ingo Molnar @ 2016-08-03 16:38 ` Pavel Machek -1 siblings, 0 replies; 28+ messages in thread From: Pavel Machek @ 2016-08-03 16:38 UTC (permalink / raw) To: Ingo Molnar Cc: Linus Torvalds, Greg Kroah-Hartman, Heiko Carstens, Baole Ni, Russell King - ARM Linux, linux-arm-kernel, Linux Kernel Mailing List, chuansheng.liu On Wed 2016-08-03 10:11:40, Ingo Molnar wrote: > > * Linus Torvalds <torvalds@linux-foundation.org> 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@ucw.cz> 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 I see 0400 and 0444 making sense, but does 0440 really make sense? I assume it will be uid/gid 0/0? Is gid 0 really estabilished well enough to give it special permissions? And yes, these macros actually help readability. > PERM__wx______ 0300 > PERM__wx_wx___ 0330 > PERM__wx_wx_wx 0333 Uh. This is for sysfs. Do we event want any __x variants? _wx would certainly be strange. (And yes, we can keep people from using strange permissions by simply not defining those macros.) > 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? I see two bugs but only one hole. How can you exploit s-bit without corresponding x-bit? I'd delete these: I don't think we should encourage their use: > +#define PERM_r__r_____ 0440 > +#define PERM_rw_r_____ 0640 > +#define PERM_rw_rw_r__ 0664 > + > +#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 Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH] Add file permission mode helpers @ 2016-08-03 16:38 ` Pavel Machek 0 siblings, 0 replies; 28+ messages in thread From: Pavel Machek @ 2016-08-03 16:38 UTC (permalink / raw) To: linux-arm-kernel On Wed 2016-08-03 10:11:40, Ingo Molnar wrote: > > * Linus Torvalds <torvalds@linux-foundation.org> 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@ucw.cz> 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 I see 0400 and 0444 making sense, but does 0440 really make sense? I assume it will be uid/gid 0/0? Is gid 0 really estabilished well enough to give it special permissions? And yes, these macros actually help readability. > PERM__wx______ 0300 > PERM__wx_wx___ 0330 > PERM__wx_wx_wx 0333 Uh. This is for sysfs. Do we event want any __x variants? _wx would certainly be strange. (And yes, we can keep people from using strange permissions by simply not defining those macros.) > 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? I see two bugs but only one hole. How can you exploit s-bit without corresponding x-bit? I'd delete these: I don't think we should encourage their use: > +#define PERM_r__r_____ 0440 > +#define PERM_rw_r_____ 0640 > +#define PERM_rw_rw_r__ 0664 > + > +#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 Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2016-08-15 16:38 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-02 20:58 Please don't replace numeric parameter like 0444 with macro Linus Torvalds 2016-08-02 20:58 ` Linus Torvalds 2016-08-02 21:53 ` Rob Landley 2016-08-02 21:53 ` Rob Landley 2016-08-02 23:39 ` [PATCH] checkpatch: Look for symbolic permissions and suggest octal instead Joe Perches 2016-08-03 0:15 ` Al Viro 2016-08-03 0:30 ` Joe Perches 2016-08-15 16:38 ` Joe Perches 2016-08-03 0:42 ` Please don't replace numeric parameter like 0444 with macro Al Viro 2016-08-03 0:42 ` Al Viro 2016-08-03 8:07 ` Konstantin Khlebnikov 2016-08-03 8:07 ` Konstantin Khlebnikov 2016-08-03 8:30 ` Richard Weinberger 2016-08-03 8:30 ` Richard Weinberger 2016-08-03 8:11 ` [PATCH] Add file permission mode helpers Ingo Molnar 2016-08-03 8:11 ` Ingo Molnar 2016-08-03 8:28 ` Greg Kroah-Hartman 2016-08-03 8:28 ` Greg Kroah-Hartman 2016-08-03 8:39 ` Ingo Molnar 2016-08-03 8:39 ` Ingo Molnar 2016-08-03 9:21 ` Willy Tarreau 2016-08-03 9:21 ` Willy Tarreau 2016-08-03 9:53 ` Marcel Holtmann 2016-08-03 9:53 ` Marcel Holtmann 2016-08-03 15:49 ` Joe Perches 2016-08-03 15:49 ` Joe Perches 2016-08-03 16:38 ` Pavel Machek 2016-08-03 16:38 ` Pavel Machek
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.