Patchwork [19/25] mlx4: Randomizing mac addresses for slaves

login
register
mail settings
Submitter Yevgeny Petrilin
Date Nov. 4, 2009, 3:31 p.m.
Message ID <4AF19E69.8070605@mellanox.co.il>
Download mbox | patch
Permalink /patch/37566/
State Awaiting Upstream
Delegated to: David Miller
Headers show

Comments

Yevgeny Petrilin - Nov. 4, 2009, 3:31 p.m.
Signed-off-by: Yevgeny Petrilin <yevgenyp@mellanox.co.il>
---
 drivers/net/mlx4/fw.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
Roland Dreier - Nov. 4, 2009, 8:04 p.m.
> +#define MLX4_MAC_HEAD		0x2c9000000ULL
 > +			random_ether_addr(rand_mac);
 > +			caps->def_mac[i] = MLX4_MAC_HEAD | rand_mac[0] |
 > +				((u64)(rand_mac[1]) << 8) | ((u64)(rand_mac[2]) << 16);

Is this a good idea?  You're basically choosing 24 random bits within
your OUI... seems the chance of collision with another MAC used on the
same network is high enough that it could easily happen in practice on a
moderately big network.  Can you pick a reserved range or something?

Also I'm not sure if there's much point in using random_ether_addr if
you're going to override the high order part anyway, since
random_ether_addr just makes sure it doesn't pick a multicast address.

 - R.
--
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
Or Gerlitz - Nov. 4, 2009, 9:33 p.m.
On Wed, Nov 4, 2009 at 10:04 PM, Roland Dreier <rdreier@cisco.com> wrote:
>> +#define MLX4_MAC_HEAD               0x2c9000000ULL

> Is this a good idea?  You're basically choosing 24 random bits within your OUI...
> seems the chance of collision with another MAC used on the same network is
> high enough that it could easily happen in practice on a moderately big network.

yes, this has been brought by Stephen and others on this last back on
September 11th, this year @
http://marc.info/?l=linux-netdev&m=125263488409128

> Can you pick a reserved range or something?

Using different OUI for the VF device wouldn't help either I think,
since the #VF becomes fairly big even on a modest side cluster with
(say) a VM consuming VF per 1-2 cores.

Or.
--
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
Liran Liss - Nov. 5, 2009, 1:38 p.m.
This approach seems to be common practice now (e.g., drivers/net/igb/igb_main.c:1332).
In any case, the user can change the randomized mac.
--Liran

-----Original Message-----
From: Or Gerlitz [mailto:or.gerlitz@gmail.com] 
Sent: Wednesday, November 04, 2009 11:33 PM
To: Roland Dreier
Cc: Yevgeny Petrilin; linux-rdma@vger.kernel.org; netdev@vger.kernel.org; Liran Liss; Tziporet Koren
Subject: Re: [PATCH 19/25] mlx4: Randomizing mac addresses for slaves

On Wed, Nov 4, 2009 at 10:04 PM, Roland Dreier <rdreier@cisco.com> wrote:
>> +#define MLX4_MAC_HEAD               0x2c9000000ULL

> Is this a good idea?  You're basically choosing 24 random bits within your OUI...
> seems the chance of collision with another MAC used on the same 
> network is high enough that it could easily happen in practice on a moderately big network.

yes, this has been brought by Stephen and others on this last back on September 11th, this year @
http://marc.info/?l=linux-netdev&m=125263488409128

> Can you pick a reserved range or something?

Using different OUI for the VF device wouldn't help either I think, since the #VF becomes fairly big even on a modest side cluster with
(say) a VM consuming VF per 1-2 cores.

Or.
--
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
Roland Dreier - Nov. 5, 2009, 3:58 p.m.
> This approach seems to be common practice now (e.g., drivers/net/igb/igb_main.c:1332).
 > In any case, the user can change the randomized mac.

igb uses the full output of random_ether_addr().  I'd be fine with
that.  However setting the OUI means you only get 24 bits of randomness
which makes a collision a lot more likely.

 - R.
