Re: [Qemu-devel] [PATCH v3] Add support for fd: protocol

[Alexander Graf]

On 26.07.2011, at 18:57, Corey Bryant wrote:

> 
> Kevin, thanks for the input.
> 
> On 07/26/2011 11:18 AM, Kevin Wolf wrote:
>> Am 26.07.2011 14:51, schrieb Corey Bryant:
>>> >  sVirt provides SELinux MAC isolation for Qemu guest processes and their
>>> >  corresponding resources (image files). sVirt provides this support
>>> >  by labeling guests and resources with security labels that are stored
>>> >  in file system extended attributes. Some file systems, such as NFS, do
>>> >  not support the extended attribute security namespace, which is needed
>>> >  for image file isolation when using the sVirt SELinux security driver
>>> >  in libvirt.
>>> >
>>> >  The proposed solution entails a combination of Qemu, libvirt, and
>>> >  SELinux patches that work together to isolate multiple guests' images
>>> >  when they're stored in the same NFS mount. This results in an
>>> >  environment where sVirt isolation and NFS image file isolation can both
>>> >  be provided.
>>> >
>>> >  This patch contains the Qemu code to support this solution. I would
>>> >  like to solicit input from the libvirt community prior to starting
>>> >  the libvirt patch.
>>> >
>>> >  Currently, Qemu opens an image file in addition to performing the
>>> >  necessary read and write operations. The proposed solution will move
>>> >  the open out of Qemu and into libvirt. Once libvirt opens an image
>>> >  file for the guest, it will pass the file descriptor to Qemu via a
>>> >  new fd: protocol.
>>> >
>>> >  If the image file resides in an NFS mount, the following SELinux policy
>>> >  changes will provide image isolation:
>>> >
>>> >     - A new SELinux boolean is created (e.g. virt_read_write_nfs) to
>>> >       allow Qemu (svirt_t) to only have SELinux read and write
>>> >       permissions on nfs_t files
>>> >
>>> >     - Qemu (svirt_t) also gets SELinux use permissions on libvirt
>>> >       (virtd_t) file descriptors
>>> >
>>> >  Following is a sample invocation of Qemu using the fd: protocol on
>>> >  the command line:
>>> >
>>> >       qemu -drive file=fd:4,format=qcow2
>>> >
>>> >  The fd: protocol is also supported by the drive_add monitor command.
>>> >  This requires that the specified file descriptor is passed to the
>>> >  monitor alongside a prior getfd monitor command.
>>> >
>>> >  There are some additional features provided by certain image types
>>> >  where Qemu reopens the image file. All of these scenarios will be
>>> >  unsupported for the fd: protocol, at least for this patch:
>>> >
>>> >     - The -snapshot command line option
>>> >     - The savevm monitor command
>>> >     - The snapshot_blkdev monitor command
>>> >     - Use of copy-on-write image files
>>> >     - The -cdrom command line option
>>> >     - The -drive command line option with media=cdrom
>>> >     - The change monitor command
>>> >
>>> >  The thought is that this support can be added in the future, but is
>>> >  not required for the initial fd: support.
>>> >
>>> >  This patch was tested with the following formats: raw, cow, qcow,
>>> >  qcow2, qed, and vmdk, using the fd: protocol from the command line
>>> >  and the monitor. Tests were also run to verify existing file name
>>> >  support and qemu-img were not regressed. Non-valid file descriptors,
>>> >  fd: without format, snapshot and backing files, and cdrom were also
>>> >  tested.
>>> >
>>> >  v2:
>>> >     - Add drive_add monitor command support
>>> >     - Fence off unsupported features that re-open image file
>>> >
>>> >  v3:
>>> >     - Fence off cdrom and change monitor command support
>>> >
>>> >  Signed-off-by: Corey Bryant<address@hidden>
>>> >  ---
>>> >    block.c           |   16 ++++++++++
>>> >    block.h           |    1 +
>>> >    block/cow.c       |    5 +++
>>> >    block/qcow.c      |    5 +++
>>> >    block/qcow2.c     |    5 +++
>>> >    block/qed.c       |    4 ++
>>> >    block/raw-posix.c |   81 
>>> > +++++++++++++++++++++++++++++++++++++++++++++++------
>>> >    block/vmdk.c      |    5 +++
>>> >    block_int.h       |    1 +
>>> >    blockdev.c        |   19 ++++++++++++
>>> >    monitor.c         |    5 +++
>>> >    monitor.h         |    1 +
>>> >    qemu-options.hx   |    8 +++--
>>> >    qemu-tool.c       |    5 +++
>>> >    14 files changed, 149 insertions(+), 12 deletions(-)
>>> >
>>> >  diff --git a/block.c b/block.c
>>> >  index 24a25d5..500db84 100644
>>> >  --- a/block.c
>>> >  +++ b/block.c
>>> >  @@ -536,6 +536,10 @@ int bdrv_open(BlockDriverState *bs, const char 
>>> > *filename, int flags,
>>> >            char tmp_filename[PATH_MAX];
>>> >            char backing_filename[PATH_MAX];
>>> >
>>> >  +        if (bdrv_is_fd_protocol(bs)) {
>>> >  +            return -ENOTSUP;
>>> >  +        }
>> Hm, shouldn't that just work even with fd?
>> 
> 
> My thought was that the proposed SELinux changes would prevent the open of 
> the temporary backing file if the file corresponding to fd resides on NFS.  
> But perhaps the backing file is not opened on NFS?

Then make a new flag that allows you to prohibit backing files.

> 
>>> >  +
>>> >            /* if snapshot, we create a temporary backing file and open it
>>> >               instead of opening 'filename' directly */
>>> >
>>> >  @@ -585,6 +589,10 @@ int bdrv_open(BlockDriverState *bs, const char 
>>> > *filename, int flags,
>>> >
>>> >        /* Find the right image format driver */
>>> >        if (!drv) {
>>> >  +        /* format must be specified for fd: protocol */
>>> >  +        if (bdrv_is_fd_protocol(bs)) {
>>> >  +            return -ENOTSUP;
>>> >  +        }
>> This isn't really required to make fd work.
>> 
> 
> If format is not specified, the file needs to be opened and probed to 
> determine the format.  The proposed SELinux changes should prevent the open 
> if it resides on NFS.

You're mixing together completely different semantics. This is not the "libvirt 
super-uber-great-special block driver", it's an fd driver. Everything SELinux 
should be separate. So again, add an option somewhere that allows you to 
generically disable probing of drivers. Or if all you care about is libvirt, 
simply always pass a format= option.

> 
>>> >            ret = find_image_format(filename,&drv);
>>> >        }
>>> >
>>> >  @@ -1460,6 +1468,11 @@ int bdrv_enable_write_cache(BlockDriverState *bs)
>>> >        return bs->enable_write_cache;
>>> >    }
>>> >
>>> >  +int bdrv_is_fd_protocol(BlockDriverState *bs)
>>> >  +{
>>> >  +    return bs->fd_protocol;
>>> >  +}
>> The generic block layer shouldn't have any special cases based on the
>> format driver. If you need to do something different for fd, think about
>> what property of fd it is that requires the special case (like can't
>> reopen).
>> 
> 
> I'm not sure I completely understand what you're saying here.  I understand 
> the desire to not have special cases based on the fd protocol.  I have a 
> number special case checks that prevent re-opens, which presumably would not 
> be allowed by the proposed SELinux changes. Should there be no special cases 
> at all (e.g. should I provide solutions to all the re-opens now) or would you 
> rather special cases be provided at a different layer?

Whatever you do, don't ever try to make anything but your fd driver aware of fd 
drivers. a NO_REOPEN flag would be one way to achieve what you're trying to do.

> 
>>> >  +
>>> >    /* XXX: no longer used */
>>> >    void bdrv_set_change_cb(BlockDriverState *bs,
>>> >                            void (*change_cb)(void *opaque, int reason),
>>> >  @@ -1964,6 +1977,9 @@ int bdrv_snapshot_create(BlockDriverState *bs,
>>> >        BlockDriver *drv = bs->drv;
>>> >        if (!drv)
>>> >            return -ENOMEDIUM;
>>> >  +    if (bdrv_is_fd_protocol(bs)) {
>>> >  +        return -ENOTSUP;
>>> >  +    }
>> What's the problem with internal snapshots in images accesses over an fd?
>> 
> 
> I was thinking the proposed SELinux changes would prevent the open of the 
> snapshot file if the file corresponding to fd resides on NFS.  But perhaps it 
> is not opened on NFS?

Same as above - this has nothing to do with the fd driver.


Alex