| Message ID | 1371476111-4449-2-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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?
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
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; + } + } + } + } /**
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(+)