linux-kernel - Re: [PATCH] checkpatch: validate commit tag ordering

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <51eab1fa-5ea6-41f2-8b5b-fc5c16a9bca3@oss.qualcomm.com>
Date: Thu, 24 Jul 2025 12:10:41 -0700
From: Jeff Johnson <jeff.johnson@....qualcomm.com>
To: Hendrik Hamerlinck <hendrik.hamerlinck@...mernet.be>,
        dwaipayanray1@...il.com, lukas.bulwahn@...il.com, joe@...ches.com,
        corbet@....net, apw@...onical.com
Cc: skhan@...uxfoundation.org, linux-kernel-mentees@...ts.linux.dev,
        workflows@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] checkpatch: validate commit tag ordering

On 7/24/2025 12:20 AM, Hendrik Hamerlinck wrote:
> Modified the checkpatch script to ensure that commit tags (e.g.,
> Signed-off-by, Reviewed-by, Acked-by, Tested-by, etc.) appear in the
> correct order according to kernel conventions [1].
> 
> checkpatch.pl will now emit a BAD_TAG_ORDER warning when tags are out of
> the expected sequence. Multiple tags of the same type are allowed, but
> they must also follow the order. 'Link:' tags in the changelog are still
> allowed before the tag sequence begins, but once the sequence has started,
> any 'Link:' tags must follow the ordered commit tags. 
> 
> Link: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#ordering-of-commit-tags # [1]
> 
> Signed-off-by: Hendrik Hamerlinck <hendrik.hamerlinck@...mernet.be>
> ---
>  Documentation/dev-tools/checkpatch.rst |  6 ++++
>  scripts/checkpatch.pl                  | 40 ++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index 76bd0ddb0041..696b42bf4ff5 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -599,6 +599,12 @@ Commit message
>  
>      See: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
>  
> +  **BAD_TAG_ORDER**
> +    The tags in the commit message are not in the correct order according to
> +    community conventions. Common tags like Signed-off-by, Reviewed-by,
> +    Tested-by, Acked-by, Fixes, Cc, etc., should follow a standardized sequence.
> +
> +    See: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#ordering-of-commit-tags
>  
>  Comparison style
>  ----------------
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 664f7b7a622c..267ec02de9ec 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -661,6 +661,24 @@ foreach my $entry (@link_tags) {
>  }
>  $link_tags_search = "(?:${link_tags_search})";
>  
> +# Ordered commit tags
> +our @commit_tags = (
> +	"Fixes:",
> +	"Reported-by:",
> +	"Closes:",
> +	"Originally-by:",
> +	"Suggested-by:",
> +	"Co-developed-by:",
> +	"Signed-off-by:",
> +	"Tested-by:",
> +	"Reviewed-by",
> +	"Acked-by:",
> +	"Cc:",
> +	"Link:"
> +);
> +our $commit_tag_pattern = join '|', map { quotemeta($_) } @commit_tags;
> +our $commit_tags_regex = qr{(?xi: ^\s*($commit_tag_pattern))};
> +
>  our $tracing_logging_tags = qr{(?xi:
>  	[=-]*> |
>  	<[=-]* |
> @@ -2712,6 +2730,8 @@ sub process {
>  
>  	my $checklicenseline = 1;
>  
> +	my $last_matched_tag;
> +
>  	sanitise_line_reset();
>  	my $line;
>  	foreach my $rawline (@rawlines) {
> @@ -3258,6 +3278,26 @@ sub process {
>  			}
>  		}
>  
> +# Check commit tags sorting
> +		if (!$in_header_lines && $line =~ $commit_tags_regex) {
> +			my $tag = $1;
> +			my ($tag_index) = grep { lc($commit_tags[$_]) eq lc($tag) } 0..$#commit_tags;
> +
> +			if ($last_matched_tag &&
> +			    $last_matched_tag->{tag_index} > $tag_index) {
> +				WARN("BAD_TAG_ORDER",
> +				     "Tag '$tag' is out of order. Should come before '$last_matched_tag->{tag}'\n" . $herecurr);
> +			}
> +
> +			# Allow link tags to occur before the commit tags
> +			if (lc($tag) ne "link:" || defined $last_matched_tag) {
> +				$last_matched_tag = {
> +					tag       => $tag,
> +					tag_index => $tag_index,
> +				};
> +			}
> +		}
> +
>  # Check email subject for common tools that don't need to be mentioned
>  		if ($in_header_lines &&
>  		    $line =~ /^Subject:.*\b(?:checkpatch|sparse|smatch)\b[^:]/i) {

Seems this logic would fail when there are multiple reporters, such as in
commit 9a44b5e36cd699fdd2150a63fab225ac510c1971

Fixes: 49e47223ecc4 ("wifi: cfg80211: allocate memory for link_station info structure")
Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
Closes: https://lore.kernel.org/all/81f30515-a83d-4b05-a9d1-e349969df9e9@sabinyo.mountain/
Reported-by: syzbot+4ba6272678aa468132c8@...kaller.appspotmail.com
Closes: https://lore.kernel.org/all/68655325.a70a0220.5d25f.0316.GAE@google.com