* [PATCH v2 0/2] net/tap: minor fixes to interaction with the bridge helper @ 2021-05-12 15:36 Daniel P. Berrangé 2021-05-12 15:36 ` [PATCH v2 1/2] net/tap: fix FreeBSD portability problem receiving TAP FD Daniel P. Berrangé 2021-05-12 15:36 ` [PATCH v2 2/2] net/tap: fix error reporting when bridge helper forgets to send an FD Daniel P. Berrangé 0 siblings, 2 replies; 5+ messages in thread From: Daniel P. Berrangé @ 2021-05-12 15:36 UTC (permalink / raw) To: qemu-devel; +Cc: Jason Wang, Daniel P. Berrangé QEMU fails to report a good error message if the bridge helper exits with success code, but forgets to pass back an FD. There is also a minor portability problem impacting FreeBSD/NetBSD dealing with cmsg initialization. Changed in v2: - include helper binary name in error message Daniel P. Berrangé (2): net/tap: fix FreeBSD portability problem receiving TAP FD net/tap: fix error reporting when bridge helper forgets to send an FD net/tap.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/2] net/tap: fix FreeBSD portability problem receiving TAP FD 2021-05-12 15:36 [PATCH v2 0/2] net/tap: minor fixes to interaction with the bridge helper Daniel P. Berrangé @ 2021-05-12 15:36 ` Daniel P. Berrangé 2021-05-12 16:35 ` Philippe Mathieu-Daudé 2021-05-12 15:36 ` [PATCH v2 2/2] net/tap: fix error reporting when bridge helper forgets to send an FD Daniel P. Berrangé 1 sibling, 1 reply; 5+ messages in thread From: Daniel P. Berrangé @ 2021-05-12 15:36 UTC (permalink / raw) To: qemu-devel; +Cc: Connor Kuehl, Jason Wang, Daniel P. Berrangé The CMSG_LEN and CMSG_SPACE macros must not be assumed to return the same value. The msg_controllen field must be initialized using CMSG_SPACE when using SCM_RIGHTS. This ought to fix any FD receive issues users might be hitting on 64-bit FeeBSD / NetBSD platforms. The flaw was noticed first in GNULIB https://lists.gnu.org/archive/html/bug-gnulib/2021-02/msg00066.html and QEMU's code has the same logic bug. Reviewed-by: Connor Kuehl <ckuehl@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- net/tap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/tap.c b/net/tap.c index bae895e287..276a9077fc 100644 --- a/net/tap.c +++ b/net/tap.c @@ -467,7 +467,7 @@ static int recv_fd(int c) cmsg->cmsg_level = SOL_SOCKET; cmsg->cmsg_type = SCM_RIGHTS; cmsg->cmsg_len = CMSG_LEN(sizeof(fd)); - msg.msg_controllen = cmsg->cmsg_len; + msg.msg_controllen = CMSG_SPACE(sizeof(fd)); iov.iov_base = req; iov.iov_len = sizeof(req); -- 2.31.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] net/tap: fix FreeBSD portability problem receiving TAP FD 2021-05-12 15:36 ` [PATCH v2 1/2] net/tap: fix FreeBSD portability problem receiving TAP FD Daniel P. Berrangé @ 2021-05-12 16:35 ` Philippe Mathieu-Daudé 2021-05-14 12:05 ` Daniel P. Berrangé 0 siblings, 1 reply; 5+ messages in thread From: Philippe Mathieu-Daudé @ 2021-05-12 16:35 UTC (permalink / raw) To: Daniel P. Berrangé, qemu-devel; +Cc: Connor Kuehl, Jason Wang On 5/12/21 5:36 PM, Daniel P. Berrangé wrote: > The CMSG_LEN and CMSG_SPACE macros must not be assumed to return the > same value. The msg_controllen field must be initialized using > CMSG_SPACE when using SCM_RIGHTS. > > This ought to fix any FD receive issues users might be hitting on > 64-bit FeeBSD / NetBSD platforms. The flaw was noticed first in > GNULIB > > https://lists.gnu.org/archive/html/bug-gnulib/2021-02/msg00066.html > > and QEMU's code has the same logic bug. > > Reviewed-by: Connor Kuehl <ckuehl@redhat.com> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > net/tap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/tap.c b/net/tap.c > index bae895e287..276a9077fc 100644 > --- a/net/tap.c > +++ b/net/tap.c > @@ -467,7 +467,7 @@ static int recv_fd(int c) > cmsg->cmsg_level = SOL_SOCKET; > cmsg->cmsg_type = SCM_RIGHTS; > cmsg->cmsg_len = CMSG_LEN(sizeof(fd)); > - msg.msg_controllen = cmsg->cmsg_len; > + msg.msg_controllen = CMSG_SPACE(sizeof(fd)); > > iov.iov_base = req; > iov.iov_len = sizeof(req); > Possibly more: -- >8 -- diff --git a/contrib/ivshmem-client/ivshmem-client.c b/contrib/ivshmem-client/ivshmem-client.c index 182c79d27cf..f4ee226deba 100644 --- a/contrib/ivshmem-client/ivshmem-client.c +++ b/contrib/ivshmem-client/ivshmem-client.c @@ -44 +44 @@ ivshmem_client_read_one_msg(IvshmemClient *client, int64_t *index, int *fd) - msg.msg_controllen = sizeof(msg_control); + msg.msg_controllen = CMSG_SPACE(sizeof(msg_control)); diff --git a/contrib/ivshmem-server/ivshmem-server.c b/contrib/ivshmem-server/ivshmem-server.c index 39a6ffdb5df..209eb120716 100644 --- a/contrib/ivshmem-server/ivshmem-server.c +++ b/contrib/ivshmem-server/ivshmem-server.c @@ -55 +55 @@ ivshmem_server_send_one_msg(int sock_fd, int64_t peer_id, int fd) - msg.msg_controllen = sizeof(msg_control); + msg.msg_controllen = CMSG_SPACE(sizeof(msg_control)); diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c b/contrib/vhost-user-gpu/vhost-user-gpu.c index f73f292c9f7..e72a59b135a 100644 --- a/contrib/vhost-user-gpu/vhost-user-gpu.c +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c @@ -161 +161 @@ vg_sock_fd_write(int sock, const void *buf, ssize_t buflen, int fd) - msg.msg_controllen = sizeof(cmsgu.control); + msg.msg_controllen = CMSG_SPACE(sizeof(cmsgu.control)); diff --git a/io/channel-socket.c b/io/channel-socket.c index de259f7eed2..753d0d2f8ef 100644 --- a/io/channel-socket.c +++ b/io/channel-socket.c @@ -498 +498 @@ static ssize_t qio_channel_socket_readv(QIOChannel *ioc, - msg.msg_controllen = sizeof(control); + msg.msg_controllen = CMSG_SPACE(sizeof(control)); diff --git a/net/tap.c b/net/tap.c index bae895e2874..05ab4c8a530 100644 --- a/net/tap.c +++ b/net/tap.c @@ -459 +459 @@ static int recv_fd(int c) - .msg_controllen = sizeof(msgbuf), + .msg_controllen = CMSG_SPACE(sizeof(msgbuf)), diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c index a26e1663f02..983b4347523 100644 --- a/qemu-bridge-helper.c +++ b/qemu-bridge-helper.c @@ -189 +189 @@ static int send_fd(int c, int fd) - .msg_controllen = sizeof(msgbuf), + .msg_controllen = CMSG_SPACE(sizeof(msgbuf)), @@ -199 +199 @@ static int send_fd(int c, int fd) - msg.msg_controllen = cmsg->cmsg_len; + msg.msg_controllen = CMSG_SPACE(sizeof(fd)); diff --git a/subprojects/libvhost-user/libvhost-user.c b/subprojects/libvhost-user/libvhost-user.c index fab7ca17ee1..f083db4ea16 100644 --- a/subprojects/libvhost-user/libvhost-user.c +++ b/subprojects/libvhost-user/libvhost-user.c @@ -281 +281 @@ vu_message_read_default(VuDev *dev, int conn_fd, VhostUserMsg *vmsg) - .msg_controllen = sizeof(control), + .msg_controllen = CMSG_SPACE(sizeof(control)), diff --git a/tests/qemu-iotests/socket_scm_helper.c b/tests/qemu-iotests/socket_scm_helper.c index eb76d31aa94..aeba41edea0 100644 --- a/tests/qemu-iotests/socket_scm_helper.c +++ b/tests/qemu-iotests/socket_scm_helper.c @@ -42 +42 @@ static int send_fd(int fd, int fd_to_send) - msg.msg_controllen = sizeof(control); + msg.msg_controllen = CMSG_SPACE(sizeof(control)); --- ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] net/tap: fix FreeBSD portability problem receiving TAP FD 2021-05-12 16:35 ` Philippe Mathieu-Daudé @ 2021-05-14 12:05 ` Daniel P. Berrangé 0 siblings, 0 replies; 5+ messages in thread From: Daniel P. Berrangé @ 2021-05-14 12:05 UTC (permalink / raw) To: Philippe Mathieu-Daudé; +Cc: Connor Kuehl, Jason Wang, qemu-devel On Wed, May 12, 2021 at 06:35:57PM +0200, Philippe Mathieu-Daudé wrote: > On 5/12/21 5:36 PM, Daniel P. Berrangé wrote: > > The CMSG_LEN and CMSG_SPACE macros must not be assumed to return the > > same value. The msg_controllen field must be initialized using > > CMSG_SPACE when using SCM_RIGHTS. > > > > This ought to fix any FD receive issues users might be hitting on > > 64-bit FeeBSD / NetBSD platforms. The flaw was noticed first in > > GNULIB > > > > https://lists.gnu.org/archive/html/bug-gnulib/2021-02/msg00066.html > > > > and QEMU's code has the same logic bug. > > > > Reviewed-by: Connor Kuehl <ckuehl@redhat.com> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > net/tap.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/tap.c b/net/tap.c > > index bae895e287..276a9077fc 100644 > > --- a/net/tap.c > > +++ b/net/tap.c > > @@ -467,7 +467,7 @@ static int recv_fd(int c) > > cmsg->cmsg_level = SOL_SOCKET; > > cmsg->cmsg_type = SCM_RIGHTS; > > cmsg->cmsg_len = CMSG_LEN(sizeof(fd)); > > - msg.msg_controllen = cmsg->cmsg_len; > > + msg.msg_controllen = CMSG_SPACE(sizeof(fd)); > > > > iov.iov_base = req; > > iov.iov_len = sizeof(req); > > > > Possibly more: Ewwwww, yes :-( Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] net/tap: fix error reporting when bridge helper forgets to send an FD 2021-05-12 15:36 [PATCH v2 0/2] net/tap: minor fixes to interaction with the bridge helper Daniel P. Berrangé 2021-05-12 15:36 ` [PATCH v2 1/2] net/tap: fix FreeBSD portability problem receiving TAP FD Daniel P. Berrangé @ 2021-05-12 15:36 ` Daniel P. Berrangé 1 sibling, 0 replies; 5+ messages in thread From: Daniel P. Berrangé @ 2021-05-12 15:36 UTC (permalink / raw) To: qemu-devel Cc: Connor Kuehl, Philippe Mathieu-Daudé, Jason Wang, Daniel P. Berrangé The recv_fd() method returns -1 on error, 0 on end of file, or an FD number on success. Technically 0 is also a valid FD number, so the return value is ambiguous. The caller doesn't even consider the possibility of 0 meaning end of file, so just blindly assume it is a valid FD. IOW if the bridge helper exits with 0 status code and forgets to send an FD, QEMU will accidentally try to use STDIN as a TAP FD. Fortunately we get an error shortly thereafter $ qemu-system-x86_64 -netdev bridge,br=br99,helper=/bin/true,id=ns0 qemu-system-x86_64: -netdev bridge,br=br99,helper=/bin/true,id=ns0: Unable to query TUNGETIFF on FD 0: Inappropriate ioctl for device It is better if we correctly diagnose this broken bridge helper though. To do this we need to return the FD in an output parameter to remove the ambiguity and then fix the caller to check for the end of file condition. With this done we now get $ qemu-system-x86_64 -netdev bridge,br=br99,helper=/bin/true,id=ns0 qemu-system-x86_64: -netdev bridge,br=br99,helper=/bin/true,id=ns0: bridge helper did not send a file descriptor Fixes: https://gitlab.com/qemu-project/qemu/-/issues/166 Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Connor Kuehl <ckuehl@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- net/tap.c | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/net/tap.c b/net/tap.c index 276a9077fc..92761546b7 100644 --- a/net/tap.c +++ b/net/tap.c @@ -450,9 +450,11 @@ static void launch_script(const char *setup_script, const char *ifname, } } -static int recv_fd(int c) +/* + * Returns: -1 on error, 0 on end of file, 1 if an FD was received + */ +static int recv_fd(int c, int *fd) { - int fd; uint8_t msgbuf[CMSG_SPACE(sizeof(fd))]; struct msghdr msg = { .msg_control = msgbuf, @@ -476,12 +478,12 @@ static int recv_fd(int c) msg.msg_iovlen = 1; len = recvmsg(c, &msg, 0); - if (len > 0) { - memcpy(&fd, CMSG_DATA(cmsg), sizeof(fd)); - return fd; + if (len <= 0) { + return len; } - return len; + memcpy(fd, CMSG_DATA(cmsg), sizeof(*fd)); + return 1; } static int net_bridge_run_helper(const char *helper, const char *bridge, @@ -564,14 +566,15 @@ static int net_bridge_run_helper(const char *helper, const char *bridge, _exit(1); } else { - int fd; + int ret; + int fd = -1; int saved_errno; close(sv[1]); do { - fd = recv_fd(sv[0]); - } while (fd == -1 && errno == EINTR); + ret = recv_fd(sv[0], &fd); + } while (ret == -1 && errno == EINTR); saved_errno = errno; close(sv[0]); @@ -580,7 +583,7 @@ static int net_bridge_run_helper(const char *helper, const char *bridge, /* loop */ } sigprocmask(SIG_SETMASK, &oldmask, NULL); - if (fd < 0) { + if (ret < 0) { error_setg_errno(errp, saved_errno, "failed to recv file descriptor"); return -1; @@ -589,6 +592,17 @@ static int net_bridge_run_helper(const char *helper, const char *bridge, error_setg(errp, "bridge helper failed"); return -1; } + + /* + * ret == 0 means EOF, and if status == 0 then helper + * exited cleanly but forgot to send us an FD. Oops... + */ + if (ret == 0) { + error_setg(errp, + "bridge helper '%s' did not send a file descriptor", + helper); + return -1; + } return fd; } } -- 2.31.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-05-14 12:12 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-12 15:36 [PATCH v2 0/2] net/tap: minor fixes to interaction with the bridge helper Daniel P. Berrangé 2021-05-12 15:36 ` [PATCH v2 1/2] net/tap: fix FreeBSD portability problem receiving TAP FD Daniel P. Berrangé 2021-05-12 16:35 ` Philippe Mathieu-Daudé 2021-05-14 12:05 ` Daniel P. Berrangé 2021-05-12 15:36 ` [PATCH v2 2/2] net/tap: fix error reporting when bridge helper forgets to send an FD Daniel P. Berrangé
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).