Re: [Qemu-devel] [Qemu-block] [PATCH v2 10/10] file-posix: Make auto-read-only dynamic

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2660 bytes --]

On 11.03.19 17:50, Kevin Wolf wrote:
> Until now, with auto-read-only=on we tried to open the file read-write
> first and if that failed, read-only was tried. This is actually not good
> enough for libvirt, which gives QEMU SELinux permissions for read-write
> only as soon as it actually intends to write to the image. So we need to
> be able to switch between read-only and read-write at runtime.
> 
> This patch makes auto-read-only dynamic, i.e. the file is opened
> read-only as long as no user of the node has requested write
> permissions, but it is automatically reopened read-write as soon as the
> first writer is attached. Conversely, if the last writer goes away, the
> file is reopened read-only again.
> 
> bs->read_only is no longer set for auto-read-only=on files even if the
> file descriptor is opened read-only because it will be transparently
> upgraded as soon as a writer is attached. This changes the output of
> qemu-iotests 232.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/file-posix.c         | 36 +++++++++++++++++-------------------
>  tests/qemu-iotests/232.out | 12 ++++++------
>  2 files changed, 23 insertions(+), 25 deletions(-)

https://bugzilla.redhat.com/show_bug.cgi?id=1703793 seems to be caused
by this patch: When the mirror job completes, it drops all permissions
on its target BB with an &error_abort.  As of this patch, this may
result in file-posix attempting to reopen the FD, which may fail.

There are two problems I can see: First, the previous patch should have
updated s->open_flags along with s->fd when the FD is switched.  As it
is now, s->open_flags is not updated, so it stays on O_RDONLY and every
time the permissions are checked, the FD is reconfigured and then switched.

That's simple to fix, just add BDRVRawState.perm_change_flags and set it
to open_flags after raw_reconfigure_getfd() returned a ret != s->fd
(when s->perm_change_fd is set).

That fixes the problem of file-posix attempting to reopen the FD to
O_RDWR all the time, which caused the crash.

But that gives us another crash, because now dropping the permissions
(correctly) reopens the FD to O_RDONLY, with the exact same implications
as above: If the target becomes unavailable, opening the new FD will
fail, and qemu will crash.

I don't know what to do about this.  In the spirit of "dropping
permissions should always work", I presume raw_reconfigure_getfd()
should just return the old FD if it had more permissions than the new
one would have.  But if the user issues an explicit reopen command, they
probably want such an error to be reported.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]