From 0bb6f945fa95857d566759a108b1f5bc60186bdd Mon Sep 17 00:00:00 2001 From: Roxana Nicolescu Date: Wed, 3 Jun 2026 17:20:05 +0200 Subject: [PATCH] check_kernel_commits.py: Fix corner case around Fixes: commits (cve-bf) Original issue: https://github.com/ctrliq/kernel-src-tree/pull/1212#issuecomment-4421837799 In the above link, we had a weird case where commit X was reverted by commit Y, but then our cve tracker picked the X commit again (why is irrelevant to this story). check_kernel_commits.py did not detect any issues locally, but it did flag it during the validate step when the PR was created. The discrepancy is due to the fact that, locally, the whole pr/local branch is fetched, but during Pr reviews, only the new commits are fetches, hence the history is way limited, which is more than enough considering that the 'fixes:' (cve-bf) commits should be applied after the commit of interest. While the different results locally vs during Pr review make sense, we need to catch issues like this locally, during development as well. Because locally we cannot fetch just the new commits (we need the whole history to make sure we cherry pick commits properly), check_kernel_commits now checks the interval of commits it has to check. And since we ran into this weird case described at the beginning, flag a warnining in case a commit X has a cve-bf that was applied beforehand so that the developer sees it and acts accordingly and not be confused during Pr review. Signed-off-by: Roxana Nicolescu --- check_kernel_commits.py | 67 ++++++++++++++++++++++++++++++++++++- ciq-cherry-pick.py | 6 ++-- kt/ktlib/ciq_helpers.py | 73 +++++++++++++++++++++++++++++++++-------- 3 files changed, 130 insertions(+), 16 deletions(-) diff --git a/check_kernel_commits.py b/check_kernel_commits.py index 5a16864..8aca3fd 100644 --- a/check_kernel_commits.py +++ b/check_kernel_commits.py @@ -7,6 +7,7 @@ import textwrap from kt.ktlib.ciq_helpers import ( + CIQ_filter_unapplied_commits, CIQ_find_fixes_in_mainline, CIQ_find_matching_cve, CIQ_get_commit_body, @@ -126,6 +127,9 @@ def main(): any_findings = False out_lines = [] + # len(pr_commits) >= 1 + oldest_pr_commit = pr_commits[-1] + newest_pr_commit = pr_commits[0] for sha in reversed(pr_commits): # oldest first short_hash, subject = get_short_hash_and_subject(args.repo, sha) pr_commit_desc = f"{short_hash} ({subject})" @@ -155,7 +159,68 @@ def main(): ) out_lines.append("") # blank line continue - fixes = CIQ_find_fixes_in_mainline(args.repo, args.pr_branch, upstream_ref, uhash) + + # Find commits that contains Fixes: in upstream_ref + fixes = CIQ_find_fixes_in_mainline(repo=args.repo, upstream_ref=upstream_ref, hash_=uhash) + + # Filter the above commits if they were unapplied in the pr_branch (whole fetched history is used) + fixes_unapplied_without_limit = CIQ_filter_unapplied_commits( + repo=args.repo, pr_branch=args.pr_branch, commits=fixes + ) + + # Filter the above commits if they were unapplied in the pr_branch (oldest_pr_commit..newest_pr_commit interval is used) + fixes_unapplied_with_limit = CIQ_filter_unapplied_commits( + repo=args.repo, pr_branch=args.pr_branch, commits=fixes, interval=(oldest_pr_commit, newest_pr_commit) + ) + + # Issue a warning if we found that a commit that contains Fixes: has been applied before + # Very specific case, but check this PR where it did happen https://github.com/ctrliq/kernel-src-tree/pull/1212#issuecomment-4421837799 + # It is up to the developer to assess the situation if this happens. In the example case, a commit X was pushed, then it was reverted by commit Y, + # then commit X was pushed again but automation locally did not flag that it was reverted before because its dependency Y appeared to have been applied + # in the local tree. + fixes_applied_before_commit = set(fixes_unapplied_with_limit) - set(fixes_unapplied_without_limit) + if fixes_applied_before_commit: + # Build the fixes display text + fixes_lines = [] + for fix_hash, display_str in fixes_applied_before_commit: + fixes_lines.append(display_str) + + fixes_text = "\n".join(fixes_lines) + any_findings = True + if args.markdown: + fixes_block = " " + fixes_text.replace("\n", "\n ") + out_lines.append( + f"- ❗ PR commit `{pr_commit_desc}` references upstream commit \n" + f" `{short_uhash}` which has been referenced by a `Fixes:` tag in the upstream \n" + f" Linux kernel:\n\n" + f"```text\n{fixes_block}\n```\n" + " was applied after its deps, not before\n" + ) + else: + prefix = "[FIXES-WRONG-ORDER] " + header = ( + f"{prefix}PR commit {pr_commit_desc} references upstream commit " + f"{short_uhash}, which has Fixes tags:" + ) + out_lines.append( + wrap_paragraph( + header, width=80, initial_indent="", subsequent_indent=" " * len(prefix) + ) # spaces for '[FIXES-WRONG-ORDER] ' + ) + out_lines.append("") # blank line after 'Fixes tags:' + for line in fixes_text.splitlines(): + out_lines.append(" " + line) + + out_lines.append( + wrap_paragraph( + text="was applied after its deps, not before\n", + initial_indent=" " * len(prefix), + ) + ) + out_lines.append("") # blank line + + # We continue the usual checks for fixes that were unapplied after the commit of interest + fixes = fixes_unapplied_with_limit if fixes: any_findings = True diff --git a/ciq-cherry-pick.py b/ciq-cherry-pick.py index db670e6..3a7194b 100644 --- a/ciq-cherry-pick.py +++ b/ciq-cherry-pick.py @@ -12,7 +12,7 @@ from kt.ktlib.ciq_helpers import ( CIQ_cherry_pick_commit_standardization, CIQ_commit_exists_in_current_branch, - CIQ_find_fixes_in_mainline_current_branch, + CIQ_find_fixes_in_mainline_current_branch_unapplied, CIQ_find_matching_cve, CIQ_fixes_references, CIQ_get_full_hash, @@ -251,7 +251,9 @@ def cherry_pick_fixes( Fixes: . If any, these will also be cherry picked with the ciq tag = cve-bf. If the tag was cve-pre, it stays the same. """ - fixes_in_mainline = CIQ_find_fixes_in_mainline_current_branch(os.getcwd(), upstream_ref, sha) + fixes_in_mainline = CIQ_find_fixes_in_mainline_current_branch_unapplied( + repo=os.getcwd(), upstream_ref=upstream_ref, hash_=sha + ) # Replace cve with cve-bf # Leave cve-pre and cve-bf as they are diff --git a/kt/ktlib/ciq_helpers.py b/kt/ktlib/ciq_helpers.py index 8791770..7cb942d 100644 --- a/kt/ktlib/ciq_helpers.py +++ b/kt/ktlib/ciq_helpers.py @@ -229,21 +229,33 @@ def CIQ_hash_exists_in_ref(repo, pr_ref, hash_): return False -def CIQ_commit_exists_in_branch(repo, pr_branch, upstream_hash_): +def CIQ_commit_exists_in_branch(repo, pr_branch, upstream_hash_, interval=None): """ - Return True if upstream_hash_ has been backported and it exists in the pr branch + Return True if upstream_hash_ has been backported and it exists in the pr branch. + If interval that consists of a pair of commit hashes is not None, we only + check the interval[0]..interval[1], otherwise the whole history of the pr_branch will be used. """ # First check if the commit has been backported by CIQ - output = CIQ_run_git(repo, ["log", pr_branch, "--grep", "^commit " + upstream_hash_]) + git_command = ["log", pr_branch, "--grep", "^commit " + upstream_hash_] + if interval: + oldest_commit, newest_commit = interval + git_command.append(f"{oldest_commit}..{newest_commit}") + + output = CIQ_run_git(repo_path=repo, args=git_command) if output: return True + # If we are searching in the interval .., + # we do not need to check way back if the commit came from upstream as it is + if interval: + return False + # If it was not backported by CIQ, maybe it came from upstream as it is - return CIQ_hash_exists_in_ref(repo, pr_branch, upstream_hash_) + return CIQ_hash_exists_in_ref(repo=repo, pr_ref=pr_branch, hash_=upstream_hash_) -def CIQ_commit_exists_in_current_branch(repo, upstream_hash_): +def CIQ_commit_exists_in_current_branch(repo, upstream_hash_, interval=None): """ Return True if upstream_hash_ has been backported and it exists in the current branch """ @@ -251,13 +263,14 @@ def CIQ_commit_exists_in_current_branch(repo, upstream_hash_): current_branch = CIQ_get_current_branch(repo) full_upstream_hash = CIQ_get_full_hash(repo, upstream_hash_) - return CIQ_commit_exists_in_branch(repo, current_branch, full_upstream_hash) + return CIQ_commit_exists_in_branch( + repo=repo, pr_branch=current_branch, upstream_hash_=full_upstream_hash, interval=interval + ) -def CIQ_find_fixes_in_mainline(repo, pr_branch, upstream_ref, hash_): +def CIQ_find_fixes_in_mainline(repo, upstream_ref, hash_): """ - Return unique commits in upstream_ref that have Fixes: in their message, case-insensitive, - if they have not been committed in the pr_branch. + Return unique commits in upstream_ref that have Fixes: in their message, case-insensitive. Start from 12 chars and work down to 6, but do not include duplicates if already found at a longer length. Returns a list of tuples: (full_hash, display_string) """ @@ -295,17 +308,51 @@ def CIQ_find_fixes_in_mainline(repo, pr_branch, upstream_ref, hash_): for fix in fixes: for prefix in hash_prefixes: if fix.lower().startswith(prefix.lower()): - if not CIQ_commit_exists_in_branch(repo, pr_branch, full_hash): - results.append((full_hash, display_string)) + results.append((full_hash, display_string)) break return results -def CIQ_find_fixes_in_mainline_current_branch(repo, upstream_ref, hash_): +def CIQ_filter_unapplied_commits(repo, pr_branch, commits, interval=None): + """ + Receives a list of tuples: (full_hash, display_string) + Returns filtered list with commits that were not applied in the pr_branch. + If interval that consists of a pair of commit hashes is not None, we only + check the interval[0]..interval[1], otherwise the whole history of the pr_branch will be used. + + Returns a list of tuples: (full_hash, display_string) + """ + + results = [] + for commit in commits: + full_hash = commit[0] + if not CIQ_commit_exists_in_branch(repo=repo, pr_branch=pr_branch, upstream_hash_=full_hash, interval=interval): + results.append(commit) + + return results + + +def CIQ_find_fixes_in_mainline_unapplied(repo, pr_branch, upstream_ref, hash_, interval=None): + """ + Return unique commits in upstream_ref that have Fixes: in their message, case-insensitive, + if they have not been committed in the pr_branch. + If interval that consists of a pair of commit hashes is not None, we only + check the interval[0]..interval[1], otherwise the whole history of the pr_branch will be used. + Start from 12 chars and work down to 6, but do not include duplicates if already found at a longer length. + Returns a list of tuples: (full_hash, display_string) + """ + + fixes = CIQ_find_fixes_in_mainline(repo=repo, upstream_ref=upstream_ref, hash_=hash_) + return CIQ_filter_unapplied_commits(repo=repo, pr_branch=pr_branch, commits=fixes, interval=interval) + + +def CIQ_find_fixes_in_mainline_current_branch_unapplied(repo, upstream_ref, hash_, interval=None): current_branch = CIQ_get_current_branch(repo) - return CIQ_find_fixes_in_mainline(repo, current_branch, upstream_ref, hash_) + return CIQ_find_fixes_in_mainline_unapplied( + repo=repo, pr_branch=current_branch, upstream_ref=upstream_ref, hash_=hash_, interval=interval + ) def CIQ_reset_HEAD(repo):