Messages in this thread | | | Date | Fri, 20 Jan 2017 19:57:53 +0200 | From | Jarkko Sakkinen <> | Subject | Re: [tpmdd-devel] [PATCH 1/2] tpm2: add session handle isolation to tpm spaces |
| |
On Fri, Jan 20, 2017 at 09:39:13AM -0500, James Bottomley wrote: > On Fri, 2017-01-20 at 15:23 +0200, Jarkko Sakkinen wrote: > > On Thu, Jan 19, 2017 at 07:11:23AM -0500, James Bottomley wrote: > > > On Thu, 2017-01-19 at 13:58 +0200, Jarkko Sakkinen wrote: > > > > On Wed, Jan 18, 2017 at 10:09:46AM -0500, James Bottomley wrote: > > > > > sessions should be isolated during each instance of a tpm > > > > > space. This means that spaces shouldn't be able to see each > > > > > other's sessions and also when a space is closed, all the > > > > > sessions belonging to it should be flushed. > > > > > > > > > > This is implemented by adding a session_tbl to the space to > > > > > track the created session handles. Sessions can be flushed > > > > > either by not setting the continueSession attribute in the > > > > > session table or by an explicit flush. In the first case we > > > > > have to mark the session as being ready to flush and explicitly > > > > > forget it if the command completes successfully and in the > > > > > second case we have to intercept the flush instruction and > > > > > clear the session from our table. > > > > > > > > You could do this without these nasty corner cases by arbage > > > > collecting when a command emits a new session handle. > > > > > > I could for this patch set. However, the global session accounting > > > RFC requires strict accounting, because it needs to know exactly > > > when to retry a command that failed because we were out of sessions > > > and because we don't want to needlessly evict a session if there > > > was one available which we didn't see because of lazy accounting. > > > It would be a lot of churn to do it lazily in this patch set and > > > then switch to strict in that one, so I chose to account sessions > > > strictly always. > > > > Lazy is kind of ambiguous word so I'll have to check that we have > > same definition for it in this context. > > > > I'm talking about not trying to detect if something gets deleted. > > When something gets created you would go through the global list of > > sessions and check if it is used. If so, it must be that the session > > was deleted at some point. > > That's my terminology too. We're talking about lazy and strict > tracking of session flushing. > > > Your argument is that in this scheme sometimes there might be a > > session marked as "reserved" but it is in fact free. This might lead > > to useless eviction. Am I correct? > > Yes, but not just that, it will also lead to over long waits because we > can no longer wake the waiters the moment a session becomes free. > > > My argument is that the lazy scheme is more generic (does not require > > special cases). As a subsystem maintainer I tend to be more fond of > > that kind of solutions. Having special cases raises questios like > > (for example): > > > > 1. What if standard gets added something that does not fall into the > > current set of special cases? You never know. > > 2. What about vendor specific commands? The lazy scheme is compatible > > with them. The standard does not put any kind of constraints for > > vendor specific commands. > > We rely on the assertion in the Manual that sessions are only returned > in the handle area (as we do for objects). We also rely on the > guarantee that they're only destroyed by flush or continueSession being > 0 in the session attributes. > > If some mad vendor introduces a command that creates an object and > doesn't return it in the handle area, we'll get screwed for both > transient objects and sessions. Sessions also could have issues if > some mad vendor creates a command that flushes them outside of the > above description. That's why the standard has all these caveats about > handle and session creation. In theory the vendors are not allowed to > violate them in their own commands ... > > > You could solve the problem you are stating by getting the full the > > list of alive sessions with CAP_HANDLES and mark dangling sessions > > as free. > > That's a command which produces a huge output ... I'd have to do it at > the end of every input command to get the list of current handles ... I > don't really think it's a better solution. > > Let me describe the failure case with strict destruction accounting: > supposing a mad vendor does introduce a command that flushes a session > outside the standards prescribed way. What happens is that it gets re > -used before the TPM exhausts handles, and tpm2_session_chip_add will > simply to replace what it currently has. The only consequence is a > single missed wakeup. So even in the face of vendor failure, this > scheme will work almost all of the time and it will always work better > than a lazy scheme because the failure case gives us properties > identical to the lazy case. To make this case iron, we should get a > failure on context save, so I can use that failure to drop the handle > and I think the strict scheme will then always perform better than the > lazy scheme (let's call it strict with lazy backup) will that suffice? > > > PS. I've started to think that maybe also with sessions it is better > > to have just one change that implements full eviction like we have > > for transient objects after seeing your breakdown. I'm sorry about > > putting you extra trouble doing the isolation only patch. It's better > > to do this right once... > > Well, I can put them back together again, but you could just apply them > together as two patches ... they are now bisectable.
Sure forgot this last comment. It's really irrelevant. I'll reply properly later on.
> James
/Jarkko
| |