2015-11-13 0:30 GMT+02:00 Jiri Denemark <jdenemar@xxxxxxxxxx>:
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;
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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list