[PATCH] r8169: Randomise invalid MAC addresses

[PATCH] r8169: Randomise invalid MAC addresses

[Torne (Richard Coles)]

From: "Torne (Richard Coles)" <torne@google.com>

If the default MAC address stored in the card is invalid, replace it
with a random address and complain about it.

Signed-off-by: Torne (Richard Coles) <torne@google.com>
---
 drivers/net/ethernet/realtek/r8169.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 7a0c800..ec5ebbb 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4103,6 +4103,14 @@ rtl8169_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* Get MAC address */
 	for (i = 0; i < ETH_ALEN; i++)
 		dev->dev_addr[i] = RTL_R8(MAC0 + i);
+
+	if (!is_valid_ether_addr(dev->dev_addr)) {
+		/* Report it and use a random ethernet address instead */
+		netdev_err(dev, "Invalid MAC address: %pM\n", dev->dev_addr);
+		random_ether_addr(dev->dev_addr);
+		netdev_info(dev, "Using random MAC address: %pM\n",
+			    dev->dev_addr);
+	}
 	memcpy(dev->perm_addr, dev->dev_addr, dev->addr_len);
 
 	SET_ETHTOOL_OPS(dev, &rtl8169_ethtool_ops);
-- 
1.7.7.3

Re: [PATCH] r8169: Randomise invalid MAC addresses

[Joe Perches]

On Mon, 2012-01-23 at 18:32 +0000, Torne (Richard Coles) wrote:
> From: "Torne (Richard Coles)" <torne@google.com>
> 
> If the default MAC address stored in the card is invalid, replace it
> with a random address and complain about it.
[]
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
[]
> @@ -4103,6 +4103,14 @@ rtl8169_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	/* Get MAC address */
>  	for (i = 0; i < ETH_ALEN; i++)
>  		dev->dev_addr[i] = RTL_R8(MAC0 + i);
> +
> +	if (!is_valid_ether_addr(dev->dev_addr)) {
> +		/* Report it and use a random ethernet address instead */
> +		netdev_err(dev, "Invalid MAC address: %pM\n", dev->dev_addr);
> +		random_ether_addr(dev->dev_addr);
> +		netdev_info(dev, "Using random MAC address: %pM\n",
> +			    dev->dev_addr);

Perhaps this should be on 1 line.
If KERN_level filtering is higher then KERN_INFO,
the new MAC will not be shown.

	if (!is_valid_ether_addr(dev->dev_addr)) {
		u8 mac[ETH_ALEN];
		random_ether_addr(mac);
		netdev_err(dev, "Hardware has invalid mac address %pM, using %pM\n",
			   dev->dev_addr, mac);
		memcpy(dev->dev_addr, mac, ETH_ALEN);
	}

Re: [PATCH] r8169: Randomise invalid MAC addresses

[Paul Gortmaker]

On Mon, Jan 23, 2012 at 1:32 PM, Torne (Richard Coles) <torne@google.com> wrote:
> From: "Torne (Richard Coles)" <torne@google.com>
>
> If the default MAC address stored in the card is invalid, replace it
> with a random address and complain about it.

You might want to have a look at this thread and its outcome.

https://lkml.org/lkml/2012/1/14/75

Paul.

>
> Signed-off-by: Torne (Richard Coles) <torne@google.com>
> ---
>  drivers/net/ethernet/realtek/r8169.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 7a0c800..ec5ebbb 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -4103,6 +4103,14 @@ rtl8169_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>        /* Get MAC address */
>        for (i = 0; i < ETH_ALEN; i++)
>                dev->dev_addr[i] = RTL_R8(MAC0 + i);
> +
> +       if (!is_valid_ether_addr(dev->dev_addr)) {
> +               /* Report it and use a random ethernet address instead */
> +               netdev_err(dev, "Invalid MAC address: %pM\n", dev->dev_addr);
> +               random_ether_addr(dev->dev_addr);
> +               netdev_info(dev, "Using random MAC address: %pM\n",
> +                           dev->dev_addr);
> +       }
>        memcpy(dev->perm_addr, dev->dev_addr, dev->addr_len);
>
>        SET_ETHTOOL_OPS(dev, &rtl8169_ethtool_ops);
> --
> 1.7.7.3
>
> --
> 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

Re: [PATCH] r8169: Randomise invalid MAC addresses

[Alan Cox]

On Mon, 23 Jan 2012 18:32:20 +0000
"Torne (Richard Coles)" <torne@google.com> wrote:

> From: "Torne (Richard Coles)" <torne@google.com>
> 
> If the default MAC address stored in the card is invalid, replace it
> with a random address and complain about it.

See the discussion about the pch ethernet card. Detect the error and warn
about it, block opening it and let the user set it with ifconfig xx hw
aa:bb:cc:dd:ee:ff

Alan

Re: [PATCH] r8169: Randomise invalid MAC addresses

[Torne (Richard Coles)]

On 23 January 2012 20:53, Paul Gortmaker <paul.gortmaker@gmail.com> wrote:
> On Mon, Jan 23, 2012 at 1:32 PM, Torne (Richard Coles) <torne@google.com> wrote:
>> From: "Torne (Richard Coles)" <torne@google.com>
>>
>> If the default MAC address stored in the card is invalid, replace it
>> with a random address and complain about it.
>
> You might want to have a look at this thread and its outcome.
>
> https://lkml.org/lkml/2012/1/14/75

While I can appreciate the argument that people shouldn't build
hardware that relies on this kind of behaviour, I am in the same
situation as Darren there: I have a retail device I have bought that
has a garbage MAC address (an OpenPeak Joggler), and so do all the
others. Requiring that this be worked around in userspace makes it
tricky to just install a standard Linux distribution onto this piece
of hardware that's otherwise pretty much x86-pc-compatible (albeit
with a weird bootloader)

