[PATCH 1/8] net: validate MAC address directly in dev_set_mac_address()

[PATCH 1/8] net: validate MAC address directly in dev_set_mac_address()

[Danny Kukawka]

Validate the given MAC address directly in dev_set_mac_address()
if a .ndo_validate_addr function is available before calling
the .ndo_set_mac_address function.

Changed .ndo_validate_addr to take a second parameter containing
a sockaddr struct to be checked instead of the net_device dev_addr.
The behaviour of .ndo_validate_addr is now: if the second parameter
is NULL the net_device->dev_addr gets validate, if != NULL
the given parameter/sockaddr gets validated instead.

Removed is_valid_ether_addr() check from eth_mac_addr() since
this is now done in dev_set_mac_address(). Adapted eth_validate_addr()
to the changes.

Signed-off-by: Danny Kukawka <danny.kukawka@bisect.de>
---
 include/linux/etherdevice.h |    2 +-
 include/linux/netdevice.h   |    7 +++++--
 net/core/dev.c              |    7 ++++++-
 net/ethernet/eth.c          |   12 +++++++++---
 4 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index 8a18358..ee3091f 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -44,7 +44,7 @@ extern void eth_header_cache_update(struct hh_cache *hh,
 				    const unsigned char *haddr);
 extern int eth_mac_addr(struct net_device *dev, void *p);
 extern int eth_change_mtu(struct net_device *dev, int new_mtu);
-extern int eth_validate_addr(struct net_device *dev);
+extern int eth_validate_addr(struct net_device *dev, void *addr);
 
 
 
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f1b7d03..08186e3 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -772,8 +772,10 @@ struct netdev_fcoe_hbainfo {
  *	needs to be changed. If this interface is not defined, the
  *	mac address can not be changed.
  *
- * int (*ndo_validate_addr)(struct net_device *dev);
+ * int (*ndo_validate_addr)(struct net_device *dev, void *addr);
  *	Test if Media Access Control address is valid for the device.
+ *      If addr is NULL the Media Access Control address of the device
+ *      get validated, otherwise the MAC of the net_device.
  *
  * int (*ndo_do_ioctl)(struct net_device *dev, struct ifreq *ifr, int cmd);
  *	Called when a user request an ioctl which can't be handled by
@@ -919,7 +921,8 @@ struct net_device_ops {
 	void			(*ndo_set_rx_mode)(struct net_device *dev);
 	int			(*ndo_set_mac_address)(struct net_device *dev,
 						       void *addr);
-	int			(*ndo_validate_addr)(struct net_device *dev);
+	int			(*ndo_validate_addr)(struct net_device *dev,
+						     void *addr);
 	int			(*ndo_do_ioctl)(struct net_device *dev,
 					        struct ifreq *ifr, int cmd);
 	int			(*ndo_set_config)(struct net_device *dev,
diff --git a/net/core/dev.c b/net/core/dev.c
index 763a0ed..b26a287 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1162,7 +1162,7 @@ static int __dev_open(struct net_device *dev)
 	set_bit(__LINK_STATE_START, &dev->state);
 
 	if (ops->ndo_validate_addr)
-		ret = ops->ndo_validate_addr(dev);
+		ret = ops->ndo_validate_addr(dev, NULL);
 
 	if (!ret && ops->ndo_open)
 		ret = ops->ndo_open(dev);
@@ -4820,6 +4820,11 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa)
 		return -EINVAL;
 	if (!netif_device_present(dev))
 		return -ENODEV;
+	if (ops->ndo_validate_addr) {
+                err = ops->ndo_validate_addr(dev, sa);
+		if (err)
+			return err;
+	}
 	err = ops->ndo_set_mac_address(dev, sa);
 	if (!err)
 		call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index a93af86..d25cc7a 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -285,8 +285,6 @@ int eth_mac_addr(struct net_device *dev, void *p)
 
 	if (netif_running(dev))
 		return -EBUSY;
-	if (!is_valid_ether_addr(addr->sa_data))
-		return -EADDRNOTAVAIL;
 	memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
 	/* if device marked as NET_ADDR_RANDOM, reset it */
 	dev->addr_assign_type &= ~NET_ADDR_RANDOM;
@@ -311,8 +309,16 @@ int eth_change_mtu(struct net_device *dev, int new_mtu)
 }
 EXPORT_SYMBOL(eth_change_mtu);
 
-int eth_validate_addr(struct net_device *dev)
+int eth_validate_addr(struct net_device *dev, void *addr)
 {
+	struct sockaddr *saddr;
+
+	if (addr) {
+		saddr = addr;
+		if (!is_valid_ether_addr(saddr->sa_data))
+			return -EADDRNOTAVAIL;
+	}
+
 	if (!is_valid_ether_addr(dev->dev_addr))
 		return -EADDRNOTAVAIL;
 
-- 
1.7.8.3

[PATCH 2/8] bnx2x: adopt bnx2x_validate_addr() to .ndo_validate_addr changes

[Danny Kukawka]

Adopted bnx2x_validate_addr() to changes in .ndo_validate_addr,
handle second parameter to be validated.

Signed-off-by: Danny Kukawka <danny.kukawka@bisect.de>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index b4afef6..d25ef1e 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -10532,12 +10532,19 @@ static void poll_bnx2x(struct net_device *dev)
 }
 #endif
 
-static int bnx2x_validate_addr(struct net_device *dev)
+static int bnx2x_validate_addr(struct net_device *dev, void *addr)
 {
 	struct bnx2x *bp = netdev_priv(dev);
+	struct sockaddr *saddr; 
 
-	if (!bnx2x_is_valid_ether_addr(bp, dev->dev_addr))
-		return -EADDRNOTAVAIL;
+	if (addr) {
+		saddr = addr;
+		if (!bnx2x_is_valid_ether_addr(bp, saddr->sa_data))	
+			return -EADDRNOTAVAIL;
+	} else {
+		if (!bnx2x_is_valid_ether_addr(bp, dev->dev_addr))
+			return -EADDRNOTAVAIL;
+	}
 	return 0;
 }
 
-- 
1.7.8.3

[PATCH 3/8] cris/eth_v10: use dev_set_mac_address() instead of e100_set_mac_address()

[Danny Kukawka]

Use dev_set_mac_address() instead of e100_set_mac_address() directly
to get validation checks for free.

Signed-off-by: Danny Kukawka <danny.kukawka@bisect.de>
---
 drivers/net/cris/eth_v10.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/net/cris/eth_v10.c b/drivers/net/cris/eth_v10.c
index 7cb2785..ce6f3e6 100644
--- a/drivers/net/cris/eth_v10.c
+++ b/drivers/net/cris/eth_v10.c
@@ -377,8 +377,7 @@ etrax_ethernet_init(void)
 	}
 
 	/* set the default MAC address */
