Skip to content

Commit

Permalink
checkpatch: check for missing Fixes tags
Browse files Browse the repository at this point in the history
This check looks for common words that probably indicate a patch
is a fix.  For now the regex is:

	(?:(?:BUG: K.|UB)SAN: |Call Trace:|stable\@|syzkaller)/)

Why are stable patches encouraged to have a fixes tag?  Some people mark
their stable patches as "# 5.10" etc.  This is useful but a Fixes tag is
still a good idea.  For example, 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.  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 someone updated their
debug output and included before and after Call Traces.  Or when crashes
are introduced deliberately for testing.  In those cases, you should
just ignore checkpatch.

Link: https://lkml.kernel.org/r/ZmhUgZBKeF_8ixA6@moroto
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Dwaipayan Ray <dwaipayanray1@gmail.com>
Cc: Joe Perches <joe@perches.com>
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Cc: Sasha Levin <sashal@kernel.org>
Cc: Thorsten Leemhuis <linux@leemhuis.info>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
  • Loading branch information
Dan Carpenter authored and akpm00 committed Jun 25, 2024
1 parent d6bb395 commit d5d6281
Showing 1 changed file with 24 additions and 0 deletions.
24 changes: 24 additions & 0 deletions scripts/checkpatch.pl
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
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;
Expand Down Expand Up @@ -88,6 +89,7 @@ sub help {
-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
Expand Down Expand Up @@ -295,6 +297,7 @@ sub load_docs {
'v|verbose!' => \$verbose,
'tree!' => \$tree,
'signoff!' => \$chk_signoff,
'fixes-tag!' => \$chk_fixes_tag,
'patch!' => \$chk_patch,
'emacs!' => \$emacs,
'terse!' => \$terse,
Expand Down Expand Up @@ -1257,6 +1260,7 @@ sub git_commit_info {
}

$chk_signoff = 0 if ($file);
$chk_fixes_tag = 0 if ($file);

my @rawlines = ();
my @lines = ();
Expand Down Expand Up @@ -2636,6 +2640,9 @@ sub process {

our $clean = 1;
my $signoff = 0;
my $fixes_tag = 0;
my $is_revert = 0;
my $needs_fixes_tag = "";
my $author = '';
my $authorsignoff = 0;
my $author_sob = '';
Expand Down Expand Up @@ -3189,6 +3196,16 @@ sub process {
}
}

# These indicate a bug fix
if (!$in_header_lines && !$is_patch &&
$line =~ /^This reverts commit/) {
$is_revert = 1;
}

if (!$in_header_lines && !$is_patch &&
$line =~ /((?:(?:BUG: K.|UB)SAN: |Call Trace:|stable\@|syzkaller))/) {
$needs_fixes_tag = $1;
}

# Check Fixes: styles is correct
if (!$in_header_lines &&
Expand All @@ -3201,6 +3218,7 @@ sub process {
my $id_length = 1;
my $id_case = 1;
my $title_has_quotes = 0;
$fixes_tag = 1;

if ($line =~ /(\s*fixes:?)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {
my $tag = $1;
Expand Down Expand Up @@ -7697,6 +7715,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 ne "" && !$is_revert && !$fixes_tag) {
WARN("MISSING_FIXES_TAG",
"The commit message has '$needs_fixes_tag', perhaps it also needs a 'Fixes:' tag?\n");
}
}
if ($is_patch && $has_commit_log && $chk_signoff) {
if ($signoff == 0) {
ERROR("MISSING_SIGN_OFF",
Expand Down

0 comments on commit d5d6281

Please sign in to comment.