[Qemu-devel] [PATCH 0/2] fix macaddr allocation

[Qemu-devel] [PATCH 0/2] fix macaddr allocation

[Amos Kong]

This patchset adds a check in allocating mac to new nic,
it will avoid to use repeated mac. The second extends
the address space.

Amos Kong (2):
  avoid to allcate used macaddr to to new nic
  extend the macaddr space to 0xffffffff

 net/net.c | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/2] avoid to allcate used macaddr to to new nic

[Amos Kong]

QEMU allocates macaddr to nic if user doesn't assigne macaddr.
But we didn't check if the allocated macaddr is used, it might
cause macaddr repeated.

 # qemu -device e1000,netdev=h1,mac=52:54:00:12:34:56
  (qemu) device_add e1000
  (qemu) info network
  e1000.0: index=0,type=nic,model=e1000,macaddr=52:54:00:12:34:56
   \ h1: index=0,type=user,net=10.0.2.0,restrict=off
  e1000.1: index=0,type=nic,model=e1000,macaddr=52:54:00:12:34:56

This patch adds a check in allocating macaddr, reallocate macaddr
if it's used by other nic.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 net/net.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/net/net.c b/net/net.c
index 43a74e4..f019da4 100644
--- a/net/net.c
+++ b/net/net.c
@@ -143,15 +143,43 @@ void qemu_macaddr_default_if_unset(MACAddr *macaddr)
 {
     static int index = 0;
     static const MACAddr zero = { .a = { 0,0,0,0,0,0 } };
+    char info_str[256];
+    NetClientState *nc, *peer;
+    NetClientOptionsKind type;
 
     if (memcmp(macaddr, &zero, sizeof(zero)) != 0)
         return;
+
+realloc_mac:
     macaddr->a[0] = 0x52;
     macaddr->a[1] = 0x54;
     macaddr->a[2] = 0x00;
     macaddr->a[3] = 0x12;
     macaddr->a[4] = 0x34;
     macaddr->a[5] = 0x56 + index++;
+
+    QTAILQ_FOREACH(nc, &net_clients, next) {
+        peer = nc->peer;
+        type = nc->info->type;
+
+        if (net_hub_id_for_client(nc, NULL) == 0) {
+            continue;
+        }
+
+        if (!peer || type == NET_CLIENT_OPTIONS_KIND_NIC) {
+            snprintf(info_str, sizeof(info_str),
+                     "model=%s,macaddr=%02x:%02x:%02x:%02x:%02x:%02x",
+                     nc->model,
+                     macaddr->a[0], macaddr->a[1], macaddr->a[2],
+                     macaddr->a[3], macaddr->a[4], macaddr->a[5]);
+
+            /* reallocate macaddr if it's used by other nic */
+            if (!strcmp(nc->info_str, info_str)) {
+                goto realloc_mac;
+            }
+        }
+    }
+
 }
 
 /**
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/2] avoid to allcate used macaddr to to new nic

[Michael S. Tsirkin]

On Mon, Jun 17, 2013 at 09:35:10PM +0800, Amos Kong wrote:
> QEMU allocates macaddr to nic if user doesn't assigne macaddr.
> But we didn't check if the allocated macaddr is used, it might
> cause macaddr repeated.
> 
>  # qemu -device e1000,netdev=h1,mac=52:54:00:12:34:56
>   (qemu) device_add e1000
>   (qemu) info network
>   e1000.0: index=0,type=nic,model=e1000,macaddr=52:54:00:12:34:56
>    \ h1: index=0,type=user,net=10.0.2.0,restrict=off
>   e1000.1: index=0,type=nic,model=e1000,macaddr=52:54:00:12:34:56
> 
> This patch adds a check in allocating macaddr, reallocate macaddr
> if it's used by other nic.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>

I'm not sure this is not exactly what was intended in this case.
Also this ptotects against an unlikely case of mixing
implicit and explicit addresses, but not against
a likely case of multiple qemu on same LAN using same MAC.

In short, implicit mac is a bad idea, don't use it.
I'd ack a patch that marks mac non-optional in
help text and documentation.

> ---
>  net/net.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/net/net.c b/net/net.c
> index 43a74e4..f019da4 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -143,15 +143,43 @@ void qemu_macaddr_default_if_unset(MACAddr *macaddr)
>  {
>      static int index = 0;
>      static const MACAddr zero = { .a = { 0,0,0,0,0,0 } };
> +    char info_str[256];
> +    NetClientState *nc, *peer;
> +    NetClientOptionsKind type;
>  
>      if (memcmp(macaddr, &zero, sizeof(zero)) != 0)
>          return;
> +
> +realloc_mac:
>      macaddr->a[0] = 0x52;
>      macaddr->a[1] = 0x54;
>      macaddr->a[2] = 0x00;
>      macaddr->a[3] = 0x12;
>      macaddr->a[4] = 0x34;
>      macaddr->a[5] = 0x56 + index++;
> +
> +    QTAILQ_FOREACH(nc, &net_clients, next) {
> +        peer = nc->peer;
> +        type = nc->info->type;
> +
> +        if (net_hub_id_for_client(nc, NULL) == 0) {
> +            continue;
> +        }
> +
> +        if (!peer || type == NET_CLIENT_OPTIONS_KIND_NIC) {
> +            snprintf(info_str, sizeof(info_str),
> +                     "model=%s,macaddr=%02x:%02x:%02x:%02x:%02x:%02x",
> +                     nc->model,
> +                     macaddr->a[0], macaddr->a[1], macaddr->a[2],
> +                     macaddr->a[3], macaddr->a[4], macaddr->a[5]);
> +
> +            /* reallocate macaddr if it's used by other nic */
> +            if (!strcmp(nc->info_str, info_str)) {
> +                goto realloc_mac;

Please don't code loops like that.

> +            }
> +        }
> +    }
> +
>  }
>  
>  /**
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH 1/2] avoid to allcate used macaddr to to new nic

[Fam Zheng]

On Mon, 06/17 17:00, Michael S. Tsirkin wrote:
> On Mon, Jun 17, 2013 at 09:35:10PM +0800, Amos Kong wrote:
> > QEMU allocates macaddr to nic if user doesn't assigne macaddr.
> > But we didn't check if the allocated macaddr is used, it might
> > cause macaddr repeated.
> > 
> >  # qemu -device e1000,netdev=h1,mac=52:54:00:12:34:56
> >   (qemu) device_add e1000
> >   (qemu) info network
> >   e1000.0: index=0,type=nic,model=e1000,macaddr=52:54:00:12:34:56
> >    \ h1: index=0,type=user,net=10.0.2.0,restrict=off
> >   e1000.1: index=0,type=nic,model=e1000,macaddr=52:54:00:12:34:56
> > 
> > This patch adds a check in allocating macaddr, reallocate macaddr
> > if it's used by other nic.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> 
> I'm not sure this is not exactly what was intended in this case.
> Also this ptotects against an unlikely case of mixing
> implicit and explicit addresses, but not against
> a likely case of multiple qemu on same LAN using same MAC.

IMHO, either way we can do little to protect against collision of
multiple qemu on the same LAN, but at least this patch protects against
repeated MAC address in one qemu instance. Better in some degree.

Leaving it to user, and asking for address explictly, absolutely helps,
but makes the interface a bit harder to use: there are still cases user
wants it generated automatically.

Just wondering if a random one could be better?

-- 
Fam

Re: [Qemu-devel] [PATCH 1/2] avoid to allcate used macaddr to to new nic

[Michael S. Tsirkin]

On Tue, Jun 18, 2013 at 10:05:58AM +0800, Fam Zheng wrote:
> On Mon, 06/17 17:00, Michael S. Tsirkin wrote:
> > On Mon, Jun 17, 2013 at 09:35:10PM +0800, Amos Kong wrote:
> > > QEMU allocates macaddr to nic if user doesn't assigne macaddr.
> > > But we didn't check if the allocated macaddr is used, it might
> > > cause macaddr repeated.
> > > 
> > >  # qemu -device e1000,netdev=h1,mac=52:54:00:12:34:56
> > >   (qemu) device_add e1000
> > >   (qemu) info network
> > >   e1000.0: index=0,type=nic,model=e1000,macaddr=52:54:00:12:34:56
> > >    \ h1: index=0,type=user,net=10.0.2.0,restrict=off
> > >   e1000.1: index=0,type=nic,model=e1000,macaddr=52:54:00:12:34:56
> > > 
> > > This patch adds a check in allocating macaddr, reallocate macaddr
> > > if it's used by other nic.
> > > 
> > > Signed-off-by: Amos Kong <akong@redhat.com>
> > 
> > I'm not sure this is not exactly what was intended in this case.
> > Also this ptotects against an unlikely case of mixing
> > implicit and explicit addresses, but not against
> > a likely case of multiple qemu on same LAN using same MAC.
> 
> IMHO, either way we can do little to protect against collision of
> multiple qemu on the same LAN, but at least this patch protects against
> repeated MAC address in one qemu instance. Better in some degree.

This is a policy, we should not dictate it.
Maybe you want same MAC for some reason?

> Leaving it to user, and asking for address explictly, absolutely helps,
> but makes the interface a bit harder to use: there are still cases user
> wants it generated automatically.

A user that does not want to know what "MAC" even means
is the only one I'm aware of. This is not such a case.

> Just wondering if a random one could be better?

If we are talking about a guest with multiple NICs,
if you generate MACs randomly guest won't know which is which.

It also breaks assumptions guests make that MAC
is a static property of hardware. E.g. it can force windows
re-activation, break resume from suspend etc

> -- 
> Fam

[Qemu-devel] [PATCH 2/2] extend the macaddr space to 0xffffffff

[Amos Kong]

Currently we only support to allocate 0xff mac-addresses,
if we start guest by pci-bridge/multiple-func, the macaddr
are not enough.

This patch extends the mac-address space to 0xffffffff

52:54:00:00:00:00 ~ 52:54:ff:ff:ff:ff

Signed-off-by: Amos Kong <akong@redhat.com>
---
 net/net.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/net.c b/net/net.c
index f019da4..78bb080 100644
--- a/net/net.c
+++ b/net/net.c
@@ -153,10 +153,12 @@ void qemu_macaddr_default_if_unset(MACAddr *macaddr)
 realloc_mac:
     macaddr->a[0] = 0x52;
     macaddr->a[1] = 0x54;
-    macaddr->a[2] = 0x00;
-    macaddr->a[3] = 0x12;
-    macaddr->a[4] = 0x34;
-    macaddr->a[5] = 0x56 + index++;
+    macaddr->a[2] = 0x00 + ((0x120000 + 0x3400 + 0x56 + index) >> 24 & 0xff);
+    macaddr->a[3] = 0x12 + ((0x3400 + 0x56 + index) >> 16 & 0xff);
+    macaddr->a[4] = 0x34 + ((0x56 + index) >> 8 & 0xff);
+    macaddr->a[5] = 0x56 + (index & 0xff);
+
+    index++;
 
     QTAILQ_FOREACH(nc, &net_clients, next) {
         peer = nc->peer;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 2/2] extend the macaddr space to 0xffffffff

[Michael S. Tsirkin]

On Mon, Jun 17, 2013 at 09:35:11PM +0800, Amos Kong wrote:
> Currently we only support to allocate 0xff mac-addresses,
> if we start guest by pci-bridge/multiple-func, the macaddr
> are not enough.
> 
> This patch extends the mac-address space to 0xffffffff
> 
> 52:54:00:00:00:00 ~ 52:54:ff:ff:ff:ff
> 
> Signed-off-by: Amos Kong <akong@redhat.com>


And then there's even more chance a user error
(forgot to specify mac) will lead to broken
LANs because of collisions.

Why is it that whoever wants >256 NICs can't just
specify the addresses explicitly?

> ---
>  net/net.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/net/net.c b/net/net.c
> index f019da4..78bb080 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -153,10 +153,12 @@ void qemu_macaddr_default_if_unset(MACAddr *macaddr)
>  realloc_mac:
>      macaddr->a[0] = 0x52;
>      macaddr->a[1] = 0x54;
> -    macaddr->a[2] = 0x00;
> -    macaddr->a[3] = 0x12;
> -    macaddr->a[4] = 0x34;
> -    macaddr->a[5] = 0x56 + index++;
> +    macaddr->a[2] = 0x00 + ((0x120000 + 0x3400 + 0x56 + index) >> 24 & 0xff);
> +    macaddr->a[3] = 0x12 + ((0x3400 + 0x56 + index) >> 16 & 0xff);
> +    macaddr->a[4] = 0x34 + ((0x56 + index) >> 8 & 0xff);
> +    macaddr->a[5] = 0x56 + (index & 0xff);
> +
> +    index++;
>  
>      QTAILQ_FOREACH(nc, &net_clients, next) {
>          peer = nc->peer;
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH 2/2] extend the macaddr space to 0xffffffff

[Amos Kong]

On Mon, Jun 17, 2013 at 05:04:03PM +0300, Michael S. Tsirkin wrote:
> On Mon, Jun 17, 2013 at 09:35:11PM +0800, Amos Kong wrote:
> > Currently we only support to allocate 0xff mac-addresses,
> > if we start guest by pci-bridge/multiple-func, the macaddr
> > are not enough.
> > 
> > This patch extends the mac-address space to 0xffffffff
> > 
> > 52:54:00:00:00:00 ~ 52:54:ff:ff:ff:ff
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> 
> 
> And then there's even more chance a user error
> (forgot to specify mac) will lead to broken
> LANs because of collisions.
>
> Why is it that whoever wants >256 NICs can't just
> specify the addresses explicitly?

We should lead user to use assigned mac, those two patches
are just considered for the lazy users ;)


-- 
			Amos.

Re: [Qemu-devel] [PATCH 2/2] extend the macaddr space to 0xffffffff

[Michael S. Tsirkin]

On Tue, Jun 18, 2013 at 09:51:44AM +0800, Amos Kong wrote:
> On Mon, Jun 17, 2013 at 05:04:03PM +0300, Michael S. Tsirkin wrote:
> > On Mon, Jun 17, 2013 at 09:35:11PM +0800, Amos Kong wrote:
> > > Currently we only support to allocate 0xff mac-addresses,
> > > if we start guest by pci-bridge/multiple-func, the macaddr
> > > are not enough.
> > > 
> > > This patch extends the mac-address space to 0xffffffff
> > > 
> > > 52:54:00:00:00:00 ~ 52:54:ff:ff:ff:ff
> > > 
> > > Signed-off-by: Amos Kong <akong@redhat.com>
> > 
> > 
> > And then there's even more chance a user error
> > (forgot to specify mac) will lead to broken
> > LANs because of collisions.
> >
> > Why is it that whoever wants >256 NICs can't just
> > specify the addresses explicitly?
> 
> We should lead user to use assigned mac, those two patches
> are just considered for the lazy users ;)

Let's just fail address allocation once the low byte overflows.
If you want >256 NICs, specify MAC addresses.

> 
> -- 
> 			Amos.

Re: [Qemu-devel] [PATCH 0/2] fix macaddr allocation

[Stefan Hajnoczi]

On Mon, Jun 17, 2013 at 09:35:09PM +0800, Amos Kong wrote:
> This patchset adds a check in allocating mac to new nic,
> it will avoid to use repeated mac. The second extends
> the address space.
> 
> Amos Kong (2):
>   avoid to allcate used macaddr to to new nic
>   extend the macaddr space to 0xffffffff
> 
>  net/net.c | 38 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 34 insertions(+), 4 deletions(-)

I agree with Michael here and think neither patch should be merged.

Both of these patches provide some level of convenience but they cannot
solve the problem.  Things already work fine automatically in small
setups where the user depends on QEMU to handle MAC allocation.

In bigger setups you absolutely have to manage MACs or face collisions.
So I don't think it's worth trying to maintain the illusion in
scenarios that are too complex to automatically allocate for.

Stefan

Re: [Qemu-devel] [PATCH 0/2] fix macaddr allocation

[Amos Kong]

On Tue, Jun 18, 2013 at 01:21:11PM +0200, Stefan Hajnoczi wrote:
> On Mon, Jun 17, 2013 at 09:35:09PM +0800, Amos Kong wrote:
> > This patchset adds a check in allocating mac to new nic,
> > it will avoid to use repeated mac. The second extends
> > the address space.
> > 
> > Amos Kong (2):
> >   avoid to allcate used macaddr to to new nic
> >   extend the macaddr space to 0xffffffff
> > 
> >  net/net.c | 38 ++++++++++++++++++++++++++++++++++----
> >  1 file changed, 34 insertions(+), 4 deletions(-)
> 
> I agree with Michael here and think neither patch should be merged.


So did I, thanks for your comments.
 
> Both of these patches provide some level of convenience but they cannot
> solve the problem.  Things already work fine automatically in small
> setups where the user depends on QEMU to handle MAC allocation.
> 
> In bigger setups you absolutely have to manage MACs or face collisions.
> So I don't think it's worth trying to maintain the illusion in
> scenarios that are too complex to automatically allocate for.
>
> Stefan

-- 
			Amos.