[PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Stefan Assmann]

From: Stefan Assmann <sassmann@redhat.com>

Reserve a bit in struct net_device to indicate whether an interface
generates its MAC address randomly, and expose the information via
sysfs.
May look like this:
/sys/devices/pci0000:00/0000:00:01.0/0000:01:00.0/net/eth0/ifrndmac

By default the value of ifrndmac is 0. Any driver that generates the MAC
address randomly should return a value to 1.

This simplifies the handling of network devices with random MAC addresses
as user-space may just query sysfs to find out if the MAC is real or fake.
Udev may check sysfs for devices that generate their MAC address
randomly and create persistent net rules by using the unique device path
if the value returned is 1.

Also introducing a helper function to assist driver adoption.

Signed-off-by: Stefan Assmann <sassmann@redhat.com>
---
 include/linux/etherdevice.h |   14 ++++++++++++++
 include/linux/netdevice.h   |    1 +
 net/core/net-sysfs.c        |   12 ++++++++++++
 3 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index 3d7a668..ebb34ac 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -127,6 +127,20 @@ static inline void random_ether_addr(u8 *addr)
 }

 /**
+ * dev_hw_addr_random - Create random MAC and set device flag
+ * @dev: pointer to net_device structure
+ * @addr: Pointer to a six-byte array containing the Ethernet address
+ *
+ * Generate random MAC to be used by a device and set NETIF_F_RNDMAC flag
+ * so the state can be read by sysfs and be used by udev.
+ */
+static inline void dev_hw_addr_random(struct net_device *dev, u8 *hwaddr)
+{
+	dev->features |= NETIF_F_RNDMAC;
+	random_ether_addr(hwaddr);
+}
+
+/**
  * compare_ether_addr - Compare two Ethernet addresses
  * @addr1: Pointer to a six-byte array containing the Ethernet address
  * @addr2: Pointer other six-byte array containing the Ethernet address
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index b626289..2ea0298 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -845,6 +845,7 @@ struct net_device {
 #define NETIF_F_FCOE_MTU	(1 << 26) /* Supports max FCoE MTU, 2158 bytes*/
 #define NETIF_F_NTUPLE		(1 << 27) /* N-tuple filters supported */
 #define NETIF_F_RXHASH		(1 << 28) /* Receive hashing offload */
+#define NETIF_F_RNDMAC		(1 << 29) /* Interface with random MAC address */

 	/* Segmentation offload features */
 #define NETIF_F_GSO_SHIFT	16
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index d2b5965..91a9808 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -188,6 +188,17 @@ static ssize_t show_dormant(struct device *dev,
 	return -EINVAL;
 }

+static ssize_t show_ifrndmac(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct net_device *net = to_net_dev(dev);
+
+	if (net->features & NETIF_F_RNDMAC)
+		return sprintf(buf, fmt_dec, 1);
+	else
+		return sprintf(buf, fmt_dec, 0);
+}
+
 static const char *const operstates[] = {
 	"unknown",
 	"notpresent", /* currently unused */
@@ -300,6 +311,7 @@ static struct device_attribute net_class_attributes[] = {
 	__ATTR(ifalias, S_IRUGO | S_IWUSR, show_ifalias, store_ifalias),
 	__ATTR(iflink, S_IRUGO, show_iflink, NULL),
 	__ATTR(ifindex, S_IRUGO, show_ifindex, NULL),
+	__ATTR(ifrndmac, S_IRUGO, show_ifrndmac, NULL),
 	__ATTR(features, S_IRUGO, show_features, NULL),
 	__ATTR(type, S_IRUGO, show_type, NULL),
 	__ATTR(link_mode, S_IRUGO, show_link_mode, NULL),
-- 
1.6.5.2

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Ben Hutchings]

On Tue, 2010-07-20 at 12:50 +0200, Stefan Assmann wrote:
> From: Stefan Assmann <sassmann@redhat.com>
> 
> Reserve a bit in struct net_device to indicate whether an interface
> generates its MAC address randomly, and expose the information via
> sysfs.
> May look like this:
> /sys/devices/pci0000:00/0000:00:01.0/0000:01:00.0/net/eth0/ifrndmac
> 
> By default the value of ifrndmac is 0. Any driver that generates the MAC
> address randomly should return a value to 1.

The name should incorporate 'address', not 'mac', for consistency with
the generic 'address' attribute.

What about devices that 'steal' MAC addresses from slave devices?
Currently I believe udev has special cases for them but ideally these
drivers would indicate explicitly that their addresses are not stable
identifiers (even though they aren't random).

[...]
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index b626289..2ea0298 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -845,6 +845,7 @@ struct net_device {
>  #define NETIF_F_FCOE_MTU	(1 << 26) /* Supports max FCoE MTU, 2158 bytes*/
>  #define NETIF_F_NTUPLE		(1 << 27) /* N-tuple filters supported */
>  #define NETIF_F_RXHASH		(1 << 28) /* Receive hashing offload */
> +#define NETIF_F_RNDMAC		(1 << 29) /* Interface with random MAC address */
[...]

This is not really a feature, and we are running out of real feature
bits.  Can you find somewhere else to put this flag?

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Stefan Assmann]

On 20.07.2010 13:20, Ben Hutchings wrote:
> On Tue, 2010-07-20 at 12:50 +0200, Stefan Assmann wrote:
>> From: Stefan Assmann <sassmann@redhat.com>
>>
>> Reserve a bit in struct net_device to indicate whether an interface
>> generates its MAC address randomly, and expose the information via
>> sysfs.
>> May look like this:
>> /sys/devices/pci0000:00/0000:00:01.0/0000:01:00.0/net/eth0/ifrndmac
>>
>> By default the value of ifrndmac is 0. Any driver that generates the MAC
>> address randomly should return a value to 1.
> 
> The name should incorporate 'address', not 'mac', for consistency with
> the generic 'address' attribute.

We can call it "ifrndhwaddr" if that's more consistent.

> 
> What about devices that 'steal' MAC addresses from slave devices?
> Currently I believe udev has special cases for them but ideally these
> drivers would indicate explicitly that their addresses are not stable
> identifiers (even though they aren't random).

It's really up to the driver to decide whether it makes sense to set the
flag or not. The question is what should udev do with these MAC address
stealing devices apart from ignoring them? Sorry I have no higher
insight into it.
This flag has the purpose to allow udev to explicitly handle devices
that generate their MAC address randomly and generate a persistent
rule based on the device path instead of the MAC address.
I'm open for suggestions but I'm not sure we can handle both cases with
a single flag.

JFYI this is an alternative approach to changing the kernel name of VFs
to vfeth. The advantage of this way should be that we don't break any
user-space applications that rely on network interfaces being names
"ethX". Actually this goes in the direction of "fixing udev" which was
what you asked for in your comment on my first patch concering vfeth. :)

> 
> [...]
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index b626289..2ea0298 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -845,6 +845,7 @@ struct net_device {
>>  #define NETIF_F_FCOE_MTU	(1 << 26) /* Supports max FCoE MTU, 2158 bytes*/
>>  #define NETIF_F_NTUPLE		(1 << 27) /* N-tuple filters supported */
>>  #define NETIF_F_RXHASH		(1 << 28) /* Receive hashing offload */
>> +#define NETIF_F_RNDMAC		(1 << 29) /* Interface with random MAC address */
> [...]
> 
> This is not really a feature, and we are running out of real feature
> bits.  Can you find somewhere else to put this flag?

Actually Dave Miller suggested to put it there. What other place is
there to put it?

  Stefan
--
Stefan Assmann         | Red Hat GmbH
Software Engineer      | Otto-Hahn-Strasse 20, 85609 Dornach
                       | HR: Amtsgericht Muenchen HRB 153243
                       | GF: Brendan Lane, Charlie Peters,
sassmann at redhat.com |     Michael Cunningham, Charles Cachera

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Alex Badea]

Hi,

On 07/20/2010 02:47 PM, Stefan Assmann wrote:
>> What about devices that 'steal' MAC addresses from slave devices?

Might I suggest an attribute such as "address_type", which could report
"permanent", "random", "stolen", or something else in the future?

My two cents,
Alex

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Stefan Assmann]

On 20.07.2010 13:58, Alex Badea wrote:
> Hi,
> 
> On 07/20/2010 02:47 PM, Stefan Assmann wrote:
>>> What about devices that 'steal' MAC addresses from slave devices?
> 
> Might I suggest an attribute such as "address_type", which could report
> "permanent", "random", "stolen", or something else in the future?

In case there's demand for such a multi-purpose attribute I see no
reason to object. More thoughts on this?

Thanks for your suggestion Alex.

  Stefan
--
Stefan Assmann         | Red Hat GmbH
Software Engineer      | Otto-Hahn-Strasse 20, 85609 Dornach
                       | HR: Amtsgericht Muenchen HRB 153243
                       | GF: Brendan Lane, Charlie Peters,
sassmann at redhat.com |     Michael Cunningham, Charles Cachera

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[David Miller]

From: Stefan Assmann <sassmann@redhat.com>
Date: Tue, 20 Jul 2010 14:17:30 +0200

> On 20.07.2010 13:58, Alex Badea wrote:
>> Hi,
>> 
>> On 07/20/2010 02:47 PM, Stefan Assmann wrote:
>>>> What about devices that 'steal' MAC addresses from slave devices?
>> 
>> Might I suggest an attribute such as "address_type", which could report
>> "permanent", "random", "stolen", or something else in the future?
> 
> In case there's demand for such a multi-purpose attribute I see no
> reason to object. More thoughts on this?

I think this is a great idea.

Now it makes sense to use a new 'u8' in struct netdevice or similar to
store this, since we'll have more than a boolean here.

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Stefan Assmann]

