On Tue, Dec 12, 2023 at 12:40:05AM +0000, rkolchmeyer@xxxxxxxxxx wrote:
Hi all, 5.10.194 includes 331d85f0bc6e (ovl: Always reevaluate the file signature for IMA), which resulted in a performance regression for workloads that use IMA and run from overlayfs. 5.10.202 includes cd5a262a07a5 (ima: detect changes to the backing overlay file), which resolved the regression in the upstream kernel. However, from my testing [1], this change doesn't resolve the regression on stable kernels. From what I can tell, cd5a262a07a5 (ima: detect changes to the backing overlay file) depends on both db1d1e8b9867 (IMA: use vfs_getattr_nosec to get the i_version) and a1175d6b1bda (vfs: plumb i_version handling into struct kstat). These two dependent changes were not backported to stable kernels. As a result, IMA seems to be caching the wrong i_version value when using overlayfs. From my testing, backporting these two dependent changes is sufficient to resolve the issue in stable kernels.
Thanks for triaging and proposing a resolution to the issue!
Would it make sense to backport those changes to stable kernels? It's possible that they may not follow the stable kernel patching rules. I think the issue can also be fixed directly in stable trees with the following diff (which doesn't make sense in the upstream kernel): diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index 70efd4aa1bd1..c84ae6b62b3a 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -239,7 +239,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint, * which do not support i_version, support is limited to an initial * measurement/appraisal/audit. */ - i_version = inode_query_iversion(inode); + i_version = inode_query_iversion(real_inode); hash.hdr.algo = algo; /* Initialize hash digest to 0's in case of failure */ I've verified that this diff resolves the performance regression. Which approach would make the most sense to fix the issue in stable kernels? Backporting the dependent commits, or merging the above diff?
Looking at the dependencies you've identified, it probably makes sense to just take them as is (as it's something we would have done if these dependencies were identified explicitly). I'll plan to queue them up after the current round of releases is out. -- Thanks, Sasha
