If driver implements ndo_validate_addr,
core should check the mac address before ndo_set_mac_address.

Signed-off-by: Jijie Shao <shaojijie@huawei.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
ChangeLog:
v2 -> v3:
  - Use ndo_validate_addr() instead of is_valid_ether_addr()
    in dev_set_mac_address(), suggested by Jakub and Andrew.
  v2: https://lore.kernel.org/all/20240820140154.137876-1-shaojijie@huawei.com/
---
 net/core/dev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 22c3f14d9287..00e0f473ed44 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9087,6 +9087,11 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
 		return -EOPNOTSUPP;
 	if (sa->sa_family != dev->type)
 		return -EINVAL;
+	if (ops->ndo_validate_addr) {
+		err = ops->ndo_validate_addr(dev);
+		if (err)
+			return err;
+	}
 	if (!netif_device_present(dev))
 		return -ENODEV;
 	err = dev_pre_changeaddr_notify(dev, sa->sa_data, extack);
-- 
2.33.0

On Tue, Sep 10, 2024 at 1:36 PM Jijie Shao <shaojijie@huawei.com> wrote:
>
> If driver implements ndo_validate_addr,
> core should check the mac address before ndo_set_mac_address.
>
> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
> ChangeLog:
> v2 -> v3:
>   - Use ndo_validate_addr() instead of is_valid_ether_addr()
>     in dev_set_mac_address(), suggested by Jakub and Andrew.
>   v2: https://lore.kernel.org/all/20240820140154.137876-1-shaojijie@huawei.com/
> ---
>  net/core/dev.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 22c3f14d9287..00e0f473ed44 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9087,6 +9087,11 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
>                 return -EOPNOTSUPP;
>         if (sa->sa_family != dev->type)
>                 return -EINVAL;
> +       if (ops->ndo_validate_addr) {
> +               err = ops->ndo_validate_addr(dev);
> +               if (err)
> +                       return err;
> +       }
[Kalesh] It would be better to move this code after
netif_device_present() check. Minor nit and there will not be any
functional impact.
>         if (!netif_device_present(dev))
>                 return -ENODEV;
>         err = dev_pre_changeaddr_notify(dev, sa->sa_data, extack);
> --
> 2.33.0
>


-- 
Regards,
Kalesh A P

on 2024/9/10 16:39, Kalesh Anakkur Purayil wrote:
> On Tue, Sep 10, 2024 at 1:36 PM Jijie Shao <shaojijie@huawei.com> wrote:
>> If driver implements ndo_validate_addr,
>> core should check the mac address before ndo_set_mac_address.
>>
>> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
>> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>> ---
>> ChangeLog:
>> v2 -> v3:
>>    - Use ndo_validate_addr() instead of is_valid_ether_addr()
>>      in dev_set_mac_address(), suggested by Jakub and Andrew.
>>    v2: https://lore.kernel.org/all/20240820140154.137876-1-shaojijie@huawei.com/
>> ---
>>   net/core/dev.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 22c3f14d9287..00e0f473ed44 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -9087,6 +9087,11 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
>>                  return -EOPNOTSUPP;
>>          if (sa->sa_family != dev->type)
>>                  return -EINVAL;
>> +       if (ops->ndo_validate_addr) {
>> +               err = ops->ndo_validate_addr(dev);
>> +               if (err)
>> +                       return err;
>> +       }

This patch may not work as expected.

ndo_validate_addr() has only one parameter.
The sa parameter of the MAC address is not transferred to the function.
So ndo_validate_addr() checks the old MAC address, not the new MAC address.

I haven't found a better way to fix it yet.
This patch may be dropped in v10.

Sorry,
	Jijie Shao

> This patch may not work as expected.
> 
> ndo_validate_addr() has only one parameter.
> The sa parameter of the MAC address is not transferred to the function.
> So ndo_validate_addr() checks the old MAC address, not the new MAC address.

Yes, i agree. The current API does not lend itself to validation
before change.

> I haven't found a better way to fix it yet.

Maybe in dev_set_mac_address(), make a copy of dev->dev_addr. Call
ops->ndo_set_mac_address(). If there is no error call
ndo_validate_addr(). If that returns an error, call
ops->ndo_set_mac_address() again with the old MAC address, and then
return the error from ndo_validate_addr().

It is not great, but it is the best i can think of.

Since this is not as simple i was expecting, feel free to drop it from
your patchset, and submit it as a standalone patch.

	Andrew

on 2024/9/10 16:39, Kalesh Anakkur Purayil wrote:
> On Tue, Sep 10, 2024 at 1:36 PM Jijie Shao <shaojijie@huawei.com> wrote:
>> +++ b/net/core/dev.c
>> @@ -9087,6 +9087,11 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
>>                  return -EOPNOTSUPP;
>>          if (sa->sa_family != dev->type)
>>                  return -EINVAL;
>> +       if (ops->ndo_validate_addr) {
>> +               err = ops->ndo_validate_addr(dev);
>> +               if (err)
>> +                       return err;
>> +       }
> [Kalesh] It would be better to move this code after
> netif_device_present() check. Minor nit and there will not be any
> functional impact.

Yes, You are right,
For other reasons I need to send v10, I will move it.

Thanks,
	Jijie Shao