On 20.07.2010 22:18, David Miller wrote:
> From: Stefan Assmann <sassmann@redhat.com>
> Date: Tue, 20 Jul 2010 14:17:30 +0200
> 
>> On 20.07.2010 13:58, Alex Badea wrote:
>>> Hi,
>>>
>>> On 07/20/2010 02:47 PM, Stefan Assmann wrote:
>>>>> What about devices that 'steal' MAC addresses from slave devices?
>>>
>>> Might I suggest an attribute such as "address_type", which could report
>>> "permanent", "random", "stolen", or something else in the future?
>>
>> In case there's demand for such a multi-purpose attribute I see no
>> reason to object. More thoughts on this?
> 
> I think this is a great idea.
> 
> Now it makes sense to use a new 'u8' in struct netdevice or similar to
> store this, since we'll have more than a boolean here.
> 

I put Alex' idea into code for further discussion, keeping the names
mentioned here until we agree on the scope of this attribute. When we
have settled I'll post a patch with proper patch description.

  Stefan
---
 include/linux/etherdevice.h |   14 ++++++++++++++
 include/linux/netdevice.h   |    6 ++++++
 net/core/net-sysfs.c        |    2 ++
 3 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index 3d7a668..f15cac8 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -127,6 +127,20 @@ static inline void random_ether_addr(u8 *addr)
 }

 /**
+ * dev_hw_addr_random - Create random MAC and set device flag
+ * @dev: pointer to net_device structure
+ * @addr: Pointer to a six-byte array containing the Ethernet address
+ *
+ * Generate random MAC to be used by a device and set addr_type
+ * so the state can be read by sysfs and be used by udev.
+ */
+static inline void dev_hw_addr_random(struct net_device *dev, u8 *hwaddr)
+{
+	dev->addr_type |= NET_ADDR_RANDOM;
+	random_ether_addr(hwaddr);
+}
+
+/**
  * compare_ether_addr - Compare two Ethernet addresses
  * @addr1: Pointer to a six-byte array containing the Ethernet address
  * @addr2: Pointer other six-byte array containing the Ethernet address
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index b626289..2718895 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -66,6 +66,11 @@ struct wireless_dev;
 #define HAVE_FREE_NETDEV		/* free_netdev() */
 #define HAVE_NETDEV_PRIV		/* netdev_priv() */

+/* hardware address types */
+#define NET_ADDR_PERM		0	/* address is permanent (default) */
+#define NET_ADDR_RANDOM		1	/* address is generated randomly */
+#define NET_ADDR_STOLEN		2	/* address is stolen from other device */
+
 /* Backlog congestion levels */
 #define NET_RX_SUCCESS		0	/* keep 'em coming, baby */
 #define NET_RX_DROP		1	/* packet dropped */
