qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] net/tap: minor fixes to interaction with the bridge helper
@ 2021-05-05 12:12 Daniel P. Berrangé
  2021-05-05 12:12 ` [PATCH 1/2] net/tap: fix FreeBSD portability problem receiving TAP FD Daniel P. Berrangé
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Daniel P. Berrangé @ 2021-05-05 12:12 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.

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 | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

-- 
2.31.1




^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] net/tap: fix FreeBSD portability problem receiving TAP FD
  2021-05-05 12:12 [PATCH 0/2] net/tap: minor fixes to interaction with the bridge helper Daniel P. Berrangé
@ 2021-05-05 12:12 ` Daniel P. Berrangé
  2021-05-05 12:12 ` [PATCH 2/2] net/tap: fix error reporting when bridge helper forgets to send an FD Daniel P. Berrangé
  2021-05-11 18:54 ` [PATCH 0/2] net/tap: minor fixes to interaction with the bridge helper Connor Kuehl
  2 siblings, 0 replies; 8+ messages in thread
From: Daniel P. Berrangé @ 2021-05-05 12:12 UTC (permalink / raw)
  To: qemu-devel; +Cc: 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.

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] 8+ messages in thread

* [PATCH 2/2] net/tap: fix error reporting when bridge helper forgets to send an FD
  2021-05-05 12:12 [PATCH 0/2] net/tap: minor fixes to interaction with the bridge helper Daniel P. Berrangé
  2021-05-05 12:12 ` [PATCH 1/2] net/tap: fix FreeBSD portability problem receiving TAP FD Daniel P. Berrangé
@ 2021-05-05 12:12 ` Daniel P. Berrangé
  2021-05-05 12:32   ` Philippe Mathieu-Daudé
  2021-05-11 18:53   ` Connor Kuehl
  2021-05-11 18:54 ` [PATCH 0/2] net/tap: minor fixes to interaction with the bridge helper Connor Kuehl
  2 siblings, 2 replies; 8+ messages in thread
From: Daniel P. Berrangé @ 2021-05-05 12:12 UTC (permalink / raw)
  To: qemu-devel; +Cc: 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
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 net/tap.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index 276a9077fc..02046c9fd2 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,15 @@ 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. Opps...
+         */
+        if (ret == 0) {
+            error_setg(errp, "bridge helper did not send a file descriptor");
+            return -1;
+        }
         return fd;
     }
 }
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] net/tap: fix error reporting when bridge helper forgets to send an FD
  2021-05-05 12:12 ` [PATCH 2/2] net/tap: fix error reporting when bridge helper forgets to send an FD Daniel P. Berrangé
@ 2021-05-05 12:32   ` Philippe Mathieu-Daudé
  2021-05-12 15:36     ` Daniel P. Berrangé
  2021-05-11 18:53   ` Connor Kuehl
  1 sibling, 1 reply; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-05 12:32 UTC (permalink / raw)
  To: Daniel P. Berrangé, qemu-devel; +Cc: Jason Wang

On 5/5/21 2:12 PM, Daniel P. Berrangé wrote:
> 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
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  net/tap.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)

> +        /*
> +         * ret == 0 means EOF, and if status == 0 then helper
> +         * exited cleanly but forgot to send us an FD. Opps...

Opps or Oops?

Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] net/tap: fix error reporting when bridge helper forgets to send an FD
  2021-05-05 12:12 ` [PATCH 2/2] net/tap: fix error reporting when bridge helper forgets to send an FD Daniel P. Berrangé
  2021-05-05 12:32   ` Philippe Mathieu-Daudé
@ 2021-05-11 18:53   ` Connor Kuehl
  2021-05-12 15:34     ` Daniel P. Berrangé
  1 sibling, 1 reply; 8+ messages in thread
From: Connor Kuehl @ 2021-05-11 18:53 UTC (permalink / raw)
  To: Daniel P. Berrangé, qemu-devel; +Cc: Jason Wang

On 5/5/21 7:12 AM, Daniel P. Berrangé wrote:
> [..]
> The recv_fd() method returns -1 on error, 0 on end of file, or an FD
>   $ 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
> [..]
>          }
> +
> +        /*
> +         * ret == 0 means EOF, and if status == 0 then helper
> +         * exited cleanly but forgot to send us an FD. Opps...
> +         */
> +        if (ret == 0) {
> +            error_setg(errp, "bridge helper did not send a file
> descriptor");

Is it possible to include the name of the helper that's at fault here?
Could be helpful for debugging if the command line includes multiple
netdev opts with a helper parameter (unless that's literally impossible).

Connor



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/2] net/tap: minor fixes to interaction with the bridge helper
  2021-05-05 12:12 [PATCH 0/2] net/tap: minor fixes to interaction with the bridge helper Daniel P. Berrangé
  2021-05-05 12:12 ` [PATCH 1/2] net/tap: fix FreeBSD portability problem receiving TAP FD Daniel P. Berrangé
  2021-05-05 12:12 ` [PATCH 2/2] net/tap: fix error reporting when bridge helper forgets to send an FD Daniel P. Berrangé
@ 2021-05-11 18:54 ` Connor Kuehl
  2 siblings, 0 replies; 8+ messages in thread
