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):
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?
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.
On Tue, Dec 12, 2023 at 12:40:05AM +0000, rkolchmeyer@google.com 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.
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
On Tue, Dec 12, 2023 at 04:37:31PM -0800, Robert Kolchmeyer wrote:
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
On Mon, Dec 18, 2023 at 12:57:20PM +0100, Greg KH wrote:
On Tue, Dec 12, 2023 at 04:37:31PM -0800, Robert Kolchmeyer wrote:
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?
Also don't forget 18b44bc5a672 ("ovl: Always reevaluate the file signature for IMA") either. There might be more...
thanks,
greg k-h
On Mon, Dec 18, 2023 at 3:58 AM Greg KH greg@kroah.com wrote:
On Mon, Dec 18, 2023 at 12:57:20PM +0100, Greg KH wrote:
On Tue, Dec 12, 2023 at 04:37:31PM -0800, Robert Kolchmeyer wrote:
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?
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
linux-stable-mirror@lists.linaro.org
-
Greg KH -
rkolchmeyer＠google.com -
Robert Kolchmeyer
-
Sasha Levin