| Message ID | 20231026-ethtool_puts_impl-v2-2-0d67cbdd0538@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ethtool: Add ethtool_puts() | expand |
On Thu, Oct 26, 2023 at 09:56:08PM +0000, Justin Stitt wrote: > Add some warnings for using ethtool_sprintf() where a simple > ethtool_puts() would suffice. > > The two cases are: > > 1) Use ethtool_sprintf() with just two arguments: > | ethtool_sprintf(&data, driver[i].name); > or > 2) Use ethtool_sprintf() with a standalone "%s" fmt string: > | ethtool_sprintf(&data, "%s", driver[i].name); > > The former may cause -Wformat-security warnings while the latter is just > not preferred. Both are safely in the category of warnings, not errors. > > Signed-off-by: Justin Stitt <justinstitt@google.com> > --- > scripts/checkpatch.pl | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 25fdb7fda112..22f007131337 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -7011,6 +7011,25 @@ sub process { > "Prefer strscpy, strscpy_pad, or __nonstring over strncpy - see: https://github.com/KSPP/linux/issues/90\n" . $herecurr); > } > > +# ethtool_sprintf uses that should likely be ethtool_puts > + if ($line =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) { > + if(WARN("ETHTOOL_SPRINTF", > + "Prefer ethtool_puts over ethtool_sprintf with only two arguments\n" . $herecurr) && > + $fix) { > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(/ethtool_puts\(/; > + } > + } > + > + # use $rawline because $line loses %s via sanitization and thus we can't match against it. > + if ($rawline =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/) { > + if(WARN("ETHTOOL_SPRINTF", > + "Prefer ethtool_puts over ethtool_sprintf with standalone \"%s\" specifier\n" . $herecurr) && > + $fix) { > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(\s*(.*?),.*?,(.*?)\)/ethtool_puts\($1,$2)/; > + } > + } > + > + > # typecasts on min/max could be min_t/max_t > if ($perl_version_ok && > defined $stat && > > -- > 2.42.0.820.g83a721a137-goog > I don't really know Perl, but does the indentation and coding style here conform to any rules, or is it just free-form? The rest of the script looks almost as you'd expect from C. This is unreadable to me.
On Thu, Oct 26, 2023 at 3:12 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > On Thu, Oct 26, 2023 at 09:56:08PM +0000, Justin Stitt wrote: > > Add some warnings for using ethtool_sprintf() where a simple > > ethtool_puts() would suffice. > > > > The two cases are: > > > > 1) Use ethtool_sprintf() with just two arguments: > > | ethtool_sprintf(&data, driver[i].name); > > or > > 2) Use ethtool_sprintf() with a standalone "%s" fmt string: > > | ethtool_sprintf(&data, "%s", driver[i].name); > > > > The former may cause -Wformat-security warnings while the latter is just > > not preferred. Both are safely in the category of warnings, not errors. > > > > Signed-off-by: Justin Stitt <justinstitt@google.com> > > --- > > scripts/checkpatch.pl | 19 +++++++++++++++++++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > index 25fdb7fda112..22f007131337 100755 > > --- a/scripts/checkpatch.pl > > +++ b/scripts/checkpatch.pl > > @@ -7011,6 +7011,25 @@ sub process { > > "Prefer strscpy, strscpy_pad, or __nonstring over strncpy - see: https://github.com/KSPP/linux/issues/90\n" . $herecurr); > > } > > > > +# ethtool_sprintf uses that should likely be ethtool_puts > > + if ($line =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) { > > + if(WARN("ETHTOOL_SPRINTF", > > + "Prefer ethtool_puts over ethtool_sprintf with only two arguments\n" . $herecurr) && > > + $fix) { > > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(/ethtool_puts\(/; > > + } > > + } > > + > > + # use $rawline because $line loses %s via sanitization and thus we can't match against it. > > + if ($rawline =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/) { > > + if(WARN("ETHTOOL_SPRINTF", > > + "Prefer ethtool_puts over ethtool_sprintf with standalone \"%s\" specifier\n" . $herecurr) && > > + $fix) { > > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(\s*(.*?),.*?,(.*?)\)/ethtool_puts\($1,$2)/; > > + } > > + } > > + > > + > > # typecasts on min/max could be min_t/max_t > > if ($perl_version_ok && > > defined $stat && > > > > -- > > 2.42.0.820.g83a721a137-goog > > > > I don't really know Perl, but does the indentation and coding style here > conform to any rules, or is it just free-form? The rest of the script > looks almost as you'd expect from C. This is unreadable to me. There was some discussion here [1] but AFAICT I need to use EMACS or configure my vim in a very particular way to get the same formatting But yeah, look around line 7000 -- lots of this pattern matching code is pretty hard to read. Not sure there's much to be done as far as readability is concerned. [1]: https://lore.kernel.org/all/137a309b313cc8a295f3affc704f0da049f233aa.camel@perches.com/ Thanks Justin
On Thu, Oct 26, 2023 at 03:24:54PM -0700, Justin Stitt wrote: > There was some discussion here [1] but AFAICT I need to use EMACS > or configure my vim in a very particular way to get the same formatting > > But yeah, look around line 7000 -- lots of this pattern matching code is > pretty hard to read. Not sure there's much to be done as far as readability > is concerned. > > [1]: https://lore.kernel.org/all/137a309b313cc8a295f3affc704f0da049f233aa.camel@perches.com/ Hard to read because of pattern matching is one thing, but your indentation is unlike anything else in this file. There are inner curly brackets which are less indented than the outer curly brackets. I cannot read/review this, sorry, I hope somebody else can.
On Thu, 2023-10-26 at 21:56 +0000, Justin Stitt wrote: > Add some warnings for using ethtool_sprintf() where a simple > ethtool_puts() would suffice. [] > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > @@ -7011,6 +7011,25 @@ sub process { > "Prefer strscpy, strscpy_pad, or __nonstring over strncpy - see: https://github.com/KSPP/linux/issues/90\n" . $herecurr); > } > > +# ethtool_sprintf uses that should likely be ethtool_puts > + if ($line =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) { > + if(WARN("ETHTOOL_SPRINTF", > + "Prefer ethtool_puts over ethtool_sprintf with only two arguments\n" . $herecurr) && > + $fix) { > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(/ethtool_puts\(/; > + } > + } > + > + # use $rawline because $line loses %s via sanitization and thus we can't match against it. > + if ($rawline =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/) { > + if(WARN("ETHTOOL_SPRINTF", > + "Prefer ethtool_puts over ethtool_sprintf with standalone \"%s\" specifier\n" . $herecurr) && > + $fix) { > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(\s*(.*?),.*?,(.*?)\)/ethtool_puts\($1,$2)/; Thanks, but: This fix wouldn't work if the first argument was itself a function with multiple arguments. And this is whitespace formatted incorrectly. It: o needs a space after if o needs a space after the comma in the fix o needs to use the appropriate amount and tabs for indentation o needs alignment to open parentheses o the backslashes are not required in the fix block Do you want me to push what I wrote in the link below? https://lore.kernel.org/lkml/7eec92d9e72d28e7b5202f41b02a383eb28ddd26.camel@perches.com/ > + } > + } > + > + > # typecasts on min/max could be min_t/max_t > if ($perl_version_ok && > defined $stat && >
On Thu, Oct 26, 2023 at 3:39 PM Joe Perches <joe@perches.com> wrote: > > On Thu, 2023-10-26 at 21:56 +0000, Justin Stitt wrote: > > Add some warnings for using ethtool_sprintf() where a simple > > ethtool_puts() would suffice. > [] > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > [] > > @@ -7011,6 +7011,25 @@ sub process { > > "Prefer strscpy, strscpy_pad, or __nonstring over strncpy - see: https://github.com/KSPP/linux/issues/90\n" . $herecurr); > > } > > > > +# ethtool_sprintf uses that should likely be ethtool_puts > > + if ($line =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) { > > + if(WARN("ETHTOOL_SPRINTF", > > + "Prefer ethtool_puts over ethtool_sprintf with only two arguments\n" . $herecurr) && > > + $fix) { > > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(/ethtool_puts\(/; > > + } > > + } > > + > > + # use $rawline because $line loses %s via sanitization and thus we can't match against it. > > + if ($rawline =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/) { > > + if(WARN("ETHTOOL_SPRINTF", > > + "Prefer ethtool_puts over ethtool_sprintf with standalone \"%s\" specifier\n" . $herecurr) && > > + $fix) { > > + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(\s*(.*?),.*?,(.*?)\)/ethtool_puts\($1,$2)/; > > Thanks, but: > > This fix wouldn't work if the first argument was itself a function > with multiple arguments. > > And this is whitespace formatted incorrectly. > > It: > > o needs a space after if > o needs a space after the comma in the fix > o needs to use the appropriate amount and tabs for indentation > o needs alignment to open parentheses > o the backslashes are not required in the fix block > > Do you want me to push what I wrote in the link below? > https://lore.kernel.org/lkml/7eec92d9e72d28e7b5202f41b02a383eb28ddd26.camel@perches.com/ Ah, I didn't see you provided a diff in previous thread. Yeah you can push it but it's not really a standalone so perhaps I'll just steal the diff and wrap into v3? > > > + } > > + } > > + > > + > > # typecasts on min/max could be min_t/max_t > > if ($perl_version_ok && > > defined $stat && > > > Thanks Justin
On 2023-10-27 12:40, Justin Stitt wrote: > Yeah you can push it but it's not really a standalone so perhaps I'll > just steal the diff and > wrap into v3? Fine by me. No need for my sign off.
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 25fdb7fda112..22f007131337 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -7011,6 +7011,25 @@ sub process { "Prefer strscpy, strscpy_pad, or __nonstring over strncpy - see: https://github.com/KSPP/linux/issues/90\n" . $herecurr); } +# ethtool_sprintf uses that should likely be ethtool_puts + if ($line =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) { + if(WARN("ETHTOOL_SPRINTF", + "Prefer ethtool_puts over ethtool_sprintf with only two arguments\n" . $herecurr) && + $fix) { + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(/ethtool_puts\(/; + } + } + + # use $rawline because $line loses %s via sanitization and thus we can't match against it. + if ($rawline =~ /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/) { + if(WARN("ETHTOOL_SPRINTF", + "Prefer ethtool_puts over ethtool_sprintf with standalone \"%s\" specifier\n" . $herecurr) && + $fix) { + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(\s*(.*?),.*?,(.*?)\)/ethtool_puts\($1,$2)/; + } + } + + # typecasts on min/max could be min_t/max_t if ($perl_version_ok && defined $stat &&
Add some warnings for using ethtool_sprintf() where a simple ethtool_puts() would suffice. The two cases are: 1) Use ethtool_sprintf() with just two arguments: | ethtool_sprintf(&data, driver[i].name); or 2) Use ethtool_sprintf() with a standalone "%s" fmt string: | ethtool_sprintf(&data, "%s", driver[i].name); The former may cause -Wformat-security warnings while the latter is just not preferred. Both are safely in the category of warnings, not errors. Signed-off-by: Justin Stitt <justinstitt@google.com> --- scripts/checkpatch.pl | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)