[libvirt] [PATCH v2] storage: Check for invalid storage mode before opening
Daniel P. Berrange
berrange at redhat.com
Mon May 24 10:36:56 UTC 2010
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 :|
More information about the libvir-list
mailing list