diff mbox series

[V9,net-next,11/11] net: add ndo_validate_addr check in dev_set_mac_address

Message ID 20240910075942.1270054-12-shaojijie@huawei.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Add support of HIBMCGE Ethernet Driver | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 4 of 4 maintainers
netdev/build_clang success Errors and warnings before: 16 this patch: 16
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 21 this patch: 21
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 11 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 79 this patch: 79
netdev/source_inline success Was 0 now: 0

Commit Message

Jijie Shao Sept. 10, 2024, 7:59 a.m. UTC 
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(+)

Comments

Kalesh Anakkur Purayil Sept. 10, 2024, 8:39 a.m. UTC | #1 
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
>
Jijie Shao Sept. 10, 2024, 2:01 p.m. UTC | #2 
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
Jijie Shao Sept. 11, 2024, 1:30 p.m. UTC | #3 
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
Andrew Lunn Sept. 11, 2024, 2:41 p.m. UTC | #4 
> 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
diff mbox series

Patch

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);