diff mbox

[RFC,v2,3/4] xen-netback: use a random MAC address

Message ID 1392433180-16052-4-git-send-email-mcgrof@do-not-panic.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Luis R. Rodriguez Feb. 15, 2014, 2:59 a.m. UTC 
From: "Luis R. Rodriguez" <mcgrof@suse.com>

The purpose of using a static MAC address of FE:FF:FF:FF:FF:FF
was to prevent our backend interfaces from being used by the
bridge and nominating our interface as a root bridge. This was
possible given that the bridge code will use the lowest MAC
address for a port once a new interface gets added to the bridge.
The bridge code has a generic feature now to allow interfaces
to opt out from root bridge nominations, use that instead.

Cc: Paul Durrant <Paul.Durrant@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: xen-devel@lists.xenproject.org
Cc: netdev@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/net/xen-netback/interface.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

Comments

David Vrabel Feb. 17, 2014, 10:29 a.m. UTC | #1 
On 15/02/14 02:59, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> 
> The purpose of using a static MAC address of FE:FF:FF:FF:FF:FF
> was to prevent our backend interfaces from being used by the
> bridge and nominating our interface as a root bridge. This was
> possible given that the bridge code will use the lowest MAC
> address for a port once a new interface gets added to the bridge.
> The bridge code has a generic feature now to allow interfaces
> to opt out from root bridge nominations, use that instead.
[...]
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -42,6 +42,8 @@
>  #define XENVIF_QUEUE_LENGTH 32
>  #define XENVIF_NAPI_WEIGHT  64
>  
> +static const u8 xen_oui[3] = { 0x00, 0x16, 0x3e };

You shouldn't use a vendor prefix with a random MAC address.  You should
set the locally administered bit and clear the multicast/unicast bit and
randomize the remaining 46 bits.

(If existing VIF scripts are doing something similar, they also need to
be fixed.)

David
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ian Campbell Feb. 18, 2014, 11:22 a.m. UTC | #2 
On Mon, 2014-02-17 at 10:29 +0000, David Vrabel wrote:
> On 15/02/14 02:59, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > 
> > The purpose of using a static MAC address of FE:FF:FF:FF:FF:FF
> > was to prevent our backend interfaces from being used by the
> > bridge and nominating our interface as a root bridge. This was
> > possible given that the bridge code will use the lowest MAC
> > address for a port once a new interface gets added to the bridge.
> > The bridge code has a generic feature now to allow interfaces
> > to opt out from root bridge nominations, use that instead.
> [...]
> > --- a/drivers/net/xen-netback/interface.c
> > +++ b/drivers/net/xen-netback/interface.c
> > @@ -42,6 +42,8 @@
> >  #define XENVIF_QUEUE_LENGTH 32
> >  #define XENVIF_NAPI_WEIGHT  64
> >  
> > +static const u8 xen_oui[3] = { 0x00, 0x16, 0x3e };
> 
> You shouldn't use a vendor prefix with a random MAC address.  You should
> set the locally administered bit and clear the multicast/unicast bit and
> randomize the remaining 46 bits.

I'd have thought that eth_hw_addr_random would get this right, *checks*
yes it does. And then this patch tramples overt the top three bytes.

Might there be any requirement to have a specific MAC on the vif device?
IOW do we need to figure out a way to plumb this through the Xen tools
(perhaps having the vif script sort it out).

Speaking of which -- do the Xen tools not overwrite this random mac from
xen-network-common.sh:_setup_bridge_port. What is the plan to change
that (in a forwards/backwards compatible manner).

Ian.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luis R. Rodriguez Feb. 18, 2014, 9:30 p.m. UTC | #3 
On Tue, Feb 18, 2014 at 3:22 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Mon, 2014-02-17 at 10:29 +0000, David Vrabel wrote:
>> On 15/02/14 02:59, Luis R. Rodriguez wrote:
>> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
>> >
>> > The purpose of using a static MAC address of FE:FF:FF:FF:FF:FF
>> > was to prevent our backend interfaces from being used by the
>> > bridge and nominating our interface as a root bridge. This was
>> > possible given that the bridge code will use the lowest MAC
>> > address for a port once a new interface gets added to the bridge.
>> > The bridge code has a generic feature now to allow interfaces
>> > to opt out from root bridge nominations, use that instead.
>> [...]
>> > --- a/drivers/net/xen-netback/interface.c
>> > +++ b/drivers/net/xen-netback/interface.c
>> > @@ -42,6 +42,8 @@
>> >  #define XENVIF_QUEUE_LENGTH 32
>> >  #define XENVIF_NAPI_WEIGHT  64
>> >
>> > +static const u8 xen_oui[3] = { 0x00, 0x16, 0x3e };
>>
>> You shouldn't use a vendor prefix with a random MAC address.  You should
>> set the locally administered bit and clear the multicast/unicast bit and
>> randomize the remaining 46 bits.
>
> I'd have thought that eth_hw_addr_random would get this right, *checks*
> yes it does. And then this patch tramples overt the top three bytes.
>
> Might there be any requirement to have a specific MAC on the vif device?
> IOW do we need to figure out a way to plumb this through the Xen tools
> (perhaps having the vif script sort it out).

Based on Stephen's feedback we should be setting IFLA_BRPORT_PROTECT
to the xen-netback and TAP interfaces in topologies where it makes
sense prior to adding them to the bridge. Userspace can surely deal
with the MAC address but I believe removing the static MAC address
would be good once we get userspace to use the IFLA_BRPORT_PROTECT
flag, to avoid the IPv6 duplication issue incurred by the current
static MAC address. The MAC address consideration remains given that
as per Zoltan there are topologies where the xen-netback interfaces
can make use of a either an IPv4 or IPv6 address.

> Speaking of which -- do the Xen tools not overwrite this random mac from
> xen-network-common.sh:_setup_bridge_port. What is the plan to change
> that (in a forwards/backwards compatible manner).

I'm not seeing that happen now ?

  Luis
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index fff8cdd..d380e3f 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -42,6 +42,8 @@ 
 #define XENVIF_QUEUE_LENGTH 32
 #define XENVIF_NAPI_WEIGHT  64
 
+static const u8 xen_oui[3] = { 0x00, 0x16, 0x3e };
+
 int xenvif_schedulable(struct xenvif *vif)
 {
 	return netif_running(vif->dev) && netif_carrier_ok(vif->dev);
@@ -347,15 +349,9 @@  struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
 	for (i = 0; i < MAX_PENDING_REQS; i++)
 		vif->mmap_pages[i] = NULL;
 
-	/*
-	 * Initialise a dummy MAC address. We choose the numerically
-	 * largest non-broadcast address to prevent the address getting
-	 * stolen by an Ethernet bridge for STP purposes.
-	 * (FE:FF:FF:FF:FF:FF)
-	 */
-	memset(dev->dev_addr, 0xFF, ETH_ALEN);
-	dev->dev_addr[0] &= ~0x01;
-
+	eth_hw_addr_random(dev);
+	memcpy(dev->dev_addr, xen_oui, 3);
+	dev->priv_flags |= IFF_BRIDGE_NON_ROOT;
 	netif_napi_add(dev, &vif->napi, xenvif_poll, XENVIF_NAPI_WEIGHT);
 
 	netif_carrier_off(dev);