--
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
Simon Horman - Nov. 6, 2009, 4:06 a.m.
On Thu, Nov 05, 2009 at 07:58:12AM -0800, Roland Dreier wrote:
> 
>  > This approach seems to be common practice now (e.g., drivers/net/igb/igb_main.c:1332).
>  > In any case, the user can change the randomized mac.
> 
> igb uses the full output of random_ether_addr().  I'd be fine with
> that.  However setting the OUI means you only get 24 bits of randomness
> which makes a collision a lot more likely.

IIRC that was precisely why the OUI isn't used for the igb driver.

Perhaps some infrastructure (by which I mean a random_mac() function)
is warranted so at least this discussion can be concentrated around that
rather than repeating it for each driver that needs random mac addresses.


--
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
Roland Dreier - Nov. 6, 2009, 6:08 a.m.
> > igb uses the full output of random_ether_addr().  I'd be fine with
 > > that.  However setting the OUI means you only get 24 bits of randomness
 > > which makes a collision a lot more likely.
 > 
 > IIRC that was precisely why the OUI isn't used for the igb driver.
 > 
 > Perhaps some infrastructure (by which I mean a random_mac() function)
 > is warranted so at least this discussion can be concentrated around that
 > rather than repeating it for each driver that needs random mac addresses.

What would be the difference between random_mac() and the existing
random_ether_addr() function?

If one chooses a random address with a given OUI, then with only 24 bits
of randomness, the birthday paradox says it takes only a few thousand
addresses to get a collision (easy to hit given even a modest-sized
virtualization setup).  With the 46 bits that random_ether_addr() gives
it takes millions of addresses to be likely to get a collision, which is
probably comfortable for most ethernets.

So it seems that random_ether_addr() is exactly what we should be using
for VFs -- the only alternative I see is for the manufacturer to
allocate N extra ethernet addresses for a NIC that supports N virtual
functions, and use those assigned addresses.  But if the kernel is
making up ethernet addresses then we better use all the bits we can.

 - R.
--
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
Simon Horman - Nov. 6, 2009, 7:07 a.m.
On Thu, Nov 05, 2009 at 10:08:58PM -0800, Roland Dreier wrote:
> 
>  > > igb uses the full output of random_ether_addr().  I'd be fine with
>  > > that.  However setting the OUI means you only get 24 bits of randomness
>  > > which makes a collision a lot more likely.
>  > 
>  > IIRC that was precisely why the OUI isn't used for the igb driver.
>  > 
>  > Perhaps some infrastructure (by which I mean a random_mac() function)
>  > is warranted so at least this discussion can be concentrated around that
>  > rather than repeating it for each driver that needs random mac addresses.
> 
> What would be the difference between random_mac() and the existing
> random_ether_addr() function?

Sorry, I was mistaken. igb is using random_ether_addr(),
which is what I had in mind.
--
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

Patch

diff --git a/drivers/net/mlx4/fw.c b/drivers/net/mlx4/fw.c
index 60889d3..9028592 100644
--- a/drivers/net/mlx4/fw.c
+++ b/drivers/net/mlx4/fw.c
@@ -32,6 +32,7 @@ 
  * SOFTWARE.
  */
 
+#include <linux/etherdevice.h>
 #include <linux/mlx4/cmd.h>
 #include <linux/cache.h>
 
@@ -148,6 +149,7 @@  int mlx4_QUERY_SLAVE_CAP_wrapper(struct mlx4_dev *dev, int slave, struct mlx4_vh
 						       struct mlx4_cmd_mailbox *outbox)
 {
 	struct mlx4_caps *caps = outbox->buf;
+	u8 rand_mac[6];
 	int i;
 
 	memcpy(caps, &dev->caps, sizeof *caps);
@@ -165,6 +167,10 @@  int mlx4_QUERY_SLAVE_CAP_wrapper(struct mlx4_dev *dev, int slave, struct mlx4_vh
 		for (i = 1; i <= dev->caps.num_ports; ++i) {
 			caps->gid_table_len[i] = 1;
 			caps->pkey_table_len[i] = 1;
+#define MLX4_MAC_HEAD		0x2c9000000ULL
+			random_ether_addr(rand_mac);
+			caps->def_mac[i] = MLX4_MAC_HEAD | rand_mac[0] |
+				((u64)(rand_mac[1]) << 8) | ((u64)(rand_mac[2]) << 16);
 		}
 	} else {
 		caps->sqp_demux = dev->num_slaves;