Patchwork add mac address collision checking for device_add & pci_add

login
register
mail settings
Submitter Lin Ma
Date Nov. 12, 2012, 11:12 a.m.
Message ID <1352718766-5663-1-git-send-email-lma@suse.com>
Download mbox | patch
Permalink /patch/198364/
State New
Headers show

Comments

Lin Ma - Nov. 12, 2012, 11:12 a.m.
QEMU doesn't check if there are mac collisions when adding nics.
It causes mac address collisions in guest if adding the nics which include existing physical address.
This patch fixes the issue.

Signed-off-by: Lin Ma <lma@suse.com>
---
 hw/qdev-properties.c |    9 +++++++++
 net.c                |   28 ++++++++++++++++++++++++++++
 net.h                |    1 +
 3 files changed, 38 insertions(+)
Daniel P. Berrange - Nov. 12, 2012, 11:18 a.m.
On Mon, Nov 12, 2012 at 07:12:46PM +0800, Lin Ma wrote:
> QEMU doesn't check if there are mac collisions when adding nics.
> It causes mac address collisions in guest if adding the nics which
> include existing physical address.
> This patch fixes the issue.

I understand the issue, but are there not use cases where it is
reasonable to have multiple NICs with the same MAC address ? To
me this kind of policy enforcement belongs at a higher level in
the mgmt stack.

Daniel
Paolo Bonzini - Nov. 12, 2012, 11:26 a.m.
Il 12/11/2012 12:18, Daniel P. Berrange ha scritto:
>> > QEMU doesn't check if there are mac collisions when adding nics.
>> > It causes mac address collisions in guest if adding the nics which
>> > include existing physical address.
>> > This patch fixes the issue.
> I understand the issue, but are there not use cases where it is
> reasonable to have multiple NICs with the same MAC address ? To
> me this kind of policy enforcement belongs at a higher level in
> the mgmt stack.

I agree.

Paolo
Lin Ma - Nov. 12, 2012, 11:49 a.m.
>>> Paolo Bonzini <pbonzini@redhat.com> 11/12/12 7:27 PM >>>
Il 12/11/2012 12:18, Daniel P. Berrange ha scritto:
>> > QEMU doesn't check if there are mac collisions when adding nics.
>> > It causes mac address collisions in guest if adding the nics which
>> > include existing physical address.
>> > This patch fixes the issue.
> I understand the issue, but are there not use cases where it is
> reasonable to have multiple NICs with the same MAC address ? To
> me this kind of policy enforcement belongs at a higher level in
> the mgmt stack.

>>>I agree.

Yes, Users won't intentionally add multiple NICs with the same MAC address.
But what if there is typos in command-line or upper layer applications pass a conflicting mac option to qemu?
We can't fully trust other applications to do this. So qemu shouldn't depend on other applications for the verifying mac.
No matter upper layer applications verify mac or not, qemu should check the mac itself again to ensure the correctness.

Lin
Stefan Hajnoczi - Nov. 12, 2012, 12:28 p.m.
On Mon, Nov 12, 2012 at 12:49 PM, Lin Ma <lma@suse.com> wrote:
>>>> Paolo Bonzini <pbonzini@redhat.com> 11/12/12 7:27 PM >>>
>
> Il 12/11/2012 12:18, Daniel P. Berrange ha scritto:
>>> > QEMU doesn't check if there are mac collisions when adding nics.
>>> > It causes mac address collisions in guest if adding the nics which
>>> > include existing physical address.
>>> > This patch fixes the issue.
>> I understand the issue, but are there not use cases where it is
>> reasonable to have multiple NICs with the same MAC address ? To
>> me this kind of policy enforcement belongs at a higher level in
>> the mgmt stack.
>
>>>>I agree.
>
> Yes, Users won't intentionally add multiple NICs with the same MAC address.
> But what if there is typos in command-line or upper layer applications pass
> a conflicting mac option to qemu?
> We can't fully trust other applications to do this. So qemu shouldn't depend
> on other applications for the verifying mac.
> No matter upper layer applications verify mac or not, qemu should check the
> mac itself again to ensure the correctness.

It's perfectly okay to use the same MAC address on multiple cards from
a hardware emulation perspective (which is the scope of QEMU).

Higher level tools can perform this check if they want this policy.

Stefan

Patch

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 8aca0d4..23318af 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -761,6 +761,15 @@  static void set_mac(Object *obj, Visitor *v, void *opaque,
         }
         mac->a[i] = strtol(str+pos, &p, 16);
     }
+
+    if (qemu_check_macaddr_uniqueness(mac->a) < 0) {
+        goto collision;
+    }
+    g_free(str);
+    return;
+
+collision:
+    error_set_from_qdev_prop_error(errp, -EEXIST, dev, prop, str);
     g_free(str);
     return;
 
diff --git a/net.c b/net.c
index e8ae13e..3077405 100644
--- a/net.c
+++ b/net.c
@@ -535,6 +535,29 @@  int qemu_find_nic_model(NICInfo *nd, const char * const *models,
     return -1;
 }
 
+int qemu_check_macaddr_uniqueness(uint8_t *macaddr)
+{
+    NetClientState *nc;
+    uint8_t existing_mac[6];
+    char *mac_str;
+    int i;
+
+    QTAILQ_FOREACH(nc, &net_clients, next) {
+        if (nc->info->type == NET_CLIENT_OPTIONS_KIND_NIC) {
+            mac_str = strstr(nc->info_str, "macaddr=") + strlen("macaddr=");
+            for (i = 0; i < 6; i++) {
+                existing_mac[i] = strtol(mac_str, (char **)&mac_str, 16);
+                mac_str++;
+            }
+            if (memcmp(macaddr, &existing_mac, sizeof(existing_mac)) == 0) {
+                return -1;
+            }
+        }
+    }
+
+    return 0;
+}
+
 static int net_init_nic(const NetClientOptions *opts, const char *name,
                         NetClientState *peer)
 {
@@ -582,6 +605,11 @@  static int net_init_nic(const NetClientOptions *opts, const char *name,
     }
     qemu_macaddr_default_if_unset(&nd->macaddr);
 
+    if (qemu_check_macaddr_uniqueness(nd->macaddr.a) < 0) {
+        error_report("ethernet address collision");
+        return -1;
+    }
+
     if (nic->has_vectors) {
         if (nic->vectors > 0x7ffffff) {
             error_report("invalid # of vectors: %"PRIu32, nic->vectors);
diff --git a/net.h b/net.h
index 04fda1d..227e47d 100644
--- a/net.h
+++ b/net.h
@@ -99,6 +99,7 @@  int qemu_show_nic_models(const char *arg, const char *const *models);
 void qemu_check_nic_model(NICInfo *nd, const char *model);
 int qemu_find_nic_model(NICInfo *nd, const char * const *models,
                         const char *default_model);
+int qemu_check_macaddr_uniqueness(uint8_t *macaddr);
 
 ssize_t qemu_deliver_packet(NetClientState *sender,
                             unsigned flags,