[libvirt] [PATCH v3 0/5] file descriptor passing using pass-fd
libvirt's sVirt security driver provides SELinux MAC isolation for Qemu guest processes and their corresponding image files. In other words, sVirt uses SELinux to prevent a QEMU process from opening files that do not belong to it. sVirt provides this support by labeling guests and resources with security labels that are stored in file system extended attributes. Some file systems, such as NFS, do not support the extended attribute security namespace, and therefore cannot support sVirt isolation. A solution to this problem is to provide fd passing support, where libvirt opens files and passes file descriptors to QEMU. This, along with SELinux policy to prevent QEMU from opening files, can provide image file isolation for NFS files stored on the same NFS mount. This patch series adds the pass-fd QMP monitor command, which allows an fd to be passed via SCM_RIGHTS, and returns the received file descriptor. Support is also added to the block layer to allow QEMU to dup the fd when the filename is of the /dev/fd/X format. This is useful if MAC policy prevents QEMU from opening specific types of files. One nice thing about this approach is that no new SELinux policy is required to prevent open of NFS files (files with type nfs_t). The virt_use_nfs boolean type simply needs to be set to false, and open will be prevented (and dup will be allowed). For example: # setsebool virt_use_nfs 0 # getsebool virt_use_nfs virt_use_nfs --> off Corey Bryant (5): qapi: Convert getfd and closefd qapi: Add pass-fd QMP command osdep: Enable qemu_open to dup pre-opened fd block: Convert open calls to qemu_open block: Prevent /dev/fd/X filename from being detected as floppy block/raw-posix.c | 22 ++++++++++--------- block/raw-win32.c | 4 ++-- block/vdi.c | 5 +++-- block/vmdk.c | 21 ++++++++---------- block/vpc.c | 2 +- block/vvfat.c | 21 +++++++++--------- hmp-commands.hx | 6 ++---- hmp.c | 18 ++++++++++++++++ hmp.h | 2 ++ monitor.c | 61 +++++++++++++++++++++++++++++++++++++++-------------- osdep.c | 13 ++++++++++++ qapi-schema.json | 54 +++++++++++++++++++++++++++++++++++++++++++++++ qmp-commands.hx | 48 +++++++++++++++++++++++++++++++++++++---- 13 files changed, 216 insertions(+), 61 deletions(-) -- 1.7.10.2
Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> --- v2: -Convert getfd and closefd to QAPI (lcapitulino@redhat.com) -Remove changes that returned fd from getfd (lcapitulino@redhat.com) -Wrap hmp_* functions around qmp_* functions (kwolf@redhat.com) -Move hmp_* functions to hmp.c (lcapitulino@redhat.com) -Drop .user_print lines (lcapitulino@redhat.com) -Use 'cmd' instead of 'cmd_new' for HMP (lcapitulino@redhat.com) -Change QMP command existance back to 0.14 (lcapitulino@redhat.com) v3: -Remove unnecessary return statements from qmp_* functions (lcapitulino@redhat.com) -Add notes to QMP command describing behavior in more detail (lcapitulino@redhat.com, eblake@redhat.com) hmp-commands.hx | 6 ++---- hmp.c | 18 ++++++++++++++++++ hmp.h | 2 ++ monitor.c | 32 ++++++++++++++------------------ qapi-schema.json | 35 +++++++++++++++++++++++++++++++++++ qmp-commands.hx | 14 ++++++++++---- 6 files changed, 81 insertions(+), 26 deletions(-) diff --git a/hmp-commands.hx b/hmp-commands.hx index f5d9d91..eea8b32 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1236,8 +1236,7 @@ ETEXI .args_type = "fdname:s", .params = "getfd name", .help = "receive a file descriptor via SCM rights and assign it a name", - .user_print = monitor_user_noop, - .mhandler.cmd_new = do_getfd, + .mhandler.cmd = hmp_getfd, }, STEXI @@ -1253,8 +1252,7 @@ ETEXI .args_type = "fdname:s", .params = "closefd name", .help = "close a file descriptor previously passed via SCM rights", - .user_print = monitor_user_noop, - .mhandler.cmd_new = do_closefd, + .mhandler.cmd = hmp_closefd, }, STEXI diff --git a/hmp.c b/hmp.c index 2ce8cb9..6241856 100644 --- a/hmp.c +++ b/hmp.c @@ -999,3 +999,21 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict) qmp_netdev_del(id, &err); hmp_handle_error(mon, &err); } + +void hmp_getfd(Monitor *mon, const QDict *qdict) +{ + const char *fdname = qdict_get_str(qdict, "fdname"); + Error *errp = NULL; + + qmp_getfd(fdname, &errp); + hmp_handle_error(mon, &errp); +} + +void hmp_closefd(Monitor *mon, const QDict *qdict) +{ + const char *fdname = qdict_get_str(qdict, "fdname"); + Error *errp = NULL; + + qmp_closefd(fdname, &errp); + hmp_handle_error(mon, &errp); +} diff --git a/hmp.h b/hmp.h index 79d138d..8d2b0d7 100644 --- a/hmp.h +++ b/hmp.h @@ -64,5 +64,7 @@ void hmp_device_del(Monitor *mon, const QDict *qdict); void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict); void hmp_netdev_add(Monitor *mon, const QDict *qdict); void hmp_netdev_del(Monitor *mon, const QDict *qdict); +void hmp_getfd(Monitor *mon, const QDict *qdict); +void hmp_closefd(Monitor *mon, const QDict *qdict); #endif diff --git a/monitor.c b/monitor.c index a3bc2c7..1a7f7e7 100644 --- a/monitor.c +++ b/monitor.c @@ -2182,48 +2182,45 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict) } #endif -static int do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data) +void qmp_getfd(const char *fdname, Error **errp) { - const char *fdname = qdict_get_str(qdict, "fdname"); mon_fd_t *monfd; int fd; - fd = qemu_chr_fe_get_msgfd(mon->chr); + fd = qemu_chr_fe_get_msgfd(cur_mon->chr); if (fd == -1) { - qerror_report(QERR_FD_NOT_SUPPLIED); - return -1; + error_set(errp, QERR_FD_NOT_SUPPLIED); + return; } if (qemu_isdigit(fdname[0])) { - qerror_report(QERR_INVALID_PARAMETER_VALUE, "fdname", - "a name not starting with a digit"); - return -1; + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname", + "a name not starting with a digit"); + return; } - QLIST_FOREACH(monfd, &mon->fds, next) { + QLIST_FOREACH(monfd, &cur_mon->fds, next) { if (strcmp(monfd->name, fdname) != 0) { continue; } close(monfd->fd); monfd->fd = fd; - return 0; + return; } monfd = g_malloc0(sizeof(mon_fd_t)); monfd->name = g_strdup(fdname); monfd->fd = fd; - QLIST_INSERT_HEAD(&mon->fds, monfd, next); - return 0; + QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next); } -static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data) +void qmp_closefd(const char *fdname, Error **errp) { - const char *fdname = qdict_get_str(qdict, "fdname"); mon_fd_t *monfd; - QLIST_FOREACH(monfd, &mon->fds, next) { + QLIST_FOREACH(monfd, &cur_mon->fds, next) { if (strcmp(monfd->name, fdname) != 0) { continue; } @@ -2232,11 +2229,10 @@ static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data) close(monfd->fd); g_free(monfd->name); g_free(monfd); - return 0; + return; } - qerror_report(QERR_FD_NOT_FOUND, fdname); - return -1; + error_set(errp, QERR_FD_NOT_FOUND, fdname); } static void do_loadvm(Monitor *mon, const QDict *qdict) diff --git a/qapi-schema.json b/qapi-schema.json index 3b6e346..26a6b84 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1862,3 +1862,38 @@ # Since: 0.14.0 ## { 'command': 'netdev_del', 'data': {'id': 'str'} } + +## +# @getfd: +# +# Receive a file descriptor via SCM rights and assign it a name +# +# @fdname: file descriptor name +# +# Returns: Nothing on success +# If file descriptor was not received, FdNotSupplied +# If @fdname is not valid, InvalidParameterType +# +# Since: 0.14.0 +# +# Notes: If @fdname already exists, the file descriptor assigned to +# it will be closed and replaced by the received file +# descriptor. +# The 'closefd' command can be used to explicitly close the +# file descriptor when it is no longer needed. +## +{ 'command': 'getfd', 'data': {'fdname': 'str'} } + +## +# @closefd: +# +# Close a file descriptor previously passed via SCM rights +# +# @fdname: file descriptor name +# +# Returns: Nothing on success +# If @fdname is not found, FdNotFound +# +# Since: 0.14.0 +## +{ 'command': 'closefd', 'data': {'fdname': 'str'} } diff --git a/qmp-commands.hx b/qmp-commands.hx index 2e1a38e..e3cf3c5 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -873,8 +873,7 @@ EQMP .args_type = "fdname:s", .params = "getfd name", .help = "receive a file descriptor via SCM rights and assign it a name", - .user_print = monitor_user_noop, - .mhandler.cmd_new = do_getfd, + .mhandler.cmd_new = qmp_marshal_input_getfd, }, SQMP @@ -892,6 +891,14 @@ Example: -> { "execute": "getfd", "arguments": { "fdname": "fd1" } } <- { "return": {} } +Notes: + +(1) If the name specified by the "fdname" argument already exists, + the file descriptor assigned to it will be closed and replaced + by the received file descriptor. +(2) The 'closefd' command can be used to explicitly close the file + descriptor when it is no longer needed. + EQMP { @@ -899,8 +906,7 @@ EQMP .args_type = "fdname:s", .params = "closefd name", .help = "close a file descriptor previously passed via SCM rights", - .user_print = monitor_user_noop, - .mhandler.cmd_new = do_closefd, + .mhandler.cmd_new = qmp_marshal_input_closefd, }, SQMP -- 1.7.10.2
This patch adds the pass-fd QMP command using the QAPI framework. Like the getfd command, it is used to pass a file descriptor via SCM_RIGHTS. However, the pass-fd command also returns the received file descriptor, which is a difference in behavior from the getfd command, which returns nothing. The closefd command can be used to close a file descriptor that was passed with the pass-fd command. Note that when using getfd or pass-fd, there are some commands (e.g. migrate with fd:name) that implicitly close the named fd. When this is not the case, closefd must be used to avoid fd leaks. Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> --- v2: -Introduce new QMP command to pass/return fd (lcapitulino@redhat.com) -Use passfd as command name (berrange@redhat.com) v3: -Use pass-fd as command name (lcapitulino@redhat.com) -Fail pass-fd if fdname already exists (lcapitulino@redhat.com) -Add notes to QMP command describing behavior in more detail (lcapitulino@redhat.com, eblake@redhat.com) -Add note about fd leakage (eblake@redhat.com) monitor.c | 33 +++++++++++++++++++++++++++++++++ qapi-schema.json | 19 +++++++++++++++++++ qmp-commands.hx | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+) diff --git a/monitor.c b/monitor.c index 1a7f7e7..6d99368 100644 --- a/monitor.c +++ b/monitor.c @@ -2182,6 +2182,39 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict) } #endif +int64_t qmp_pass_fd(const char *fdname, Error **errp) +{ + mon_fd_t *monfd; + int fd; + + fd = qemu_chr_fe_get_msgfd(cur_mon->chr); + if (fd == -1) { + error_set(errp, QERR_FD_NOT_SUPPLIED); + return -1; + } + + if (qemu_isdigit(fdname[0])) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname", + "a name not starting with a digit"); + return -1; + } + + QLIST_FOREACH(monfd, &cur_mon->fds, next) { + if (strcmp(monfd->name, fdname) == 0) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname", + "a name that does not already exist"); + return -1; + } + } + + monfd = g_malloc0(sizeof(mon_fd_t)); + monfd->name = g_strdup(fdname); + monfd->fd = fd; + + QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next); + return fd; +} + void qmp_getfd(const char *fdname, Error **errp) { mon_fd_t *monfd; diff --git a/qapi-schema.json b/qapi-schema.json index 26a6b84..ed99f23 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1864,6 +1864,25 @@ { 'command': 'netdev_del', 'data': {'id': 'str'} } ## +# @pass-fd: +# +# Pass a file descriptor via SCM rights and assign it a name +# +# @fdname: file descriptor name +# +# Returns: The QEMU file descriptor that was received +# If file descriptor was not received, FdNotSupplied +# If @fdname is not valid, InvalidParameterType +# +# Since: 1.2.0 +# +# Notes: If @fdname already exists, the command will fail. +# The 'closefd' command can be used to explicitly close the +# file descriptor when it is no longer needed. +## +{ 'command': 'pass-fd', 'data': {'fdname': 'str'}, 'returns': 'int' } + +## # @getfd: # # Receive a file descriptor via SCM rights and assign it a name diff --git a/qmp-commands.hx b/qmp-commands.hx index e3cf3c5..c039947 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -869,6 +869,40 @@ Example: EQMP { + .name = "pass-fd", + .args_type = "fdname:s", + .params = "pass-fd name", + .help = "pass a file descriptor via SCM rights and assign it a name", + .mhandler.cmd_new = qmp_marshal_input_pass_fd, + }, + +SQMP +pass-fd +------- + +Pass a file descriptor via SCM rights and assign it a name. + +Arguments: + +- "fdname": file descriptor name (json-string) + +Return a json-int with the QEMU file descriptor that was received. + +Example: + +-> { "execute": "pass-fd", "arguments": { "fdname": "fd1" } } +<- { "return": 42 } + +Notes: + +(1) If the name specified by the "fdname" argument already exists, + the command will fail. +(2) The 'closefd' command can be used to explicitly close the file + descriptor when it is no longer needed. + +EQMP + + { .name = "getfd", .args_type = "fdname:s", .params = "getfd name", -- 1.7.10.2
On Thu, 14 Jun 2012 11:55:02 -0400 Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
This patch adds the pass-fd QMP command using the QAPI framework. Like the getfd command, it is used to pass a file descriptor via SCM_RIGHTS. However, the pass-fd command also returns the received file descriptor, which is a difference in behavior from the getfd command, which returns nothing.
The closefd command can be used to close a file descriptor that was passed with the pass-fd command.
Note that when using getfd or pass-fd, there are some commands (e.g. migrate with fd:name) that implicitly close the named fd. When this is not the case, closefd must be used to avoid fd leaks.
Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> --- v2: -Introduce new QMP command to pass/return fd (lcapitulino@redhat.com) -Use passfd as command name (berrange@redhat.com)
v3: -Use pass-fd as command name (lcapitulino@redhat.com) -Fail pass-fd if fdname already exists (lcapitulino@redhat.com) -Add notes to QMP command describing behavior in more detail (lcapitulino@redhat.com, eblake@redhat.com) -Add note about fd leakage (eblake@redhat.com)
monitor.c | 33 +++++++++++++++++++++++++++++++++ qapi-schema.json | 19 +++++++++++++++++++ qmp-commands.hx | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+)
diff --git a/monitor.c b/monitor.c index 1a7f7e7..6d99368 100644 --- a/monitor.c +++ b/monitor.c @@ -2182,6 +2182,39 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict) } #endif
+int64_t qmp_pass_fd(const char *fdname, Error **errp) +{ + mon_fd_t *monfd; + int fd; + + fd = qemu_chr_fe_get_msgfd(cur_mon->chr); + if (fd == -1) { + error_set(errp, QERR_FD_NOT_SUPPLIED); + return -1; + } + + if (qemu_isdigit(fdname[0])) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname", + "a name not starting with a digit"); + return -1; + } + + QLIST_FOREACH(monfd, &cur_mon->fds, next) { + if (strcmp(monfd->name, fdname) == 0) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname", + "a name that does not already exist"); + return -1; + } + }
Returning the same error class for two different errors is not a good idea. I think you have two options here. You could return QERR_INVALID_PARAMETER for the "already exists" case or introduce QERR_FD_EXISTS. The later is certainly nicer, but we were trying to avoid having too specific errors...
+ + monfd = g_malloc0(sizeof(mon_fd_t)); + monfd->name = g_strdup(fdname); + monfd->fd = fd;
Maybe you could try to move this to a separate function to share code with qmp_getfd()?
+ + QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next); + return fd; +} + void qmp_getfd(const char *fdname, Error **errp) { mon_fd_t *monfd; diff --git a/qapi-schema.json b/qapi-schema.json index 26a6b84..ed99f23 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1864,6 +1864,25 @@ { 'command': 'netdev_del', 'data': {'id': 'str'} }
## +# @pass-fd: +# +# Pass a file descriptor via SCM rights and assign it a name +# +# @fdname: file descriptor name +# +# Returns: The QEMU file descriptor that was received +# If file descriptor was not received, FdNotSupplied +# If @fdname is not valid, InvalidParameterType +# +# Since: 1.2.0 +# +# Notes: If @fdname already exists, the command will fail. +# The 'closefd' command can be used to explicitly close the +# file descriptor when it is no longer needed. +## +{ 'command': 'pass-fd', 'data': {'fdname': 'str'}, 'returns': 'int' } + +## # @getfd: # # Receive a file descriptor via SCM rights and assign it a name diff --git a/qmp-commands.hx b/qmp-commands.hx index e3cf3c5..c039947 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -869,6 +869,40 @@ Example: EQMP
{ + .name = "pass-fd", + .args_type = "fdname:s", + .params = "pass-fd name", + .help = "pass a file descriptor via SCM rights and assign it a name", + .mhandler.cmd_new = qmp_marshal_input_pass_fd, + }, + +SQMP +pass-fd +------- + +Pass a file descriptor via SCM rights and assign it a name. + +Arguments: + +- "fdname": file descriptor name (json-string) + +Return a json-int with the QEMU file descriptor that was received. + +Example: + +-> { "execute": "pass-fd", "arguments": { "fdname": "fd1" } } +<- { "return": 42 } + +Notes: + +(1) If the name specified by the "fdname" argument already exists, + the command will fail. +(2) The 'closefd' command can be used to explicitly close the file + descriptor when it is no longer needed. + +EQMP + + { .name = "getfd", .args_type = "fdname:s", .params = "getfd name",
On 06/15/2012 10:32 AM, Luiz Capitulino wrote:
On Thu, 14 Jun 2012 11:55:02 -0400 Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
This patch adds the pass-fd QMP command using the QAPI framework. Like the getfd command, it is used to pass a file descriptor via SCM_RIGHTS. However, the pass-fd command also returns the received file descriptor, which is a difference in behavior from the getfd command, which returns nothing.
The closefd command can be used to close a file descriptor that was passed with the pass-fd command.
Note that when using getfd or pass-fd, there are some commands (e.g. migrate with fd:name) that implicitly close the named fd. When this is not the case, closefd must be used to avoid fd leaks.
Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> --- v2: -Introduce new QMP command to pass/return fd (lcapitulino@redhat.com) -Use passfd as command name (berrange@redhat.com)
v3: -Use pass-fd as command name (lcapitulino@redhat.com) -Fail pass-fd if fdname already exists (lcapitulino@redhat.com) -Add notes to QMP command describing behavior in more detail (lcapitulino@redhat.com, eblake@redhat.com) -Add note about fd leakage (eblake@redhat.com)
monitor.c | 33 +++++++++++++++++++++++++++++++++ qapi-schema.json | 19 +++++++++++++++++++ qmp-commands.hx | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+)
diff --git a/monitor.c b/monitor.c index 1a7f7e7..6d99368 100644 --- a/monitor.c +++ b/monitor.c @@ -2182,6 +2182,39 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict) } #endif
+int64_t qmp_pass_fd(const char *fdname, Error **errp) +{ + mon_fd_t *monfd; + int fd; + + fd = qemu_chr_fe_get_msgfd(cur_mon->chr); + if (fd == -1) { + error_set(errp, QERR_FD_NOT_SUPPLIED); + return -1; + } + + if (qemu_isdigit(fdname[0])) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname", + "a name not starting with a digit"); + return -1; + } + + QLIST_FOREACH(monfd, &cur_mon->fds, next) { + if (strcmp(monfd->name, fdname) == 0) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname", + "a name that does not already exist"); + return -1; + } + }
Returning the same error class for two different errors is not a good idea. I think you have two options here. You could return QERR_INVALID_PARAMETER for the "already exists" case or introduce QERR_FD_EXISTS. The later is certainly nicer, but we were trying to avoid having too specific errors...
I'm not clear on what the problem is with returning the same error class for two different errors. Could you explain? I don't have a problem changing it if it's an issue.
+ + monfd = g_malloc0(sizeof(mon_fd_t)); + monfd->name = g_strdup(fdname); + monfd->fd = fd;
Maybe you could try to move this to a separate function to share code with qmp_getfd()?
Sure, no problem. I can do that.
+ + QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next); + return fd; +} + void qmp_getfd(const char *fdname, Error **errp) { mon_fd_t *monfd; diff --git a/qapi-schema.json b/qapi-schema.json index 26a6b84..ed99f23 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1864,6 +1864,25 @@ { 'command': 'netdev_del', 'data': {'id': 'str'} }
## +# @pass-fd: +# +# Pass a file descriptor via SCM rights and assign it a name +# +# @fdname: file descriptor name +# +# Returns: The QEMU file descriptor that was received +# If file descriptor was not received, FdNotSupplied +# If @fdname is not valid, InvalidParameterType +# +# Since: 1.2.0 +# +# Notes: If @fdname already exists, the command will fail. +# The 'closefd' command can be used to explicitly close the +# file descriptor when it is no longer needed. +## +{ 'command': 'pass-fd', 'data': {'fdname': 'str'}, 'returns': 'int' } + +## # @getfd: # # Receive a file descriptor via SCM rights and assign it a name diff --git a/qmp-commands.hx b/qmp-commands.hx index e3cf3c5..c039947 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -869,6 +869,40 @@ Example: EQMP
{ + .name = "pass-fd", + .args_type = "fdname:s", + .params = "pass-fd name", + .help = "pass a file descriptor via SCM rights and assign it a name", + .mhandler.cmd_new = qmp_marshal_input_pass_fd, + }, + +SQMP +pass-fd +------- + +Pass a file descriptor via SCM rights and assign it a name. + +Arguments: + +- "fdname": file descriptor name (json-string) + +Return a json-int with the QEMU file descriptor that was received. + +Example: + +-> { "execute": "pass-fd", "arguments": { "fdname": "fd1" } } +<- { "return": 42 } + +Notes: + +(1) If the name specified by the "fdname" argument already exists, + the command will fail. +(2) The 'closefd' command can be used to explicitly close the file + descriptor when it is no longer needed. + +EQMP + + { .name = "getfd", .args_type = "fdname:s", .params = "getfd name",
-- Regards, Corey
On Fri, 15 Jun 2012 11:04:16 -0400 Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
On 06/15/2012 10:32 AM, Luiz Capitulino wrote:
On Thu, 14 Jun 2012 11:55:02 -0400 Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
This patch adds the pass-fd QMP command using the QAPI framework. Like the getfd command, it is used to pass a file descriptor via SCM_RIGHTS. However, the pass-fd command also returns the received file descriptor, which is a difference in behavior from the getfd command, which returns nothing.
The closefd command can be used to close a file descriptor that was passed with the pass-fd command.
Note that when using getfd or pass-fd, there are some commands (e.g. migrate with fd:name) that implicitly close the named fd. When this is not the case, closefd must be used to avoid fd leaks.
Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> --- v2: -Introduce new QMP command to pass/return fd (lcapitulino@redhat.com) -Use passfd as command name (berrange@redhat.com)
v3: -Use pass-fd as command name (lcapitulino@redhat.com) -Fail pass-fd if fdname already exists (lcapitulino@redhat.com) -Add notes to QMP command describing behavior in more detail (lcapitulino@redhat.com, eblake@redhat.com) -Add note about fd leakage (eblake@redhat.com)
monitor.c | 33 +++++++++++++++++++++++++++++++++ qapi-schema.json | 19 +++++++++++++++++++ qmp-commands.hx | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+)
diff --git a/monitor.c b/monitor.c index 1a7f7e7..6d99368 100644 --- a/monitor.c +++ b/monitor.c @@ -2182,6 +2182,39 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict) } #endif
+int64_t qmp_pass_fd(const char *fdname, Error **errp) +{ + mon_fd_t *monfd; + int fd; + + fd = qemu_chr_fe_get_msgfd(cur_mon->chr); + if (fd == -1) { + error_set(errp, QERR_FD_NOT_SUPPLIED); + return -1; + } + + if (qemu_isdigit(fdname[0])) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname", + "a name not starting with a digit"); + return -1; + } + + QLIST_FOREACH(monfd, &cur_mon->fds, next) { + if (strcmp(monfd->name, fdname) == 0) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname", + "a name that does not already exist"); + return -1; + } + }
Returning the same error class for two different errors is not a good idea. I think you have two options here. You could return QERR_INVALID_PARAMETER for the "already exists" case or introduce QERR_FD_EXISTS. The later is certainly nicer, but we were trying to avoid having too specific errors...
I'm not clear on what the problem is with returning the same error class for two different errors. Could you explain? I don't have a problem changing it if it's an issue.
Because an mngt app/user won't know if what has happened was that the fd name is invalid or if the fd name already exists.
+ + monfd = g_malloc0(sizeof(mon_fd_t)); + monfd->name = g_strdup(fdname); + monfd->fd = fd;
Maybe you could try to move this to a separate function to share code with qmp_getfd()?
Sure, no problem. I can do that.
+ + QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next); + return fd; +} + void qmp_getfd(const char *fdname, Error **errp) { mon_fd_t *monfd; diff --git a/qapi-schema.json b/qapi-schema.json index 26a6b84..ed99f23 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1864,6 +1864,25 @@ { 'command': 'netdev_del', 'data': {'id': 'str'} }
## +# @pass-fd: +# +# Pass a file descriptor via SCM rights and assign it a name +# +# @fdname: file descriptor name +# +# Returns: The QEMU file descriptor that was received +# If file descriptor was not received, FdNotSupplied +# If @fdname is not valid, InvalidParameterType +# +# Since: 1.2.0 +# +# Notes: If @fdname already exists, the command will fail. +# The 'closefd' command can be used to explicitly close the +# file descriptor when it is no longer needed. +## +{ 'command': 'pass-fd', 'data': {'fdname': 'str'}, 'returns': 'int' } + +## # @getfd: # # Receive a file descriptor via SCM rights and assign it a name diff --git a/qmp-commands.hx b/qmp-commands.hx index e3cf3c5..c039947 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -869,6 +869,40 @@ Example: EQMP
{ + .name = "pass-fd", + .args_type = "fdname:s", + .params = "pass-fd name", + .help = "pass a file descriptor via SCM rights and assign it a name", + .mhandler.cmd_new = qmp_marshal_input_pass_fd, + }, + +SQMP +pass-fd +------- + +Pass a file descriptor via SCM rights and assign it a name. + +Arguments: + +- "fdname": file descriptor name (json-string) + +Return a json-int with the QEMU file descriptor that was received. + +Example: + +-> { "execute": "pass-fd", "arguments": { "fdname": "fd1" } } +<- { "return": 42 } + +Notes: + +(1) If the name specified by the "fdname" argument already exists, + the command will fail. +(2) The 'closefd' command can be used to explicitly close the file + descriptor when it is no longer needed. + +EQMP + + { .name = "getfd", .args_type = "fdname:s", .params = "getfd name",
On 06/15/2012 11:14 AM, Luiz Capitulino wrote:
On Fri, 15 Jun 2012 11:04:16 -0400 Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
On 06/15/2012 10:32 AM, Luiz Capitulino wrote:
On Thu, 14 Jun 2012 11:55:02 -0400 Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
This patch adds the pass-fd QMP command using the QAPI framework. Like the getfd command, it is used to pass a file descriptor via SCM_RIGHTS. However, the pass-fd command also returns the received file descriptor, which is a difference in behavior from the getfd command, which returns nothing.
The closefd command can be used to close a file descriptor that was passed with the pass-fd command.
Note that when using getfd or pass-fd, there are some commands (e.g. migrate with fd:name) that implicitly close the named fd. When this is not the case, closefd must be used to avoid fd leaks.
Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> --- v2: -Introduce new QMP command to pass/return fd (lcapitulino@redhat.com) -Use passfd as command name (berrange@redhat.com)
v3: -Use pass-fd as command name (lcapitulino@redhat.com) -Fail pass-fd if fdname already exists (lcapitulino@redhat.com) -Add notes to QMP command describing behavior in more detail (lcapitulino@redhat.com, eblake@redhat.com) -Add note about fd leakage (eblake@redhat.com)
monitor.c | 33 +++++++++++++++++++++++++++++++++ qapi-schema.json | 19 +++++++++++++++++++ qmp-commands.hx | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+)
diff --git a/monitor.c b/monitor.c index 1a7f7e7..6d99368 100644 --- a/monitor.c +++ b/monitor.c @@ -2182,6 +2182,39 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict) } #endif
+int64_t qmp_pass_fd(const char *fdname, Error **errp) +{ + mon_fd_t *monfd; + int fd; + + fd = qemu_chr_fe_get_msgfd(cur_mon->chr); + if (fd == -1) { + error_set(errp, QERR_FD_NOT_SUPPLIED); + return -1; + } + + if (qemu_isdigit(fdname[0])) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname", + "a name not starting with a digit"); + return -1; + } + + QLIST_FOREACH(monfd, &cur_mon->fds, next) { + if (strcmp(monfd->name, fdname) == 0) { + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname", + "a name that does not already exist"); + return -1; + } + }
Returning the same error class for two different errors is not a good idea. I think you have two options here. You could return QERR_INVALID_PARAMETER for the "already exists" case or introduce QERR_FD_EXISTS. The later is certainly nicer, but we were trying to avoid having too specific errors...
I'm not clear on what the problem is with returning the same error class for two different errors. Could you explain? I don't have a problem changing it if it's an issue.
Because an mngt app/user won't know if what has happened was that the fd name is invalid or if the fd name already exists.
I agree, it makes sense so the management app can program based on the error class. It just seems like the error classes in general should be able to be used multiple times if it fits, especially if we're trying to avoid having too specific errors. Maybe (in the future) something like a sub-class could be used? In other words to allow using the same error class with unique sub-classes. I'll plan on updating to use a different error class for this.
+ + monfd = g_malloc0(sizeof(mon_fd_t)); + monfd->name = g_strdup(fdname); + monfd->fd = fd;
Maybe you could try to move this to a separate function to share code with qmp_getfd()?
Sure, no problem. I can do that.
+ + QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next); + return fd; +} + void qmp_getfd(const char *fdname, Error **errp) { mon_fd_t *monfd; diff --git a/qapi-schema.json b/qapi-schema.json index 26a6b84..ed99f23 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1864,6 +1864,25 @@ { 'command': 'netdev_del', 'data': {'id': 'str'} }
## +# @pass-fd: +# +# Pass a file descriptor via SCM rights and assign it a name +# +# @fdname: file descriptor name +# +# Returns: The QEMU file descriptor that was received +# If file descriptor was not received, FdNotSupplied +# If @fdname is not valid, InvalidParameterType +# +# Since: 1.2.0 +# +# Notes: If @fdname already exists, the command will fail. +# The 'closefd' command can be used to explicitly close the +# file descriptor when it is no longer needed. +## +{ 'command': 'pass-fd', 'data': {'fdname': 'str'}, 'returns': 'int' } + +## # @getfd: # # Receive a file descriptor via SCM rights and assign it a name diff --git a/qmp-commands.hx b/qmp-commands.hx index e3cf3c5..c039947 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -869,6 +869,40 @@ Example: EQMP
{ + .name = "pass-fd", + .args_type = "fdname:s", + .params = "pass-fd name", + .help = "pass a file descriptor via SCM rights and assign it a name", + .mhandler.cmd_new = qmp_marshal_input_pass_fd, + }, + +SQMP +pass-fd +------- + +Pass a file descriptor via SCM rights and assign it a name. + +Arguments: + +- "fdname": file descriptor name (json-string) + +Return a json-int with the QEMU file descriptor that was received. + +Example: + +-> { "execute": "pass-fd", "arguments": { "fdname": "fd1" } } +<- { "return": 42 } + +Notes: + +(1) If the name specified by the "fdname" argument already exists, + the command will fail. +(2) The 'closefd' command can be used to explicitly close the file + descriptor when it is no longer needed. + +EQMP + + { .name = "getfd", .args_type = "fdname:s", .params = "getfd name",
-- Regards, Corey
On Fri, 15 Jun 2012 11:29:03 -0400 Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
I'm not clear on what the problem is with returning the same error class for two different errors. Could you explain? I don't have a problem changing it if it's an issue.
Because an mngt app/user won't know if what has happened was that the fd name is invalid or if the fd name already exists.
I agree, it makes sense so the management app can program based on the error class. It just seems like the error classes in general should be able to be used multiple times if it fits, especially if we're trying to avoid having too specific errors.
Maybe (in the future) something like a sub-class could be used? In other words to allow using the same error class with unique sub-classes.
There's a discussion going on which is more or less the same topic: http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg02243.html
This patch adds support to qemu_open to dup(fd) a pre-opened file descriptor if the filename is of the format /dev/fd/X. This can be used when QEMU is restricted from opening files, and the management application opens files on QEMU's behalf. If the fd was passed to the monitor with the pass-fd command, it must be explicitly closed with the 'closefd' command when it is no longer required, in order to prevent fd leaks. Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> --- v2: -Get rid of file_open and move dup code to qemu_open (kwolf@redhat.com) -Use strtol wrapper instead of atoi (kwolf@redhat.com) v3: -Add note about fd leakage (eblake@redhat.com) osdep.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/osdep.c b/osdep.c index 3e6bada..c17cdcb 100644 --- a/osdep.c +++ b/osdep.c @@ -82,6 +82,19 @@ int qemu_open(const char *name, int flags, ...) int ret; int mode = 0; +#ifndef _WIN32 + const char *p; + + /* Attempt dup of fd for pre-opened file */ + if (strstart(name, "/dev/fd/", &p)) { + ret = qemu_parse_fd(p); + if (ret == -1) { + return -1; + } + return dup(ret); + } +#endif + if (flags & O_CREAT) { va_list ap; -- 1.7.10.2
On 06/14/2012 09:55 AM, Corey Bryant wrote:
This patch adds support to qemu_open to dup(fd) a pre-opened file descriptor if the filename is of the format /dev/fd/X.
+++ b/osdep.c @@ -82,6 +82,19 @@ int qemu_open(const char *name, int flags, ...) int ret; int mode = 0;
+#ifndef _WIN32 + const char *p; + + /* Attempt dup of fd for pre-opened file */ + if (strstart(name, "/dev/fd/", &p)) { + ret = qemu_parse_fd(p); + if (ret == -1) { + return -1; + } + return dup(ret);
I think you need to honor flags so that the end use of the fd will be as if qemu had directly opened the file, rather than just doing a blind dup with a resulting fd that is in a different state than the caller expected. I can think of at least the following cases (there may be more): if (flags & O_TRUNCATE || ((flags & (O_CREAT|O_EXCL)) == (O_CREAT|O_EXCL)) ftruncate(ret, 0); Why do I truncate on O_CREAT|O_EXCL? Because that's a case where open() guarantees that the file will have just been created empty. if (flags & O_CLOEXEC) use fcntl(F_DUPFD_CLOEXEC) instead of dup(), if possible else fallback to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC non-atomically Oh, and are we using MSG_CMSG_CLOEXEC where possible (and where not possible, falling back to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC) on all fds received by 'getfd' and 'pass-fd'? I can't think of any reason why 'migrate fd:name' would need to be inheritable, and in the case of /dev/fd/ parsing, while the dup() result may need to be inheritable, the original that we are dup'ing from should most certainly be cloexec. if (flags & O_NONBLOCK) use fcntl(F_GETFL/F_SETFL) to set O_NONBLOCK else use fcntl(F_GETFL/F_SETFL) to clear O_NONBLOCK or maybe we document that callers of pass-fd must always pass fds with O_NONBLOCK clear instead of clearing it ourselves. Or maybe we make sure part of the process of tying name with fd in the lookup list of named fds is determining the current O_NONBLOCK state in case future qemu_open() need it in the opposite state. Treat O_APPEND similarly to O_NONBLOCK (with fcntl(F_GETFL/F_SETFL) to match the desired open() setting). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/15/2012 11:16 AM, Eric Blake wrote:
On 06/14/2012 09:55 AM, Corey Bryant wrote:
This patch adds support to qemu_open to dup(fd) a pre-opened file descriptor if the filename is of the format /dev/fd/X.
+++ b/osdep.c @@ -82,6 +82,19 @@ int qemu_open(const char *name, int flags, ...) int ret; int mode = 0;
+#ifndef _WIN32 + const char *p; + + /* Attempt dup of fd for pre-opened file */ + if (strstart(name, "/dev/fd/", &p)) { + ret = qemu_parse_fd(p); + if (ret == -1) { + return -1; + } + return dup(ret);
I think you need to honor flags so that the end use of the fd will be as if qemu had directly opened the file, rather than just doing a blind dup with a resulting fd that is in a different state than the caller expected. I can think of at least the following cases (there may be more):
I was thinking libvirt would handle all the flag settings on open (obviously since that's how I coded it). I think you're right with this approach though as QEMU will re-open the same file various times with different flags. There are some flags that I don't think we'll be able to change. For example: O_RDONLY, O_WRONLY, O_RDWR. I assume libvirt would open all files O_RDWR.
if (flags & O_TRUNCATE || ((flags & (O_CREAT|O_EXCL)) == (O_CREAT|O_EXCL)) ftruncate(ret, 0);
Why do I truncate on O_CREAT|O_EXCL? Because that's a case where open() guarantees that the file will have just been created empty.
That makes sense.
if (flags & O_CLOEXEC) use fcntl(F_DUPFD_CLOEXEC) instead of dup(), if possible else fallback to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC non-atomically
That makes sense too.
Oh, and are we using MSG_CMSG_CLOEXEC where possible (and where not possible, falling back to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC) on all fds received by 'getfd' and 'pass-fd'? I can't think of any reason why 'migrate fd:name' would need to be inheritable, and in the case of /dev/fd/ parsing, while the dup() result may need to be inheritable, the original that we are dup'ing from should most certainly be cloexec.
It doesn't look like we use MSG_CMSG_CLOEXEC anywhere in QEMU. I don't think we can modify getfd at this point (compatibility) but we could update pass-fd to set it. It may make more sense to set it with fcntl(FD_CLOEXEC) in qmp_pass_fd().
if (flags & O_NONBLOCK) use fcntl(F_GETFL/F_SETFL) to set O_NONBLOCK else use fcntl(F_GETFL/F_SETFL) to clear O_NONBLOCK
or maybe we document that callers of pass-fd must always pass fds with O_NONBLOCK clear instead of clearing it ourselves. Or maybe we make sure part of the process of tying name with fd in the lookup list of named fds is determining the current O_NONBLOCK state in case future qemu_open() need it in the opposite state.
Just documenting it seems error-prone. Why not just set/clear it based on the flag passed to qemu_open?
Treat O_APPEND similarly to O_NONBLOCK (with fcntl(F_GETFL/F_SETFL) to match the desired open() setting).
Ok -- Regards, Corey
On 06/15/2012 12:16 PM, Corey Bryant wrote:
I think you need to honor flags so that the end use of the fd will be as if qemu had directly opened the file, rather than just doing a blind dup with a resulting fd that is in a different state than the caller expected. I can think of at least the following cases (there may be more):
I was thinking libvirt would handle all the flag settings on open (obviously since that's how I coded it). I think you're right with this approach though as QEMU will re-open the same file various times with different flags.
How will libvirt know whether qemu wanted to open a file with O_TRUNCATE vs. reusing an entire file, or with O_NONBLOCK or not, and so forth? I think there are just too many qmeu_open() calls with different flags to expect libvirt to know how to pre-open all possible fds in such a manner that /dev/fd/nnn will be a valid replacement for what qemu would have done, in the cases where qemu can instead correct flags itself.
There are some flags that I don't think we'll be able to change. For example: O_RDONLY, O_WRONLY, O_RDWR. I assume libvirt would open all files O_RDWR.
I agree that this can't be changed, so this is one case where libvirt will have to know what the file will used for. But it is also a case where qemu can at least check whether the fd passed in has sufficient permissions (fcntl(F_GETFL)) for what qemu wanted to use, and should error out here rather than silently succeed here then have a weird EBADF failure down the road when the dup'd fd is finally used. You are right that libvirt should always be safe in passing in an O_RDWR fd for whatever qemu wants, although that may represent security holes (there's reasons for O_RDONLY); and in cases where libvirt is instead passing in one end of a pipe, the fd will necessarily be O_RDONLY or O_WRONLY (since you can't have an O_RDWR pipe).
Oh, and are we using MSG_CMSG_CLOEXEC where possible (and where not possible, falling back to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC) on all fds received by 'getfd' and 'pass-fd'? I can't think of any reason why 'migrate fd:name' would need to be inheritable, and in the case of /dev/fd/ parsing, while the dup() result may need to be inheritable, the original that we are dup'ing from should most certainly be cloexec.
It doesn't look like we use MSG_CMSG_CLOEXEC anywhere in QEMU. I don't think we can modify getfd at this point (compatibility) but we could update pass-fd to set it. It may make more sense to set it with fcntl(FD_CLOEXEC) in qmp_pass_fd().
That's not atomic. If we don't care about atomicity (for example, if we can guarantee that no other thread in qemu can possibly fork/exec in between the monitor thread receiving an fd then converting it to cloexec, based on how we hold a mutex), then that's fine. OR, if we make sure _all_ fork() calls sanitize themselves, by closing all fds in the getfd/pass-fd list prior to calling exec(), then we don't have to even worry about cloexec (but then you have to worry about locking the getfd name list, since locking a list to iterate it is not an async-safe action and probably can't be done between fork() and exec()). Otherwise, using MSG_CMSG_CLOEXEC is the only safe way to avoid leaking unintended fds into another thread's child process.
if (flags & O_NONBLOCK) use fcntl(F_GETFL/F_SETFL) to set O_NONBLOCK else use fcntl(F_GETFL/F_SETFL) to clear O_NONBLOCK
or maybe we document that callers of pass-fd must always pass fds with O_NONBLOCK clear instead of clearing it ourselves. Or maybe we make sure part of the process of tying name with fd in the lookup list of named fds is determining the current O_NONBLOCK state in case future qemu_open() need it in the opposite state.
Just documenting it seems error-prone. Why not just set/clear it based on the flag passed to qemu_open?
Yep, back to my argument that making libvirt have to know every context that qemu will want to open, and what flags it would be using in those contexts, is painful compared to having qemu just do the right thing in the first place, or gracefully erroring out right away at the point of the qemu_open(/dev/fd/nnn). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/15/2012 02:42 PM, Eric Blake wrote:
On 06/15/2012 12:16 PM, Corey Bryant wrote:
I think you need to honor flags so that the end use of the fd will be as if qemu had directly opened the file, rather than just doing a blind dup with a resulting fd that is in a different state than the caller expected. I can think of at least the following cases (there may be more):
I was thinking libvirt would handle all the flag settings on open (obviously since that's how I coded it). I think you're right with this approach though as QEMU will re-open the same file various times with different flags.
How will libvirt know whether qemu wanted to open a file with O_TRUNCATE vs. reusing an entire file, or with O_NONBLOCK or not, and so forth? I think there are just too many qmeu_open() calls with different flags to expect libvirt to know how to pre-open all possible fds in such a manner that /dev/fd/nnn will be a valid replacement for what qemu would have done, in the cases where qemu can instead correct flags itself.
I agree completely.
There are some flags that I don't think we'll be able to change. For example: O_RDONLY, O_WRONLY, O_RDWR. I assume libvirt would open all files O_RDWR.
I agree that this can't be changed, so this is one case where libvirt will have to know what the file will used for. But it is also a case where qemu can at least check whether the fd passed in has sufficient permissions (fcntl(F_GETFL)) for what qemu wanted to use, and should error out here rather than silently succeed here then have a weird EBADF failure down the road when the dup'd fd is finally used. You are right that libvirt should always be safe in passing in an O_RDWR fd for whatever qemu wants, although that may represent security holes (there's reasons for O_RDONLY); and in cases where libvirt is instead passing in one end of a pipe, the fd will necessarily be O_RDONLY or O_WRONLY (since you can't have an O_RDWR pipe).
Good points. In addition to flag setting, I'll add some flag checking for those flags that can't be set (ie. O_RDONLY, O_WRONLY).
Oh, and are we using MSG_CMSG_CLOEXEC where possible (and where not possible, falling back to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC) on all fds received by 'getfd' and 'pass-fd'? I can't think of any reason why 'migrate fd:name' would need to be inheritable, and in the case of /dev/fd/ parsing, while the dup() result may need to be inheritable, the original that we are dup'ing from should most certainly be cloexec.
It doesn't look like we use MSG_CMSG_CLOEXEC anywhere in QEMU. I don't think we can modify getfd at this point (compatibility) but we could update pass-fd to set it. It may make more sense to set it with fcntl(FD_CLOEXEC) in qmp_pass_fd().
That's not atomic. If we don't care about atomicity (for example, if we can guarantee that no other thread in qemu can possibly fork/exec in between the monitor thread receiving an fd then converting it to cloexec, based on how we hold a mutex), then that's fine. OR, if we make sure _all_ fork() calls sanitize themselves, by closing all fds in the getfd/pass-fd list prior to calling exec(), then we don't have to even worry about cloexec (but then you have to worry about locking the getfd name list, since locking a list to iterate it is not an async-safe action and probably can't be done between fork() and exec()). Otherwise, using MSG_CMSG_CLOEXEC is the only safe way to avoid leaking unintended fds into another thread's child process.
Ok I'm sold. I'll first look into setting MSG_CMSG_CLOEXEC.
if (flags & O_NONBLOCK) use fcntl(F_GETFL/F_SETFL) to set O_NONBLOCK else use fcntl(F_GETFL/F_SETFL) to clear O_NONBLOCK
or maybe we document that callers of pass-fd must always pass fds with O_NONBLOCK clear instead of clearing it ourselves. Or maybe we make sure part of the process of tying name with fd in the lookup list of named fds is determining the current O_NONBLOCK state in case future qemu_open() need it in the opposite state.
Just documenting it seems error-prone. Why not just set/clear it based on the flag passed to qemu_open?
Yep, back to my argument that making libvirt have to know every context that qemu will want to open, and what flags it would be using in those contexts, is painful compared to having qemu just do the right thing in the first place, or gracefully erroring out right away at the point of the qemu_open(/dev/fd/nnn).
I agree. -- Regards, Corey
Am 15.06.2012 20:16, schrieb Corey Bryant:
On 06/15/2012 11:16 AM, Eric Blake wrote:
On 06/14/2012 09:55 AM, Corey Bryant wrote:
This patch adds support to qemu_open to dup(fd) a pre-opened file descriptor if the filename is of the format /dev/fd/X.
+++ b/osdep.c @@ -82,6 +82,19 @@ int qemu_open(const char *name, int flags, ...) int ret; int mode = 0;
+#ifndef _WIN32 + const char *p; + + /* Attempt dup of fd for pre-opened file */ + if (strstart(name, "/dev/fd/", &p)) { + ret = qemu_parse_fd(p); + if (ret == -1) { + return -1; + } + return dup(ret);
I think you need to honor flags so that the end use of the fd will be as if qemu had directly opened the file, rather than just doing a blind dup with a resulting fd that is in a different state than the caller expected. I can think of at least the following cases (there may be more):
I was thinking libvirt would handle all the flag settings on open (obviously since that's how I coded it). I think you're right with this approach though as QEMU will re-open the same file various times with different flags.
There are some flags that I don't think we'll be able to change. For example: O_RDONLY, O_WRONLY, O_RDWR. I assume libvirt would open all files O_RDWR.
I think we need to check all of them and fail qemu_open() if they don't match. Those that qemu can change, should be just changed, of course.
Oh, and are we using MSG_CMSG_CLOEXEC where possible (and where not possible, falling back to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC) on all fds received by 'getfd' and 'pass-fd'? I can't think of any reason why 'migrate fd:name' would need to be inheritable, and in the case of /dev/fd/ parsing, while the dup() result may need to be inheritable, the original that we are dup'ing from should most certainly be cloexec.
It doesn't look like we use MSG_CMSG_CLOEXEC anywhere in QEMU. I don't think we can modify getfd at this point (compatibility) but we could update pass-fd to set it. It may make more sense to set it with fcntl(FD_CLOEXEC) in qmp_pass_fd().
In which scenario would any client break if we set FD_CLOEXEC? I don't think compatibility means we can't fix any bugs.
if (flags & O_NONBLOCK) use fcntl(F_GETFL/F_SETFL) to set O_NONBLOCK else use fcntl(F_GETFL/F_SETFL) to clear O_NONBLOCK
or maybe we document that callers of pass-fd must always pass fds with O_NONBLOCK clear instead of clearing it ourselves. Or maybe we make sure part of the process of tying name with fd in the lookup list of named fds is determining the current O_NONBLOCK state in case future qemu_open() need it in the opposite state.
Just documenting it seems error-prone. Why not just set/clear it based on the flag passed to qemu_open?
I agree. We could just check and return an error if they aren't set correctly, but I think adjusting the flags is nicer. Kevin
On 06/15/2012 02:46 PM, Kevin Wolf wrote:
Am 15.06.2012 20:16, schrieb Corey Bryant:
On 06/15/2012 11:16 AM, Eric Blake wrote:
On 06/14/2012 09:55 AM, Corey Bryant wrote:
This patch adds support to qemu_open to dup(fd) a pre-opened file descriptor if the filename is of the format /dev/fd/X.
+++ b/osdep.c @@ -82,6 +82,19 @@ int qemu_open(const char *name, int flags, ...) int ret; int mode = 0;
+#ifndef _WIN32 + const char *p; + + /* Attempt dup of fd for pre-opened file */ + if (strstart(name, "/dev/fd/", &p)) { + ret = qemu_parse_fd(p); + if (ret == -1) { + return -1; + } + return dup(ret);
I think you need to honor flags so that the end use of the fd will be as if qemu had directly opened the file, rather than just doing a blind dup with a resulting fd that is in a different state than the caller expected. I can think of at least the following cases (there may be more):
I was thinking libvirt would handle all the flag settings on open (obviously since that's how I coded it). I think you're right with this approach though as QEMU will re-open the same file various times with different flags.
There are some flags that I don't think we'll be able to change. For example: O_RDONLY, O_WRONLY, O_RDWR. I assume libvirt would open all files O_RDWR.
I think we need to check all of them and fail qemu_open() if they don't match. Those that qemu can change, should be just changed, of course.
Ok. I remember a scenario where QEMU opens a file read-only (perhaps to check headers and determine the file format) before re-opening it read-write. Perhaps this is only when format= isn't specified with -drive. I'm thinking we may need to change flags to read-write where they used to be read-only, in some circumstances.
Oh, and are we using MSG_CMSG_CLOEXEC where possible (and where not possible, falling back to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC) on all fds received by 'getfd' and 'pass-fd'? I can't think of any reason why 'migrate fd:name' would need to be inheritable, and in the case of /dev/fd/ parsing, while the dup() result may need to be inheritable, the original that we are dup'ing from should most certainly be cloexec.
It doesn't look like we use MSG_CMSG_CLOEXEC anywhere in QEMU. I don't think we can modify getfd at this point (compatibility) but we could update pass-fd to set it. It may make more sense to set it with fcntl(FD_CLOEXEC) in qmp_pass_fd().
In which scenario would any client break if we set FD_CLOEXEC? I don't think compatibility means we can't fix any bugs.
I don't know if it breaks any client. Maybe it's not a compatibility error. It dopes change behavior down the line though. If you think it's ok to set FD_CLOEXEC for getfd too, then I'm happy to do it.
if (flags & O_NONBLOCK) use fcntl(F_GETFL/F_SETFL) to set O_NONBLOCK else use fcntl(F_GETFL/F_SETFL) to clear O_NONBLOCK
or maybe we document that callers of pass-fd must always pass fds with O_NONBLOCK clear instead of clearing it ourselves. Or maybe we make sure part of the process of tying name with fd in the lookup list of named fds is determining the current O_NONBLOCK state in case future qemu_open() need it in the opposite state.
Just documenting it seems error-prone. Why not just set/clear it based on the flag passed to qemu_open?
I agree. We could just check and return an error if they aren't set correctly, but I think adjusting the flags is nicer.
Kevin
Ok thanks for the input! -- Regards, Corey
On 06/15/2012 01:19 PM, Corey Bryant wrote:
There are some flags that I don't think we'll be able to change. For example: O_RDONLY, O_WRONLY, O_RDWR. I assume libvirt would open all files O_RDWR.
I think we need to check all of them and fail qemu_open() if they don't match. Those that qemu can change, should be just changed, of course.
Ok. I remember a scenario where QEMU opens a file read-only (perhaps to check headers and determine the file format) before re-opening it read-write. Perhaps this is only when format= isn't specified with -drive. I'm thinking we may need to change flags to read-write where they used to be read-only, in some circumstances.
In those situations, libvirt would pass fd with O_RDWR, and qemu_open() would be fine requesting O_RDONLY the first time (subset is okay), and O_RDWR the second time. Where you have to error out is where libvirt passes O_RDONLY but qemu wants O_RDWR, and so forth.
In which scenario would any client break if we set FD_CLOEXEC? I don't think compatibility means we can't fix any bugs.
I don't know if it breaks any client. Maybe it's not a compatibility error. It dopes change behavior down the line though. If you think it's ok to set FD_CLOEXEC for getfd too, then I'm happy to do it.
The only case that a client might break is if there were a way to pass an fd into qemu and then intentionally see that fd in a child process of qemu. But in the case of 'migrate fd:nnn', you aren't spawning a child process, and even in the case of 'migrate exec:command' (which libvirt no longer uses if fd:nnn works), I don't see how the client could have ever intentionally tried to use 'getfd' in advance to pass an extra fd for use inside the 'exec:command' child. Besides, before 'pass-fd' was around, how would the management app triggering the 'exec:command' even know what fd number would accidentally be inherited into the exec:command child? I think it is pretty much a straight bug-fix for 'getfd' to always set FD_CLOEXEC, and preferably set it atomically via MSG_CMSG_CLOEXEC. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/15/2012 04:00 PM, Eric Blake wrote:
On 06/15/2012 01:19 PM, Corey Bryant wrote:
There are some flags that I don't think we'll be able to change. For example: O_RDONLY, O_WRONLY, O_RDWR. I assume libvirt would open all files O_RDWR.
I think we need to check all of them and fail qemu_open() if they don't match. Those that qemu can change, should be just changed, of course.
Ok. I remember a scenario where QEMU opens a file read-only (perhaps to check headers and determine the file format) before re-opening it read-write. Perhaps this is only when format= isn't specified with -drive. I'm thinking we may need to change flags to read-write where they used to be read-only, in some circumstances.
In those situations, libvirt would pass fd with O_RDWR, and qemu_open() would be fine requesting O_RDONLY the first time (subset is okay), and O_RDWR the second time. Where you have to error out is where libvirt passes O_RDONLY but qemu wants O_RDWR, and so forth.
I'll plan on going with this approach.
In which scenario would any client break if we set FD_CLOEXEC? I don't think compatibility means we can't fix any bugs.
I don't know if it breaks any client. Maybe it's not a compatibility error. It dopes change behavior down the line though. If you think
s/dopes/does
it's ok to set FD_CLOEXEC for getfd too, then I'm happy to do it.
The only case that a client might break is if there were a way to pass an fd into qemu and then intentionally see that fd in a child process of qemu. But in the case of 'migrate fd:nnn', you aren't spawning a child process, and even in the case of 'migrate exec:command' (which libvirt no longer uses if fd:nnn works), I don't see how the client could have ever intentionally tried to use 'getfd' in advance to pass an extra fd for use inside the 'exec:command' child. Besides, before 'pass-fd' was around, how would the management app triggering the 'exec:command' even know what fd number would accidentally be inherited into the exec:command child? I think it is pretty much a straight bug-fix for 'getfd' to always set FD_CLOEXEC, and preferably set it atomically via MSG_CMSG_CLOEXEC.
Alright, I'll go ahead and make this update in the next version of the patch series. Thanks for all the input! -- Regards, Corey
Am 15.06.2012 22:00, schrieb Eric Blake:
On 06/15/2012 01:19 PM, Corey Bryant wrote:
There are some flags that I don't think we'll be able to change. For example: O_RDONLY, O_WRONLY, O_RDWR. I assume libvirt would open all files O_RDWR.
I think we need to check all of them and fail qemu_open() if they don't match. Those that qemu can change, should be just changed, of course.
Ok. I remember a scenario where QEMU opens a file read-only (perhaps to check headers and determine the file format) before re-opening it read-write. Perhaps this is only when format= isn't specified with -drive. I'm thinking we may need to change flags to read-write where they used to be read-only, in some circumstances.
In those situations, libvirt would pass fd with O_RDWR, and qemu_open() would be fine requesting O_RDONLY the first time (subset is okay), and O_RDWR the second time. Where you have to error out is where libvirt passes O_RDONLY but qemu wants O_RDWR, and so forth.
Let's try it with requiring an exact match first. If you pass the format, I think the probing is completely avoided indeed, and having read-only images really opened O_RDONLY protects against stupid mistakes. Or if we really need to open the file for probing, maybe we could add a flag that relaxes the check and that isn't used in the real bdrv_open(). Kevin
On 06/18/2012 04:10 AM, Kevin Wolf wrote:
Am 15.06.2012 22:00, schrieb Eric Blake:
On 06/15/2012 01:19 PM, Corey Bryant wrote:
There are some flags that I don't think we'll be able to change. For example: O_RDONLY, O_WRONLY, O_RDWR. I assume libvirt would open all files O_RDWR.
I think we need to check all of them and fail qemu_open() if they don't match. Those that qemu can change, should be just changed, of course.
Ok. I remember a scenario where QEMU opens a file read-only (perhaps to check headers and determine the file format) before re-opening it read-write. Perhaps this is only when format= isn't specified with -drive. I'm thinking we may need to change flags to read-write where they used to be read-only, in some circumstances.
In those situations, libvirt would pass fd with O_RDWR, and qemu_open() would be fine requesting O_RDONLY the first time (subset is okay), and O_RDWR the second time. Where you have to error out is where libvirt passes O_RDONLY but qemu wants O_RDWR, and so forth.
Let's try it with requiring an exact match first. If you pass the format, I think the probing is completely avoided indeed, and having read-only images really opened O_RDONLY protects against stupid mistakes.
Or if we really need to open the file for probing, maybe we could add a flag that relaxes the check and that isn't used in the real bdrv_open().
Kevin
I haven't heard any objection to this so I'll be checking for exact match, and implementing a flag to relax the check only if it's necessary. -- Regards, Corey
This patch converts all block layer open calls to qemu_open. This enables all block layer open paths to dup(X) a pre-opened file descriptor if the filename is of the format /dev/fd/X. This is useful if QEMU is restricted from opening certain files. Note that this adds the O_CLOEXEC flag to the changed open paths when the O_CLOEXEC macro is defined. Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> --- v2: -Convert calls to qemu_open instead of file_open (kwolf@redhat.com) -Mention introduction of O_CLOEXEC (kwolf@redhat.com) v3: -No changes block/raw-posix.c | 18 +++++++++--------- block/raw-win32.c | 4 ++-- block/vdi.c | 5 +++-- block/vmdk.c | 21 +++++++++------------ block/vpc.c | 2 +- block/vvfat.c | 21 +++++++++++---------- 6 files changed, 35 insertions(+), 36 deletions(-) diff --git a/block/raw-posix.c b/block/raw-posix.c index 03fcfcc..d8eff2f 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -568,8 +568,8 @@ static int raw_create(const char *filename, QEMUOptionParameter *options) options++; } - fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, - 0644); + fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, + 0644); if (fd < 0) { result = -errno; } else { @@ -741,7 +741,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags) if ( bsdPath[ 0 ] != '\0' ) { strcat(bsdPath,"s0"); /* some CDs don't have a partition 0 */ - fd = open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE); + fd = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE); if (fd < 0) { bsdPath[strlen(bsdPath)-1] = '1'; } else { @@ -798,7 +798,7 @@ static int fd_open(BlockDriverState *bs) #endif return -EIO; } - s->fd = open(bs->filename, s->open_flags & ~O_NONBLOCK); + s->fd = qemu_open(bs->filename, s->open_flags & ~O_NONBLOCK); if (s->fd < 0) { s->fd_error_time = get_clock(); s->fd_got_error = 1; @@ -872,7 +872,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options) options++; } - fd = open(filename, O_WRONLY | O_BINARY); + fd = qemu_open(filename, O_WRONLY | O_BINARY); if (fd < 0) return -errno; @@ -950,7 +950,7 @@ static int floppy_probe_device(const char *filename) if (strstart(filename, "/dev/fd", NULL)) prio = 50; - fd = open(filename, O_RDONLY | O_NONBLOCK); + fd = qemu_open(filename, O_RDONLY | O_NONBLOCK); if (fd < 0) { goto out; } @@ -1003,7 +1003,7 @@ static void floppy_eject(BlockDriverState *bs, bool eject_flag) close(s->fd); s->fd = -1; } - fd = open(bs->filename, s->open_flags | O_NONBLOCK); + fd = qemu_open(bs->filename, s->open_flags | O_NONBLOCK); if (fd >= 0) { if (ioctl(fd, FDEJECT, 0) < 0) perror("FDEJECT"); @@ -1053,7 +1053,7 @@ static int cdrom_probe_device(const char *filename) int prio = 0; struct stat st; - fd = open(filename, O_RDONLY | O_NONBLOCK); + fd = qemu_open(filename, O_RDONLY | O_NONBLOCK); if (fd < 0) { goto out; } @@ -1177,7 +1177,7 @@ static int cdrom_reopen(BlockDriverState *bs) */ if (s->fd >= 0) close(s->fd); - fd = open(bs->filename, s->open_flags, 0644); + fd = qemu_open(bs->filename, s->open_flags, 0644); if (fd < 0) { s->fd = -1; return -EIO; diff --git a/block/raw-win32.c b/block/raw-win32.c index e4b0b75..8d7838d 100644 --- a/block/raw-win32.c +++ b/block/raw-win32.c @@ -255,8 +255,8 @@ static int raw_create(const char *filename, QEMUOptionParameter *options) options++; } - fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, - 0644); + fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, + 0644); if (fd < 0) return -EIO; set_sparse(fd); diff --git a/block/vdi.c b/block/vdi.c index 119d3c7..6183fdf 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -648,8 +648,9 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options) options++; } - fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, - 0644); + fd = qemu_open(filename, + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, + 0644); if (fd < 0) { return -errno; } diff --git a/block/vmdk.c b/block/vmdk.c index 18e9b4c..557dc1b 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1161,10 +1161,9 @@ static int vmdk_create_extent(const char *filename, int64_t filesize, VMDK4Header header; uint32_t tmp, magic, grains, gd_size, gt_size, gt_count; - fd = open( - filename, - O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, - 0644); + fd = qemu_open(filename, + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, + 0644); if (fd < 0) { return -errno; } @@ -1484,15 +1483,13 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options) (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4), total_size / (int64_t)(63 * 16 * 512)); if (split || flat) { - fd = open( - filename, - O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, - 0644); + fd = qemu_open(filename, + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, + 0644); } else { - fd = open( - filename, - O_WRONLY | O_BINARY | O_LARGEFILE, - 0644); + fd = qemu_open(filename, + O_WRONLY | O_BINARY | O_LARGEFILE, + 0644); } if (fd < 0) { return -errno; diff --git a/block/vpc.c b/block/vpc.c index 5cd13d1..60ebf5a 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -678,7 +678,7 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options) } /* Create the file */ - fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644); + fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644); if (fd < 0) { return -EIO; } diff --git a/block/vvfat.c b/block/vvfat.c index 0fd3367..81c3a19 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -1152,16 +1152,17 @@ static inline mapping_t* find_mapping_for_cluster(BDRVVVFATState* s,int cluster_ static int open_file(BDRVVVFATState* s,mapping_t* mapping) { if(!mapping) - return -1; + return -1; if(!s->current_mapping || - strcmp(s->current_mapping->path,mapping->path)) { - /* open file */ - int fd = open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE); - if(fd<0) - return -1; - vvfat_close_current_file(s); - s->current_fd = fd; - s->current_mapping = mapping; + strcmp(s->current_mapping->path, mapping->path)) { + /* open file */ + int fd = qemu_open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE); + if (fd < 0) { + return -1; + } + vvfat_close_current_file(s); + s->current_fd = fd; + s->current_mapping = mapping; } return 0; } @@ -2215,7 +2216,7 @@ static int commit_one_file(BDRVVVFATState* s, for (i = s->cluster_size; i < offset; i += s->cluster_size) c = modified_fat_get(s, c); - fd = open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666); + fd = qemu_open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666); if (fd < 0) { fprintf(stderr, "Could not open %s... (%s, %d)\n", mapping->path, strerror(errno), errno); -- 1.7.10.2
On Thu, 14 Jun 2012 11:55:04 -0400 Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
This patch converts all block layer open calls to qemu_open. This enables all block layer open paths to dup(X) a pre-opened file descriptor if the filename is of the format /dev/fd/X. This is useful if QEMU is restricted from opening certain files.
Note that this adds the O_CLOEXEC flag to the changed open paths when the O_CLOEXEC macro is defined.
Very minor comment, but I'd exchange this with the previous patch. Because this change is not only related to the /dev/fd/X feature and also because as soon as you introduce the /dev/fd/X it will work for all file formats.
Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> --- v2: -Convert calls to qemu_open instead of file_open (kwolf@redhat.com) -Mention introduction of O_CLOEXEC (kwolf@redhat.com)
v3: -No changes
block/raw-posix.c | 18 +++++++++--------- block/raw-win32.c | 4 ++-- block/vdi.c | 5 +++-- block/vmdk.c | 21 +++++++++------------ block/vpc.c | 2 +- block/vvfat.c | 21 +++++++++++---------- 6 files changed, 35 insertions(+), 36 deletions(-)
diff --git a/block/raw-posix.c b/block/raw-posix.c index 03fcfcc..d8eff2f 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -568,8 +568,8 @@ static int raw_create(const char *filename, QEMUOptionParameter *options) options++; }
- fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, - 0644); + fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, + 0644); if (fd < 0) { result = -errno; } else { @@ -741,7 +741,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags) if ( bsdPath[ 0 ] != '\0' ) { strcat(bsdPath,"s0"); /* some CDs don't have a partition 0 */ - fd = open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE); + fd = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE); if (fd < 0) { bsdPath[strlen(bsdPath)-1] = '1'; } else { @@ -798,7 +798,7 @@ static int fd_open(BlockDriverState *bs) #endif return -EIO; } - s->fd = open(bs->filename, s->open_flags & ~O_NONBLOCK); + s->fd = qemu_open(bs->filename, s->open_flags & ~O_NONBLOCK); if (s->fd < 0) { s->fd_error_time = get_clock(); s->fd_got_error = 1; @@ -872,7 +872,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options) options++; }
- fd = open(filename, O_WRONLY | O_BINARY); + fd = qemu_open(filename, O_WRONLY | O_BINARY); if (fd < 0) return -errno;
@@ -950,7 +950,7 @@ static int floppy_probe_device(const char *filename) if (strstart(filename, "/dev/fd", NULL)) prio = 50;
- fd = open(filename, O_RDONLY | O_NONBLOCK); + fd = qemu_open(filename, O_RDONLY | O_NONBLOCK); if (fd < 0) { goto out; } @@ -1003,7 +1003,7 @@ static void floppy_eject(BlockDriverState *bs, bool eject_flag) close(s->fd); s->fd = -1; } - fd = open(bs->filename, s->open_flags | O_NONBLOCK); + fd = qemu_open(bs->filename, s->open_flags | O_NONBLOCK); if (fd >= 0) { if (ioctl(fd, FDEJECT, 0) < 0) perror("FDEJECT"); @@ -1053,7 +1053,7 @@ static int cdrom_probe_device(const char *filename) int prio = 0; struct stat st;
- fd = open(filename, O_RDONLY | O_NONBLOCK); + fd = qemu_open(filename, O_RDONLY | O_NONBLOCK); if (fd < 0) { goto out; } @@ -1177,7 +1177,7 @@ static int cdrom_reopen(BlockDriverState *bs) */ if (s->fd >= 0) close(s->fd); - fd = open(bs->filename, s->open_flags, 0644); + fd = qemu_open(bs->filename, s->open_flags, 0644); if (fd < 0) { s->fd = -1; return -EIO; diff --git a/block/raw-win32.c b/block/raw-win32.c index e4b0b75..8d7838d 100644 --- a/block/raw-win32.c +++ b/block/raw-win32.c @@ -255,8 +255,8 @@ static int raw_create(const char *filename, QEMUOptionParameter *options) options++; }
- fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, - 0644); + fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, + 0644); if (fd < 0) return -EIO; set_sparse(fd); diff --git a/block/vdi.c b/block/vdi.c index 119d3c7..6183fdf 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -648,8 +648,9 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options) options++; }
- fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, - 0644); + fd = qemu_open(filename, + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, + 0644); if (fd < 0) { return -errno; } diff --git a/block/vmdk.c b/block/vmdk.c index 18e9b4c..557dc1b 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1161,10 +1161,9 @@ static int vmdk_create_extent(const char *filename, int64_t filesize, VMDK4Header header; uint32_t tmp, magic, grains, gd_size, gt_size, gt_count;
- fd = open( - filename, - O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, - 0644); + fd = qemu_open(filename, + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, + 0644); if (fd < 0) { return -errno; } @@ -1484,15 +1483,13 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options) (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4), total_size / (int64_t)(63 * 16 * 512)); if (split || flat) { - fd = open( - filename, - O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, - 0644); + fd = qemu_open(filename, + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, + 0644); } else { - fd = open( - filename, - O_WRONLY | O_BINARY | O_LARGEFILE, - 0644); + fd = qemu_open(filename, + O_WRONLY | O_BINARY | O_LARGEFILE, + 0644); } if (fd < 0) { return -errno; diff --git a/block/vpc.c b/block/vpc.c index 5cd13d1..60ebf5a 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -678,7 +678,7 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options) }
/* Create the file */ - fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644); + fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644); if (fd < 0) { return -EIO; } diff --git a/block/vvfat.c b/block/vvfat.c index 0fd3367..81c3a19 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -1152,16 +1152,17 @@ static inline mapping_t* find_mapping_for_cluster(BDRVVVFATState* s,int cluster_ static int open_file(BDRVVVFATState* s,mapping_t* mapping) { if(!mapping) - return -1; + return -1; if(!s->current_mapping || - strcmp(s->current_mapping->path,mapping->path)) { - /* open file */ - int fd = open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE); - if(fd<0) - return -1; - vvfat_close_current_file(s); - s->current_fd = fd; - s->current_mapping = mapping; + strcmp(s->current_mapping->path, mapping->path)) { + /* open file */ + int fd = qemu_open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE); + if (fd < 0) { + return -1; + } + vvfat_close_current_file(s); + s->current_fd = fd; + s->current_mapping = mapping; } return 0; } @@ -2215,7 +2216,7 @@ static int commit_one_file(BDRVVVFATState* s, for (i = s->cluster_size; i < offset; i += s->cluster_size) c = modified_fat_get(s, c);
- fd = open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666); + fd = qemu_open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666); if (fd < 0) { fprintf(stderr, "Could not open %s... (%s, %d)\n", mapping->path, strerror(errno), errno);
On 06/15/2012 10:36 AM, Luiz Capitulino wrote:
On Thu, 14 Jun 2012 11:55:04 -0400 Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
This patch converts all block layer open calls to qemu_open. This enables all block layer open paths to dup(X) a pre-opened file descriptor if the filename is of the format /dev/fd/X. This is useful if QEMU is restricted from opening certain files.
Note that this adds the O_CLOEXEC flag to the changed open paths when the O_CLOEXEC macro is defined.
Very minor comment, but I'd exchange this with the previous patch. Because this change is not only related to the /dev/fd/X feature and also because as soon as you introduce the /dev/fd/X it will work for all file formats.
Sure, not a problem.
Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> --- v2: -Convert calls to qemu_open instead of file_open (kwolf@redhat.com) -Mention introduction of O_CLOEXEC (kwolf@redhat.com)
v3: -No changes
block/raw-posix.c | 18 +++++++++--------- block/raw-win32.c | 4 ++-- block/vdi.c | 5 +++-- block/vmdk.c | 21 +++++++++------------ block/vpc.c | 2 +- block/vvfat.c | 21 +++++++++++---------- 6 files changed, 35 insertions(+), 36 deletions(-)
diff --git a/block/raw-posix.c b/block/raw-posix.c index 03fcfcc..d8eff2f 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -568,8 +568,8 @@ static int raw_create(const char *filename, QEMUOptionParameter *options) options++; }
- fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, - 0644); + fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, + 0644); if (fd < 0) { result = -errno; } else { @@ -741,7 +741,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags) if ( bsdPath[ 0 ] != '\0' ) { strcat(bsdPath,"s0"); /* some CDs don't have a partition 0 */ - fd = open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE); + fd = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE); if (fd < 0) { bsdPath[strlen(bsdPath)-1] = '1'; } else { @@ -798,7 +798,7 @@ static int fd_open(BlockDriverState *bs) #endif return -EIO; } - s->fd = open(bs->filename, s->open_flags & ~O_NONBLOCK); + s->fd = qemu_open(bs->filename, s->open_flags & ~O_NONBLOCK); if (s->fd < 0) { s->fd_error_time = get_clock(); s->fd_got_error = 1; @@ -872,7 +872,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options) options++; }
- fd = open(filename, O_WRONLY | O_BINARY); + fd = qemu_open(filename, O_WRONLY | O_BINARY); if (fd < 0) return -errno;
@@ -950,7 +950,7 @@ static int floppy_probe_device(const char *filename) if (strstart(filename, "/dev/fd", NULL)) prio = 50;
- fd = open(filename, O_RDONLY | O_NONBLOCK); + fd = qemu_open(filename, O_RDONLY | O_NONBLOCK); if (fd < 0) { goto out; } @@ -1003,7 +1003,7 @@ static void floppy_eject(BlockDriverState *bs, bool eject_flag) close(s->fd); s->fd = -1; } - fd = open(bs->filename, s->open_flags | O_NONBLOCK); + fd = qemu_open(bs->filename, s->open_flags | O_NONBLOCK); if (fd >= 0) { if (ioctl(fd, FDEJECT, 0) < 0) perror("FDEJECT"); @@ -1053,7 +1053,7 @@ static int cdrom_probe_device(const char *filename) int prio = 0; struct stat st;
- fd = open(filename, O_RDONLY | O_NONBLOCK); + fd = qemu_open(filename, O_RDONLY | O_NONBLOCK); if (fd < 0) { goto out; } @@ -1177,7 +1177,7 @@ static int cdrom_reopen(BlockDriverState *bs) */ if (s->fd >= 0) close(s->fd); - fd = open(bs->filename, s->open_flags, 0644); + fd = qemu_open(bs->filename, s->open_flags, 0644); if (fd < 0) { s->fd = -1; return -EIO; diff --git a/block/raw-win32.c b/block/raw-win32.c index e4b0b75..8d7838d 100644 --- a/block/raw-win32.c +++ b/block/raw-win32.c @@ -255,8 +255,8 @@ static int raw_create(const char *filename, QEMUOptionParameter *options) options++; }
- fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, - 0644); + fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, + 0644); if (fd < 0) return -EIO; set_sparse(fd); diff --git a/block/vdi.c b/block/vdi.c index 119d3c7..6183fdf 100644 --- a/block/vdi.c +++ b/block/vdi.c @@ -648,8 +648,9 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options) options++; }
- fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, - 0644); + fd = qemu_open(filename, + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, + 0644); if (fd < 0) { return -errno; } diff --git a/block/vmdk.c b/block/vmdk.c index 18e9b4c..557dc1b 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -1161,10 +1161,9 @@ static int vmdk_create_extent(const char *filename, int64_t filesize, VMDK4Header header; uint32_t tmp, magic, grains, gd_size, gt_size, gt_count;
- fd = open( - filename, - O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, - 0644); + fd = qemu_open(filename, + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, + 0644); if (fd < 0) { return -errno; } @@ -1484,15 +1483,13 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options) (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4), total_size / (int64_t)(63 * 16 * 512)); if (split || flat) { - fd = open( - filename, - O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, - 0644); + fd = qemu_open(filename, + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, + 0644); } else { - fd = open( - filename, - O_WRONLY | O_BINARY | O_LARGEFILE, - 0644); + fd = qemu_open(filename, + O_WRONLY | O_BINARY | O_LARGEFILE, + 0644); } if (fd < 0) { return -errno; diff --git a/block/vpc.c b/block/vpc.c index 5cd13d1..60ebf5a 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -678,7 +678,7 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options) }
/* Create the file */ - fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644); + fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644); if (fd < 0) { return -EIO; } diff --git a/block/vvfat.c b/block/vvfat.c index 0fd3367..81c3a19 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -1152,16 +1152,17 @@ static inline mapping_t* find_mapping_for_cluster(BDRVVVFATState* s,int cluster_ static int open_file(BDRVVVFATState* s,mapping_t* mapping) { if(!mapping) - return -1; + return -1; if(!s->current_mapping || - strcmp(s->current_mapping->path,mapping->path)) { - /* open file */ - int fd = open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE); - if(fd<0) - return -1; - vvfat_close_current_file(s); - s->current_fd = fd; - s->current_mapping = mapping; + strcmp(s->current_mapping->path, mapping->path)) { + /* open file */ + int fd = qemu_open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE); + if (fd < 0) { + return -1; + } + vvfat_close_current_file(s); + s->current_fd = fd; + s->current_mapping = mapping; } return 0; } @@ -2215,7 +2216,7 @@ static int commit_one_file(BDRVVVFATState* s, for (i = s->cluster_size; i < offset; i += s->cluster_size) c = modified_fat_get(s, c);
- fd = open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666); + fd = qemu_open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666); if (fd < 0) { fprintf(stderr, "Could not open %s... (%s, %d)\n", mapping->path, strerror(errno), errno);
-- Regards, Corey
On 06/14/2012 09:55 AM, Corey Bryant wrote:
This patch converts all block layer open calls to qemu_open. This enables all block layer open paths to dup(X) a pre-opened file descriptor if the filename is of the format /dev/fd/X. This is useful if QEMU is restricted from opening certain files.
Note that this adds the O_CLOEXEC flag to the changed open paths when the O_CLOEXEC macro is defined.
@@ -741,7 +741,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags) if ( bsdPath[ 0 ] != '\0' ) { strcat(bsdPath,"s0"); /* some CDs don't have a partition 0 */ - fd = open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE); + fd = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
Why are we even bothering with O_LARGEFILE? Shouldn't we be compiling with large file support always enabled, so that we are always calling open64 in the cases where it matters, without having to explicitly add the O_LARGEFILE flag ourselves?
+++ b/block/vdi.c @@ -648,8 +648,9 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options) options++; }
- fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, - 0644); + fd = qemu_open(filename, + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
More instances. In fact, scrubbing O_LARGEFILE, is probably worth a separate patch. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/15/2012 11:21 AM, Eric Blake wrote:
On 06/14/2012 09:55 AM, Corey Bryant wrote:
This patch converts all block layer open calls to qemu_open. This enables all block layer open paths to dup(X) a pre-opened file descriptor if the filename is of the format /dev/fd/X. This is useful if QEMU is restricted from opening certain files.
Note that this adds the O_CLOEXEC flag to the changed open paths when the O_CLOEXEC macro is defined.
@@ -741,7 +741,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags) if ( bsdPath[ 0 ] != '\0' ) { strcat(bsdPath,"s0"); /* some CDs don't have a partition 0 */ - fd = open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE); + fd = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
Why are we even bothering with O_LARGEFILE? Shouldn't we be compiling with large file support always enabled, so that we are always calling open64 in the cases where it matters, without having to explicitly add the O_LARGEFILE flag ourselves?
That might be a no-op too since large file support looks to already be set at compile time: 257 QEMU_CFLAGS="-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE $QEMU_CFLAGS"
+++ b/block/vdi.c @@ -648,8 +648,9 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options) options++; }
- fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE, - 0644); + fd = qemu_open(filename, + O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
More instances. In fact, scrubbing O_LARGEFILE, is probably worth a separate patch.
I'm happy to do it. Kevin or anyone else want to weigh in first? -- Regards, Corey
Reported-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> --- v3: -This patch is new in v3. It was previously submitted on its own, and is now being included in this series. block/raw-posix.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/block/raw-posix.c b/block/raw-posix.c index d8eff2f..68886cd 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -946,9 +946,11 @@ static int floppy_probe_device(const char *filename) int prio = 0; struct floppy_struct fdparam; struct stat st; + const char *p; - if (strstart(filename, "/dev/fd", NULL)) + if (strstart(filename, "/dev/fd", &p) && p[0] != '/') { prio = 50; + } fd = qemu_open(filename, O_RDONLY | O_NONBLOCK); if (fd < 0) { -- 1.7.10.2
On Thu, 14 Jun 2012 11:55:05 -0400 Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
Reported-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
This too, would be better to move before the /dev/fd/X feature so that we avoid having introducing a bug and fixing it in a later commit. But again, it's not a huge issue.
--- v3: -This patch is new in v3. It was previously submitted on its own, and is now being included in this series.
block/raw-posix.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/block/raw-posix.c b/block/raw-posix.c index d8eff2f..68886cd 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -946,9 +946,11 @@ static int floppy_probe_device(const char *filename) int prio = 0; struct floppy_struct fdparam; struct stat st; + const char *p;
- if (strstart(filename, "/dev/fd", NULL)) + if (strstart(filename, "/dev/fd", &p) && p[0] != '/') { prio = 50; + }
fd = qemu_open(filename, O_RDONLY | O_NONBLOCK); if (fd < 0) {
On 06/15/2012 10:38 AM, Luiz Capitulino wrote:
On Thu, 14 Jun 2012 11:55:05 -0400 Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
Reported-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
This too, would be better to move before the /dev/fd/X feature so that we avoid having introducing a bug and fixing it in a later commit. But again, it's not a huge issue.
Yes, this one makes even more sense to move earlier in the series. Thanks.
--- v3: -This patch is new in v3. It was previously submitted on its own, and is now being included in this series.
block/raw-posix.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/block/raw-posix.c b/block/raw-posix.c index d8eff2f..68886cd 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -946,9 +946,11 @@ static int floppy_probe_device(const char *filename) int prio = 0; struct floppy_struct fdparam; struct stat st; + const char *p;
- if (strstart(filename, "/dev/fd", NULL)) + if (strstart(filename, "/dev/fd", &p) && p[0] != '/') { prio = 50; + }
fd = qemu_open(filename, O_RDONLY | O_NONBLOCK); if (fd < 0) {
-- Regards, Corey
On 06/14/2012 09:55 AM, Corey Bryant wrote:
This patch series adds the pass-fd QMP monitor command, which allows an fd to be passed via SCM_RIGHTS, and returns the received file descriptor. Support is also added to the block layer to allow QEMU to dup the fd when the filename is of the /dev/fd/X format. This is useful if MAC policy prevents QEMU from opening specific types of files.
Something to think about on how this will interact with Jeff's work on block-commit. That is an interface where qemu must reopen backing files which were previously O_RDONLY to now be O_RDWR. By default (when open() is supported in qemu, and qemu is using the actual file name), this means 'block-commit' works without needing any fd names. But with this new fd-passing approach, a file originally opened as O_RDONLY /dev/fd/21 will need to be reopened, but the reopened fd will (likely) not be 21. In other words, we need to make sure 'block-commit' supports the ability to pass in optional arguments that specify the file name of the backing file to be reopened, so that libvirt can pass in O_RDWR fds to replace the existing O_RDONLY fd, and be aware that the /dev/fd/nn naming of the reopen will be different. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Am 19.06.2012 17:46, schrieb Eric Blake:
On 06/14/2012 09:55 AM, Corey Bryant wrote:
This patch series adds the pass-fd QMP monitor command, which allows an fd to be passed via SCM_RIGHTS, and returns the received file descriptor. Support is also added to the block layer to allow QEMU to dup the fd when the filename is of the /dev/fd/X format. This is useful if MAC policy prevents QEMU from opening specific types of files.
Something to think about on how this will interact with Jeff's work on block-commit. That is an interface where qemu must reopen backing files which were previously O_RDONLY to now be O_RDWR. By default (when open() is supported in qemu, and qemu is using the actual file name), this means 'block-commit' works without needing any fd names. But with this new fd-passing approach, a file originally opened as O_RDONLY /dev/fd/21 will need to be reopened, but the reopened fd will (likely) not be 21. In other words, we need to make sure 'block-commit' supports the ability to pass in optional arguments that specify the file name of the backing file to be reopened, so that libvirt can pass in O_RDWR fds to replace the existing O_RDONLY fd, and be aware that the /dev/fd/nn naming of the reopen will be different.
Adding an extra argument to each command that reopens (as in bdrv_reopen(), i.e. changes flags) internally is one option. In my opinion not a particularly nice one, though. Maybe it's better to have a monitor command that just prepares a reopen and means "for the next reopen of /dev/fd/42, the passed FD will have the right flags (if it hasn't, the reopen will fail)". We can use dup2() to keep the "name" stable. Kevin
On 06/19/2012 09:57 AM, Kevin Wolf wrote:
this new fd-passing approach, a file originally opened as O_RDONLY /dev/fd/21 will need to be reopened, but the reopened fd will (likely) not be 21. In other words, we need to make sure 'block-commit' supports the ability to pass in optional arguments that specify the file name of the backing file to be reopened,
Adding an extra argument to each command that reopens (as in bdrv_reopen(), i.e. changes flags) internally is one option. In my opinion not a particularly nice one, though.
Agreed, as that's a lot of work for a lot of commands.
Maybe it's better to have a monitor command that just prepares a reopen and means "for the next reopen of /dev/fd/42, the passed FD will have the right flags (if it hasn't, the reopen will fail)". We can use dup2() to keep the "name" stable.
Indeed, having one additional up-front command in the pass-fd/closefd family might make this easier. But how would it work reliably? Remember, the current proposal is: libvirt opens backing file O_RDONLY, and calls 'pass-fd name' qemu returns 21 libvirt tells qemu to hotplug a drive with /dev/fd/21 as backing file qemu dup()s 21, and proceeds to use fd 22 for all its real work libvirt calls 'closefd name', to avoid the leak on fd 21 sometime later... qemu has opened something else that now occupies fd 21 libvirt wants to call 'block-commit', and knows qemu now needs O_RDWR access to the file under your idea, that would mean libvirt would call something like 'pass-reopen-fd name /dev/fd/21', and qemu would get a new fd (let's assume 23), so that qemu would now know that fd 23 should be used the next time any qemu interface wants to reopen '/dev/fd/21' But that's not safe - there could easily have been more than one 'pass-fd' all resulting in 21, as long as each was separated by 'closefd' in the meantime; and since libvirt already called 'closefd' to avoid an indefinite fd leak, qemu is tracking neither /dev/fd/21 nor 'name' in its passfd list. That is, the only place tracking '/dev/fd/21' is the block driver where we used /dev/fd to pass in the O_RDONLY fd in the first place. Qemu can't dup2() to move 23 into 21 at the time of the 'pass-reopen-fd', as that might not be safe. Unless you have any bright ideas that I'm overlooking, I don't see how an additional 'pass-reopen-fd' can be made to do what we want. I'm afraid that the only way to make a reopen operation reliable is to be able to specify a new file name to pass in a new /dev/fd/nnn to use as part of any reopen operation, which means touching every command like 'block-commit' that needs to do a reopen. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Am 19.06.2012 18:14, schrieb Eric Blake:
Maybe it's better to have a monitor command that just prepares a reopen and means "for the next reopen of /dev/fd/42, the passed FD will have the right flags (if it hasn't, the reopen will fail)". We can use dup2() to keep the "name" stable.
Indeed, having one additional up-front command in the pass-fd/closefd family might make this easier. But how would it work reliably? Remember, the current proposal is:
libvirt opens backing file O_RDONLY, and calls 'pass-fd name' qemu returns 21 libvirt tells qemu to hotplug a drive with /dev/fd/21 as backing file qemu dup()s 21, and proceeds to use fd 22 for all its real work libvirt calls 'closefd name', to avoid the leak on fd 21
Right, I didn't consider that we really use a dup()ed fd. I'm not completely clear about when libvirt should call closefd, and what the lifetime of fd 21 is. If I'm not mistaken, the reason for using dup() and requiring an explicit closefd was that qemu can reopen the file multiple times, for example for probing, but it's basically the same with commit. If so, nothing else must become fd 21 while the image is still in use as qemu might decide to reopen. This might mean that libvirt should only closefd the file when it becomes unused (like after hot unplug); or that qemu must keep it open internally even after closefd as long as the block device is still in use. Kevin
On Wed, Jun 20, 2012 at 09:25:29AM +0200, Kevin Wolf wrote:
Am 19.06.2012 18:14, schrieb Eric Blake:
Maybe it's better to have a monitor command that just prepares a reopen and means "for the next reopen of /dev/fd/42, the passed FD will have the right flags (if it hasn't, the reopen will fail)". We can use dup2() to keep the "name" stable.
Indeed, having one additional up-front command in the pass-fd/closefd family might make this easier. But how would it work reliably? Remember, the current proposal is:
libvirt opens backing file O_RDONLY, and calls 'pass-fd name' qemu returns 21 libvirt tells qemu to hotplug a drive with /dev/fd/21 as backing file qemu dup()s 21, and proceeds to use fd 22 for all its real work libvirt calls 'closefd name', to avoid the leak on fd 21
Right, I didn't consider that we really use a dup()ed fd.
I'm not completely clear about when libvirt should call closefd, and what the lifetime of fd 21 is. If I'm not mistaken, the reason for using dup() and requiring an explicit closefd was that qemu can reopen the file multiple times, for example for probing, but it's basically the same with commit. If so, nothing else must become fd 21 while the image is still in use as qemu might decide to reopen.
This might mean that libvirt should only closefd the file when it becomes unused (like after hot unplug); or that qemu must keep it open internally even after closefd as long as the block device is still in use.
As it works today, the only time libvirt would call "closefd", is if the monitor command it was trying to use the FD with (eg drive_add) failed. If drive_add was successfully run, then libvirt would not be invoking closefd. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 06/20/2012 02:31 AM, Daniel P. Berrange wrote:
This might mean that libvirt should only closefd the file when it becomes unused (like after hot unplug); or that qemu must keep it open internally even after closefd as long as the block device is still in use.
As it works today, the only time libvirt would call "closefd", is if the monitor command it was trying to use the FD with (eg drive_add) failed. If drive_add was successfully run, then libvirt would not be invoking closefd.
But right now, the only time libvirt uses 'getfd' is with commands like 'migrate' that implicitly close the fd after it is used by name; we don't have any experience in using fds by '/dev/fd/nn' notation instead of name. It is the fact that /dev/fd/nn will allow us to use 'pass-fd' in more situations in before, by dup()ing the fd, that libvirt all the sudden becomes responsible for using 'closefd' at the appropriate moments. I guess I can live with a rule that libvirt must not call 'closefd' on any fd that it might later want to reassign to a new copy of the fd; that is, with backing chains, if libvirt originally calls 'pass-fd drive-virtio1' with an O_RDONLY fd and gets back 21, then libvirt must not call 'closefd drive-virtio1' until it knows drive-virtio1 is no longer needed. Converting the 'drive-virtio1' fd to O_RDWR while still keeping it at /dev/fd/21 could be done via 'pass-fd -f drive-virtio1', where -f is an optional bool parameter to force a reassociation of a given name back to the previously assigned value instead of the normal error path for accidentally passing an fd to an already in-use name. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/20/2012 07:24 AM, Eric Blake wrote:
On 06/20/2012 02:31 AM, Daniel P. Berrange wrote:
This might mean that libvirt should only closefd the file when it becomes unused (like after hot unplug); or that qemu must keep it open internally even after closefd as long as the block device is still in use.
As it works today, the only time libvirt would call "closefd", is if the monitor command it was trying to use the FD with (eg drive_add) failed. If drive_add was successfully run, then libvirt would not be invoking closefd.
But right now, the only time libvirt uses 'getfd' is with commands like 'migrate' that implicitly close the fd after it is used by name; we don't have any experience in using fds by '/dev/fd/nn' notation instead of name. It is the fact that /dev/fd/nn will allow us to use 'pass-fd' in more situations in before, by dup()ing the fd, that libvirt all the sudden becomes responsible for using 'closefd' at the appropriate moments.
I guess I can live with a rule that libvirt must not call 'closefd' on any fd that it might later want to reassign to a new copy of the fd; that is, with backing chains, if libvirt originally calls 'pass-fd drive-virtio1' with an O_RDONLY fd and gets back 21, then libvirt must not call 'closefd drive-virtio1' until it knows drive-virtio1 is no longer needed. Converting the 'drive-virtio1' fd to O_RDWR while still keeping it at /dev/fd/21 could be done via 'pass-fd -f drive-virtio1', where -f is an optional bool parameter to force a reassociation of a given name back to the previously assigned value instead of the normal error path for accidentally passing an fd to an already in-use name.
It sounds like the flow would be: 'pass-fd drive-virtio1' of O_RDONLY fd --> guest gets fd 21 'pass-fd -f drive-virtio1' of O_WRONLY fd --> guest gets fd 21? But I'm not clear as to how you would retain file descriptor 21 in the guest when using 'pass-fd -f drive-virtio1'. The fd is created as a result of passing via SCM_RIGHTS, which assigns the next available fd. We don't have control over what fd is assigned, do we? -- Regards, Corey
On 06/20/2012 07:31 AM, Corey Bryant wrote:
It sounds like the flow would be: 'pass-fd drive-virtio1' of O_RDONLY fd --> guest gets fd 21
No -f, so qemu errors out if an fd named 'drive-virtio1' already exists; otherwise it succeeds, and returns the fd assigned by SCM_RIGHTS as well as adding the name to its internal list.
'pass-fd -f drive-virtio1' of O_WRONLY fd --> guest gets fd 21?
-f says to fail if 'drive-virtio1' does not already exist in the internal list. Otherwise, this is a reopen attempt, and the fd passed in by SCM_RIGHTS (let's assume it is 23 at this time) is then passed through dup2() to overwrite the fd already associated with 'drive-virtio1' (21 in this case), then the SCM_RIGHTS fd (23) is closed. In this way, the name 'drive-virtio1' remains associated with fd 21, but we have reopened it with different mode. At this point, code that wants to reopen /dev/fd/21 with a new mode will see the new permissions on the reassigned fd. And yes, it means that libvirt would not be allowed to call 'closefd drive-virtio1' until the block device for drive-virtio1 is no longer around, whether or not the /dev/fd/nn reuses the fd as-is or whether it dup()s the fd to something else (say 22) for use by the block device.
But I'm not clear as to how you would retain file descriptor 21 in the guest when using 'pass-fd -f drive-virtio1'. The fd is created as a result of passing via SCM_RIGHTS, which assigns the next available fd. We don't have control over what fd is assigned, do we?
The use of -f says that 'pass-fd' uses dup2() to reuse an existing fd. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/20/2012 10:53 AM, Eric Blake wrote:
On 06/20/2012 07:31 AM, Corey Bryant wrote:
It sounds like the flow would be: 'pass-fd drive-virtio1' of O_RDONLY fd --> guest gets fd 21
No -f, so qemu errors out if an fd named 'drive-virtio1' already exists; otherwise it succeeds, and returns the fd assigned by SCM_RIGHTS as well as adding the name to its internal list.
'pass-fd -f drive-virtio1' of O_WRONLY fd --> guest gets fd 21?
-f says to fail if 'drive-virtio1' does not already exist in the internal list. Otherwise, this is a reopen attempt, and the fd passed in by SCM_RIGHTS (let's assume it is 23 at this time) is then passed through dup2() to overwrite the fd already associated with 'drive-virtio1' (21 in this case), then the SCM_RIGHTS fd (23) is closed. In this way, the name 'drive-virtio1' remains associated with fd 21, but we have reopened it with different mode. At this point, code that wants to reopen /dev/fd/21 with a new mode will see the new permissions on the reassigned fd. And yes, it means that libvirt would not be allowed to call 'closefd drive-virtio1' until the block device for drive-virtio1 is no longer around, whether or not the /dev/fd/nn reuses the fd as-is or whether it dup()s the fd to something else (say 22) for use by the block device.
That makes sense. Thanks for the explanation. Essentially dup2(23, 21) will give fd 21 the new access mode. I assume we can go ahead and close fd 23 after the dup2? I'll go ahead and add this support in the next version of the patch series if there are no objections. -- Regards, Corey
But I'm not clear as to how you would retain file descriptor 21 in the guest when using 'pass-fd -f drive-virtio1'. The fd is created as a result of passing via SCM_RIGHTS, which assigns the next available fd. We don't have control over what fd is assigned, do we?
The use of -f says that 'pass-fd' uses dup2() to reuse an existing fd.
participants (5)
-
Corey Bryant -
Daniel P. Berrange -
Eric Blake -
Kevin Wolf -
Luiz Capitulino