@@ -920,6 +925,7 @@ struct net_device {
 	/* Interface address info. */
 	unsigned char		perm_addr[MAX_ADDR_LEN]; /* permanent hw address */
 	unsigned char		addr_len;	/* hardware address length	*/
+	unsigned char		addr_type;	/* hardware address type	*/
 	unsigned short          dev_id;		/* for shared network cards */

 	spinlock_t		addr_list_lock;
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index d2b5965..052ab14 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -96,6 +96,7 @@ static ssize_t netdev_store(struct device *dev, struct device_attribute *attr,

 NETDEVICE_SHOW(dev_id, fmt_hex);
 NETDEVICE_SHOW(addr_len, fmt_dec);
+NETDEVICE_SHOW(addr_type, fmt_dec);
 NETDEVICE_SHOW(iflink, fmt_dec);
 NETDEVICE_SHOW(ifindex, fmt_dec);
 NETDEVICE_SHOW(features, fmt_long_hex);
@@ -296,6 +297,7 @@ static ssize_t show_ifalias(struct device *dev,

 static struct device_attribute net_class_attributes[] = {
 	__ATTR(addr_len, S_IRUGO, show_addr_len, NULL),
+	__ATTR(addr_type, S_IRUGO, show_addr_type, NULL),
 	__ATTR(dev_id, S_IRUGO, show_dev_id, NULL),
 	__ATTR(ifalias, S_IRUGO | S_IWUSR, show_ifalias, store_ifalias),
 	__ATTR(iflink, S_IRUGO, show_iflink, NULL),
-- 
1.7.1

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Ben Hutchings]

On Wed, 2010-07-21 at 10:10 +0200, Stefan Assmann wrote:
> On 20.07.2010 22:18, David Miller wrote:
> > From: Stefan Assmann <sassmann@redhat.com>
> > Date: Tue, 20 Jul 2010 14:17:30 +0200
> > 
> >> On 20.07.2010 13:58, Alex Badea wrote:
> >>> Hi,
> >>>
> >>> On 07/20/2010 02:47 PM, Stefan Assmann wrote:
> >>>>> What about devices that 'steal' MAC addresses from slave devices?
> >>>
> >>> Might I suggest an attribute such as "address_type", which could report
> >>> "permanent", "random", "stolen", or something else in the future?
> >>
> >> In case there's demand for such a multi-purpose attribute I see no
> >> reason to object. More thoughts on this?
> > 
> > I think this is a great idea.
> > 
> > Now it makes sense to use a new 'u8' in struct netdevice or similar to
> > store this, since we'll have more than a boolean here.
> > 
> 
> I put Alex' idea into code for further discussion, keeping the names
> mentioned here until we agree on the scope of this attribute. When we
> have settled I'll post a patch with proper patch description.
[...]

Just a little nitpick: I think it would be clearer to use a more
specific term like 'address source' or 'address assignment type' rather
than 'address type'.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH net-next] sysfs: add attribute to indicate hw address assignment type

[Stefan Assmann]

On 21.07.2010 15:54, Ben Hutchings wrote:
> On Wed, 2010-07-21 at 10:10 +0200, Stefan Assmann wrote:
>> I put Alex' idea into code for further discussion, keeping the names
>> mentioned here until we agree on the scope of this attribute. When we
>> have settled I'll post a patch with proper patch description.
> [...]
> 
> Just a little nitpick: I think it would be clearer to use a more
> specific term like 'address source' or 'address assignment type' rather
> than 'address type'.

Here's a proposal for the final patch.

  Stefan

From: Stefan Assmann <sassmann@redhat.com>

Add addr_assign_type to struct net_device and expose it via sysfs.
This new attribute has the purpose of giving user-space the ability to
distinguish between different assignment types of MAC addresses.

For example user-space can treat NICs with randomly generated MAC
addresses differently than NICs that have permanent (locally assigned)
MAC addresses.
For the former udev could write a persistent net rule by matching the
device path instead of the MAC address.
There's also the case of devices that 'steal' MAC addresses from slave
devices. In which it is also be beneficial for user-space to be aware
of the fact.

This patch also introduces a helper function to assist adoption of
drivers that generate MAC addresses randomly.

Signed-off-by: Stefan Assmann <sassmann@redhat.com>
---
 include/linux/etherdevice.h |   14 ++++++++++++++
 include/linux/netdevice.h   |    6 ++++++
 net/core/net-sysfs.c        |    2 ++
 3 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index 3d7a668..848480b 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -127,6 +127,20 @@ static inline void random_ether_addr(u8 *addr)
 }

 /**
+ * dev_hw_addr_random - Create random MAC and set device flag
+ * @dev: pointer to net_device structure
+ * @addr: Pointer to a six-byte array containing the Ethernet address
+ *
+ * Generate random MAC to be used by a device and set addr_assign_type
+ * so the state can be read by sysfs and be used by udev.
+ */
+static inline void dev_hw_addr_random(struct net_device *dev, u8 *hwaddr)
+{
+	dev->addr_assign_type |= NET_ADDR_RANDOM;
+	random_ether_addr(hwaddr);
+}
+
+/**
  * compare_ether_addr - Compare two Ethernet addresses
  * @addr1: Pointer to a six-byte array containing the Ethernet address
  * @addr2: Pointer other six-byte array containing the Ethernet address
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index b626289..1bca617 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -66,6 +66,11 @@ struct wireless_dev;
 #define HAVE_FREE_NETDEV		/* free_netdev() */
 #define HAVE_NETDEV_PRIV		/* netdev_priv() */

+/* hardware address assignment types */
+#define NET_ADDR_PERM		0	/* address is permanent (default) */
+#define NET_ADDR_RANDOM		1	/* address is generated randomly */
+#define NET_ADDR_STOLEN		2	/* address is stolen from other device */
+
 /* Backlog congestion levels */
 #define NET_RX_SUCCESS		0	/* keep 'em coming, baby */
 #define NET_RX_DROP		1	/* packet dropped */
@@ -919,6 +924,7 @@ struct net_device {

 	/* Interface address info. */
 	unsigned char		perm_addr[MAX_ADDR_LEN]; /* permanent hw address */
+	unsigned char		addr_assign_type; /* hw address assignment type */
 	unsigned char		addr_len;	/* hardware address length	*/
 	unsigned short          dev_id;		/* for shared network cards */

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index d2b5965..af4dfba 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -95,6 +95,7 @@ static ssize_t netdev_store(struct device *dev, struct device_attribute *attr,
 }

 NETDEVICE_SHOW(dev_id, fmt_hex);
+NETDEVICE_SHOW(addr_assign_type, fmt_dec);
 NETDEVICE_SHOW(addr_len, fmt_dec);
 NETDEVICE_SHOW(iflink, fmt_dec);
 NETDEVICE_SHOW(ifindex, fmt_dec);
@@ -295,6 +296,7 @@ static ssize_t show_ifalias(struct device *dev,
 }

 static struct device_attribute net_class_attributes[] = {
+	__ATTR(addr_assign_type, S_IRUGO, show_addr_assign_type, NULL),
 	__ATTR(addr_len, S_IRUGO, show_addr_len, NULL),
 	__ATTR(dev_id, S_IRUGO, show_dev_id, NULL),
 	__ATTR(ifalias, S_IRUGO | S_IWUSR, show_ifalias, store_ifalias),
-- 
1.6.5.2

Re: [PATCH net-next] sysfs: add attribute to indicate hw address assignment type

[Ben Hutchings]

On Thu, 2010-07-22 at 14:50 +0200, Stefan Assmann wrote:
> On 21.07.2010 15:54, Ben Hutchings wrote:
> > On Wed, 2010-07-21 at 10:10 +0200, Stefan Assmann wrote:
> >> I put Alex' idea into code for further discussion, keeping the names
> >> mentioned here until we agree on the scope of this attribute. When we
> >> have settled I'll post a patch with proper patch description.
> > [...]
> > 
> > Just a little nitpick: I think it would be clearer to use a more
> > specific term like 'address source' or 'address assignment type' rather
> > than 'address type'.
> 
> Here's a proposal for the final patch.

Looks good, but...

[...]
>  /**
> + * dev_hw_addr_random - Create random MAC and set device flag
> + * @dev: pointer to net_device structure
> + * @addr: Pointer to a six-byte array containing the Ethernet address
> + *
> + * Generate random MAC to be used by a device and set addr_assign_type
> + * so the state can be read by sysfs and be used by udev.
> + */
> +static inline void dev_hw_addr_random(struct net_device *dev, u8 *hwaddr)
> +{
> +	dev->addr_assign_type |= NET_ADDR_RANDOM;
> +	random_ether_addr(hwaddr);
> +}
[...]

...why '|=' and not '='?

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next] sysfs: add attribute to indicate hw address assignment type

[Stefan Assmann]

On 22.07.2010 16:07, Ben Hutchings wrote:
> On Thu, 2010-07-22 at 14:50 +0200, Stefan Assmann wrote:
>> On 21.07.2010 15:54, Ben Hutchings wrote:
>>> On Wed, 2010-07-21 at 10:10 +0200, Stefan Assmann wrote:
>>>> I put Alex' idea into code for further discussion, keeping the names
>>>> mentioned here until we agree on the scope of this attribute. When we
>>>> have settled I'll post a patch with proper patch description.
>>> [...]
>>>
>>> Just a little nitpick: I think it would be clearer to use a more
>>> specific term like 'address source' or 'address assignment type' rather
>>> than 'address type'.
>>
>> Here's a proposal for the final patch.
> 
> Looks good, but...
> 
> [...]
>>  /**
>> + * dev_hw_addr_random - Create random MAC and set device flag
>> + * @dev: pointer to net_device structure
>> + * @addr: Pointer to a six-byte array containing the Ethernet address
>> + *
>> + * Generate random MAC to be used by a device and set addr_assign_type
>> + * so the state can be read by sysfs and be used by udev.
>> + */
>> +static inline void dev_hw_addr_random(struct net_device *dev, u8 *hwaddr)
>> +{
>> +	dev->addr_assign_type |= NET_ADDR_RANDOM;
>> +	random_ether_addr(hwaddr);
>> +}
> [...]
> 
> ...why '|=' and not '='?

The intention is to use addr_assign_type as a bit field.

Okay it it might not make too much sense to 'steal' a random MAC
address but in case we add more types later it might get useful.

  Stefan
--
Stefan Assmann         | Red Hat GmbH
Software Engineer      | Otto-Hahn-Strasse 20, 85609 Dornach
                       | HR: Amtsgericht Muenchen HRB 153243
                       | GF: Brendan Lane, Charlie Peters,
sassmann at redhat.com |     Michael Cunningham, Charles Cachera

Re: [PATCH net-next] sysfs: add attribute to indicate hw address assignment type

[David Miller]

From: Stefan Assmann <sassmann@redhat.com>
Date: Thu, 22 Jul 2010 16:47:40 +0200

> On 22.07.2010 16:07, Ben Hutchings wrote:
>> On Thu, 2010-07-22 at 14:50 +0200, Stefan Assmann wrote:
>>> On 21.07.2010 15:54, Ben Hutchings wrote:
>>>> On Wed, 2010-07-21 at 10:10 +0200, Stefan Assmann wrote:
>>>>> I put Alex' idea into code for further discussion, keeping the names
>>>>> mentioned here until we agree on the scope of this attribute. When we
>>>>> have settled I'll post a patch with proper patch description.
>>>> [...]
>>>>
>>>> Just a little nitpick: I think it would be clearer to use a more
>>>> specific term like 'address source' or 'address assignment type' rather
>>>> than 'address type'.
>>>
>>> Here's a proposal for the final patch.
>> 
>> Looks good, but...
 ...
>> ...why '|=' and not '='?
> 
> The intention is to use addr_assign_type as a bit field.
> 
> Okay it it might not make too much sense to 'steal' a random MAC
> address but in case we add more types later it might get useful.

I think the patch is good enough to go into net-next-2.6 as-is, anything
remaining is a refinement or one sort or another.

So applied to net-next-2.6, thanks.

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Ben Hutchings]

On Tue, 2010-07-20 at 13:47 +0200, Stefan Assmann wrote:
> On 20.07.2010 13:20, Ben Hutchings wrote:
> > On Tue, 2010-07-20 at 12:50 +0200, Stefan Assmann wrote:
> >> From: Stefan Assmann <sassmann@redhat.com>
> >>
> >> Reserve a bit in struct net_device to indicate whether an interface
> >> generates its MAC address randomly, and expose the information via
> >> sysfs.
> >> May look like this:
> >> /sys/devices/pci0000:00/0000:00:01.0/0000:01:00.0/net/eth0/ifrndmac
> >>
> >> By default the value of ifrndmac is 0. Any driver that generates the MAC
> >> address randomly should return a value to 1.
> > 
> > The name should incorporate 'address', not 'mac', for consistency with
> > the generic 'address' attribute.
> 
> We can call it "ifrndhwaddr" if that's more consistent.
> 
> > 
> > What about devices that 'steal' MAC addresses from slave devices?
> > Currently I believe udev has special cases for them but ideally these
> > drivers would indicate explicitly that their addresses are not stable
> > identifiers (even though they aren't random).
> 
> It's really up to the driver to decide whether it makes sense to set the
> flag or not. The question is what should udev do with these MAC address
> stealing devices apart from ignoring them? Sorry I have no higher
> insight into it.
> This flag has the purpose to allow udev to explicitly handle devices
> that generate their MAC address randomly and generate a persistent
> rule based on the device path instead of the MAC address.
> I'm open for suggestions but I'm not sure we can handle both cases with
> a single flag.

OK, then call it something like 'address_temporary'.

> JFYI this is an alternative approach to changing the kernel name of VFs
> to vfeth. The advantage of this way should be that we don't break any
> user-space applications that rely on network interfaces being names
> "ethX". Actually this goes in the direction of "fixing udev" which was
> what you asked for in your comment on my first patch concering vfeth. :)
> 
> > 
> > [...]
> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> >> index b626289..2ea0298 100644
> >> --- a/include/linux/netdevice.h
> >> +++ b/include/linux/netdevice.h
> >> @@ -845,6 +845,7 @@ struct net_device {
> >>  #define NETIF_F_FCOE_MTU	(1 << 26) /* Supports max FCoE MTU, 2158 bytes*/
> >>  #define NETIF_F_NTUPLE		(1 << 27) /* N-tuple filters supported */
> >>  #define NETIF_F_RXHASH		(1 << 28) /* Receive hashing offload */
> >> +#define NETIF_F_RNDMAC		(1 << 29) /* Interface with random MAC address */
> > [...]
> > 
> > This is not really a feature, and we are running out of real feature
> > bits.  Can you find somewhere else to put this flag?
> 
> Actually Dave Miller suggested to put it there. What other place is
> there to put it?

If Dave said that then I'm sure it's OK.

However, if you define this as an interface flag (net_device::flags;
<linux/if.h>) and add it to the set of changeable flags in
__dev_change_flags(), user-space will be able to clear the flag if it
later sets a stable address.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Stefan Assmann]

On 20.07.2010 14:07, Ben Hutchings wrote:
> On Tue, 2010-07-20 at 13:47 +0200, Stefan Assmann wrote:
>> On 20.07.2010 13:20, Ben Hutchings wrote:
>>> On Tue, 2010-07-20 at 12:50 +0200, Stefan Assmann wrote:
>>>> From: Stefan Assmann <sassmann@redhat.com>

[snip]

>> Actually Dave Miller suggested to put it there. What other place is
>> there to put it?
> 
> If Dave said that then I'm sure it's OK.
> 
> However, if you define this as an interface flag (net_device::flags;
> <linux/if.h>) and add it to the set of changeable flags in
> __dev_change_flags(), user-space will be able to clear the flag if it
> later sets a stable address.

As I said I'm not that knowledgeable about this MAC address stealing
thing and I'm assuming that's what you're aiming at. Would you really
want/need it to be user-space writable? Currently all I can think of is
the scenario where you set a "stable" address that outlasts a reboot so
udev might be able to assign it a permanent name after it gets stable.

So it might make sense to have it writable, but I'd like to avoid to add
unnecessary complexity that may cause errors if it's not necessary.
Read-only is simple, just read the flag and deal with it.

Btw, the driver itself could also alter the flag. Then we'd have a well
defined way of setting a stable address.

  Stefan
--
Stefan Assmann         | Red Hat GmbH
Software Engineer      | Otto-Hahn-Strasse 20, 85609 Dornach
                       | HR: Amtsgericht Muenchen HRB 153243
                       | GF: Brendan Lane, Charlie Peters,
sassmann at redhat.com |     Michael Cunningham, Charles Cachera

> 
> Ben.
> 

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Ben Hutchings]