The vendor's distribution clones the USB wifi adapter's MAC onto the
ethernet device in userspace at boot, which is rather unpleasant (it
never uses both interfaces at once, admittedly).

So, is this just not going to be acceptable in any form? What about
refactoring the existing drivers that do this so that this code
doesn't need to be repeated in every driver, if that would help? I'd
really quite like to get standard linux distros to be compatible with
the Joggler, and this is one of the few changes that's actually needed
(one way or another).

-- 
Torne (Richard Coles)
torne@google.com

Re: [PATCH] r8169: Randomise invalid MAC addresses

[Alan Cox]

> So, is this just not going to be acceptable in any form? What about
> refactoring the existing drivers that do this so that this code
> doesn't need to be repeated in every driver, if that would help? I'd
> really quite like to get standard linux distros to be compatible with
> the Joggler, and this is one of the few changes that's actually needed
> (one way or another).

It ought to be about a ten line change in Red Hat/Fedora, you could also
do it generically for most distributions as a udev rule.

Alan

Re: [PATCH] r8169: Randomise invalid MAC addresses

[Torne (Richard Coles)]

On 23 January 2012 21:43, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> So, is this just not going to be acceptable in any form? What about
>> refactoring the existing drivers that do this so that this code
>> doesn't need to be repeated in every driver, if that would help? I'd
>> really quite like to get standard linux distros to be compatible with
>> the Joggler, and this is one of the few changes that's actually needed
>> (one way or another).
>
> It ought to be about a ten line change in Red Hat/Fedora, you could also
> do it generically for most distributions as a udev rule.

OK; I'll work on that instead. Thanks for the feedback all.

-- 
Torne (Richard Coles)
torne@google.com

Re: [PATCH] r8169: Randomise invalid MAC addresses

[Pavel Machek]

Hi!

> > If the default MAC address stored in the card is invalid, replace it
> > with a random address and complain about it.
> 
> See the discussion about the pch ethernet card. Detect the error and warn
> about it, block opening it and let the user set it with ifconfig xx hw
> aa:bb:cc:dd:ee:ff

Kernel should provide hw abstraction, and that should mean doing
something about commonly-bad ethernet cards.

Userspace does not quite cut it, think about nfsroot for example.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] r8169: Randomise invalid MAC addresses

[Francois Romieu]

Pavel Machek <pavel@ucw.cz> :
[...]
> Kernel should provide hw abstraction, and that should mean doing
> something about commonly-bad ethernet cards.

Ask for a refund ?

> Userspace does not quite cut it, think about nfsroot for example.

nfsroot without pxe or such ? A bit contrieved imho.

The eeprom may be writable though. Or not :o/

-- 
Ueimor

Re: [PATCH] r8169: Randomise invalid MAC addresses

[Ben Hutchings]

On Tue, 2012-01-24 at 19:28 +0100, Francois Romieu wrote:
> Pavel Machek <pavel@ucw.cz> :
> [...]
> > Kernel should provide hw abstraction, and that should mean doing
> > something about commonly-bad ethernet cards.
> 
> Ask for a refund ?
[...]

This is mostly being done in embedded systems where the system
developers control both the kernel and all of userspace and can easily
bodge things in the userland init process.

Then some adventurous users want to run custom kernels on those systems
and have a reasonable way to deal with that.  The original system worked
and they cannot replace just the network interface, so it is no good
asking for a refund.

None of the push-back from netdev is going to have any effect on the
embedded system developers who are failing to program MAC addresses
properly; it's only going to hurt those adventurous users.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
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] r8169: Randomise invalid MAC addresses

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Tue, 24 Jan 2012 18:47:33 +0000

> None of the push-back from netdev is going to have any effect on the
> embedded system developers who are failing to program MAC addresses
> properly; it's only going to hurt those adventurous users.

The implementation of denying interface bringup until a valid MAC is
configured is actually helping these adventurous users if the embedded
guys (which in the Intel situation, was the case) put the hack into
userspace to set the MAC address before bringing the interface up.

And this is really the only solution I think is warranted.