Message-Id: <D4EPMF7G3E05.1VHS9CVG3DZDE@kernel.org>
Date: Tue, 24 Sep 2024 21:01:47 +0300
From: "Jarkko Sakkinen" <jarkko@...nel.org>
To: "Jarkko Sakkinen" <jarkko@...nel.org>, "James Bottomley"
 <James.Bottomley@...senPartnership.com>, <linux-integrity@...r.kernel.org>
Cc: <roberto.sassu@...wei.com>, <mapengyu@...il.com>, "Mimi Zohar"
 <zohar@...ux.ibm.com>, "David Howells" <dhowells@...hat.com>, "Paul Moore"
 <paul@...l-moore.com>, "James Morris" <jmorris@...ei.org>, "Serge E.
 Hallyn" <serge@...lyn.com>, "Peter Huewe" <peterhuewe@....de>, "Jason
 Gunthorpe" <jgg@...pe.ca>, <keyrings@...r.kernel.org>,
 <linux-security-module@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 0/5] Lazy flush for the auth session

On Tue Sep 24, 2024 at 8:28 PM EEST, Jarkko Sakkinen wrote:
> On Tue Sep 24, 2024 at 8:26 PM EEST, Jarkko Sakkinen wrote:
> > On Tue Sep 24, 2024 at 7:36 PM EEST, Jarkko Sakkinen wrote:
> > > On Tue Sep 24, 2024 at 7:33 PM EEST, James Bottomley wrote:
> > > > On Tue, 2024-09-24 at 19:29 +0300, Jarkko Sakkinen wrote:
> > > > > On Tue Sep 24, 2024 at 4:48 PM EEST, James Bottomley wrote:
> > > > [...]
> > > > > > Patch 3 is completely unnecessary: the null key is only used to
> > > > > > salt the session and is not required to be resident while the
> > > > > > session is used (so can be flushed after session creation)
> > > > > > therefore keeping it around serves no purpose once the session is
> > > > > > created and simply clutters up the TPM volatile handle slots. (I
> > > > > > don't know of a case where we use all the slots in a kernel
> > > > > > operation, but since we don't need it lets not find out when we get
> > > > > > one).  So I advise dropping patch 3.
> > > > > 
> > > > > Let's go this through just to check I'm understanding.
> > > > > 
> > > > > Holding null key had radical effect on boot time: it cut it down by
> > > > > 5 secons down to 15 seconds:
> > > > > 
> > > > > https://lore.kernel.org/linux-integrity/CALSz7m1WG7fZ9UuO0URgCZEDG7r_wB4Ev_4mOHJThH_d1Ed1nw@mail.gmail.com/
> > > > > 
> > > > > Then in subsequent version I implemented lazy auth session and boot
> > > > > time went down to 9.7 seconds.
> > > > > 
> > > > > So is the point you're trying to make that since auth session is 
> > > > > already held as long as we can and they flushed in synchronous
> > > > > point too, I can just as well drop patch 3?
> > > >
> > > > Yes, because the null key is only used in session generation which is
> > > > now lazy, it adds or subtracts nothing from the timings.  When you're
> > > > forced to flush the session, the null key goes too, so you again have
> > > > to restore it from the context.  When you can keep the session you
> > > > don't need the null key because you're not regenerating it.
> > >
> > > Yeah, OK, then we're in sync with this. It's evolutionary cruft.
> > >
> > > Just had to check that the logic matches how I projected your earlier
> > > comment because these are sensitive changes.
> >
> > I'm definitely going keeep 1/5 and 2/5 as they are still bug fixes.
> >
> > So they will appear in v6 unchanged and perf fixes (which are not
> > functional fixes) should not be built on top of broken code.
>
> And 3/5 is actually required because it saves of doing flush during
> the boot if nothing else.
>
> We are wasting more time so I don't want to waste it for nothing.

Anything beyong 50 ms matters and that flush certainly costs more than
that. As I already stated in earlier version, we need to find more of
these "50 ms and 100 ms there sites.

The functional fixes are required because perf fix is always *less
critical* than perf fix.

Please pay more attention to proper error rollback next time, that's
all I can say on that. It's not my fault that it is broken.

BR, Jarkko

Powered by blists - more mailing lists