On Tue, 2010-07-20 at 14:41 +0200, Stefan Assmann wrote:
> On 20.07.2010 14:07, Ben Hutchings wrote:
> > On Tue, 2010-07-20 at 13:47 +0200, Stefan Assmann wrote:
> >> On 20.07.2010 13:20, Ben Hutchings wrote:
> >>> On Tue, 2010-07-20 at 12:50 +0200, Stefan Assmann wrote:
> >>>> From: Stefan Assmann <sassmann@redhat.com>
> 
> [snip]
> 
> >> Actually Dave Miller suggested to put it there. What other place is
> >> there to put it?
> > 
> > If Dave said that then I'm sure it's OK.
> > 
> > However, if you define this as an interface flag (net_device::flags;
> > <linux/if.h>) and add it to the set of changeable flags in
> > __dev_change_flags(), user-space will be able to clear the flag if it
> > later sets a stable address.
> 
> As I said I'm not that knowledgeable about this MAC address stealing
> thing and I'm assuming that's what you're aiming at. Would you really
> want/need it to be user-space writable? Currently all I can think of is
> the scenario where you set a "stable" address that outlasts a reboot so
> udev might be able to assign it a permanent name after it gets stable.
>
> So it might make sense to have it writable, but I'd like to avoid to add
> unnecessary complexity that may cause errors if it's not necessary.
> Read-only is simple, just read the flag and deal with it.

Once this flag has been added, it may be used by any tool and not just
udev, so it ought to indicate the status of the currently assigned
address.  That requires that it be writable.

> Btw, the driver itself could also alter the flag. Then we'd have a well
> defined way of setting a stable address.

The driver can't know whether an address assigned by the user is stable.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Tue, 20 Jul 2010 15:29:54 +0100

