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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> 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.

"Force-unlock only what you locked yourself" is easier to stipulate than
to adhere to when the tools can't give you a hint on who did the
locking.  This is particularly true when "you" is a human, with human
imperfect memory.

I understand that this locking can't provide complete protection, and
merely aims to catch certain common accidents.

However, to avoid a false sense of security, its limitations need to be
clearly documented.  This very much includes the rule "force-unlock only
what you locked yourself".  In my opinion, it should also include the
raciness.

Sometimes, solving a problem is easier than documenting it.

>> 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