-
-	e100_set_mac_address(dev, &default_mac);
+	dev_set_mac_address(dev, &default_mac);
 
 	/* Initialize speed indicator stuff. */
 
-- 
1.7.8.3

[PATCH 4/8] bcm63xx_enet: use dev_set_mac_address() instead of bcm_enet_set_mac_address()

[Danny Kukawka]

Use dev_set_mac_address() instead of bcm_enet_set_mac_address() directly
to get validation checks for free.

Signed-off-by: Danny Kukawka <danny.kukawka@bisect.de>
---
 drivers/net/ethernet/broadcom/bcm63xx_enet.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcm63xx_enet.c b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
index c7ca7ec..28184a8 100644
--- a/drivers/net/ethernet/broadcom/bcm63xx_enet.c
+++ b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
@@ -858,7 +858,7 @@ static int bcm_enet_open(struct net_device *dev)
 
 	/* write device mac address */
 	memcpy(addr.sa_data, dev->dev_addr, ETH_ALEN);
-	bcm_enet_set_mac_address(dev, &addr);
+	dev_set_mac_address(dev, &addr);
 
 	/* allocate rx dma ring */
 	size = priv->rx_ring_size * sizeof(struct bcm_enet_desc);
-- 
1.7.8.3

[PATCH 5/8] ethoc: add .ndo_validate_addr to net_device_ops

[Danny Kukawka]

Add eth_validate_addr() to .ndo_validate_addr to get validation
checks in dev_set_mac_address() working. Remove
is_valid_ether_addr() from ethoc_set_mac_address() to prevent
double check.

Signed-off-by: Danny Kukawka <danny.kukawka@bisect.de>
---
 drivers/net/ethernet/ethoc.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/ethoc.c b/drivers/net/ethernet/ethoc.c