> On Tue, 2010-07-20 at 14:41 +0200, Stefan Assmann wrote:
>> Btw, the driver itself could also alter the flag. Then we'd have a well
>> defined way of setting a stable address.
> 
> The driver can't know whether an address assigned by the user is stable.

If userspace can somehow obtain a persistent address, it can kick
udev too.

I really don't see any real value provided by letting userspace mess
with this.  Because the permanence communicated in this value is from
the perspective of the kernel driver, it's really therefore about the
thing that's in ->perm_addr[] not what happens to be in ->addr[] right
now.

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Stephen Hemminger]

On Tue, 20 Jul 2010 13:17:48 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Tue, 20 Jul 2010 15:29:54 +0100
> 
> > On Tue, 2010-07-20 at 14:41 +0200, Stefan Assmann wrote:
> >> Btw, the driver itself could also alter the flag. Then we'd have a well
> >> defined way of setting a stable address.
> > 
> > The driver can't know whether an address assigned by the user is stable.
> 
> If userspace can somehow obtain a persistent address, it can kick
> udev too.
> 
> I really don't see any real value provided by letting userspace mess
> with this.  Because the permanence communicated in this value is from
> the perspective of the kernel driver, it's really therefore about the
> thing that's in ->perm_addr[] not what happens to be in ->addr[] right
> now.

No one mentioned that the first octet of an Ethernet address already
indicates "software generated" Ethernet address. Per the standard,
if bit 1 is set it means address is locally assigned.

static inline bool is_locally_assigned_ether(const u8 *addr)
{
	return (addr[0] & 0x2) != 0;
}

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Tue, 20 Jul 2010 14:18:16 -0700

> No one mentioned that the first octet of an Ethernet address already
> indicates "software generated" Ethernet address. Per the standard,
> if bit 1 is set it means address is locally assigned.
> 
> static inline bool is_locally_assigned_ether(const u8 *addr)
> {
> 	return (addr[0] & 0x2) != 0;
> }

W00t!

Indeed, can udev just use that?  :-)

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Harald Hoyer]

On 07/20/2010 11:20 PM, David Miller wrote:
> From: Stephen Hemminger<shemminger@vyatta.com>
> Date: Tue, 20 Jul 2010 14:18:16 -0700
>
>> No one mentioned that the first octet of an Ethernet address already
>> indicates "software generated" Ethernet address. Per the standard,
>> if bit 1 is set it means address is locally assigned.
>>
>> static inline bool is_locally_assigned_ether(const u8 *addr)
>> {
>> 	return (addr[0]&  0x2) != 0;
>> }
>
> W00t!
>
> Indeed, can udev just use that?  :-)

It already does:
see /lib/udev/rules.d/75-persistent-net-generator.rules

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[David Miller]

From: Harald Hoyer <harald@redhat.com>
Date: Wed, 21 Jul 2010 08:26:27 +0200

> On 07/20/2010 11:20 PM, David Miller wrote:
>> From: Stephen Hemminger<shemminger@vyatta.com>
>> Date: Tue, 20 Jul 2010 14:18:16 -0700
>>
>>> No one mentioned that the first octet of an Ethernet address already
>>> indicates "software generated" Ethernet address. Per the standard,
>>> if bit 1 is set it means address is locally assigned.
>>>
>>> static inline bool is_locally_assigned_ether(const u8 *addr)
>>> {
>>> 	return (addr[0]&  0x2) != 0;
>>> }
>>
>> W00t!
>>
>> Indeed, can udev just use that?  :-)
> 
> It already does:
> see /lib/udev/rules.d/75-persistent-net-generator.rules

So... why doesn't this work?

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Harald Hoyer]

On 07/21/2010 08:34 AM, David Miller wrote:
> From: Harald Hoyer<harald@redhat.com>
> Date: Wed, 21 Jul 2010 08:26:27 +0200
>
>> On 07/20/2010 11:20 PM, David Miller wrote:
>>> From: Stephen Hemminger<shemminger@vyatta.com>
>>> Date: Tue, 20 Jul 2010 14:18:16 -0700
>>>
>>>> No one mentioned that the first octet of an Ethernet address already
>>>> indicates "software generated" Ethernet address. Per the standard,
>>>> if bit 1 is set it means address is locally assigned.
>>>>
>>>> static inline bool is_locally_assigned_ether(const u8 *addr)
>>>> {
>>>> 	return (addr[0]&   0x2) != 0;
>>>> }
>>>
>>> W00t!
>>>
>>> Indeed, can udev just use that?  :-)
>>
>> It already does:
>> see /lib/udev/rules.d/75-persistent-net-generator.rules
>
> So... why doesn't this work?

It works.. but the information, that the MAC is randomly generated would be 
valuable. So, for the non-random locally assigned MAC (with bit 1), we could 
easily make persistent rules based on the MAC, instead of completely ignoring 
them, like we do currently.

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Andy Gospodarek]

On Wed, Jul 21, 2010 at 08:47:36AM +0200, Harald Hoyer wrote:
> On 07/21/2010 08:34 AM, David Miller wrote:
>> From: Harald Hoyer<harald@redhat.com>
>> Date: Wed, 21 Jul 2010 08:26:27 +0200
>>
>>> On 07/20/2010 11:20 PM, David Miller wrote:
>>>> From: Stephen Hemminger<shemminger@vyatta.com>
>>>> Date: Tue, 20 Jul 2010 14:18:16 -0700
>>>>
>>>>> No one mentioned that the first octet of an Ethernet address already
>>>>> indicates "software generated" Ethernet address. Per the standard,
>>>>> if bit 1 is set it means address is locally assigned.
>>>>>
>>>>> static inline bool is_locally_assigned_ether(const u8 *addr)
>>>>> {
>>>>> 	return (addr[0]&   0x2) != 0;
>>>>> }
>>>>
>>>> W00t!
>>>>
>>>> Indeed, can udev just use that?  :-)
>>>
>>> It already does:
>>> see /lib/udev/rules.d/75-persistent-net-generator.rules
>>
>> So... why doesn't this work?
>
> It works.. but the information, that the MAC is randomly generated would 
> be valuable. So, for the non-random locally assigned MAC (with bit 1), we 
> could easily make persistent rules based on the MAC, instead of 
> completely ignoring them, like we do currently.

Agreed.  The subtle difference between a locally assigned address that
is persistent and one that is random would be helpful.

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Casey Leedom]

| From: Andy Gospodarek <andy@greyhouse.net>
| Date: Wednesday, July 21, 2010 08:07 am
| 
| Agreed.  The subtle difference between a locally assigned address that
| is persistent and one that is random would be helpful.

  And just to point out that this case _does_ exist: the igb/igbvf drivers use 
random_ether_addr() to generate a random, locally assigned MAC address for the 
PCI-E SR-IOV Virtual Function MAC Addresses while the cxgb4/cxgb4vf drivers use 
a persistent, non-random locally assigned MAC Addresses.

  Note that I am neither arguing for nor against the proposal.  I'm just 
pointing out an existence case for the distinction.  And yes, bit 1 being set in 
the first octet of a MAC address for locally assigned MAC Addresses is part of 
the IEEE 802 specification just as bit 0 being set in the same octet indicates 
that it's a multi-station address.

Casey

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Stephen Hemminger]

On Wed, 21 Jul 2010 09:34:27 -0700
Casey Leedom <leedom@chelsio.com> wrote:

