[libvirt] [PATCH v2] 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 a single function which runs stat() before any open() call. Directory pools can then proceed along, ignoring the invalid files. v2: Use ATTRIBUTE_NONNULL. Drop stat check, just open with O_NONBLOCK|O_NOCTTY. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/storage/storage_backend.c | 23 +++++++++++++++++------ src/storage/storage_backend.h | 2 ++ src/storage/storage_backend_fs.c | 7 ++----- src/storage/storage_backend_logical.c | 9 +++------ src/storage/storage_backend_mpath.c | 9 +++------ src/storage/storage_backend_scsi.c | 17 ++++++++--------- 6 files changed, 35 insertions(+), 32 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index f4124df..bbd3375 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -872,6 +872,20 @@ virStorageBackendForType(int type) { return NULL; } +int +virStorageBackendVolOpen(const char *path) +{ + int fd; + + if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) { + virReportSystemError(errno, + _("cannot open volume '%s'"), + path); + return -1; + } + + return fd; +} int virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target, @@ -880,13 +894,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, diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 907c4bc..45683ec 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -80,6 +80,8 @@ struct _virStorageBackend { virStorageBackendPtr virStorageBackendForType(int type); +int virStorageBackendVolOpen(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..1650177 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -61,12 +61,9 @@ virStorageBackendProbeTarget(virStorageVolTargetPtr target, if (encryption) *encryption = NULL; - if ((fd = open(target->path, O_RDONLY)) < 0) { - virReportSystemError(errno, - _("cannot open volume '%s'"), - target->path); + if ((ret = virStorageBackendVolOpen(target->path)) < 0) return -1; - } + fd = ret; if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, allocation, 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 8d0a92a..a62dc3d 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; } -- 1.6.6.1
On 05/21/2010 11:03 AM, 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 a single function which runs stat() before any open() call. Directory
This sentence of the commit log is out-of-date...
pools can then proceed along, ignoring the invalid files.
v2: Use ATTRIBUTE_NONNULL. Drop stat check, just open with O_NONBLOCK|O_NOCTTY.
...given this sentence.
+int +virStorageBackendVolOpen(const char *path) +{ + int fd; + + if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) { + virReportSystemError(errno, + _("cannot open volume '%s'"), + path); + return -1; + }
I'm thinking that we still want to use fstat() (after the open()) and reject directories, sockets, and pipes (or more accurately, accept only regular files, block devices, and possibly character devices). In other words, you got rid of the stat/open race (good), but also got rid of the strictness provided by validating [f]stat (not so good). Probably worth a v3, so we get virStorageBackendVolOpen right. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/21/2010 02:17 PM, Eric Blake wrote:
On 05/21/2010 11:03 AM, 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 a single function which runs stat() before any open() call. Directory
This sentence of the commit log is out-of-date...
pools can then proceed along, ignoring the invalid files.
v2: Use ATTRIBUTE_NONNULL. Drop stat check, just open with O_NONBLOCK|O_NOCTTY.
...given this sentence.
+int +virStorageBackendVolOpen(const char *path) +{ + int fd; + + if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) { + virReportSystemError(errno, + _("cannot open volume '%s'"), + path); + return -1; + }
I'm thinking that we still want to use fstat() (after the open()) and reject directories, sockets, and pipes (or more accurately, accept only regular files, block devices, and possibly character devices).
In other words, you got rid of the stat/open race (good), but also got rid of the strictness provided by validating [f]stat (not so good).
Sorry, I should have pointed out that there is already code that does this, see virStorageBackendUpdateVolTargetInfoFD, which is always called after VolOpen. The existing code is all confusingly arranged though, so maybe I should take a harder look at organizing. - Cole
On Fri, May 21, 2010 at 02:24:24PM -0400, Cole Robinson wrote:
On 05/21/2010 02:17 PM, Eric Blake wrote:
On 05/21/2010 11:03 AM, 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 a single function which runs stat() before any open() call. Directory
This sentence of the commit log is out-of-date...
pools can then proceed along, ignoring the invalid files.
v2: Use ATTRIBUTE_NONNULL. Drop stat check, just open with O_NONBLOCK|O_NOCTTY.
...given this sentence.
+int +virStorageBackendVolOpen(const char *path) +{ + int fd; + + if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) { + virReportSystemError(errno, + _("cannot open volume '%s'"), + path); + return -1; + }
I'm thinking that we still want to use fstat() (after the open()) and reject directories, sockets, and pipes (or more accurately, accept only regular files, block devices, and possibly character devices).
In other words, you got rid of the stat/open race (good), but also got rid of the strictness provided by validating [f]stat (not so good).
Sorry, I should have pointed out that there is already code that does this, see virStorageBackendUpdateVolTargetInfoFD, which is always called after VolOpen. The existing code is all confusingly arranged though, so maybe I should take a harder look at organizing.
That check isn't sufficient. We need to explicitly *allow* directories to be reported, since a pool may point at a directory containing trees for container based virt root filesystems. We should drop block+char devices for directory based pools too, since those aren't relevant, being dealt with by other storage pool backends. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 05/24/2010 06:36 AM, Daniel P. Berrange wrote:
On Fri, May 21, 2010 at 02:24:24PM -0400, Cole Robinson wrote:
On 05/21/2010 02:17 PM, Eric Blake wrote:
On 05/21/2010 11:03 AM, 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 a single function which runs stat() before any open() call. Directory
This sentence of the commit log is out-of-date...
pools can then proceed along, ignoring the invalid files.
v2: Use ATTRIBUTE_NONNULL. Drop stat check, just open with O_NONBLOCK|O_NOCTTY.
...given this sentence.
+int +virStorageBackendVolOpen(const char *path) +{ + int fd; + + if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) { + virReportSystemError(errno, + _("cannot open volume '%s'"), + path); + return -1; + }
I'm thinking that we still want to use fstat() (after the open()) and reject directories, sockets, and pipes (or more accurately, accept only regular files, block devices, and possibly character devices).
In other words, you got rid of the stat/open race (good), but also got rid of the strictness provided by validating [f]stat (not so good).
Sorry, I should have pointed out that there is already code that does this, see virStorageBackendUpdateVolTargetInfoFD, which is always called after VolOpen. The existing code is all confusingly arranged though, so maybe I should take a harder look at organizing.
That check isn't sufficient. We need to explicitly *allow* directories to be reported, since a pool may point at a directory containing trees for container based virt root filesystems. We should drop block+char devices for directory based pools too, since those aren't relevant, being dealt with by other storage pool backends.
I've sent an updated patch, but I only copied the existing mode checks from virStorageBackendUpdateVolTargetInfoFD, so these points aren't addressed. They will take a bit more surgery then I expected, since volumes are opened via several different code paths. - Cole
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Eric Blake