[libvirt] [PATCH v3] storage: Check for invalid storage mode before opening
Cole Robinson
crobinso at redhat.com
Fri May 28 16:35:46 UTC 2010
On 05/24/2010 05:36 PM, Cole Robinson wrote:
> 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 to improve error reporting.
>
ping, not sure if anyone saw this.
- Cole
> Signed-off-by: Cole Robinson <crobinso at redhat.com>
> ---
> src/storage/storage_backend.c | 74 ++++++++++++++++++++++++++------
> src/storage/storage_backend.h | 5 ++
> src/storage/storage_backend_fs.c | 11 ++---
> src/storage/storage_backend_logical.c | 9 +---
> src/storage/storage_backend_mpath.c | 9 +---
> src/storage/storage_backend_scsi.c | 17 ++++----
> 6 files changed, 83 insertions(+), 42 deletions(-)
>
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index f4124df..702870a 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -872,6 +872,61 @@ virStorageBackendForType(int type) {
> return NULL;
> }
>
> +static int
> +virStorageBackendVolOpenInternal(const char *path)
> +{
> + int fd;
> + struct stat sb;
> +
> + if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) {
> + virReportSystemError(errno,
> + _("cannot open volume '%s'"),
> + 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) &&
> + !S_ISCHR(sb.st_mode) &&
> + !S_ISBLK(sb.st_mode)) {
> + close(fd);
> + return -2;
> + }
> +
> + return fd;
> +}
> +
> +/*
> + * Allows caller to silently ignore files with improper mode
> + *
> + * Returns -1 on error, -2 if file mode is unexpected.
> + */
> +int virStorageBackendVolOpenModeSkip(const char *path)
> +{
> + return virStorageBackendVolOpenInternal(path);
> +}
> +
> +int
> +virStorageBackendVolOpen(const char *path)
> +{
> + int ret;
> +
> + ret = virStorageBackendVolOpenInternal(path);
> + if (ret == -2) {
> + virStorageReportError(VIR_ERR_INTERNAL_ERROR,
> + _("unexpected storage mode for '%s'"), path);
> + return -1;
> + }
> +
> + return ret;
> +}
>
> int
> virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target,
> @@ -880,13 +935,10 @@ virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target,
> {
> int ret, fd;
>
> - if ((fd = open(target->path, O_RDONLY)) < 0) {
> - virReportSystemError(errno,
> - _("cannot open volume '%s'"),
> - target->path);
> - return -1;
> - }
> + if ((ret = virStorageBackendVolOpen(target->path)) < 0)
> + return ret;
>
> + fd = ret;
> ret = virStorageBackendUpdateVolTargetInfoFD(target,
> fd,
> allocation,
> @@ -920,12 +972,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 +996,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 907c4bc..8f54dce 100644
> --- a/src/storage/storage_backend.h
> +++ b/src/storage/storage_backend.h
> @@ -80,6 +80,11 @@ struct _virStorageBackend {
>
> virStorageBackendPtr virStorageBackendForType(int type);
>
> +int virStorageBackendVolOpen(const char *path)
> +ATTRIBUTE_NONNULL(1);
> +
> +int virStorageBackendVolOpenModeSkip(const char *path)
> +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 c96c4f3..5aceb39 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -61,18 +61,15 @@ 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 = virStorageBackendVolOpenModeSkip(target->path)) < 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 06238d1..616ca1a 100644
> --- 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 3a137eb..3d7dfcc 100644
> --- 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 40f4fd8..71492cf 100644
> --- 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;
> }
More information about the libvir-list
mailing list