> | From: Andy Gospodarek <andy@greyhouse.net>
> | Date: Wednesday, July 21, 2010 08:07 am
> | 
> | Agreed.  The subtle difference between a locally assigned address that
> | is persistent and one that is random would be helpful.
> 
>   And just to point out that this case _does_ exist: the igb/igbvf drivers use 
> random_ether_addr() to generate a random, locally assigned MAC address for the 
> PCI-E SR-IOV Virtual Function MAC Addresses while the cxgb4/cxgb4vf drivers use 
> a persistent, non-random locally assigned MAC Addresses.
> 
>   Note that I am neither arguing for nor against the proposal.  I'm just 
> pointing out an existence case for the distinction.  And yes, bit 1 being set in 
> the first octet of a MAC address for locally assigned MAC Addresses is part of 
> the IEEE 802 specification just as bit 0 being set in the same octet indicates 
> that it's a multi-station address.

IMHO no local assigned address should be used by udev. The cxgb4 driver
should be using random value.

Does anyone have an example of locally assigned address that has persistence
so that udev could use it.

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 21 Jul 2010 10:28:16 -0700

> IMHO no local assigned address should be used by udev. The cxgb4 driver
> should be using random value.
> 
> Does anyone have an example of locally assigned address that has persistence
> so that udev could use it.

The cxgb4 vf addresses are not random because they are fetched from the
card's NVRAM/EEPROM/firmware/whatever and thus are persistent.

We definitely want udev to use persistent rules for them.

This whole issue only exists because of the Intel VF case, where it
lacks persistent addresses but somehow we want to assign persistent
names to it's VF interfaces.

One idea I've proposed in other discussions about this is that if the
address is not persistent (either via the MAC address bit or the sysfs
value we're thinking of providing here) we use the device's geographic
location ("device path") as the key for udev stuff.

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Casey Leedom]

| From: David Miller <davem@davemloft.net>
| Date: Wednesday, July 21, 2010 10:32 am
| 
| From: Stephen Hemminger <shemminger@vyatta.com>
| Date: Wed, 21 Jul 2010 10:28:16 -0700
| 
| > IMHO no local assigned address should be used by udev. The cxgb4 driver
| > should be using random value.
| > 
| > Does anyone have an example of locally assigned address that has
| > persistence so that udev could use it.
| 
| The cxgb4 vf addresses are not random because they are fetched from the
| card's NVRAM/EEPROM/firmware/whatever and thus are persistent.
| 
| We definitely want udev to use persistent rules for them.
|
| This whole issue only exists because of the Intel VF case, where it
| lacks persistent addresses but somehow we want to assign persistent
| names to it's VF interfaces.

  Yes, we _explicitly_ wanted to have persistent MAC Addresses for our PCI-E SR-
IOV Virtual Functions for a whole raft of reasons.  The two most important were:

 1. Linux' model for persistent device naming today seems to be
    oriented around persistent network device addresses.

 2. Lots of data centers use MAC addresses for things like DHCP/BOOTP,
    security/filtering, etc.

Our design goal was to look as much like a normal Ethernet MAC as possible in 
order to reduce the need for software/behavior changes.

| One idea I've proposed in other discussions about this is that if the
| address is not persistent (either via the MAC address bit or the sysfs
| value we're thinking of providing here) we use the device's geographic
| location ("device path") as the key for udev stuff.

  Another option might be to have a new Net Device Operations call to ask the 
adapter for a Unique Key.  This could be formed for most devices via a tuple of 
the {PCI Vendor ID, PCI Device ID, Adapter Serial Number, Port Number, [and if 
applicable] Adapter Function ID}.  Of course this could be a fairly long string 
... :-)

Casey

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[David Miller]

From: Casey Leedom <leedom@chelsio.com>
Date: Wed, 21 Jul 2010 11:29:47 -0700

>   Another option might be to have a new Net Device Operations call to ask the 
> adapter for a Unique Key.  This could be formed for most devices via a tuple of 
> the {PCI Vendor ID, PCI Device ID, Adapter Serial Number, Port Number, [and if 
> applicable] Adapter Function ID}.  Of course this could be a fairly long string 
> ... :-)

If a unique key were available, it could be used to generate a persistent
MAC address.

And this sort of means that these drivers could use bits of the
device's geographic ID the construct persistent MAC addresses, but
only if done in a MAC namespace that could be guarenteed unique on the
local system.

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Casey Leedom]

| From: David Miller <davem@davemloft.net>
| Date: Wednesday, July 21, 2010 11:39 am
| 
| From: Casey Leedom <leedom@chelsio.com>
| Date: Wed, 21 Jul 2010 11:29:47 -0700
| 
| >   Another option might be to have a new Net Device Operations call to ask
| > the adapter for a Unique Key.  This could be formed for most devices via a
| > tuple of the {PCI Vendor ID, PCI Device ID, Adapter Serial Number, Port
| > Number, [and if applicable] Adapter Function ID}.  Of course this could
| > be a fairly long string ... :-)
| 
| If a unique key were available, it could be used to generate a persistent
| MAC address.

  True but ... the Unique Key name space is probably a lot larger (in bits) than 
the MAC Address name space (~(48-2) bits) ... :-)

| And this sort of means that these drivers could use bits of the
| device's geographic ID the construct persistent MAC addresses, but
| only if done in a MAC namespace that could be guarenteed unique on the
| local system.

  Yep.  That's the problem of trying to construct a Unique Locally Assigned MAC 
Address from a Unique Name in a larger name space.

| From: "Rose, Gregory V" <gregory.v.rose@intel.com>
| Date: Wednesday, July 21, 2010 11:43 am
| 
| I'm curious, what happens when the VM using the VF migrates to a new
| machine and has another VF assigned to with a different MAC address?

  To be honest, I still haven't wrapped my head around how Virtual Machines are 
ever going to be able to migrate when they have arbitrary PCI Devices "assigned" 
(KVM Terminology) to them (AKA "PCI Pass Through").  Allowing VMs to directly 
touch real and arbitrary hardware devices means that some abstraction of "saving 
the device state" and "restoring the device state" can be successfully 
negotiated ... which would be hard even if you quiesce the device and you 
migrate to another Physical Host with identical PCI Hardware which is then 
"assigned" to the migrated VM.  Hard, hard, hard.

  This is why most of the Hypervisor systems have used synthetic Pseudo Devices 
