[Qemu-devel] [feature request] qemu-bridge-helper

[Qemu-devel] [feature request] qemu-bridge-helper

[Mario De Chenno]

[-- Attachment #1: Type: text/plain, Size: 200 bytes --]

It could be possible to add a sort of "ifname=" option to the
qemu-bridge-helper program, so qemu could give a recognizable name to
the tap interface the helper creates? 
Thanks  all,
Mairo De Chenno

[-- Attachment #2: Type: text/html, Size: 451 bytes --]

Re: [Qemu-devel] [feature request] qemu-bridge-helper

[Corey Bryant]

On 10/10/2012 11:05 AM, Mario De Chenno wrote:
> It could be possible to add a sort of "ifname=" option to the
> qemu-bridge-helper program, so qemu could give a recognizable name to
> the tap interface the helper creates?
> Thanks  all,
> Mairo De Chenno

I don't see why not.  If ifname is not specified then we can default to 
tap0, etc.

Richa, do you want to handle this?

-- 
Regards,
Corey Bryant

[Qemu-devel] [PATCH] net: Allow specifying ifname for qemu-bridge-helper

[Mike Lovell]

This makes a few changes to allow ifname to be specified when using
qemu-bridge-helper with both the bridge and tap network interfaces. It adds
the --ifname option to qemu-bridge-helper, removes the restriction that ifname
cannot be specified with helper for the tap interface, and adds logic to
specify the --ifname option when exec'ing the helper.

Signed-off-by: Mike Lovell <mike@dev-zero.net>
---

This feature was originally requested by Mario De Chenno on the qemu-devel
mailing list. Seems pretty simple and figured it was something I could throw
together pretty quickly. I have tested the following combinations of invoking
qemu (where qbr is qemu-bridge-helper)

qemu-system-x86_64 -net nic -net tap,helper="qbr --br=test1"
qemu-system-x86_64 -net nic -net tap,helper="qbr --br=test1",ifname=vm1
qemu-system-x86_64 -net nic -net tap,helper=qbr
qemu-system-x86_64 -net nic -net tap,helper=qbr,ifname=vm1
qemu-system-x86_64 -net nic -net bridge,helper=qbr 
qemu-system-x86_64 -net nic -net bridge,helper=qbr,ifname=vm1
qemu-system-x86_64 -net nic -net bridge,helper=qbr,ifname=vm1,br=test1
qemu-system-x86_64 -net nic -net bridge,helper=qbr,br=test1

 net/tap.c            |   39 ++++++++++++++++++++++++++++-----------
 qapi-schema.json     |    3 ++-
 qemu-bridge-helper.c |   10 +++++++---
 3 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index a88ae8f..cfb5bff 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -417,11 +417,13 @@ static int recv_fd(int c)
     return len;
 }
 
-static int net_bridge_run_helper(const char *helper, const char *bridge)
+static int net_bridge_run_helper(const char *helper,
+                                 const char *bridge,
+                                 const char *ifname)
 {
     sigset_t oldmask, mask;
     int pid, status;
-    char *args[5];
+    char *args[6];
     char **parg;
     int sv[2];
 
@@ -439,7 +441,9 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
         int open_max = sysconf(_SC_OPEN_MAX), i;
         char fd_buf[6+10];
         char br_buf[6+IFNAMSIZ] = {0};
-        char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + 15];
+        char ifname_buf[10+IFNAMSIZ] = {0};
+        char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) +
+                        sizeof(ifname_buf) + 15];
 
         for (i = 0; i < open_max; i++) {
             if (i != STDIN_FILENO &&
@@ -459,8 +463,13 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
                 snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
             }
 
-            snprintf(helper_cmd, sizeof(helper_cmd), "%s %s %s %s",
-                     helper, "--use-vnet", fd_buf, br_buf);
+            if ((strstr(helper, "--ifname=") == NULL) && (ifname != NULL)) {
+                snprintf(ifname_buf, sizeof(ifname_buf), "%s%s" ,
+                         "--ifname=", ifname);
+            }
+
+            snprintf(helper_cmd, sizeof(helper_cmd), "%s %s %s %s %s",
+                     helper, "--use-vnet", fd_buf, br_buf, ifname_buf);
 
             parg = args;
             *parg++ = (char *)"sh";
@@ -473,12 +482,17 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
             /* assume helper is just the executable path name */
 
             snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
+            if (ifname != NULL) {
+                snprintf(ifname_buf, sizeof(ifname_buf), "%s%s" ,
+                         "--ifname=", ifname);
+            }
 
             parg = args;
             *parg++ = (char *)helper;
             *parg++ = (char *)"--use-vnet";
             *parg++ = fd_buf;
             *parg++ = br_buf;
+            *parg++ = ifname_buf;
             *parg++ = NULL;
 
             execv(helper, args);
@@ -517,7 +531,7 @@ int net_init_bridge(const NetClientOptions *opts, const char *name,
                     NetClientState *peer)
 {
     const NetdevBridgeOptions *bridge;
-    const char *helper, *br;
+    const char *helper, *br, *ifname;
 
     TAPState *s;
     int fd, vnet_hdr;
@@ -527,8 +541,9 @@ int net_init_bridge(const NetClientOptions *opts, const char *name,
 
     helper = bridge->has_helper ? bridge->helper : DEFAULT_BRIDGE_HELPER;
     br     = bridge->has_br     ? bridge->br     : DEFAULT_BRIDGE_INTERFACE;
+    ifname = bridge->has_ifname ? bridge->ifname : NULL;
 
-    fd = net_bridge_run_helper(helper, br);
+    fd = net_bridge_run_helper(helper, br, ifname);
     if (fd == -1) {
         return -1;
     }
@@ -622,14 +637,16 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
         model = "tap";
 
     } else if (tap->has_helper) {
-        if (tap->has_ifname || tap->has_script || tap->has_downscript ||
-            tap->has_vnet_hdr) {
-            error_report("ifname=, script=, downscript=, and vnet_hdr= "
+        if (tap->has_script || tap->has_downscript || tap->has_vnet_hdr) {
+            error_report("script=, downscript=, and vnet_hdr= "
                          "are invalid with helper=");
             return -1;
         }
 
-        fd = net_bridge_run_helper(tap->helper, DEFAULT_BRIDGE_INTERFACE);
+        const char *ifname;
+        ifname = tap->has_ifname ? tap->ifname : NULL;
+        fd = net_bridge_run_helper(tap->helper, DEFAULT_BRIDGE_INTERFACE,
+                                   ifname);
         if (fd == -1) {
             return -1;
         }
diff --git a/qapi-schema.json b/qapi-schema.json
index f9dbdae..feaac9e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2432,7 +2432,8 @@
 { 'type': 'NetdevBridgeOptions',
   'data': {
     '*br':     'str',
-    '*helper': 'str' } }
+    '*helper': 'str',
+    '*ifname': 'str' } }
 
 ##
 # @NetdevHubPortOptions
diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index 652eec9..c1d1519 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -67,7 +67,8 @@ typedef QSIMPLEQ_HEAD(ACLList, ACLRule) ACLList;
 static void usage(void)
 {
     fprintf(stderr,
-            "Usage: qemu-bridge-helper [--use-vnet] --br=bridge --fd=unixfd\n");
+            "Usage: qemu-bridge-helper [--use-vnet] [--ifname=name] "
+            "--br=bridge --fd=unixfd\n");
 }
 
 static int parse_acl_file(const char *filename, ACLList *acl_list)
@@ -239,6 +240,7 @@ int main(int argc, char **argv)
     ACLList acl_list;
     int access_allowed, access_denied;
     int ret = EXIT_SUCCESS;
+    const char *ifname = NULL;
 
 #ifdef CONFIG_LIBCAP
     /* if we're run from an suid binary, immediately drop privileges preserving
@@ -259,6 +261,8 @@ int main(int argc, char **argv)
             bridge = &argv[index][5];
         } else if (strncmp(argv[index], "--fd=", 5) == 0) {
             unixfd = atoi(&argv[index][5]);
+        } else if (strncmp(argv[index], "--ifname=", 9) == 0) {
+            ifname = &argv[index][9];
         } else {
             usage();
             return EXIT_FAILURE;
@@ -329,8 +333,8 @@ int main(int argc, char **argv)
     }
 
     /* request a tap device, disable PI, and add vnet header support if
-     * requested and it's available. */
-    prep_ifreq(&ifr, "tap%d");
+     * requested and it's available. use ifname if provided for tap name. */
+    prep_ifreq(&ifr, ifname != NULL ? ifname : "tap%d");
     ifr.ifr_flags = IFF_TAP|IFF_NO_PI;
     if (use_vnet && has_vnet_hdr(fd)) {
         ifr.ifr_flags |= IFF_VNET_HDR;
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] net: Allow specifying ifname for qemu-bridge-helper

[Mike Lovell]

On 10/12/2012 12:49 AM, Mike Lovell wrote:
> This makes a few changes to allow ifname to be specified when using
> qemu-bridge-helper with both the bridge and tap network interfaces. It adds
> the --ifname option to qemu-bridge-helper, removes the restriction that ifname
> cannot be specified with helper for the tap interface, and adds logic to
> specify the --ifname option when exec'ing the helper.
>
> Signed-off-by: Mike Lovell <mike@dev-zero.net>
> ---
>
> This feature was originally requested by Mario De Chenno on the qemu-devel
> mailing list. Seems pretty simple and figured it was something I could throw
> together pretty quickly. I have tested the following combinations of invoking
> qemu (where qbr is qemu-bridge-helper)
>
> qemu-system-x86_64 -net nic -net tap,helper="qbr --br=test1"
> qemu-system-x86_64 -net nic -net tap,helper="qbr --br=test1",ifname=vm1
> qemu-system-x86_64 -net nic -net tap,helper=qbr
> qemu-system-x86_64 -net nic -net tap,helper=qbr,ifname=vm1
> qemu-system-x86_64 -net nic -net bridge,helper=qbr
> qemu-system-x86_64 -net nic -net bridge,helper=qbr,ifname=vm1
> qemu-system-x86_64 -net nic -net bridge,helper=qbr,ifname=vm1,br=test1
> qemu-system-x86_64 -net nic -net bridge,helper=qbr,br=test1

I realized there were a couple more cases to check. I also tested the 
following.

qemu-system-x86_64 -net nic -net tap,helper="qbr --ifname=vm1"
qemu-system-x86_64 -net nic -net tap,helper="qbr --br=test1 --ifname=vm1"
qemu-system-x86_64 -net nic -net tap,helper="qbr --ifname=vm1",ifname=foo
qemu-system-x86_64 -net nic -net tap,helper="qbr --br=test1 
--ifname=vm1",ifname=foo

In the last two cases, the --ifname specified in the helper option (in 
this case, vm1) is used over the tap ifname option (in this case, foo).

mike

Re: [Qemu-devel] [PATCH] net: Allow specifying ifname for qemu-bridge-helper

[Michael Tokarev]

On 12.10.2012 10:49, Mike Lovell wrote:
>      /* request a tap device, disable PI, and add vnet header support if
> -     * requested and it's available. */
> -    prep_ifreq(&ifr, "tap%d");
> +     * requested and it's available. use ifname if provided for tap name. */
> +    prep_ifreq(&ifr, ifname != NULL ? ifname : "tap%d");

Should we check for special symbols here? prep_ifreq() does this:

    snprintf(ifr->ifr_name, IFNAMSIZ, "%s", ifname);

so at least it ensures we have length constraint.

Actually I'm not so sure anymore this is a good idea.
For example, system may have firewall (iptables) rules
in place for, say, future ppp interfaces for ppp clients,
and this way we may request the interface to be named
pppX and be allowed to send packets where we don't usually
have access to.

Maybe - at least - require some common prefix for the
interfaces created this way, so we'll live in our own,
easily distinguishable namespace -- like, qvif* (from
Qemu Virtual InterFace)?

This is not a simple question really.  And the whole
bridge helper is quite questionable too.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH] net: Allow specifying ifname for qemu-bridge-helper

[Mike Lovell]

On 10/12/2012 02:32 AM, Michael Tokarev wrote:
> On 12.10.2012 10:49, Mike Lovell wrote:
>>       /* request a tap device, disable PI, and add vnet header support if
>> -     * requested and it's available. */
>> -    prep_ifreq(&ifr, "tap%d");
>> +     * requested and it's available. use ifname if provided for tap name. */
>> +    prep_ifreq(&ifr, ifname != NULL ? ifname : "tap%d");
> Should we check for special symbols here? prep_ifreq() does this:
>
>      snprintf(ifr->ifr_name, IFNAMSIZ, "%s", ifname);
>
> so at least it ensures we have length constraint.

I tried the code as is with specifying ifnames with various random 
combinations of special characters. Some of them we just allowed 
through, some caused an error when initializing the tap device, and some 
cause problems in the shell invoking qemu. I think the linux kernel does 
the necessary checking during the TUNSETIFF ioctl and the 
qemu-bridge-helper exits with an error if there was a problem.

> Actually I'm not so sure anymore this is a good idea.
> For example, system may have firewall (iptables) rules
> in place for, say, future ppp interfaces for ppp clients,
> and this way we may request the interface to be named
> pppX and be allowed to send packets where we don't usually
> have access to.

While I admit this does have that possibility, I'm not sure its a qemu 
problem. I don't know what the original motivation for the request was 
but I could see this being the reason for the request. Some 
administrator sets up firewall rules for a variety of guests ahead of 
actually running them and needs to specify the interface at runtime. 
Also, without using the helper programs, the qemu already allows 
specifying arbitrary names such as ppp0.

> Maybe - at least - require some common prefix for the
> interfaces created this way, so we'll live in our own,
> easily distinguishable namespace -- like, qvif* (from
> Qemu Virtual InterFace)?

I do like the idea of using a common prefix for the default name of tap 
devices. Something like "qvif%d" instead of "tap%d" in tap 
initialization code. But something tells me this could break 
compatibility with external management software where something might be 
expecting the interface name to start with tap.

mike

Re: [Qemu-devel] [PATCH] net: Allow specifying ifname for qemu-bridge-helper

[Mike Lovell]

On 10/12/2012 12:49 AM, Mike Lovell wrote:
> This makes a few changes to allow ifname to be specified when using
> qemu-bridge-helper with both the bridge and tap network interfaces. It adds
> the --ifname option to qemu-bridge-helper, removes the restriction that ifname
> cannot be specified with helper for the tap interface, and adds logic to
> specify the --ifname option when exec'ing the helper.
>
> Signed-off-by: Mike Lovell <mike@dev-zero.net>
> ---
>
> This feature was originally requested by Mario De Chenno on the qemu-devel
> mailing list. Seems pretty simple and figured it was something I could throw
> together pretty quickly. I have tested the following combinations of invoking
> qemu (where qbr is qemu-bridge-helper)
>
> qemu-system-x86_64 -net nic -net tap,helper="qbr --br=test1"
> qemu-system-x86_64 -net nic -net tap,helper="qbr --br=test1",ifname=vm1
> qemu-system-x86_64 -net nic -net tap,helper=qbr
> qemu-system-x86_64 -net nic -net tap,helper=qbr,ifname=vm1
> qemu-system-x86_64 -net nic -net bridge,helper=qbr
> qemu-system-x86_64 -net nic -net bridge,helper=qbr,ifname=vm1
> qemu-system-x86_64 -net nic -net bridge,helper=qbr,ifname=vm1,br=test1
> qemu-system-x86_64 -net nic -net bridge,helper=qbr,br=test1
>
>   net/tap.c            |   39 ++++++++++++++++++++++++++++-----------
>   qapi-schema.json     |    3 ++-
>   qemu-bridge-helper.c |   10 +++++++---
>   3 files changed, 37 insertions(+), 15 deletions(-)
>
> diff --git a/net/tap.c b/net/tap.c
> index a88ae8f..cfb5bff 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -417,11 +417,13 @@ static int recv_fd(int c)
>       return len;
>   }
>   
> -static int net_bridge_run_helper(const char *helper, const char *bridge)
> +static int net_bridge_run_helper(const char *helper,
> +                                 const char *bridge,
> +                                 const char *ifname)
>   {
>       sigset_t oldmask, mask;
>       int pid, status;
> -    char *args[5];
> +    char *args[6];
>       char **parg;
>       int sv[2];
>   
> @@ -439,7 +441,9 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
>           int open_max = sysconf(_SC_OPEN_MAX), i;
>           char fd_buf[6+10];
>           char br_buf[6+IFNAMSIZ] = {0};
> -        char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + 15];
> +        char ifname_buf[10+IFNAMSIZ] = {0};
> +        char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) +
> +                        sizeof(ifname_buf) + 15];
>   
>           for (i = 0; i < open_max; i++) {
>               if (i != STDIN_FILENO &&
> @@ -459,8 +463,13 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
>                   snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
>               }
>   
> -            snprintf(helper_cmd, sizeof(helper_cmd), "%s %s %s %s",
> -                     helper, "--use-vnet", fd_buf, br_buf);
> +            if ((strstr(helper, "--ifname=") == NULL) && (ifname != NULL)) {
> +                snprintf(ifname_buf, sizeof(ifname_buf), "%s%s" ,
> +                         "--ifname=", ifname);
> +            }
> +
> +            snprintf(helper_cmd, sizeof(helper_cmd), "%s %s %s %s %s",
> +                     helper, "--use-vnet", fd_buf, br_buf, ifname_buf);
>   
>               parg = args;
>               *parg++ = (char *)"sh";
> @@ -473,12 +482,17 @@ static int net_bridge_run_helper(const char *helper, const char *bridge)
>               /* assume helper is just the executable path name */
>   
>               snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
> +            if (ifname != NULL) {
> +                snprintf(ifname_buf, sizeof(ifname_buf), "%s%s" ,
> +                         "--ifname=", ifname);
> +            }
>   
>               parg = args;
>               *parg++ = (char *)helper;
>               *parg++ = (char *)"--use-vnet";
>               *parg++ = fd_buf;
>               *parg++ = br_buf;
> +            *parg++ = ifname_buf;
>               *parg++ = NULL;
>   
>               execv(helper, args);
> @@ -517,7 +531,7 @@ int net_init_bridge(const NetClientOptions *opts, const char *name,
>                       NetClientState *peer)
>   {
>       const NetdevBridgeOptions *bridge;
> -    const char *helper, *br;
> +    const char *helper, *br, *ifname;
>   
>       TAPState *s;
>       int fd, vnet_hdr;
> @@ -527,8 +541,9 @@ int net_init_bridge(const NetClientOptions *opts, const char *name,
>   
>       helper = bridge->has_helper ? bridge->helper : DEFAULT_BRIDGE_HELPER;
>       br     = bridge->has_br     ? bridge->br     : DEFAULT_BRIDGE_INTERFACE;
> +    ifname = bridge->has_ifname ? bridge->ifname : NULL;
>   
> -    fd = net_bridge_run_helper(helper, br);
> +    fd = net_bridge_run_helper(helper, br, ifname);
>       if (fd == -1) {
>           return -1;
>       }
> @@ -622,14 +637,16 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>           model = "tap";
>   
>       } else if (tap->has_helper) {
> -        if (tap->has_ifname || tap->has_script || tap->has_downscript ||
> -            tap->has_vnet_hdr) {
> -            error_report("ifname=, script=, downscript=, and vnet_hdr= "
> +        if (tap->has_script || tap->has_downscript || tap->has_vnet_hdr) {
> +            error_report("script=, downscript=, and vnet_hdr= "
>                            "are invalid with helper=");
>               return -1;
>           }
>   
> -        fd = net_bridge_run_helper(tap->helper, DEFAULT_BRIDGE_INTERFACE);
> +        const char *ifname;
> +        ifname = tap->has_ifname ? tap->ifname : NULL;
> +        fd = net_bridge_run_helper(tap->helper, DEFAULT_BRIDGE_INTERFACE,
> +                                   ifname);
>           if (fd == -1) {
>               return -1;
>           }
> diff --git a/qapi-schema.json b/qapi-schema.json
> index f9dbdae..feaac9e 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2432,7 +2432,8 @@
>   { 'type': 'NetdevBridgeOptions',
>     'data': {
>       '*br':     'str',
> -    '*helper': 'str' } }
> +    '*helper': 'str',
> +    '*ifname': 'str' } }
>   
>   ##
>   # @NetdevHubPortOptions
> diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
> index 652eec9..c1d1519 100644
> --- a/qemu-bridge-helper.c
> +++ b/qemu-bridge-helper.c
> @@ -67,7 +67,8 @@ typedef QSIMPLEQ_HEAD(ACLList, ACLRule) ACLList;
>   static void usage(void)
>   {
>       fprintf(stderr,
> -            "Usage: qemu-bridge-helper [--use-vnet] --br=bridge --fd=unixfd\n");
> +            "Usage: qemu-bridge-helper [--use-vnet] [--ifname=name] "
> +            "--br=bridge --fd=unixfd\n");
>   }
>   
>   static int parse_acl_file(const char *filename, ACLList *acl_list)
> @@ -239,6 +240,7 @@ int main(int argc, char **argv)
>       ACLList acl_list;
>       int access_allowed, access_denied;
>       int ret = EXIT_SUCCESS;
> +    const char *ifname = NULL;
>   
>   #ifdef CONFIG_LIBCAP
>       /* if we're run from an suid binary, immediately drop privileges preserving
> @@ -259,6 +261,8 @@ int main(int argc, char **argv)
>               bridge = &argv[index][5];
>           } else if (strncmp(argv[index], "--fd=", 5) == 0) {
>               unixfd = atoi(&argv[index][5]);
> +        } else if (strncmp(argv[index], "--ifname=", 9) == 0) {
> +            ifname = &argv[index][9];
>           } else {
>               usage();
>               return EXIT_FAILURE;
> @@ -329,8 +333,8 @@ int main(int argc, char **argv)
>       }
>   
>       /* request a tap device, disable PI, and add vnet header support if
> -     * requested and it's available. */
> -    prep_ifreq(&ifr, "tap%d");
> +     * requested and it's available. use ifname if provided for tap name. */
> +    prep_ifreq(&ifr, ifname != NULL ? ifname : "tap%d");
>       ifr.ifr_flags = IFF_TAP|IFF_NO_PI;
>       if (use_vnet && has_vnet_hdr(fd)) {
>           ifr.ifr_flags |= IFF_VNET_HDR;

ping ... or syn. any other thoughts about this?

mike

Re: [Qemu-devel] [PATCH] net: Allow specifying ifname for qemu-bridge-helper

[Michael Tokarev]

Somehow I missed this email initially.. replying now.

On 12.10.2012 22:04, Mike Lovell wrote:
> On 10/12/2012 02:32 AM, Michael Tokarev wrote:
>> On 12.10.2012 10:49, Mike Lovell wrote:
>>>       /* request a tap device, disable PI, and add vnet header support if
>>> -     * requested and it's available. */
>>> -    prep_ifreq(&ifr, "tap%d");
>>> +     * requested and it's available. use ifname if provided for tap name. */
>>> +    prep_ifreq(&ifr, ifname != NULL ? ifname : "tap%d");
>> Should we check for special symbols here? prep_ifreq() does this:
>>
>>      snprintf(ifr->ifr_name, IFNAMSIZ, "%s", ifname);
>>
>> so at least it ensures we have length constraint.
> 
> I tried the code as is with specifying ifnames with various random combinations of special characters. Some of them we just allowed through, some caused an error when initializing the tap device, and some cause problems in the shell invoking qemu. I think the linux kernel does the necessary checking during the TUNSETIFF ioctl and the qemu-bridge-helper exits with an error if there was a problem.
> 
>> Actually I'm not so sure anymore this is a good idea.
>> For example, system may have firewall (iptables) rules
>> in place for, say, future ppp interfaces for ppp clients,
>> and this way we may request the interface to be named
>> pppX and be allowed to send packets where we don't usually
>> have access to.
> 
> While I admit this does have that possibility, I'm not sure its a qemu problem. I don't know what the original motivation for the request was but I could see this being the reason for the request. Some administrator sets up firewall rules for a variety of guests ahead of actually running them and needs to specify the interface at runtime. Also, without using the helper programs, the qemu already allows specifying arbitrary names such as ppp0.

qemu allows arbitrary names, yes, but it does not have extra
permissions to create them, -- only ones of the current user.
The helper, on the other hand, does have extra privileges which
a regular user does not.  That's exactly what I was talking
about.

Maybe _always_ having a common prefix is a good idea after all,
with --name=FOO appended to it, like qvifFOO.  Or use --ifnumber=NNN
instead of --name (which I dislike).

>> Maybe - at least - require some common prefix for the
>> interfaces created this way, so we'll live in our own,
>> easily distinguishable namespace -- like, qvif* (from
>> Qemu Virtual InterFace)?
> 
> I do like the idea of using a common prefix for the default name of tap devices. Something like "qvif%d" instead of "tap%d" in tap initialization code. But something tells me this could break compatibility with external management software where something might be expecting the interface name to start with tap.

Does any management interface use this bridge-helper functionality?
If it were me, I'd always created the tap fd in the management
layer and passed the tap fd# (or at least ifname= of an existing
iface) to qemu.  Bridge helper is useful for users calling qemu
directly, not for management software.  Sure, such users are also
important - including compatibility.  But I don't think current
unpredictable tapNN names was a good idea to start with, and that
it's good idea to rely on this prefix in firewall rules or whatnot.

Thanks,

/mjt

Re: [Qemu-devel] [PATCH] net: Allow specifying ifname for qemu-bridge-helper

[Paolo Bonzini]

Il 30/11/2012 11:02, Michael Tokarev ha scritto:
>> I do like the idea of using a common prefix for the default name
>> of tap devices. Something like "qvif%d" instead of "tap%d" in tap
>> initialization code. But something tells me this could break
>> compatibility with external management software where something
>> might be expecting the interface name to start with tap.
>
> Does any management interface use this bridge-helper functionality? 

Libvirt uses it if you're running libvirtd without privileges.  GNOME
Boxes is a 1-line patch away from using it, but it's Fedora-specific and
not included in any distro.

Paolo

> If it were me, I'd always created the tap fd in the management layer
> and passed the tap fd# (or at least ifname= of an existing iface) to
> qemu.  Bridge helper is useful for users calling qemu directly, not
> for management software.  Sure, such users are also important -
> including compatibility.  But I don't think current unpredictable
> tapNN names was a good idea to start with, and that it's good idea to
> rely on this prefix in firewall rules or whatnot.

Re: [Qemu-devel] [PATCH] net: Allow specifying ifname for qemu-bridge-helper

[Paolo Bonzini]

Il 30/11/2012 08:10, Mike Lovell ha scritto:
> On 10/12/2012 12:49 AM, Mike Lovell wrote:
>> This makes a few changes to allow ifname to be specified when using
>> qemu-bridge-helper with both the bridge and tap network interfaces. It
>> adds
>> the --ifname option to qemu-bridge-helper, removes the restriction
>> that ifname
>> cannot be specified with helper for the tap interface, and adds logic to
>> specify the --ifname option when exec'ing the helper.
>
> ping ... or syn. any other thoughts about this?

I share Michael's perplexity.  This feature could be exploitable.

If we want to add this, the ifname should be subject to ACL rules just
like bridge names.  For example you could have a special allow/deny
directive "allow foo@" which allows ifnames starting with "foo".

Paolo

Re: [Qemu-devel] [PATCH] net: Allow specifying ifname for qemu-bridge-helper

[Stefan Hajnoczi]

On Fri, Nov 30, 2012 at 03:35:46PM +0100, Paolo Bonzini wrote:
> Il 30/11/2012 08:10, Mike Lovell ha scritto:
> > On 10/12/2012 12:49 AM, Mike Lovell wrote:
> >> This makes a few changes to allow ifname to be specified when using
> >> qemu-bridge-helper with both the bridge and tap network interfaces. It
> >> adds
> >> the --ifname option to qemu-bridge-helper, removes the restriction
> >> that ifname
> >> cannot be specified with helper for the tap interface, and adds logic to
> >> specify the --ifname option when exec'ing the helper.
> >
> > ping ... or syn. any other thoughts about this?
> 
> I share Michael's perplexity.  This feature could be exploitable.
> 
> If we want to add this, the ifname should be subject to ACL rules just
> like bridge names.  For example you could have a special allow/deny
> directive "allow foo@" which allows ifnames starting with "foo".

This is a good idea.  The default should be that you are not allowed to
choose arbitrary interface names.

Stefan