From: Connor Kuehl @ 2021-05-11 18:54 UTC (permalink / raw)
  To: Daniel P. Berrangé, qemu-devel; +Cc: Jason Wang

On 5/5/21 7:12 AM, Daniel P. Berrangé wrote:
> 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.
> 
> 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 | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 

Just a minor question on patch 2.

Otherwise, for the series:

Reviewed-by: Connor Kuehl <ckuehl@redhat.com>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] net/tap: fix error reporting when bridge helper forgets to send an FD
  2021-05-11 18:53   ` Connor Kuehl
@ 2021-05-12 15:34     ` Daniel P. Berrangé
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel P. Berrangé @ 2021-05-12 15:34 UTC (permalink / raw)
  To: Connor Kuehl; +Cc: Jason Wang, qemu-devel

On Tue, May 11, 2021 at 01:53:39PM -0500, Connor Kuehl wrote:
> On 5/5/21 7:12 AM, Daniel P. Berrangé wrote:
> > [..]
> > The recv_fd() method returns -1 on error, 0 on end of file, or an FD
> >   $ 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
> > [..]
> >          }
> > +
> > +        /*
> > +         * ret == 0 means EOF, and if status == 0 then helper
> > +         * exited cleanly but forgot to send us an FD. Opps...
> > +         */
> > +        if (ret == 0) {
> > +            error_setg(errp, "bridge helper did not send a file
> > descriptor");
> 
> Is it possible to include the name of the helper that's at fault here?
> Could be helpful for debugging if the command line includes multiple
> netdev opts with a helper parameter (unless that's literally impossible).

Yes, easy change.

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] 8+ messages in thread

* Re: [PATCH 2/2] net/tap: fix error reporting when bridge helper forgets to send an FD
  2021-05-05 12:32   ` Philippe Mathieu-Daudé
@ 2021-05-12 15:36     ` Daniel P. Berrangé
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel P. Berrangé @ 2021-05-12 15:36 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: Jason Wang, qemu-devel

On Wed, May 05, 2021 at 02:32:04PM +0200, Philippe Mathieu-Daudé wrote:
> On 5/5/21 2:12 PM, Daniel P. Berrangé wrote:
> > 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
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  net/tap.c | 32 ++++++++++++++++++++++----------
> >  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> > +        /*
> > +         * ret == 0 means EOF, and if status == 0 then helper
> > +         * exited cleanly but forgot to send us an FD. Opps...
> 
> Opps or Oops?

Oops, it should be Oops :-)

> 
> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 

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] 8+ messages in thread

end of thread, other threads:[~2021-05-12 15:38 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-05 12:12 [PATCH 0/2] net/tap: minor fixes to interaction with the bridge helper Daniel P. Berrangé
2021-05-05 12:12 ` [PATCH 1/2] net/tap: fix FreeBSD portability problem receiving TAP FD Daniel P. Berrangé
2021-05-05 12:12 ` [PATCH 2/2] net/tap: fix error reporting when bridge helper forgets to send an FD Daniel P. Berrangé
2021-05-05 12:32   ` Philippe Mathieu-Daudé
2021-05-12 15:36     ` Daniel P. Berrangé
2021-05-11 18:53   ` Connor Kuehl
2021-05-12 15:34     ` Daniel P. Berrangé
2021-05-11 18:54 ` [PATCH 0/2] net/tap: minor fixes to interaction with the bridge helper Connor Kuehl

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).