storage: Check for invalid storage mode before opening

If a directory pool contains pipes or sockets, a pool start can fail or hang:

https://bugzilla.redhat.com/show_bug.cgi?id=589577

We already try to avoid these special files, but only attempt after
opening the path, which is where the problems lie. Unify volume opening
into helper functions, which use the proper open() flags to avoid error,
followed by fstat to validate storage mode.

Previously, virStorageBackendUpdateVolTargetInfoFD attempted to enforce the
storage mode check, but allowed callers to detect this case and silently
continue. In practice, only the FS backend was using this feature, the rest
were treating unknown mode as an error condition. Unfortunately the InfoFD
function wasn't raising an error message here, so error reporting was
busted.

This patch adds 2 functions: virStorageBackendVolOpen, and
virStorageBackendVolOpenModeSkip. The latter retains the original opt out
semantics, the former now throws an explicit error.

This patch maintains the previous volume mode checks: allowing specific
modes for specific pool types requires a bit of surgery, since VolOpen
is called through several different helper functions.

v2: Use ATTRIBUTE_NONNULL. Drop stat check, just open with
    O_NONBLOCK|O_NOCTTY.

v3: Move mode check logic back to VolOpen. Use 2 VolOpen functions with
    different error semantics.

v4: Make second VolOpen function more extensible. Didn't opt to change
    FS backend defaults, this can just be to fix the original bug.

v5: Prefix default flags with VIR_, use ATTRIBUTE_RETURN_CHECK

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index f4124dfda4eb55082d586bd1aabf76bfa27341e0..aba8937616df9a2af0af1a5848b1ff06e1efaa02 100644 (file)
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -873,20 +873,71 @@ virStorageBackendForType(int type) {
 }
 
 
+/*
+ * Allows caller to silently ignore files with improper mode
+ *
+ * Returns -1 on error, -2 if file mode is unexpected.
+ */
 int
-virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target,
-                                     unsigned long long *allocation,
-                                     unsigned long long *capacity)
+virStorageBackendVolOpenCheckMode(const char *path, unsigned int flags)
 {
-    int ret, fd;
+    int fd, mode = 0;
+    struct stat sb;
 
-    if ((fd = open(target->path, O_RDONLY)) < 0) {
+    if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) {
         virReportSystemError(errno,
                              _("cannot open volume '%s'"),
-                             target->path);
+                             path);
         return -1;
     }
 
+    if (fstat(fd, &sb) < 0) {
+        virReportSystemError(errno,
+                             _("cannot stat file '%s'"),
+                             path);
+        close(fd);
+        return -1;
+    }
+
+    if (S_ISREG(sb.st_mode))
+        mode = VIR_STORAGE_VOL_OPEN_REG;
+    else if (S_ISCHR(sb.st_mode))
+        mode = VIR_STORAGE_VOL_OPEN_CHAR;
+    else if (S_ISBLK(sb.st_mode))
+        mode = VIR_STORAGE_VOL_OPEN_BLOCK;
+
+    if (!(mode & flags)) {
+        close(fd);
+
+        if (mode & VIR_STORAGE_VOL_OPEN_ERROR) {
+            virStorageReportError(VIR_ERR_INTERNAL_ERROR,
+                                  _("unexpected storage mode for '%s'"), path);
+            return -1;
+        }
+
+        return -2;
+    }
+
+    return fd;
+}
+
+int virStorageBackendVolOpen(const char *path)
+{
+    return virStorageBackendVolOpenCheckMode(path,
+                                             VIR_STORAGE_VOL_OPEN_DEFAULT);
+}
+
+int
+virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target,
+                                     unsigned long long *allocation,
+                                     unsigned long long *capacity)
+{
+    int ret, fd;
+
+    if ((ret = virStorageBackendVolOpen(target->path)) < 0)
+        return ret;
+
+    fd = ret;
     ret = virStorageBackendUpdateVolTargetInfoFD(target,
                                                  fd,
                                                  allocation,
@@ -920,12 +971,11 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
  * virStorageBackendUpdateVolTargetInfoFD:
  * @conn: connection to report errors on
  * @target: target definition ptr of volume to update
- * @fd: fd of storage volume to update
+ * @fd: fd of storage volume to update, via virStorageBackendOpenVol*
  * @allocation: If not NULL, updated allocation information will be stored
  * @capacity: If not NULL, updated capacity info will be stored
  *
- * Returns 0 for success-1 on a legitimate error condition,
- *    -2 if passed FD isn't a regular, char, or block file.
+ * Returns 0 for success, -1 on a legitimate error condition.
  */
 int
 virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target,
@@ -945,11 +995,6 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target,
         return -1;
     }
 
-    if (!S_ISREG(sb.st_mode) &&
-        !S_ISCHR(sb.st_mode) &&
-        !S_ISBLK(sb.st_mode))
-        return -2;
-
     if (allocation) {
         if (S_ISREG(sb.st_mode)) {
 #ifndef WIN32

diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
index 907c4bc29a70f84855bb68f2bb5a541ef2635d10..bb4d1c053b7311897111c7354e4dd7ecf4c78849 100644 (file)
--- a/src/storage/storage_backend.h
+++ b/src/storage/storage_backend.h
@@ -80,6 +80,27 @@ struct _virStorageBackend {
 
 virStorageBackendPtr virStorageBackendForType(int type);
 
+int virStorageBackendVolOpen(const char *path)
+ATTRIBUTE_RETURN_CHECK
+ATTRIBUTE_NONNULL(1);
+
+/* VolOpenCheckMode flags */
+enum {
+    VIR_STORAGE_VOL_OPEN_ERROR  = 1 << 0, /* warn if unexpected type
+                                           * encountered */
+    VIR_STORAGE_VOL_OPEN_REG    = 1 << 1, /* regular files okay */
+    VIR_STORAGE_VOL_OPEN_BLOCK  = 1 << 2, /* block files okay */
+    VIR_STORAGE_VOL_OPEN_CHAR   = 1 << 3, /* char files okay */
+};
+
+#define VIR_STORAGE_VOL_OPEN_DEFAULT (VIR_STORAGE_VOL_OPEN_ERROR    |\
+                                      VIR_STORAGE_VOL_OPEN_REG      |\
+                                      VIR_STORAGE_VOL_OPEN_CHAR     |\
+                                      VIR_STORAGE_VOL_OPEN_BLOCK)
+
+int virStorageBackendVolOpenCheckMode(const char *path, unsigned int flags)
+ATTRIBUTE_RETURN_CHECK
+ATTRIBUTE_NONNULL(1);
 
 int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
                                    int withCapacity);

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index c96c4f3eb9c6d832df4415083952fdf038c042f3..f0cd77040ae20fac350d656909330f3a0fb61ac5 100644 (file)
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -61,18 +61,16 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target,
     if (encryption)
         *encryption = NULL;
 
-    if ((fd = open(target->path, O_RDONLY)) < 0) {
-        virReportSystemError(errno,
-                             _("cannot open volume '%s'"),
-                             target->path);
-        return -1;
-    }
+    if ((ret = virStorageBackendVolOpenCheckMode(target->path,
+                                                 (VIR_STORAGE_VOL_OPEN_DEFAULT & ~VIR_STORAGE_VOL_OPEN_ERROR))) < 0)
+        return ret; /* Take care to propagate ret, it is not always -1 */
+    fd = ret;
 
     if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd,
                                                       allocation,
                                                       capacity)) < 0) {
         close(fd);
-        return ret; /* Take care to propagate ret, it is not always -1 */
+        return -1;
     }
 
     memset(&meta, 0, sizeof(meta));

diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
index 06238d194c7deb46f247ddc6cf1627aa1319d20c..616ca1ad3eaf21832431ea0ad620d90e9d7629ae 100644 (file)
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -559,7 +559,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
                                   virStoragePoolObjPtr pool,
                                   virStorageVolDefPtr vol)
 {
-    int fd = -1;
+    int fdret, fd = -1;
     char size[100];
     const char *cmdargvnew[] = {
         LVCREATE, "--name", vol->name, "-L", size,
@@ -602,12 +602,9 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
     if (virRun(cmdargv, NULL) < 0)
         return -1;
 
-    if ((fd = open(vol->target.path, O_RDONLY)) < 0) {
-        virReportSystemError(errno,
-                             _("cannot read path '%s'"),
-                             vol->target.path);
+    if ((fdret = virStorageBackendVolOpen(vol->target.path)) < 0)
         goto cleanup;
-    }
+    fd = fdret;
 
     /* We can only chown/grp if root */
     if (getuid() == 0) {

diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c
index 3a137eb3e849343c5701ab372b08ac0f888d4138..3d7dfcc072ef805f6a9f588c29e91c4c7603f025 100644 (file)
--- a/src/storage/storage_backend_mpath.c
+++ b/src/storage/storage_backend_mpath.c
@@ -44,14 +44,11 @@ virStorageBackendMpathUpdateVolTargetInfo(virStorageVolTargetPtr target,
                                           unsigned long long *capacity)
 {
     int ret = -1;
-    int fd = -1;
+    int fdret, fd = -1;
 
-    if ((fd = open(target->path, O_RDONLY)) < 0) {
-        virReportSystemError(errno,
-                             _("cannot open volume '%s'"),
-                             target->path);
+    if ((fdret = virStorageBackendVolOpen(target->path)) < 0)
         goto out;
-    }
+    fd = fdret;
 
     if (virStorageBackendUpdateVolTargetInfoFD(target,
                                                fd,

diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
index 40f4fd83cab0c16b5a7e81e8fc9d9c14619cb02b..71492cf68fe8ce9d11fc5372a4fab9c4e5e60665 100644 (file)
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -135,14 +135,12 @@ virStorageBackendSCSIUpdateVolTargetInfo(virStorageVolTargetPtr target,
                                          unsigned long long *allocation,
                                          unsigned long long *capacity)
 {
-    int fd, ret = -1;
+    int fdret, fd = -1;
+    int ret = -1;
 
-    if ((fd = open(target->path, O_RDONLY)) < 0) {
-        virReportSystemError(errno,
-                             _("cannot open volume '%s'"),
-                             target->path);
-        return -1;
-    }
+    if ((fdret = virStorageBackendVolOpen(target->path)) < 0)
+        goto cleanup;
+    fd = fdret;
 
     if (virStorageBackendUpdateVolTargetInfoFD(target,
                                                fd,
@@ -155,8 +153,9 @@ virStorageBackendSCSIUpdateVolTargetInfo(virStorageVolTargetPtr target,
 
     ret = 0;
 
-  cleanup:
-    close(fd);
+cleanup:
+    if (fd >= 0)
+        close(fd);
 
     return ret;
 }