to allow that state of those Virtual Devices to be migrated (including the MAC 
Addresses which we've been talking about).  I actually think that the Microsoft 
HyperV approach of Virtual Ingress Queues may be a better solution.  You still 
need to make the VM-to-Hypervisor transitions but you get to avoid the Free List 
memory copy costs which are actually the dominant cost in the RX path to VMs 
using software vNICs.

  But that's straying far from the topic at hand.  The short answer is pretty 
much what David suggests: the _hardware_ PCI-E SR-IOV Virtual Function provides 
persistent, non-random MAC Addresses for use by the VF Driver -- if it wants to 
use them.  A VF Driver running in a VM is capable of specifying arbitrary MAC 
Addresses for use with the VF and may ignore the hardware MAC Addresses provided 
by the VF.  This is little different from the current situation with software 
vNICs which use manufactured MAC Addresses (which are persistent in all of the 
Hypervisor systems at which I've looked).

| From: David Miller <davem@davemloft.net>
| Date: Wednesday, July 21, 2010 11:48 am
| 
| If the VM itself is the "physical entity" of the system, the logical
| conclusion I come to is that some kind of key should be obtained
| through the VM to uniquely give the device a persistent MAC.

  Which is, as above, what all Hypervisor systems which I've looked at do.

| You could do things like have the PF controller use the root filesystem
| ID label to construct the VF's MAC address, or something like that.

  It's actually stored in the VM's meta-data.  When a VM migrates from one 
Physical Host to another all of the VM's transient and persistent state must be 
available to the new Physical Host.  Xen, for instance, has the concept of a 
Physical Host Pool where all of the Physical Hosts have common access to shared 
resources like Network Attached Storage, LAN/VLANs and the shared VM meta-data.

Casey

RE: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Rose, Gregory V]

>-----Original Message-----
>From: Casey Leedom [mailto:leedom@chelsio.com]
>Sent: Wednesday, July 21, 2010 11:30 AM
>To: David Miller
>Cc: shemminger@vyatta.com; andy@greyhouse.net; harald@redhat.com;
>bhutchings@solarflare.com; sassmann@redhat.com; netdev@vger.kernel.org;
>linux-kernel@vger.kernel.org; gospo@redhat.com; Rose, Gregory V; Duyck,
>Alexander H
>Subject: Re: [PATCH net-next] sysfs: add entry to indicate network
>interfaces with random MAC address
>
>| From: David Miller <davem@davemloft.net>
>| Date: Wednesday, July 21, 2010 10:32 am
>|
>| From: Stephen Hemminger <shemminger@vyatta.com>
>| Date: Wed, 21 Jul 2010 10:28:16 -0700
>|
>| > IMHO no local assigned address should be used by udev. The cxgb4
>driver
>| > should be using random value.
>| >
>| > Does anyone have an example of locally assigned address that has
>| > persistence so that udev could use it.
>|
>| The cxgb4 vf addresses are not random because they are fetched from
>the
>| card's NVRAM/EEPROM/firmware/whatever and thus are persistent.
>|
>| We definitely want udev to use persistent rules for them.
>|
>| This whole issue only exists because of the Intel VF case, where it
>| lacks persistent addresses but somehow we want to assign persistent
>| names to it's VF interfaces.
>
>  Yes, we _explicitly_ wanted to have persistent MAC Addresses for our
>PCI-E SR-
>IOV Virtual Functions for a whole raft of reasons.  The two most
>important were:
>
> 1. Linux' model for persistent device naming today seems to be
>    oriented around persistent network device addresses.
>
> 2. Lots of data centers use MAC addresses for things like DHCP/BOOTP,
>    security/filtering, etc.
>
>Our design goal was to look as much like a normal Ethernet MAC as
>possible in
>order to reduce the need for software/behavior changes.

I'm curious, what happens when the VM using the VF migrates to a new machine and has another VF assigned to with a different MAC address?

Intel's view of things is that we don't use persistent MAC addresses in our VFs because the MAC address belongs to the VM and when it migrates it's going to want to use another VF with the same MAC address.  If they're persistent I'm wondering how that can be done.

This discussion has come about because some folks want to use the VF in the Host VMM.  The original design goal for Intel was that VFs would be assigned to VMs and that VMM vendors would want to assign MAC addresses with their own assigned OUI's.

- Greg

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[David Miller]

From: "Rose, Gregory V" <gregory.v.rose@intel.com>
Date: Wed, 21 Jul 2010 11:43:19 -0700

> Intel's view of things is that we don't use persistent MAC addresses
> in our VFs because the MAC address belongs to the VM and when it
> migrates it's going to want to use another VF with the same MAC
> address.  If they're persistent I'm wondering how that can be done.
> 
> This discussion has come about because some folks want to use the VF
> in the Host VMM.  The original design goal for Intel was that VFs
> would be assigned to VMs and that VMM vendors would want to assign
> MAC addresses with their own assigned OUI's.

If the VM itself is the "physical entity" of the system, the logical
conclusion I come to is that some kind of key should be obtained
through the VM to uniquely give the device a persistent MAC.

You could do things like have the PF controller use the root filesystem
ID label to construct the VF's MAC address, or something like that.

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 21 Jul 2010 11:48:51 -0700 (PDT)

> You could do things like have the PF controller use the root filesystem
> ID label to construct the VF's MAC address, or something like that.

And here I of course mean the root filesystem of the guest the VF will
be given to.

RE: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Rose, Gregory V]

>-----Original Message-----
>From: David Miller [mailto:davem@davemloft.net]
>Sent: Wednesday, July 21, 2010 11:50 AM
>To: Rose, Gregory V
>Cc: leedom@chelsio.com; shemminger@vyatta.com; andy@greyhouse.net;
>harald@redhat.com; bhutchings@solarflare.com; sassmann@redhat.com;
>netdev@vger.kernel.org; linux-kernel@vger.kernel.org; gospo@redhat.com;
>Duyck, Alexander H
>Subject: Re: [PATCH net-next] sysfs: add entry to indicate network
>interfaces with random MAC address
>
>From: David Miller <davem@davemloft.net>
>Date: Wed, 21 Jul 2010 11:48:51 -0700 (PDT)
>
>> You could do things like have the PF controller use the root
>filesystem
>> ID label to construct the VF's MAC address, or something like that.
>
>And here I of course mean the root filesystem of the guest the VF will
>be given to.

I suppose you could do that but then the VM is going to have to be allowed to set its own MAC address.  There is a lot of opposition and concern about allowing VMs to set their own MAC address.

Then there's also support in the kernel now for assigning VF MAC and VLAN via the iproute2 package "ip link" commands.  The administrative SW that controls VMs should be assigning MAC addresses to VFs as needed.  When the VF is de-assigned from the guest because the VM is migrating then the administrative SW can assign another VF to the VM at the migration target machine and assign the same MAC address then.  This way it is all done administratively from the (supposedly) secure host VMM domain and we're not allowing VMs to set their own MAC address.

Ultimately I think I agree that some sort of approach such as you and others have been suggesting should be taken.  I'm agnostic about which one because my view of how things should work is a bit different.  But if the community thinks that finding some way to set a persistent MAC address for a VF is useful then I'd more than happy to make sure our drivers support that also.

As soon as it is decided what that is of course.

;-)

- Greg

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[David Miller]

From: "Rose, Gregory V" <gregory.v.rose@intel.com>
Date: Wed, 21 Jul 2010 12:02:17 -0700

>>From: David Miller <davem@davemloft.net>
>>Date: Wed, 21 Jul 2010 11:48:51 -0700 (PDT)
>>
>>> You could do things like have the PF controller use the root
>>filesystem
>>> ID label to construct the VF's MAC address, or something like that.
>>
>>And here I of course mean the root filesystem of the guest the VF will
>>be given to.
> 
> I suppose you could do that but then the VM is going to have to be
> allowed to set its own MAC address.  There is a lot of opposition
> and concern about allowing VMs to set their own MAC address.

Why would that be necessary?  The host with the PF creating the guest
has access to the "device" and thus the root filesystem of the guest,
and thus could pull in the root filesystem "key" and instantiate the
VF's MAC before booting the guest.

That was the idea, the control node sets up the VF MAC before the guest
boots or can have access to the VF device.

This is completely agnostic of migration or anything like that.  The
procedure for setting the VF MAC is always the same.

RE: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Rose, Gregory V]

>-----Original Message-----
>From: David Miller [mailto:davem@davemloft.net]
>Sent: Wednesday, July 21, 2010 12:33 PM
>To: Rose, Gregory V
>Cc: leedom@chelsio.com; shemminger@vyatta.com; andy@greyhouse.net;
>harald@redhat.com; bhutchings@solarflare.com; sassmann@redhat.com;
>netdev@vger.kernel.org; linux-kernel@vger.kernel.org; gospo@redhat.com;
>Duyck, Alexander H
>Subject: Re: [PATCH net-next] sysfs: add entry to indicate network
>interfaces with random MAC address
>
>From: "Rose, Gregory V" <gregory.v.rose@intel.com>
>Date: Wed, 21 Jul 2010 12:02:17 -0700
>
>>>From: David Miller <davem@davemloft.net>
>>>Date: Wed, 21 Jul 2010 11:48:51 -0700 (PDT)
>>>
>>>> You could do things like have the PF controller use the root
>>>filesystem
>>>> ID label to construct the VF's MAC address, or something like that.
>>>
>>>And here I of course mean the root filesystem of the guest the VF will
>>>be given to.
>>
>> I suppose you could do that but then the VM is going to have to be
>> allowed to set its own MAC address.  There is a lot of opposition
>> and concern about allowing VMs to set their own MAC address.
>
>Why would that be necessary?  The host with the PF creating the guest
>has access to the "device" and thus the root filesystem of the guest,
>and thus could pull in the root filesystem "key" and instantiate the
>VF's MAC before booting the guest.
>
>That was the idea, the control node sets up the VF MAC before the guest
>boots or can have access to the VF device.

