[libvirt] [PATCH v2] storage: Check for invalid storage mode before opening
Cole Robinson
crobinso at redhat.com
Mon May 24 21:38:58 UTC 2010
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
More information about the libvir-list
mailing list