[PATCH] checkpatch: check for missing Fixes tags

[PATCH] checkpatch: check for missing Fixes tags

[Dan Carpenter]

This check looks for common words that probably indicate a patch
is a fix.  For now the regex is:

	(BUG: KASAN|Call Trace:|syzkaller|stable\@)

Why are stable patches encouraged to have a fixes tag?  Some people mark
their stable patches as "# 5.10" etc.  This is not as useful as a Fixes
tag.  The Fixes tag helps in review.  It helps people to not cherry-pick
buggy patches without also cherry-picking the fix.

Also if a bug affects the 5.7 kernel some people will round it up to
5.10+ because 5.7 is not supported on kernel.org.  It's possible the Bad
Binder bug was caused by this sort of gap where companies outside of
kernel.org are supporting different kernels from kernel.org?

Should it be counted as a Fix when a patch just silences harmless
WARN_ON() stack trace.  Yes.  Definitely.

Is silencing compiler warnings a fix?  It seems unfair to the original
authors, but we use -Werror now, and warnings break the build so let's
just add Fixes tags for those.  I tell people that silencing static
checker warnings is not a fix but the rules on this vary by subsystem.

Is fixing a minor LTP issue (Linux Test Project) a fix?  Probably?  It's
hard to know what to do if the LTP test has technically always been
broken.

One clear false positive from this check is when a patch updated the
debug output and the commit message included before and after Call
Traces.  Sometimes you should just ignore checkpatch.

Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
I tested this by looking at the latest 500 commits in linux-next.
93 commits had Fixes tags.  Out of the remaining 407 commits then this
warning said that 9 of them should have had Fixes tags.

Of course the big rule change here is encouraging all stable@kernel.org
patches to add a Fix.  If everyone followed this checkpatch rule then
instead of 65% of stable patches having a Fixes tag it would be 75%.
(The stable tree includes a lot of other patches besides Fixes like
Stable-dep: patches etc, so it should never be 100%).

 scripts/checkpatch.pl | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 30b0b4fdb3bf..4e68de51e480 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -28,6 +28,7 @@ my %verbose_messages = ();
 my %verbose_emitted = ();
 my $tree = 1;
 my $chk_signoff = 1;
+my $chk_fixes_tag = 1;
 my $chk_patch = 1;
 my $tst_only;
 my $emacs = 0;
@@ -86,6 +87,7 @@ Options:
   -v, --verbose              verbose mode
   --no-tree                  run without a kernel tree
   --no-signoff               do not check for 'Signed-off-by' line
+  --no-fixes-tag             do not check for 'Fixes:' tag
   --patch                    treat FILE as patchfile (default)
   --emacs                    emacs compile window format
   --terse                    one line per report
