stg pick applied chunk to wrong location

[snits]

I'm not sure that there is a solution to this, but wanted to report it. I was backporting a kernel patch from upstream, and git cherry-pick applies it correctly, but stg pick finds a match in a different spot for one chunk and applies it there. I can see how it chose that location, so I'm not sure what the solution would be, but there was no indication of modified when it applied the patch.

# git --no-pager show 620bf9f981365c18cc2766c53d92bf8131c63f32
commit 620bf9f981365c18cc2766c53d92bf8131c63f32 (tag: iommu-fixes-v6.1-rc1, iommu/next, iommu/iommu/fixes)
Author: Jerry Snitselaar <[email protected]>
Date:   Wed Oct 19 08:44:47 2022 +0800

    iommu/vt-d: Clean up si_domain in the init_dmars() error path
    
    A splat from kmem_cache_destroy() was seen with a kernel prior to
    commit ee2653bbe89d ("iommu/vt-d: Remove domain and devinfo mempool")
    when there was a failure in init_dmars(), because the iommu_domain
    cache still had objects. While the mempool code is now gone, there
    still is a leak of the si_domain memory if init_dmars() fails. So
    clean up si_domain in the init_dmars() error path.
    
    Cc: Lu Baolu <[email protected]>
    Cc: Joerg Roedel <[email protected]>
    Cc: Will Deacon <[email protected]>
    Cc: Robin Murphy <[email protected]>
    Fixes: 86080ccc223a ("iommu/vt-d: Allocate si_domain in init_dmars()")
    Signed-off-by: Jerry Snitselaar <[email protected]>
    Link: https://lore.kernel.org/r/[email protected]
    Signed-off-by: Lu Baolu <[email protected]>
    Signed-off-by: Joerg Roedel <[email protected]>

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index b3cf0f991e29..48cdcd0a5cf3 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -2410,6 +2410,7 @@ static int __init si_domain_init(int hw)
 
 	if (md_domain_init(si_domain, DEFAULT_DOMAIN_ADDRESS_WIDTH)) {
 		domain_exit(si_domain);
+		si_domain = NULL;
 		return -EFAULT;
 	}
 
@@ -3052,6 +3053,10 @@ static int __init init_dmars(void)
 		disable_dmar_iommu(iommu);
 		free_dmar_iommu(iommu);
 	}
+	if (si_domain) {
+		domain_exit(si_domain);
+		si_domain = NULL;
+	}
 
 	return ret;
 }

git cherry pick case:

# git cpk 620bf9f98136
Auto-merging drivers/iommu/intel/iommu.c
[testing4 4f8cca10f877] iommu/vt-d: Clean up si_domain in the init_dmars() error path
 Date: Wed Oct 19 08:44:47 2022 +0800
 1 file changed, 5 insertions(+)


diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 2a29bc08c7bd..17fbcbf0ea21 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -2837,6 +2837,7 @@ static int __init si_domain_init(int hw)
 
 	if (md_domain_init(si_domain, DEFAULT_DOMAIN_ADDRESS_WIDTH)) {
 		domain_exit(si_domain);
+		si_domain = NULL;
 		return -EFAULT;
 	}
 
@@ -3487,6 +3488,10 @@ static int __init init_dmars(void)
 		disable_dmar_iommu(iommu);
 		free_dmar_iommu(iommu);
 	}
+	if (si_domain) {
+		domain_exit(si_domain);
+		si_domain = NULL;
+	}
 
 	kfree(g_iommus);

stg pick case:

# stg pick -x 620bf9f981365c18cc2766c53d92bf8131c63f32
- iommu-vt-d-Clean-up-si_domain-in-the-init_dmars-error-path
> iommu-vt-d-Clean-up-si_domain-in-the-init_dmars-error-path

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 2a29bc08c7bd..8c625c6776fa 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -2837,6 +2837,7 @@ static int __init si_domain_init(int hw)
 
 	if (md_domain_init(si_domain, DEFAULT_DOMAIN_ADDRESS_WIDTH)) {
 		domain_exit(si_domain);
+		si_domain = NULL;
 		return -EFAULT;
 	}
 
@@ -4027,6 +4028,10 @@ int dmar_iommu_hotplug(struct dmar_drhd_unit *dmaru, bool insert)
 		disable_dmar_iommu(iommu);
 		free_dmar_iommu(iommu);
 	}
+	if (si_domain) {
+		domain_exit(si_domain);
+		si_domain = NULL;
+	}
 
 	return ret;
 }

So in the stg case, it applied it to the dmar_iommu_hotplug() instead of init_dmars().

[snits]

 stg --version
Stacked Git 2.0.0-rc.1 (b4cd77c2c)
Copyright (C) 2005-2022 StGit authors
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
SPDX-License-Identifier: GPL-2.0-only
git version 2.37.2

[jpgrayson]

I wonder if this might be resolved if StGit used the --3way option to git apply when pushing patches?

See:

https://github.com/stacked-git/stgit/blob/f3b07102556b47b77276ab12deeadd4cd98c3637/src/stack/transaction/mod.rs#L970

and:

https://github.com/stacked-git/stgit/blob/f3b07102556b47b77276ab12deeadd4cd98c3637/src/stupid/mod.rs#L168-L174

Note the // --3way comment. The reason I was hesitant to use the --3way option here was to be conservative about introducing differences in behavior between the new (Rust) and 1.x (Python) implementations. Also, IIRC, I was not immediately able to craft a test case that showed a material benefit to using --3way at push-time.

If you are able to readily reproduce this issue, it would be very interesting to know if adding --3way to the git apply arguments in src/stupid/mod.rs fixes this case.

[snits]

Adding "--3way" to args does result in it applying the chunk to init_dmars().

[jpgrayson]

Great! Thanks for doing this test, @snits.

I'm finding that turning on --3way causes a seemingly unrelated issue with stg undo to surface in the test suite. So, my steps to get this resolved are:

• Figure out how/why stg undo is broken as demonstrated in t2103-pull-trailing.sh.
• Craft a test case for the StGit test suite that exhibits this merge problem
• Enable --3way

Thank you again for this issue report, @snits. It is very helpful.

[snits]

You are welcome @jpgrayson. It is a great tool for doing backporting work, and I've been using the rust build the past couple of months. That was kind of sneaky, and took me a couple minutes to realize why my test was failing now, when it worked before I sent the patch upstream last week. :) So I figured I should mention it.

[snits]

I went back through 4 recent series of backports that I've done (~230 patches), and didn't find any other instances, or at least the --3way version got the same results. So apparently just a rare case where everything lined up just right for it to find that other spot that matched.

[ctmarinas]

With this fix, I now get: "error: git apply: error: --cached and --3way cannot be used together"

git version 2.30.2 (Debian stable).

[snits]

Hmmm, I have 2.38.1 which worked when I've tried the new version. I see this in the diff between 2.30 and 2.38:

c0c2a37ac2b9 git-apply: allow simultaneous --cached and --3way options | 2021-04-07 | (Jerry Zhang)

I guess this would force a requirement of git v2.32.0, or some solution other than --3way?

[ctmarinas]

Thanks for digging the git versions. I can get git 2.34 from bullseye-backports and solve it for me but it may affect others that prefer to stick to vanilla Debian (or whatever you call no-backports). A quick workaround would be to re-attempt the git call without --3way if the first one failed.

Trying to enforce the version I think adds another git call every time you invoke stgit (and I like the Rust version being quick).

[snits]

@jpgrayson git versions v2.31 and earlier can't handle --cached and --3way at the same time.