Re: [Qemu-devel] [PATCH V8 08/14] Introduce file lock for the block layer

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Sep 06, 2011 at 07:55:48PM -0400, Stefan Berger wrote:
> On 09/04/2011 03:32 PM, Michael S. Tsirkin wrote:
> >On Thu, Sep 01, 2011 at 09:53:40PM -0400, Stefan Berger wrote:
> >>>Generally, what all other devices do is perform validation
> >>>as the last step in migration when device state
> >>>is restored. On failure, management can decide what to do:
> >>>retry migration or restart on source.
> >>>
> >>>Why is TPM special and needs to be treated differently?
> >>>
> >>>
> >>>
> >...
> >
> >>More detail: Typically one starts out with an empty QCoW2 file
> >>created via qemu-img. Once Qemu starts and initializes the
> >>libtpms-based TPM, it tries to read existing state from that QCoW2
> >>file. Since there is no state stored in the QCoW2, the TPM will
> >>start initializing itself to an initial 'blank' state.
> >So it looks like the problem is you access the file when guest isn't
> >running.  Delaying the initialization until the guest actually starts
> >running will solve the problem in a way that is more consistent with
> >other qemu devices.
> >
> I'd agree if there wasn't one more thing: encryption on the data
> inside the QCoW2 filesystem
> 
> First: There are two ways to encrypt the data.
> 
> One comes with the QCoW2 type of image and it comes for free. Set
> the encryption flag when creating the QCoW2 file and one has to
> provide a key to access the QCoW2. I found this mode problematic for
> users since it required me to go through the monitor every time I
> started the VM. Besides that the key is provided so late that all
> devices are already initialized and if the wrong key was provided
> the only thing the TPM can do is to go into shutdown mode since
> there is state on the QCoW2 but it cannot be decrypted. This also
> became problematic when doing migrations with libvirt for example
> and one was to have a wrong key/password installed on the target
> side -- graceful termination of the migration is impossible.
> 
> So the above drove the implementation of the encryption mode added
> in patch 10 in this series. Here the key is provided via command
> line and it can be used immediately. So I am reading the state blobs
> from the file, decrypt them, create the CRC32 on the plain data and
> check against the CRC32 stored in the 'directory'. If it doesn't
> match the expected CRC32 either the key was wrong or the state is
> corrupted and I can terminate Qemu gracefully. I can also react
> appropriately if no key was provided but one is expected and
> vice-versa. Also in case of migration this now allows me to
> terminate Qemu gracefully so it continues running on the source.
> This is an improvement over the situation described above where in
> case the target had the wrong key the TPM went into shutdown mode
> and the user would be wondering why that is -- the TPM becomes
> inaccessible. However, particularly in the case of migration with
> shared storage I need to access the QCoW2 file to check whether on
> the target I have the right key. And this happens very early during
> qemu startup on the target machine. So that's where the file locking
> on the block layer comes in. I want to serialize access to the QCoW2
> so I don't read intermediate state on the migration-target host that
> can occur if the source is currently writing TPM state data.
> 
> This patch and the command line provided key along with the
> encryption mode added in patch 10 in this series add to usability
> and help prevent failures.
> 
> Regards,
>      Stefan

Sure, it's a useful feature of validating the device early.
But as I said above, it's useful for other devices besides
TPM. However it introduces issues where state changes
since guest keeps running (such as what you have described here).

I think solving this in a way specific to TPM is a mistake.
Passing key on command line does not mean you must use it
immediately, this can still be done when guest starts running.

Further, the patchset seems to be split in
a way that introduces support headaches and makes review
harder, not easier. In this case, we will have to support
both a broken mode (key supplied later) and a non-broken one
(key supplied on init). It would be better to implement
a single mode, in a single patch.


-- 
MST