I misunderstood you.  My bad.

Thank for the further explanation.

- Greg

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Ian Campbell]

[-- Attachment #1: Type: text/plain, Size: 2163 bytes --]

On Wed, 2010-07-21 at 12:33 -0700, David Miller wrote:
> From: "Rose, Gregory V" <gregory.v.rose@intel.com>
> Date: Wed, 21 Jul 2010 12:02:17 -0700
> 
> >>From: David Miller <davem@davemloft.net>
> >>Date: Wed, 21 Jul 2010 11:48:51 -0700 (PDT)
> >>
> >>> You could do things like have the PF controller use the root
> >>filesystem
> >>> ID label to construct the VF's MAC address, or something like that.
> >>
> >>And here I of course mean the root filesystem of the guest the VF will
> >>be given to.
> > 
> > I suppose you could do that but then the VM is going to have to be
> > allowed to set its own MAC address.  There is a lot of opposition
> > and concern about allowing VMs to set their own MAC address.
> 
> Why would that be necessary?  The host with the PF creating the guest
> has access to the "device" and thus the root filesystem of the guest,
> and thus could pull in the root filesystem "key" and instantiate the
> VF's MAC before booting the guest.

Most VM host toolstacks allow you to store a MAC address for each
virtual NIC in the metadata associated with the VM. This MAC address is
either given by the user when they create the virtual NIC, random with
locally administered bit set or random in the VM vendors OID space. This
ensures the VM configuration remains consistent with time.

Why would they not continue to do the same for SR-IOV passthrough NICs?

As a fallback some toolstacks will generate a random address if the NIC
configuration doesn't specify one but if you want a persistent address
for a guest why would you not just configure it that way? Accessing the
guest root filesystem might be a nicer fallback than random generation
when users haven't explicitly configured a MAC but isn't there a chance
of a VM admin controlling the MAC address by manipulating the root
filesystem? What do you do if there is an address clash in this case,
relabelling the root filesystem is a bit of a faff. Also the root
filesystem could be contained within an LVM volume or encrypted or
whatever.

Ian.
-- 
Ian Campbell

Military intelligence is a contradiction in terms.
		-- Groucho Marx

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Stefan Assmann]

On 21.07.2010 20:43, Rose, Gregory V wrote:
> I'm curious, what happens when the VM using the VF migrates to a new machine and has another VF assigned to with a different MAC address?
> 
> Intel's view of things is that we don't use persistent MAC addresses in our VFs because the MAC address belongs to the VM and when it migrates it's going to want to use another VF with the same MAC address.  If they're persistent I'm wondering how that can be done.
> 
> This discussion has come about because some folks want to use the VF in the Host VMM.  The original design goal for Intel was that VFs would be assigned to VMs and that VMM vendors would want to assign MAC addresses with their own assigned OUI's.

Using the VF in the host is a feature and I'm sure people will think of
ways to make good use of it. However the actual problem we've seen is a
more practical one. So to pass-through a VF to a VM the host has to be
aware that the VF exists. Therefore you usually have to enable the VF in
the host (i.e. specify the max_vfs parameter). The device will be
discovered by the system and because of the random MAC address udev
ignores the new device. With the additional information we provide with
our solution udev will be able to recognize the device by it's "device
path" and handle it properly (until you decide to pass it to a VM or
just be happy with it in the host).

Remember the issue that lead to the proposal of renaming VFs to vfeth?
That's exactly the problem we try to fix. Additional benefit of an
"address assignment type" as Ben likes to call it would be the handling
of MAC address stealing NICs.

  Stefan
--
Stefan Assmann         | Red Hat GmbH
Software Engineer      | Otto-Hahn-Strasse 20, 85609 Dornach
                       | HR: Amtsgericht Muenchen HRB 153243
                       | GF: Brendan Lane, Charlie Peters,
sassmann at redhat.com |     Michael Cunningham, Charles Cachera

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Casey Leedom]

On Jul 21, 2010, at 11:53 PM, Stefan Assmann wrote:

> Using the VF in the host is a feature and I'm sure people will think of
> ways to make good use of it. However the actual problem we've seen is a
> more practical one. So to pass-through a VF to a VM the host has to be
> aware that the VF exists. Therefore you usually have to enable the VF in
> the host (i.e. specify the max_vfs parameter). The device will be
> discovered by the system and because of the random MAC address udev
> ignores the new device. With the additional information we provide with
> our solution udev will be able to recognize the device by it's "device
> path" and handle it properly (until you decide to pass it to a VM or
> just be happy with it in the host).

 Or you simply don't have the VF Driver loaded in the "Domain 0" Control OS.  When we install the cxgb4 PF Driver with "num_vf=..." this enables the PCI-E SR-IOV Capabilities within the various PFs and the corresponding VF PCI Devices are instantiated and discovered by the Domain 0 Linux OS.  But without a cxgb4vf VF Driver loaded, those devices just sit there – available for "Device Assignment" to VMs.

> Remember the issue that lead to the proposal of renaming VFs to vfeth?
> That's exactly the problem we try to fix. Additional benefit of an
> "address assignment type" as Ben likes to call it would be the handling
> of MAC address stealing NICs.

 The above was mostly to cope with some SR-IOV Drivers using random MAC addresses for the VFs.

Casey

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Stefan Assmann]

On 23.07.2010 02:26, Casey Leedom wrote:
> 
>  Or you simply don't have the VF Driver loaded in the "Domain 0" Control OS.  When we install the cxgb4 PF Driver with "num_vf=..." this enables the PCI-E SR-IOV Capabilities within the various PFs and the corresponding VF PCI Devices are instantiated and discovered by the Domain 0 Linux OS.  But without a cxgb4vf VF Driver loaded, those devices just sit there \x13 available for "Device Assignment" to VMs.
> 

Just out of curiosity, how do you prevent the VF driver from getting
loaded in the host? Except from blacklisting it.

  Stefan
--
Stefan Assmann         | Red Hat GmbH
Software Engineer      | Otto-Hahn-Strasse 20, 85609 Dornach
                       | HR: Amtsgericht Muenchen HRB 153243
                       | GF: Brendan Lane, Charlie Peters,
sassmann at redhat.com |     Michael Cunningham, Charles Cachera

Re: [PATCH net-next] sysfs: add entry to indicate network interfaces with random MAC address

[Casey Leedom]

| From: Stefan Assmann <sassmann@redhat.com>
| Date: Friday, July 23, 2010 01:08 am
| 
| On 23.07.2010 02:26, Casey Leedom wrote:
| >  Or you simply don't have the VF Driver loaded in the "Domain 0" Control
| >  OS.  When we install the cxgb4 PF Driver with "num_vf=..." this enables
| >  the PCI-E SR-IOV Capabilities within the various PFs and the
| >  corresponding VF PCI Devices are instantiated and discovered by the
| >  Domain 0 Linux OS.  But without a cxgb4vf VF Driver loaded, those
| >  devices just sit there available for "Device Assignment" to VMs.
| 
| Just out of curiosity, how do you prevent the VF driver from getting
| loaded in the host? Except from blacklisting it.

  I don't install them. :-)

  I'm actually fairly unfamiliar with the details of managing/administering 
Linux systems so I'm guessing that there are much better ways of controlling for 
which devices a Linux system will attempt to load drivers.  For instance, I 
didn't know about the concept of "blacklisting" a driver.

Casey