index a381678..ca80f19 100644
--- a/drivers/net/ethernet/ethoc.c
+++ b/drivers/net/ethernet/ethoc.c
@@ -776,9 +776,6 @@ static int ethoc_set_mac_address(struct net_device *dev, void *addr)
 	struct ethoc *priv = netdev_priv(dev);
 	u8 *mac = (u8 *)addr;
 
-	if (!is_valid_ether_addr(mac))
-		return -EADDRNOTAVAIL;
-
 	ethoc_write(priv, MAC_ADDR0, (mac[2] << 24) | (mac[3] << 16) |
 				     (mac[4] <<  8) | (mac[5] <<  0));
 	ethoc_write(priv, MAC_ADDR1, (mac[0] <<  8) | (mac[1] <<  0));
@@ -899,6 +896,7 @@ static const struct net_device_ops ethoc_netdev_ops = {
 	.ndo_change_mtu = ethoc_change_mtu,
 	.ndo_tx_timeout = ethoc_tx_timeout,
 	.ndo_start_xmit = ethoc_start_xmit,
+	.ndo_validate_addr      = eth_validate_addr,
 };
 
 /**
-- 
1.7.8.3

[PATCH 6/8] lantiq_etop: use dev_set_mac_address() instead of ltq_etop_set_mac_address()

[Danny Kukawka]

Use dev_set_mac_address() instead of ltq_etop_set_mac_address() directly
to get validation checks for free.

Signed-off-by: Danny Kukawka <danny.kukawka@bisect.de>
---
 drivers/net/ethernet/lantiq_etop.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/lantiq_etop.c b/drivers/net/ethernet/lantiq_etop.c
index 5dc9cbd..2419f51 100644
--- a/drivers/net/ethernet/lantiq_etop.c
+++ b/drivers/net/ethernet/lantiq_etop.c
@@ -650,11 +650,11 @@ ltq_etop_init(struct net_device *dev)
 		random_mac = true;
 	}
 
-	err = ltq_etop_set_mac_address(dev, &mac);
+	err = dev_set_mac_address(dev, &mac);
 	if (err)
 		goto err_netdev;
 
-	/* Set addr_assign_type here, ltq_etop_set_mac_address would reset it. */
+	/* Set addr_assign_type here, dev_set_mac_address would reset it. */
 	if (random_mac)
 		dev->addr_assign_type |= NET_ADDR_RANDOM;
 
-- 
1.7.8.3

[PATCH 7/8] neterion/s2io: fix s2io_set_mac_addr() to prevent double checks

[Danny Kukawka]

Fix s2io_set_mac_addr() to prevent double validation checks from
dev_set_mac_address().

Don't use s2io_set_mac_addr() in s2io_io_resume() since it makes
no sense to copy netdev->dev_addr to itself. Use do_s2io_prog_unicast()
instead since this is what's needed and checked here.

Signed-off-by: Danny Kukawka <danny.kukawka@bisect.de>
---
 drivers/net/ethernet/neterion/s2io.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/neterion/s2io.c b/drivers/net/ethernet/neterion/s2io.c
index 22a8de0..cf7b3eb 100644
--- a/drivers/net/ethernet/neterion/s2io.c
+++ b/drivers/net/ethernet/neterion/s2io.c
@@ -5247,9 +5247,6 @@ static int s2io_set_mac_addr(struct net_device *dev, void *p)
 {
 	struct sockaddr *addr = p;
 
-	if (!is_valid_ether_addr(addr->sa_data))
-		return -EADDRNOTAVAIL;
-
 	memcpy(dev->dev_addr, addr->sa_data, dev->addr_len);
 
 	/* store the MAC address in CAM */
@@ -8658,7 +8655,7 @@ static void s2io_io_resume(struct pci_dev *pdev)
 			return;
 		}
 
