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

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 11.01.2016 um 16:49 hat Markus Armbruster geschrieben:
>> Eric Blake <eblake@redhat.com> 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 <kwolf@redhat.com>
>> >> ---
>> >>  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?

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

   I understand this lock cannot be foolproof, but I dislike the "if
   lock stale then steal lock" TOCTTOU trap all the same.

        Process A                       Process B
        open image, ImageFileLocked
        guess not stale (correct)
                                        open image, ImageFileLocked
                                        guess not stale (correct)
                                        open image with lock override
        open image with lock override

   To avoid, you need to add a suitable unique identifier to the lock,
   then change the override to "override the lock with this unique ID".

   A smartly chosen identifier might even help with guessing whether the
   lock is stale, at least in some cases.

3. TOCTTOU within open (hypothetical, haven't read your code)

   Obviously, "open image" needs to acquire the lock.  Is open+lock
   atomic with respect to concurrent open+lock?

> Alternative design without a new error class: Accept the
> override-lock=on option on the block.c level (QAPI BlockdevOptionsBase)
> and ignore it silently for image formats that don't support locking
> (i.e. anything but qcow2), so that qemu-img and libvirt can set this
> option unconditionally even if they don't know that the image is locked
> (but libvirt would have to check first if qemu supports the option at
> all in order to maintain compatibility with old qemu versions).
>
> In my opinion, this would be worse design than adding an error class. It
> would also prevent libvirt from logging when it forcefully unlocks an
> image.

Well, QEMU could (and probably should) say something when overriding a
lock, and libvirt should certainly log whatever QEMU says.

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

An image may support a lock.  If it does, it carries an ID uniquely
identifying a particular lock - unlock period.

When you open an image supporting locking, you atomically acquire its
lock and set its lock ID.  If the user specifies the ID, we use that,
else we make one up.

To open with lock override, you need to specify something like
override=ID.

Intended use by management application, ignoring the need for lock
override:

    generate lock ID for this image and save it in persistent management
      storage
    open and lock image, setting the lock ID
    ...
    unlock and close image
    delete lock ID in persistent management storage

Now extend to override the lock when necessary:

    if persistent management storage has lock ID for this image
        open with lock override=ID
    else
        generate lock ID for this image and save...
        open and lock...

This lets the management application override its own stale locks after
a crash, but it won't override anybody else's locks, and it doesn't
engage in any guesswork.

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.  Perhaps like this:

    examine the image
    if it's locked:
        get the lock ID
        let next higher level in the stack decide whether it's stale
            (this level might be a human user)
        if it's stale:
            open with lock override=ID

No open-to-open-with-override TOCTTOU.

Thoughts?