@@ -293,6 +295,7 @@ GetOptions(
 	'v|verbose!'	=> \$verbose,
 	'tree!'		=> \$tree,
 	'signoff!'	=> \$chk_signoff,
+	'fixes-tag!'	=> \$chk_fixes_tag,
 	'patch!'	=> \$chk_patch,
 	'emacs!'	=> \$emacs,
 	'terse!'	=> \$terse,
@@ -1254,6 +1257,7 @@ sub git_commit_info {
 }
 
 $chk_signoff = 0 if ($file);
+$chk_fixes_tag = 0 if ($file);
 
 my @rawlines = ();
 my @lines = ();
@@ -2633,6 +2637,8 @@ sub process {
 
 	our $clean = 1;
 	my $signoff = 0;
+	my $fixes_tag = 0;
+	my $needs_fixes_tag = 0;
 	my $author = '';
 	my $authorsignoff = 0;
 	my $author_sob = '';
@@ -3186,6 +3192,12 @@ sub process {
 			}
 		}
 
+# These indicate a bug fix
+		if (!$in_header_lines &&
+			$line =~ /(BUG: KASAN|Call Trace:|syzkaller|stable\@)/) {
+			$needs_fixes_tag++;
+		}
+
 
 # Check Fixes: styles is correct
 		if (!$in_header_lines &&
@@ -3198,6 +3210,7 @@ sub process {
 			my $id_length = 1;
 			my $id_case = 1;
 			my $title_has_quotes = 0;
+			$fixes_tag++;
 
 			if ($line =~ /(\s*fixes:?)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {
 				my $tag = $1;
@@ -7636,6 +7649,12 @@ sub process {
 		ERROR("NOT_UNIFIED_DIFF",
 		      "Does not appear to be a unified-diff format patch\n");
 	}
+	if ($is_patch && $has_commit_log && $chk_fixes_tag) {
+		if ($needs_fixes_tag && $fixes_tag == 0) {
+			ERROR("MISSING_FIXES_TAG",
+			"This looks like a fix but there is no Fixes: tag\n");
+		}
+	}
 	if ($is_patch && $has_commit_log && $chk_signoff) {
 		if ($signoff == 0) {
 			ERROR("MISSING_SIGN_OFF",
-- 
2.39.2

Re: [PATCH] checkpatch: check for missing Fixes tags

[Greg Kroah-Hartman]

On Tue, Jun 06, 2023 at 11:30:27AM +0300, Dan Carpenter wrote:
> This check looks for common words that probably indicate a patch
> is a fix.  For now the regex is:
> 
> 	(BUG: KASAN|Call Trace:|syzkaller|stable\@)
> 
> Why are stable patches encouraged to have a fixes tag?  Some people mark
> their stable patches as "# 5.10" etc.  This is not as useful as a Fixes
> tag.  The Fixes tag helps in review.  It helps people to not cherry-pick
> buggy patches without also cherry-picking the fix.
> 
> Also if a bug affects the 5.7 kernel some people will round it up to
> 5.10+ because 5.7 is not supported on kernel.org.  It's possible the Bad
> Binder bug was caused by this sort of gap where companies outside of
> kernel.org are supporting different kernels from kernel.org?
> 
> Should it be counted as a Fix when a patch just silences harmless
> WARN_ON() stack trace.  Yes.  Definitely.
> 
> Is silencing compiler warnings a fix?  It seems unfair to the original
> authors, but we use -Werror now, and warnings break the build so let's
> just add Fixes tags for those.  I tell people that silencing static
> checker warnings is not a fix but the rules on this vary by subsystem.
> 
> Is fixing a minor LTP issue (Linux Test Project) a fix?  Probably?  It's
> hard to know what to do if the LTP test has technically always been
> broken.
> 
> One clear false positive from this check is when a patch updated the
> debug output and the commit message included before and after Call
> Traces.  Sometimes you should just ignore checkpatch.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---

Nice!

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH] checkpatch: check for missing Fixes tags

[Thorsten Leemhuis]

On 06.06.23 10:30, Dan Carpenter wrote:
> This check looks for common words that probably indicate a patch
> is a fix.  For now the regex is:
> 
> 	(BUG: KASAN|Call Trace:|syzkaller|stable\@)

Just wondering: should "regression" be in that list?

Ciao, Thorsten

Re: [PATCH] checkpatch: check for missing Fixes tags

[Joe Perches]

On Tue, 2023-06-06 at 11:30 +0300, Dan Carpenter wrote:
> This check looks for common words that probably indicate a patch
> is a fix.  For now the regex is:
> 
> 	(BUG: KASAN|Call Trace:|syzkaller|stable\@)

Seems sensible.  Style notes:


> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -3186,6 +3192,12 @@ sub process {
>  			}
>  		}
>  
> +# These indicate a bug fix
> +		if (!$in_header_lines &&
> +			$line =~ /(BUG: KASAN|Call Trace:|syzkaller|stable\@)/) {
> +			$needs_fixes_tag++;

Align to open parenthesis please.
Maybe add "Closes:"
This should not check any actual patch lines.

So this should likely use

		if (!$in_header_lines && !$is_patch &&
		    $line =~ /\b(?:BUG: KASAN|Call Trace:|Closes:|syzkaller|stable\@)/) {

There's no use of $needs_fixes_tag as something
other than a bool, so please just use

			$needs_fixes_tag = 1;

> @@ -3198,6 +3210,7 @@ sub process {
>  			my $id_length = 1;
>  			my $id_case = 1;
>  			my $title_has_quotes = 0;
> +			$fixes_tag++;

			$fixes_tag = 1;

here too

> @@ -7636,6 +7649,12 @@ sub process {
>  		ERROR("NOT_UNIFIED_DIFF",
>  		      "Does not appear to be a unified-diff format patch\n");
>  	}
> +	if ($is_patch && $has_commit_log && $chk_fixes_tag) {
> +		if ($needs_fixes_tag && $fixes_tag == 0) {

		if ($needs_fixes_tag && !$fixes_tag) {

> +			ERROR("MISSING_FIXES_TAG",
> +			"This looks like a fix but there is no Fixes: tag\n");

Alignment to open parenthesis and likely WARN not ERROR

Re: [PATCH] checkpatch: check for missing Fixes tags

[Dan Carpenter]

Thanks, Joe.  All that is reasonable.

regards,
dan carpenter