Re: [Qemu-block] [Qemu-devel] [PATCH 06/10] qemu-img: Prepare for locked images

[Kevin Wolf]

Am 12.01.2016 um 16:20 hat Markus Armbruster geschrieben:
> Kevin Wolf <address@hidden> writes:
> 
> > Am 11.01.2016 um 16:49 hat Markus Armbruster geschrieben:
> >> Eric Blake <address@hidden> writes:
> >> 
> >> > On 12/22/2015 09:46 AM, Kevin Wolf wrote:
> >> >> This patch extends qemu-img for working with locked images. It prints a
> >> >> helpful error message when trying to access a locked image read-write,
> >> >> and adds a 'qemu-img force-unlock' command as well as a 'qemu-img check
> >> >> -r all --force' option in order to override a lock left behind after a
> >> >> qemu crash.
> >> >> 
> >> >> Signed-off-by: Kevin Wolf <address@hidden>
> >> >> ---
> >> >>  include/block/block.h |  1 +
> >> >>  include/qapi/error.h  |  1 +
> >> >>  qapi/common.json      |  3 +-
> >> >>  qemu-img-cmds.hx      | 10 ++++--
> >> >>  qemu-img.c | 96
> >> >> +++++++++++++++++++++++++++++++++++++++++++--------
> >> >>  qemu-img.texi         | 20 ++++++++++-
> >> >>  6 files changed, 113 insertions(+), 18 deletions(-)
> >> >> 
> >> >
> >> >> +++ b/include/qapi/error.h
> >> >> @@ -102,6 +102,7 @@ typedef enum ErrorClass {
> >> >>      ERROR_CLASS_DEVICE_NOT_ACTIVE = QAPI_ERROR_CLASS_DEVICENOTACTIVE,
> >> >>      ERROR_CLASS_DEVICE_NOT_FOUND = QAPI_ERROR_CLASS_DEVICENOTFOUND,
> >> >>      ERROR_CLASS_KVM_MISSING_CAP = QAPI_ERROR_CLASS_KVMMISSINGCAP,
> >> >> +    ERROR_CLASS_IMAGE_FILE_LOCKED = QAPI_ERROR_CLASS_IMAGEFILELOCKED,
> >> >>  } ErrorClass;
> >> >
> >> > Wow - a new ErrorClass.  It's been a while since we could justify one of
> >> > these, but I think you might have found a case.
> >> 
> >> Spell out the rationale for the new ErrorClass, please.
> >
> > Action to be taken for this error class: Decide whether the lock is a
> > leftover from a previous qemu run that ended in an unclean shutdown. If
> > so, retry with overriding the lock.
> >
> > Currently used by qemu-img when ordered to override a lock. libvirt
> > will need to do the same.
> 
> Let's see whether I understand the intended use:
> 
>     open image
>     if open fails with ImageFileLocked:
>         guess whether the lock is stale
>         if guessing not stale:
>             error out
>         open image with lock override
> 
> Correct?

Yes. Where "guess" is more or less "check whether the management tool
started qemu with this image, but didn't cleanly shut it down". This can
guess wrong if, and only if, some other user used a different algorithm
and forced an unlock even though the image didn't belong to them before
the crash.

> Obvious troublespots:
> 
> 1. If you guess wrong, you destroy the image.  No worse than before, so
>    okay, declare documentation problem.
> 
> 2. TOCTTOU open to open with lock override
>    [...]
> 
> 3. TOCTTOU within open (hypothetical, haven't read your code)
>    [...]

Yes, these exist in theory. The question is what scenarios you want to
protect against and whether improving the mechanism to cover these cases
is worth the effort.

The answer for what I wanted to protect is a manual action on an image
that is already in use. The user isn't quick enough to manually let two
processes open the same image at the same time, so I didn't consider
that scenario relevant.

But assuming that everyone (including the human user) follows the above
protocol (force-unlock only what was yours before the crash), at least
cases 1 and 2 don't happen anyway.

> Let me try a different tack.  It may well be unworkable.
> [...]

It doesn't sound unworkable, but it might be overengineered if the goal
is just to protect people against 'qemu-img snapshot' on running VMs.

> Obviously, the management application will also need to be able to
> override stale locks from opens by someone else, say a human user
> bypassing the management application.

That's not obvious. If another user messed it up, this other user can
also clean it up. But yes, asking a higher level would work, too.

Kevin