IMA performance regression in 5.10.194 when using overlayfs
Subsystems (6)
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.
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):
security/integrity/ima/ima_api.c +1 -1
@@ -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?
I'd be happy to prepare and mail patches either way. Looking forward to your thoughts.
Thanks,
-Robert
[1] I'm benchmarking with the following:
cat <<EOF > /sys/kernel/security/ima/policy
audit func=BPRM_CHECK
audit func=FILE_MMAP mask=MAY_EXEC
EOF
docker run --rm debian bash -c "TIMEFORMAT=%3R; time bash -c :"
A good result looks like 0.002 (units are seconds), and a bad result looks like 0.034.
Sasha Levin
maintainerQuoted 10 lines · On Tue, Dec 12, 2023 at 12:40:05AM +0000, [email protected] 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 includescd5a262a07a5(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.
Quoted 8 lines
From what I can tell,
cd5a262a07a5(ima: detect changes to the backing overlay file) depends on bothdb1d1e8b9867(IMA: usevfs_getattr_nosecto get thei_version) anda1175d6b1bda(vfs: plumbi_versionhandling into struct kstat). These two dependent changes were not backported to stable kernels. As a result, IMA seems to be caching the wrongi_versionvalue 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!
Quoted 19 lines
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):
security/integrity/ima/ima_api.c +1
-1@@ -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 */
Quoted 4 lines
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.
Signature · 2 lines
Thanks, Sasha
Quoted 5 lines
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.
Sounds great, thank you!
-Robert
Quoted 7 lines · Robert Kolchmeyer
Quoted 5 lines
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.
Sounds great, thank you!
I've dropped them now as there are some reported bug fixes with just that commit that do not seem to apply properly at all, and we can't add new problems when we know we are doing so :)
So can you provide a working set of full backports for the relevant kernels that include all fixes (specifically stuff like 8a924db2d7b5 ("fs: Pass AT_GETATTR_NOSEC flag to getattr interface function")) so that we can properly queue them up then?
Or, conversely, we can revert the other commits if you think that would be better?
thanks,
greg k-h
Quoted 9 lines · Greg KH
Quoted 7 lines · Robert Kolchmeyer
Quoted 5 lines
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.
Sounds great, thank you!
Quoted 8 lines
I've dropped them now as there are some reported bug fixes with just that commit that do not seem to apply properly at all, and we can't add new problems when we know we are doing so :)
So can you provide a working set of full backports for the relevant kernels that include all fixes (specifically stuff like
8a924db2d7b5("fs: PassAT_GETATTR_NOSECflag to getattr interface function")) so that we can properly queue them up then?
Also don't forget 18b44bc5a672 ("ovl: Always reevaluate the file signature for IMA") either. There might be more...
thanks,
greg k-h
Quoted 20 lines · Greg KH
Quoted 9 lines · Greg KH
Quoted 7 lines · Robert Kolchmeyer
Quoted 5 lines
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.
Sounds great, thank you!
Quoted 8 lines
I've dropped them now as there are some reported bug fixes with just that commit that do not seem to apply properly at all, and we can't add new problems when we know we are doing so :)
So can you provide a working set of full backports for the relevant kernels that include all fixes (specifically stuff like
8a924db2d7b5("fs: PassAT_GETATTR_NOSECflag to getattr interface function")) so that we can properly queue them up then?
Quoted 6 lines
Also don't forget
18b44bc5a672("ovl: Always reevaluate the file signature for IMA") either. There might be more...thanks,
greg k-h
Thanks - from what I can tell with git log --grep, 8a924db2d7b5 ("fs: Pass AT_GETATTR_NOSEC flag to getattr interface function") is the only such fix we need to backport (18b44bc5a672 ("ovl: Always reevaluate the file signature for IMA") is already in stable trees and introduced the regression that motivated this investigation).
I'll prepare these backports and send them to the list.
Thanks,
-Robert