IMA performance regression in 5.10.194 when using overlayfs

IMA performance regression in 5.10.194 when using overlayfs

[rkolchmeyer]

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.

Re: IMA performance regression in 5.10.194 when using overlayfs

[Sasha Levin]

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.

-- 
Thanks,
Sasha

Re: IMA performance regression in 5.10.194 when using overlayfs

[Robert Kolchmeyer]

> 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

Re: IMA performance regression in 5.10.194 when using overlayfs

[Greg KH]

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

Re: IMA performance regression in 5.10.194 when using overlayfs

[Greg KH]

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

Re: IMA performance regression in 5.10.194 when using overlayfs

[Robert Kolchmeyer]

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