Re: [libvirt] [PATCH v3] Add support for fd: protocol

2011-08-11 Thread Corey Bryant 



On 07/26/2011 08:51 AM, Corey Bryant wrote:

 +static int raw_open_fd(BlockDriverState *bs, const char *filename, 
int flags)

 +{
 +BDRVRawState *s = bs-opaque;
 +const char *fd_str;
 +int fd;
 +
 +/* extract the file descriptor - fail if it's not fd: */
 +if (!strstart(filename, fd:,fd_str)) {
 +return -EINVAL;
 +}
 +
 +if (!qemu_isdigit(fd_str[0])) {
 +/* get fd from monitor */
 +fd = qemu_get_fd(fd_str);
 +if (fd == -1) {
 +return -EBADF;
 +}
 +} else {
 +char *endptr = NULL;
 +
 +fd = strtol(fd_str,endptr, 10);
 +if (*endptr || (fd == 0  fd_str == endptr)) {
 +return -EBADF;
 +}
 +}
 +
 +s-fd = fd;
 +s-type = FTYPE_FILE;
 +
 +return raw_open_common(bs, filename, flags, 0);
 +}
 +
 +static BlockDriver bdrv_file_fd = {
 +.format_name = file,
 +.protocol_name = fd,
 +.instance_size = sizeof(BDRVRawState),
 +.bdrv_probe = NULL, /* no probe for protocols */
 +.bdrv_file_open = raw_open_fd,
 +.bdrv_read = raw_read,
 +.bdrv_write = raw_write,
 +.bdrv_close = raw_close,
 +.bdrv_flush = raw_flush,
 +.bdrv_discard = raw_discard,
 +
 +.bdrv_aio_readv = raw_aio_readv,
 +.bdrv_aio_writev = raw_aio_writev,
 +.bdrv_aio_flush = raw_aio_flush,
 +
 +.bdrv_truncate = raw_truncate,
 +.bdrv_getlength = raw_getlength,
 +
 +.create_options = raw_create_options,
 +};
 +

I'm a bit unsure of how to support CD-ROM with the fd: protocol.

I don't think use of bdrv_file_fd is an option.  For example, while 
raw_open_fd may work, there is no eject support.


Another approach is to have 2 BlockDriver structs that support the fd 
protocol.  For example, creating a new BlockDriver struct that mirrors 
bdrv_host_cdrom.  So you would have bdrv_host_cdrom_fd with a 
corresponding cdrom_open_fd function.  But that doesn't appear possible 
as it appears that the protocol must be unique among all BlockDriver 
structs.


Do we need to introduce a similar protocol (maybe cdfd) to support 
passing of CD-ROM file descriptors?



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v3] Add support for fd: protocol

2011-07-26 Thread 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 cor...@linux.vnet.ibm.com
---
 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;
+}
+
 /* 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;
+}
 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;
+}
+
 /* 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;
+}
 if (drv-bdrv_snapshot_create)
 return drv-bdrv_snapshot_create(bs, sn_info);
 if (bs-file)

Re: [libvirt] [PATCH v3] Add support for fd: protocol

2011-07-26 Thread Eric Blake 

On 07/26/2011 06:51 AM, Corey Bryant wrote:

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.


Libvirt will eventually need support for fd passing on savevm, 
snapshot_blkdev, and change monitor commands, as well as for -cdrom, 
before this feature can be used to provide the desired security 
enhancements.  I agree that for an incremental patch, you don't have to 
solve all points at once, but until all places have been modified to 
support fd usage, you aren't gaining any security, except for severely 
constrained guests.


Furthermore, how do you plan to map fd: to filename?  There's already 
been big threads on why snapshot_blkdev needs both the new fd: and the 
name of the old backing file at the same time, so that qemu can write 
the correct headers into new qcow2 files.  But your proposal precludes 
that, since qemu -drive file=fd:4,format=qcow2 is not letting qemu 
know the file name of fd:4 that would later have to be written into a 
qcow2 header.  I'm afraid that we need a better solution that gets both 
fd and filename mapped together, before this stands a chance of being 
useful.  That said, I'm strongly in favor of getting the open() burden 
moved out of qemu into libvirt, because of the potential it has for 
increased security.


--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3] Add support for fd: protocol

2011-07-26 Thread Kevin Wolf 
Am 26.07.2011 16:00, schrieb Eric Blake:
 On 07/26/2011 06:51 AM, Corey Bryant wrote:
 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.
 
 Libvirt will eventually need support for fd passing on savevm, 
 snapshot_blkdev, and change monitor commands, as well as for -cdrom, 
 before this feature can be used to provide the desired security 
 enhancements.  I agree that for an incremental patch, you don't have to 
 solve all points at once, but until all places have been modified to 
 support fd usage, you aren't gaining any security, except for severely 
 constrained guests.
 
 Furthermore, how do you plan to map fd: to filename?  There's already 
 been big threads on why snapshot_blkdev needs both the new fd: and the 
 name of the old backing file at the same time, so that qemu can write 
 the correct headers into new qcow2 files. 

That's a problem to solve in snapshot_blkdev, not in -drive. In general
qemu doesn't need and shouldn't know the file name if it's meant to use
an fd.

Kevin

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v3] Add support for fd: protocol

2011-07-26 Thread Kevin Wolf 
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 cor...@linux.vnet.ibm.com
 ---
  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?

 +
  /* 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.

  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