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

[Kevin Wolf <kwolf@redhat.com>]

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

Am 29.04.2019 um 22:16 hat Max Reitz geschrieben:
> 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.

Hm, yes. But we already agreed that dropping permissions should never
fail, so maybe we should just implement that now (in block.c).

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

...and actually do s->open_flags = s->perm_change_flags in
raw_set_perm(), otherwise it doesn't help much. :-)

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

Good.

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

Yes, I think file-posix should let the operation fail (because it
actually failed) and the permission functions in block.c should ignore
the error for dropping permissions. This way, reopen will still
correctly return the error because it has a different call chain.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]