3:47 p.m.
Greetings,
I was investigating on an issue in which QEMU's dynamic ownership was
not properly working when calling qemuDomainCoreDumpWithFormat().
The core of the issue seems to be the qemuOpenFileAs() function which
does not handle the dynamic ownership. This might affect other libvirt's
features within as well.
The function is quite complicated and I was thinking about refactoring
it a bit in order to simplify the logic allowing to fix the dynamic
ownership handling as well.
In order to refactor it, I was planning to write tests in order to cover
all its use cases before actually changing the code.
The issue is that all the functions within the qemu_driver.c module are
static. I could indeed include the module itself in my tests but I'm not
sure whether this is acceptable.
Furthermore I'd like to have some clarification about the NFS related
code. It seems that some effort has been put in order to tackle
something I'm not aware of. Could someone briefly explain how to
reproduce NFS failing scenarios?
Thank you.
4:30 p.m.
New subject: [libvirt] adding tests to qemu_driver
On Thu, Nov 12, 2015 at 23:47:54 +0200, noxdafox wrote:
Greetings,
I was investigating on an issue in which QEMU's dynamic ownership was
not properly working when calling qemuDomainCoreDumpWithFormat().
Could describe this issue you are investigating?
The core of the issue seems to be the qemuOpenFileAs() function which
does not handle the dynamic ownership. This might affect other libvirt's
features within as well.
Because you are most likely looking at a wrong place; qemuOpenFileAs is
a quite low level function which is just supposed to open/create a file
accessible to a given user. It's up to the caller to decide what the
user should be.
...
The issue is that all the functions within the qemu_driver.c module
are
static. I could indeed include the module itself in my tests but I'm not
sure whether this is acceptable.
We solve this kind of issues by removing "static" from the functions and
adding a new header file (if it doesn't exist yet) called *priv.h
(qemu_driverpriv.h in this case) with the prototypes for such functions.
Only tests are allowed to include such header files.
Furthermore I'd like to have some clarification about the NFS
related
code. It seems that some effort has been put in order to tackle
something I'm not aware of. Could someone briefly explain how to
reproduce NFS failing scenarios?
The main problem with NFS which this ugly function is trying to handle
is called "root-squash". This feature maps all access from UID 0 to an
unprivileged UID. That is, libvirtd (even though it is running as root)
will not be able to access the desired file.
Jirka
2:44 a.m.
New subject: [libvirt] adding tests to qemu_driver
2015-11-13 0:30 GMT+02:00 Jiri Denemark <jdenemar(a)redhat.com>:
On Thu, Nov 12, 2015 at 23:47:54 +0200, noxdafox wrote:
> Greetings,
>
> I was investigating on an issue in which QEMU's dynamic ownership was
> not properly working when calling qemuDomainCoreDumpWithFormat().
Could describe this issue you are investigating?
When calling qemuDomainCoreDumpWithFormat() the file is create as root:root
even when the dynamic ownership is specified in the qemu.conf configuration
file.
> The core of the issue seems to be the qemuOpenFileAs() function which
> does not handle the dynamic ownership. This might affect other libvirt's
> features within as well.
Because you are most likely looking at a wrong place; qemuOpenFileAs is
a quite low level function which is just supposed to open/create a file
accessible to a given user. It's up to the caller to decide what the
user should be.
When debugging the call, the parameters are correctly forwarded:
dynamicOwnership is true and fallback_uid and fallback_gid are correct. The
function itself seems to ignore them when a new file is created.
The following patch I provided on libvirt-users list seems to fix the issue
but I'm not aware of possible side effects. This is why I'd like to provide
some tests for these core functions.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a2cc002..1b47dc6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2932,6 +2932,11 @@ qemuOpenFileAs(uid_t fallback_uid, gid_t
fallback_gid,
if (path_shared <= 0 || dynamicOwnership)
vfoflags |= VIR_FILE_OPEN_FORCE_OWNER;
+ if (dynamicOwnership) {
+ uid = fallback_uid;
+ gid = fallback_gid;
+ }
+
if (stat(path, &sb) == 0) {
/* It already exists, we don't want to delete it on error */
need_unlink = false;
...
> The issue is that all the functions within the qemu_driver.c module are
> static. I could indeed include the module itself in my tests but I'm not
> sure whether this is acceptable.
We solve this kind of issues by removing "static" from the functions and
adding a new header file (if it doesn't exist yet) called *priv.h
(qemu_driverpriv.h in this case) with the prototypes for such functions.
Only tests are allowed to include such header files.
This is a way more elegant solution but I didn't know if it was acceptable
to modify the headers.
> Furthermore I'd like to have some clarification about the
NFS related
> code. It seems that some effort has been put in order to tackle
> something I'm not aware of. Could someone briefly explain how to
> reproduce NFS failing scenarios?
The main problem with NFS which this ugly function is trying to handle
is called "root-squash". This feature maps all access from UID 0 to an
unprivileged UID. That is, libvirtd (even though it is running as root)
will not be able to access the desired file.
This is very helpful thanks!
I'll try to set some tests and provide patches later on. I guess it would
be beneficial for libvirt anyway. About the refactoring we can discuss
further later.
Jirka
6:25 a.m.
New subject: [libvirt] adding tests to qemu_driver
On Fri, Nov 13, 2015 at 10:44:01 +0200, NoxDaFox wrote:
2015-11-13 0:30 GMT+02:00 Jiri Denemark <jdenemar(a)redhat.com>:
> On Thu, Nov 12, 2015 at 23:47:54 +0200, noxdafox wrote:
> > Greetings,
> >
> > I was investigating on an issue in which QEMU's dynamic ownership was
> > not properly working when calling qemuDomainCoreDumpWithFormat().
>
> Could describe this issue you are investigating?
>
When calling qemuDomainCoreDumpWithFormat() the file is create as root:root
even when the dynamic ownership is specified in the qemu.conf configuration
file.
And what is the result? Does dumping fail because of this or does it
work anyway?
Jirka
6:10 a.m.
New subject: [libvirt] adding tests to qemu_driver
2015-11-19 14:25 GMT+02:00 Jiri Denemark <jdenemar(a)redhat.com>:
On Fri, Nov 13, 2015 at 10:44:01 +0200, NoxDaFox wrote:
> 2015-11-13 0:30 GMT+02:00 Jiri Denemark <jdenemar(a)redhat.com>:
>
> > On Thu, Nov 12, 2015 at 23:47:54 +0200, noxdafox wrote:
> > > Greetings,
> > >
> > > I was investigating on an issue in which QEMU's dynamic ownership was
> > > not properly working when calling qemuDomainCoreDumpWithFormat().
> >
> > Could describe this issue you are investigating?
> >
>
> When calling qemuDomainCoreDumpWithFormat() the file is create as
root:root
> even when the dynamic ownership is specified in the qemu.conf
configuration
> file.
And what is the result? Does dumping fail because of this or does it
work anyway?
Jirka
The file gets dumped correctly, nevertheless it's ownership is still
root:root and not the one set in the configuration.
From qemu.conf
# Whether libvirt should dynamically change file ownership
# to match the configured user/group above. Defaults to 1.
# Set to 0 to disable file ownership changes.
dynamic_ownership = 1
Now, given a user and a group, when I run qemuDomainCoreDumpWithFormat() I
get the file still belonging to root:root.
I tested the patch locally and it was working. I could create the file
anywhere and the ownership was right.
What I'd like to do though, is to refactor the function as its logic is a
bit cumbersome. Before that, I'll try to provide a set of tests to help me
during the refactoring.
It will require me a bit of time as I'm not accustomed to the code. I
provided the patch as it is, because it was suggested in the other
discussion.
Matteo.
11:18 a.m.
New subject: [libvirt] adding tests to qemu_driver
On Fri, Nov 20, 2015 at 14:10:49 +0200, NoxDaFox wrote:
2015-11-19 14:25 GMT+02:00 Jiri Denemark
<jdenemar(a)redhat.com>:
> On Fri, Nov 13, 2015 at 10:44:01 +0200, NoxDaFox wrote:
> > 2015-11-13 0:30 GMT+02:00 Jiri Denemark <jdenemar(a)redhat.com>:
> >
> > > On Thu, Nov 12, 2015 at 23:47:54 +0200, noxdafox wrote:
> > > > Greetings,
> > > >
> > > > I was investigating on an issue in which QEMU's dynamic ownership
was
> > > > not properly working when calling qemuDomainCoreDumpWithFormat().
> > >
> > > Could describe this issue you are investigating?
> > >
> >
> > When calling qemuDomainCoreDumpWithFormat() the file is create as
> root:root
> > even when the dynamic ownership is specified in the qemu.conf
> configuration
> > file.
>
> And what is the result? Does dumping fail because of this or does it
> work anyway?
The file gets dumped correctly, nevertheless it's ownership is still
root:root and not the one set in the configuration.
OK, this is actually the correct behavior.
>From qemu.conf
# Whether libvirt should dynamically change file ownership
# to match the configured user/group above. Defaults to 1.
# Set to 0 to disable file ownership changes.
dynamic_ownership = 1
Now, given a user and a group, when I run qemuDomainCoreDumpWithFormat() I
get the file still belonging to root:root.
Right, but (even though the comment doesn't say so) this applies only to
files used by a running QEMU and only when they are used. They are
chowned to root:root (they'd ideally be changed back to the original
user/group, but we are no there yet) once they are no longer used. E.g.,
a disk image file will only be owned by the user/group while it is
attached to a running domain. When the domain is not running, they will
be owned by root:root.
I tested the patch locally and it was working. I could create the
file
anywhere and the ownership was right.
Hmm, you are right, it looks like I didn't read the code carefully
enough. Anyway, the file should be owne by root:root so that other QEMU
processes cannot access it.
What I'd like to do though, is to refactor the function as its
logic
is a bit cumbersome. Before that, I'll try to provide a set of tests
to help me during the refactoring.
Oh sure, the code is a mess and if you have any idea for making saner
and more readable. Creating a test suite first is definitely a good
idea, because we don't want to accidentally change the behavior of this
ugly code.
Jirka
3:52 p.m.
New subject: [libvirt] adding tests to qemu_driver
On 20/11/15 19:18, Jiri Denemark wrote:
On Fri, Nov 20, 2015 at 14:10:49 +0200, NoxDaFox wrote:
> 2015-11-19 14:25 GMT+02:00 Jiri Denemark <jdenemar(a)redhat.com>:
>
>> On Fri, Nov 13, 2015 at 10:44:01 +0200, NoxDaFox wrote:
>>> 2015-11-13 0:30 GMT+02:00 Jiri Denemark <jdenemar(a)redhat.com>:
>>>
>>>> On Thu, Nov 12, 2015 at 23:47:54 +0200, noxdafox wrote:
>>>>> Greetings,
>>>>>
>>>>> I was investigating on an issue in which QEMU's dynamic ownership
was
>>>>> not properly working when calling qemuDomainCoreDumpWithFormat().
>>>> Could describe this issue you are investigating?
>>>>
>>> When calling qemuDomainCoreDumpWithFormat() the file is create as
>> root:root
>>> even when the dynamic ownership is specified in the qemu.conf
>> configuration
>>> file.
>> And what is the result? Does dumping fail because of this or does it
>> work anyway?
> The file gets dumped correctly, nevertheless it's ownership is still
> root:root and not the one set in the configuration.
OK, this is actually the correct behavior.
If so, I'm definitely lost here :)
> >From qemu.conf
>
> # Whether libvirt should dynamically change file ownership
> # to match the configured user/group above. Defaults to 1.
> # Set to 0 to disable file ownership changes.
> dynamic_ownership = 1
>
> Now, given a user and a group, when I run qemuDomainCoreDumpWithFormat() I
> get the file still belonging to root:root.
Right, but (even though the comment doesn't say so) this applies only to
files used by a running QEMU and only when they are used. They are
chowned to root:root (they'd ideally be changed back to the original
user/group, but we are no there yet) once they are no longer used. E.g.,
a disk image file will only be owned by the user/group while it is
attached to a running domain. When the domain is not running, they will
be owned by root:root.
Now I'm confused.
The qemuDomainCoreDumpWithFormat should return a "picture" of the memory
state of a running VM. It's a separate file which does not belong to the
running VM itself. Why permissions should be transient?
Moreover for what I've observed, the file gets root:root and remains so
no matter whether the owner process is running or not.
> I tested the patch locally and it was working. I could create the file
> anywhere and the ownership was right.
Hmm, you are right, it looks like I didn't read the code carefully
enough. Anyway, the file should be owne by root:root so that other QEMU
processes cannot access it.
May I ask why such a policy? Wouldn't be better to leave the decision to
the API user? In my case is making the logic quite complicated.
> What I'd like to do though, is to refactor the function as its logic
> is a bit cumbersome. Before that, I'll try to provide a set of tests
> to help me during the refactoring.
Oh sure, the code is a mess and if you have any idea for making saner
and more readable. Creating a test suite first is definitely a good
idea, because we don't want to accidentally change the behavior of this
ugly code.
Jirka
The code is not *that* bad but it's pretty core logic. The lack of tests
and the fragile logic suggests me this is the right place where to
refactor a bit.
As I said, my free time is quite limited and I need to get accustomed to
libvirt's tests and build process. I'll start exposing the functions to
a private header and to add basic tests for them.
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3327
days inactive
3335
days old
6 comments
3 participants
participants (3)
-
Jiri Denemark -
noxdafox -
NoxDaFox