-		if (s2io_set_mac_addr(netdev, netdev->dev_addr) == FAILURE) {
+		if (do_s2io_prog_unicast(netdev, netdev->dev_addr) == FAILURE) {
 			s2io_card_down(sp);
 			pr_err("Can't restore mac addr after reset.\n");
 			return;
-- 
1.7.8.3

[PATCH 8/8] octeon: use dev_set_mac_address() instead of octeon_mgmt_set_mac_address()

[Danny Kukawka]

Use dev_set_mac_address() instead of octeon_mgmt_set_mac_address() directly
to get validation checks for free.

Add .ndo_validate_addr = eth_validate_addr to enable validation.

Signed-off-by: Danny Kukawka <danny.kukawka@bisect.de>
---
 drivers/net/ethernet/octeon/octeon_mgmt.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/octeon/octeon_mgmt.c b/drivers/net/ethernet/octeon/octeon_mgmt.c
index cd827ff..fb6553d 100644
--- a/drivers/net/ethernet/octeon/octeon_mgmt.c
+++ b/drivers/net/ethernet/octeon/octeon_mgmt.c
@@ -551,9 +551,6 @@ static int octeon_mgmt_set_mac_address(struct net_device *netdev, void *addr)
 {
 	struct sockaddr *sa = addr;
 
-	if (!is_valid_ether_addr(sa->sa_data))
-		return -EADDRNOTAVAIL;
-
 	memcpy(netdev->dev_addr, sa->sa_data, ETH_ALEN);
 
 	octeon_mgmt_set_rx_filtering(netdev);
@@ -768,7 +765,7 @@ static int octeon_mgmt_open(struct net_device *netdev)
 	cvmx_write_csr(CVMX_AGL_GMX_PRTX_CFG(port), prtx_cfg.u64);
 
 	memcpy(sa.sa_data, netdev->dev_addr, ETH_ALEN);
-	octeon_mgmt_set_mac_address(netdev, &sa);
+	dev_set_mac_address(netdev, &sa);
 
 	octeon_mgmt_change_mtu(netdev, netdev->mtu);
 
@@ -1064,6 +1061,7 @@ static const struct net_device_ops octeon_mgmt_ops = {
 	.ndo_set_mac_address =		octeon_mgmt_set_mac_address,
 	.ndo_do_ioctl = 		octeon_mgmt_ioctl,
 	.ndo_change_mtu =		octeon_mgmt_change_mtu,
+	.ndo_validate_addr = 		eth_validate_addr,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller =		octeon_mgmt_poll_controller,
 #endif
-- 
1.7.8.3

Re: [PATCH 4/8] bcm63xx_enet: use dev_set_mac_address() instead of bcm_enet_set_mac_address()

[Florian Fainelli]

Le 02/29/12 16:42, Danny Kukawka a écrit :
> Use dev_set_mac_address() instead of bcm_enet_set_mac_address() directly
> to get validation checks for free.
>
> Signed-off-by: Danny Kukawka<danny.kukawka@bisect.de>

Acked-by: Florian Fainelli <ffainelli@freebox.fr>

> ---
>   drivers/net/ethernet/broadcom/bcm63xx_enet.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bcm63xx_enet.c b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
> index c7ca7ec..28184a8 100644
> --- a/drivers/net/ethernet/broadcom/bcm63xx_enet.c
> +++ b/drivers/net/ethernet/broadcom/bcm63xx_enet.c
> @@ -858,7 +858,7 @@ static int bcm_enet_open(struct net_device *dev)
>
>   	/* write device mac address */
>   	memcpy(addr.sa_data, dev->dev_addr, ETH_ALEN);
> -	bcm_enet_set_mac_address(dev,&addr);
> +	dev_set_mac_address(dev,&addr);
>
>   	/* allocate rx dma ring */
>   	size = priv->rx_ring_size * sizeof(struct bcm_enet_desc);

Re: [PATCH 0/8] validate MAC address before call .ndo_set_mac_address

[Ben Hutchings]

On Wed, 2012-02-29 at 16:42 +0100, Danny Kukawka wrote:
> Validate the given MAC address directly in dev_set_mac_address()
> if a .ndo_validate_addr function is available before calling
> the .ndo_set_mac_address function.
> 
> Changed .ndo_validate_addr to take a second parameter containing
> a sockaddr struct to be checked instead of the net_device dev_addr.
> The behaviour of .ndo_validate_addr is now: if the second parameter
> is NULL the net_device->dev_addr gets validate, if != NULL
> the given parameter/sockaddr gets validated instead.
> 
> This patch series include adaptations for some drivers which
> use .ndo_set_mac_address functions directly - to prevent double
> checks and to enable validations via .ndo_validate_addr.
[...]

You have to do this as a single patch.  The kernel and drivers should
still build at each stage.

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.

[PATCH 0/8] validate MAC address before call .ndo_set_mac_address

[Danny Kukawka]

Validate the given MAC address directly in dev_set_mac_address()
if a .ndo_validate_addr function is available before calling
the .ndo_set_mac_address function.

Changed .ndo_validate_addr to take a second parameter containing
a sockaddr struct to be checked instead of the net_device dev_addr.
The behaviour of .ndo_validate_addr is now: if the second parameter
is NULL the net_device->dev_addr gets validate, if != NULL
the given parameter/sockaddr gets validated instead.

This patch series include adaptations for some drivers which
use .ndo_set_mac_address functions directly - to prevent double
checks and to enable validations via .ndo_validate_addr.

If these patches get accepted, an other series will follow
to cleanup the validation checks in the .ndo_set_mac_address 
functions.

Note: resend for ML's

Danny Kukawka (8):
  net: validate MAC address directly in dev_set_mac_address()
  bnx2x: adopt bnx2x_validate_addr() to .ndo_validate_addr changes
  cris/eth_v10: use dev_set_mac_address() instead of
    e100_set_mac_address()
  bcm63xx_enet: use dev_set_mac_address() instead of
    bcm_enet_set_mac_address()
  ethoc: add .ndo_validate_addr to net_device_ops
  lantiq_etop: use dev_set_mac_address() instead of
    ltq_etop_set_mac_address()
  neterion/s2io: fix s2io_set_mac_addr() to prevent double checks
  octeon: use dev_set_mac_address() instead of
    octeon_mgmt_set_mac_address()

 drivers/net/cris/eth_v10.c                       |    3 +--
 drivers/net/ethernet/broadcom/bcm63xx_enet.c     |    2 +-
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |   13 ++++++++++---
 drivers/net/ethernet/ethoc.c                     |    4 +---
 drivers/net/ethernet/lantiq_etop.c               |    4 ++--
 drivers/net/ethernet/neterion/s2io.c             |    5 +----
 drivers/net/ethernet/octeon/octeon_mgmt.c        |    6 ++----
 include/linux/etherdevice.h                      |    2 +-
 include/linux/netdevice.h                        |    7 +++++--
 net/core/dev.c                                   |    7 ++++++-
 net/ethernet/eth.c                               |   12 +++++++++---
 11 files changed, 39 insertions(+), 26 deletions(-)

-- 
1.7.8.3

Re: [PATCH 0/8] validate MAC address before call .ndo_set_mac_address

[Danny Kukawka]

On Mittwoch, 29. Februar 2012, you wrote:
> On Wed, 2012-02-29 at 16:42 +0100, Danny Kukawka wrote:
> > Validate the given MAC address directly in dev_set_mac_address()
> > if a .ndo_validate_addr function is available before calling
> > the .ndo_set_mac_address function.
> >
> > Changed .ndo_validate_addr to take a second parameter containing
> > a sockaddr struct to be checked instead of the net_device dev_addr.
> > The behaviour of .ndo_validate_addr is now: if the second parameter
> > is NULL the net_device->dev_addr gets validate, if != NULL
> > the given parameter/sockaddr gets validated instead.
> >
> > This patch series include adaptations for some drivers which
> > use .ndo_set_mac_address functions directly - to prevent double
> > checks and to enable validations via .ndo_validate_addr.
>
> [...]
>
> You have to do this as a single patch.  The kernel and drivers should
> still build at each stage.

Okay. In this case only patch 1 and 2 needs to be merged into one commit/patch 
since the other will even work with out patch one or two. I will send a new 
version of the series as soon as I get more comments on the content.

Danny

Re: [PATCH 2/8] bnx2x: adopt bnx2x_validate_addr() to .ndo_validate_addr changes

[Dmitry Kravkov]

On Wed, 2012-02-29 at 16:42 +0100, Danny Kukawka wrote:
> Adopted bnx2x_validate_addr() to changes in .ndo_validate_addr,
> handle second parameter to be validated.
> 
> Signed-off-by: Danny Kukawka <danny.kukawka@bisect.de>
> ---
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |   13 ++++++++++---
>  1 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> index b4afef6..d25ef1e 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -10532,12 +10532,19 @@ static void poll_bnx2x(struct net_device *dev)
>  }
>  #endif
>  
> -static int bnx2x_validate_addr(struct net_device *dev)
> +static int bnx2x_validate_addr(struct net_device *dev, void *addr)
>  {
>  	struct bnx2x *bp = netdev_priv(dev);
> +	struct sockaddr *saddr; 
>  
> -	if (!bnx2x_is_valid_ether_addr(bp, dev->dev_addr))
> -		return -EADDRNOTAVAIL;
> +	if (addr) {
> +		saddr = addr;
> +		if (!bnx2x_is_valid_ether_addr(bp, saddr->sa_data))	
> +			return -EADDRNOTAVAIL;
> +	} else {
> +		if (!bnx2x_is_valid_ether_addr(bp, dev->dev_addr))
> +			return -EADDRNOTAVAIL;
> +	}
>  	return 0;
>  }
>  

Isn't it preferred to calculate the correct address for test and then
call bnx2x_is_valid_ether_addr() at the end?

Re: [PATCH 0/8] validate MAC address before call .ndo_set_mac_address

[Danny Kukawka]

On Mittwoch, 29. Februar 2012, Danny Kukawka wrote:
> On Mittwoch, 29. Februar 2012, you wrote:
> > On Wed, 2012-02-29 at 16:42 +0100, Danny Kukawka wrote:
> > > Validate the given MAC address directly in dev_set_mac_address()
> > > if a .ndo_validate_addr function is available before calling
> > > the .ndo_set_mac_address function.
> > >
> > > Changed .ndo_validate_addr to take a second parameter containing
> > > a sockaddr struct to be checked instead of the net_device dev_addr.
> > > The behaviour of .ndo_validate_addr is now: if the second parameter
> > > is NULL the net_device->dev_addr gets validate, if != NULL
> > > the given parameter/sockaddr gets validated instead.
> > >
> > > This patch series include adaptations for some drivers which
> > > use .ndo_set_mac_address functions directly - to prevent double
> > > checks and to enable validations via .ndo_validate_addr.
> >
> > [...]
> >
> > You have to do this as a single patch.  The kernel and drivers should
> > still build at each stage.
>
> Okay. In this case only patch 1 and 2 needs to be merged into one
> commit/patch since the other will even work with out patch one or two. I
> will send a new version of the series as soon as I get more comments on the
> content.

I have to send a new version anyway since I've accidently forgot to run 
checkpatch.pl. Sorry.

Danny

Re: [PATCH 2/8] bnx2x: adopt bnx2x_validate_addr() to .ndo_validate_addr changes

[Danny Kukawka]

On Mittwoch, 29. Februar 2012, Dmitry Kravkov wrote:
> On Wed, 2012-02-29 at 16:42 +0100, Danny Kukawka wrote:
> > Adopted bnx2x_validate_addr() to changes in .ndo_validate_addr,
> > handle second parameter to be validated.
> >
> > Signed-off-by: Danny Kukawka <danny.kukawka@bisect.de>
> > ---
> >  drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |   13 ++++++++++---
> >  1 files changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> > b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c index b4afef6..d25ef1e
> > 100644
> > --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> > +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> > @@ -10532,12 +10532,19 @@ static void poll_bnx2x(struct net_device *dev)
> >  }
> >  #endif
> >
> > -static int bnx2x_validate_addr(struct net_device *dev)
> > +static int bnx2x_validate_addr(struct net_device *dev, void *addr)
> >  {
> >  	struct bnx2x *bp = netdev_priv(dev);
> > +	struct sockaddr *saddr;
> >
> > -	if (!bnx2x_is_valid_ether_addr(bp, dev->dev_addr))
> > -		return -EADDRNOTAVAIL;
> > +	if (addr) {
> > +		saddr = addr;
> > +		if (!bnx2x_is_valid_ether_addr(bp, saddr->sa_data))
> > +			return -EADDRNOTAVAIL;
> > +	} else {
> > +		if (!bnx2x_is_valid_ether_addr(bp, dev->dev_addr))
> > +			return -EADDRNOTAVAIL;
> > +	}
> >  	return 0;
> >  }
>
> Isn't it preferred to calculate the correct address for test and then
> call bnx2x_is_valid_ether_addr() at the end?

Do you mean something like this: 

int eth_validate_addr(struct net_device *dev, void *addr)
{
 u8 *vaddr;

 if (addr)
    vaddr = ((struct sockaddr *) addr)->sa_data;
 else
    vaddr = dev->dev_addr;

 if (!is_valid_ether_addr(vaddr))
    return -EADDRNOTAVAIL;

 return 0;
}

Re: [PATCH 2/8] bnx2x: adopt bnx2x_validate_addr() to .ndo_validate_addr changes

[Dmitry Kravkov]

On Wed, 2012-02-29 at 18:08 +0100, Danny Kukawka wrote:

> Do you mean something like this: 
> 
> int eth_validate_addr(struct net_device *dev, void *addr)
> {
>  u8 *vaddr;
> 
>  if (addr)
>     vaddr = ((struct sockaddr *) addr)->sa_data;
>  else
>     vaddr = dev->dev_addr;
> 
>  if (!is_valid_ether_addr(vaddr))
>     return -EADDRNOTAVAIL;
> 
>  return 0;
> }
> 
Exactly :)

Re: [PATCH 2/8] bnx2x: adopt bnx2x_validate_addr() to .ndo_validate_addr changes

[Dmitry Kravkov]

On Wed, 2012-02-29 at 18:08 +0100, Danny Kukawka wrote:
> On Mittwoch, 29. Februar 2012, Dmitry Kravkov wrote:
> > On Wed, 2012-02-29 at 16:42 +0100, Danny Kukawka wrote:
> > > Adopted bnx2x_validate_addr() to changes in .ndo_validate_addr,
> > > handle second parameter to be validated.
> > >
> > > Signed-off-by: Danny Kukawka <danny.kukawka@bisect.de>
> > > ---
> > >  drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |   13 ++++++++++---
> > >  1 files changed, 10 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> > > b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c index b4afef6..d25ef1e
> > > 100644
> > > --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> > > +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> > > @@ -10532,12 +10532,19 @@ static void poll_bnx2x(struct net_device *dev)
> > >  }
> > >  #endif
> > >
> > > -static int bnx2x_validate_addr(struct net_device *dev)
> > > +static int bnx2x_validate_addr(struct net_device *dev, void *addr)
> > >  {
> > >  	struct bnx2x *bp = netdev_priv(dev);
> > > +	struct sockaddr *saddr;
> > >
> > > -	if (!bnx2x_is_valid_ether_addr(bp, dev->dev_addr))
> > > -		return -EADDRNOTAVAIL;
> > > +	if (addr) {
> > > +		saddr = addr;
> > > +		if (!bnx2x_is_valid_ether_addr(bp, saddr->sa_data))
> > > +			return -EADDRNOTAVAIL;
> > > +	} else {
> > > +		if (!bnx2x_is_valid_ether_addr(bp, dev->dev_addr))
> > > +			return -EADDRNOTAVAIL;
> > > +	}
> > >  	return 0;
> > >  }
> >
> > Isn't it preferred to calculate the correct address for test and then
> > call bnx2x_is_valid_ether_addr() at the end?
> 
> Do you mean something like this: 
BUT:

> int eth_validate_addr(struct net_device *dev, void *addr)
> {
You still need to extract bp here:
	struct bnx2x *bp = netdev_priv(dev);
>  u8 *vaddr;
> 
>  if (addr)
>     vaddr = ((struct sockaddr *) addr)->sa_data;
>  else
>     vaddr = dev->dev_addr;
> 
>  if (!is_valid_ether_addr(vaddr))
and call internal function here:
	if (!bnx2x_is_valid_ether_addr(bp, vaddr))
>     return -EADDRNOTAVAIL;
> 
>  return 0;
> }
> 

Re: [PATCH 3/8] cris/eth_v10: use dev_set_mac_address() instead of e100_set_mac_address()

[Jesper Nilsson]

On Wed, Feb 29, 2012 at 04:42:51PM +0100, Danny Kukawka wrote:
> Use dev_set_mac_address() instead of e100_set_mac_address() directly
> to get validation checks for free.
> 
> Signed-off-by: Danny Kukawka <danny.kukawka@bisect.de>
Acked-by: Jesper Nilsson <jesper.nilsson@axis.com>

> ---
>  drivers/net/cris/eth_v10.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)


/^JN - Jesper Nilsson