[PATCH net] batman-adv: fix duplicate MAC address check

[PATCH net] batman-adv: fix duplicate MAC address check

[Matthias Schiffer]

batadv_check_known_mac_addr() is both too lenient and too strict:

- It is called from batadv_hardif_add_interface(), which means that it
  checked interfaces that are not used for batman-adv at all. Move it
  to batadv_hardif_enable_interface(). Also, restrict it to hardifs of
  the same mesh interface; different mesh interfaces should not interact
  at all. The batadv_check_known_mac_addr() argument is changed from
  `struct net_device` to `struct batadv_hard_iface` to achieve this.
- The check only cares about hardifs in BATADV_IF_ACTIVE and
  BATADV_IF_TO_BE_ACTIVATED states, but interfaces in BATADV_IF_INACTIVE
  state should be checked as well, or the following steps will not
  result in a warning then they should:

  - Add two interfaces on down state with different MAC addresses to
    a mesh as hardifs
  - Change the MAC addresses so they confliect
  - Set interfaces to up state

  Now there will be two active hardifs with the same MAC address, but no
  warning. Fix by only ignoring hardifs in BATADV_IF_NOT_IN_USE state.

The RCU lock can be dropped, as we're holding RTNL anyways when the
function is called.

While we're at it, also switch from pr_warn() to netdev_warn().

Fixes: c6c8fea29769 ("net: Add batman-adv meshing protocol")
Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---

Aside: batadv_hardif_add_interface() being called for all existing
interfaces and having a global batadv_hardif_list at all is also not
very nice, but this will be addressed separately, as changing it will
require more refactoring.

 net/batman-adv/hard-interface.c | 37 ++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/net/batman-adv/hard-interface.c b/net/batman-adv/hard-interface.c
index f145f9662653..07b436626afb 100644
--- a/net/batman-adv/hard-interface.c
+++ b/net/batman-adv/hard-interface.c
@@ -506,28 +506,34 @@ batadv_hardif_is_iface_up(const struct batadv_hard_iface *hard_iface)
 	return false;
 }
 
-static void batadv_check_known_mac_addr(const struct net_device *net_dev)
+static void batadv_check_known_mac_addr(const struct batadv_hard_iface *hard_iface)
 {
-	const struct batadv_hard_iface *hard_iface;
+	const struct net_device *mesh_iface = hard_iface->mesh_iface;
+	const struct batadv_hard_iface *tmp_hard_iface;
 
-	rcu_read_lock();
-	list_for_each_entry_rcu(hard_iface, &batadv_hardif_list, list) {
-		if (hard_iface->if_status != BATADV_IF_ACTIVE &&
-		    hard_iface->if_status != BATADV_IF_TO_BE_ACTIVATED)
+	if (!mesh_iface)
+		return;
+
+	list_for_each_entry(tmp_hard_iface, &batadv_hardif_list, list) {
+		if (tmp_hard_iface == hard_iface)
+			continue;
+
+		if (tmp_hard_iface->mesh_iface != mesh_iface)
 			continue;
 
-		if (hard_iface->net_dev == net_dev)
+		if (tmp_hard_iface->if_status == BATADV_IF_NOT_IN_USE)
 			continue;
 
-		if (!batadv_compare_eth(hard_iface->net_dev->dev_addr,
-					net_dev->dev_addr))
+		if (!batadv_compare_eth(tmp_hard_iface->net_dev->dev_addr,
+					hard_iface->net_dev->dev_addr))
 			continue;
 
-		pr_warn("The newly added mac address (%pM) already exists on: %s\n",
-			net_dev->dev_addr, hard_iface->net_dev->name);
-		pr_warn("It is strongly recommended to keep mac addresses unique to avoid problems!\n");
+		netdev_warn(hard_iface->net_dev,
+			    "The newly added mac address (%pM) already exists on: %s\n",
+			    hard_iface->net_dev->dev_addr, tmp_hard_iface->net_dev->name);
+		netdev_warn(hard_iface->net_dev,
+			    "It is strongly recommended to keep mac addresses unique to avoid problems!\n");
 	}
-	rcu_read_unlock();
 }
 
 /**
@@ -764,6 +770,8 @@ int batadv_hardif_enable_interface(struct batadv_hard_iface *hard_iface,
 			    hard_iface->net_dev->name, hardif_mtu,
 			    required_mtu);
 
+	batadv_check_known_mac_addr(hard_iface);
+
 	if (batadv_hardif_is_iface_up(hard_iface))
 		batadv_hardif_activate_interface(hard_iface);
 	else
@@ -902,7 +910,6 @@ batadv_hardif_add_interface(struct net_device *net_dev)
 
 	batadv_v_hardif_init(hard_iface);
 
-	batadv_check_known_mac_addr(hard_iface->net_dev);
 	kref_get(&hard_iface->refcount);
 	list_add_tail_rcu(&hard_iface->list, &batadv_hardif_list);
 	batadv_hardif_generation++;
@@ -989,7 +996,7 @@ static int batadv_hard_if_event(struct notifier_block *this,
 		if (hard_iface->if_status == BATADV_IF_NOT_IN_USE)
 			goto hardif_put;
 
-		batadv_check_known_mac_addr(hard_iface->net_dev);
+		batadv_check_known_mac_addr(hard_iface);
 
 		bat_priv = netdev_priv(hard_iface->mesh_iface);
 		bat_priv->algo_ops->iface.update_mac(hard_iface);
-- 
2.49.0

Re: [PATCH net] batman-adv: fix duplicate MAC address check

[Paolo Abeni]

On 4/8/25 6:30 PM, Matthias Schiffer wrote:
> batadv_check_known_mac_addr() is both too lenient and too strict:
> 
> - It is called from batadv_hardif_add_interface(), which means that it
>   checked interfaces that are not used for batman-adv at all. Move it
>   to batadv_hardif_enable_interface(). Also, restrict it to hardifs of
>   the same mesh interface; different mesh interfaces should not interact
>   at all. The batadv_check_known_mac_addr() argument is changed from
>   `struct net_device` to `struct batadv_hard_iface` to achieve this.
> - The check only cares about hardifs in BATADV_IF_ACTIVE and
>   BATADV_IF_TO_BE_ACTIVATED states, but interfaces in BATADV_IF_INACTIVE
>   state should be checked as well, or the following steps will not
>   result in a warning then they should:
> 
>   - Add two interfaces on down state with different MAC addresses to
>     a mesh as hardifs
>   - Change the MAC addresses so they confliect
>   - Set interfaces to up state
> 
>   Now there will be two active hardifs with the same MAC address, but no
>   warning. Fix by only ignoring hardifs in BATADV_IF_NOT_IN_USE state.
> 
> The RCU lock can be dropped, as we're holding RTNL anyways when the
> function is called.
> 
> While we're at it, also switch from pr_warn() to netdev_warn().
> 
> Fixes: c6c8fea29769 ("net: Add batman-adv meshing protocol")
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>

Even if marked for net I assume this will eventually go first via the
batman tree.

> ---
> 
> Aside: batadv_hardif_add_interface() being called for all existing
> interfaces and having a global batadv_hardif_list at all is also not
> very nice, but this will be addressed separately, as changing it will
> require more refactoring.
> 
>  net/batman-adv/hard-interface.c | 37 ++++++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/net/batman-adv/hard-interface.c b/net/batman-adv/hard-interface.c
> index f145f9662653..07b436626afb 100644
> --- a/net/batman-adv/hard-interface.c
> +++ b/net/batman-adv/hard-interface.c
> @@ -506,28 +506,34 @@ batadv_hardif_is_iface_up(const struct batadv_hard_iface *hard_iface)
>  	return false;
>  }
>  
> -static void batadv_check_known_mac_addr(const struct net_device *net_dev)
> +static void batadv_check_known_mac_addr(const struct batadv_hard_iface *hard_iface)
>  {
> -	const struct batadv_hard_iface *hard_iface;
> +	const struct net_device *mesh_iface = hard_iface->mesh_iface;
> +	const struct batadv_hard_iface *tmp_hard_iface;
>  
> -	rcu_read_lock();
> -	list_for_each_entry_rcu(hard_iface, &batadv_hardif_list, list) {
> -		if (hard_iface->if_status != BATADV_IF_ACTIVE &&
> -		    hard_iface->if_status != BATADV_IF_TO_BE_ACTIVATED)
> +	if (!mesh_iface)
> +		return;
> +
> +	list_for_each_entry(tmp_hard_iface, &batadv_hardif_list, list) {
> +		if (tmp_hard_iface == hard_iface)
> +			continue;
> +
> +		if (tmp_hard_iface->mesh_iface != mesh_iface)
>  			continue;
>  
> -		if (hard_iface->net_dev == net_dev)
> +		if (tmp_hard_iface->if_status == BATADV_IF_NOT_IN_USE)
>  			continue;
>  
> -		if (!batadv_compare_eth(hard_iface->net_dev->dev_addr,
> -					net_dev->dev_addr))
> +		if (!batadv_compare_eth(tmp_hard_iface->net_dev->dev_addr,
> +					hard_iface->net_dev->dev_addr))
>  			continue;
>  
> -		pr_warn("The newly added mac address (%pM) already exists on: %s\n",
> -			net_dev->dev_addr, hard_iface->net_dev->name);
> -		pr_warn("It is strongly recommended to keep mac addresses unique to avoid problems!\n");
> +		netdev_warn(hard_iface->net_dev,
> +			    "The newly added mac address (%pM) already exists on: %s\n",
> +			    hard_iface->net_dev->dev_addr, tmp_hard_iface->net_dev->name);
> +		netdev_warn(hard_iface->net_dev,
> +			    "It is strongly recommended to keep mac addresses unique to avoid problems!\n");
>  	}
> -	rcu_read_unlock();
>  }

I feel like the above code mixes unnecessarily fix and refactor
(variable rename, different print helper usage).

I think the fix should be minimal, the refactor should land in a
different patch for next.

/P

Re: [PATCH net] batman-adv: fix duplicate MAC address check

[Matthias Schiffer]

[-- Attachment #1.1: Type: text/plain, Size: 4783 bytes --]

On 10/04/2025 11:38, Paolo Abeni wrote:
> On 4/8/25 6:30 PM, Matthias Schiffer wrote:
>> batadv_check_known_mac_addr() is both too lenient and too strict:
>>
>> - It is called from batadv_hardif_add_interface(), which means that it
>>    checked interfaces that are not used for batman-adv at all. Move it
>>    to batadv_hardif_enable_interface(). Also, restrict it to hardifs of
>>    the same mesh interface; different mesh interfaces should not interact
>>    at all. The batadv_check_known_mac_addr() argument is changed from
>>    `struct net_device` to `struct batadv_hard_iface` to achieve this.
>> - The check only cares about hardifs in BATADV_IF_ACTIVE and
>>    BATADV_IF_TO_BE_ACTIVATED states, but interfaces in BATADV_IF_INACTIVE
>>    state should be checked as well, or the following steps will not
>>    result in a warning then they should:
>>
>>    - Add two interfaces on down state with different MAC addresses to
>>      a mesh as hardifs
>>    - Change the MAC addresses so they confliect
>>    - Set interfaces to up state
>>
>>    Now there will be two active hardifs with the same MAC address, but no
>>    warning. Fix by only ignoring hardifs in BATADV_IF_NOT_IN_USE state.
>>
>> The RCU lock can be dropped, as we're holding RTNL anyways when the
>> function is called.
>>
>> While we're at it, also switch from pr_warn() to netdev_warn().
>>
>> Fixes: c6c8fea29769 ("net: Add batman-adv meshing protocol")
>> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> 
> Even if marked for net I assume this will eventually go first via the
> batman tree.

Yes. Should I have marked this differently?

> 
>> ---
>>
>> Aside: batadv_hardif_add_interface() being called for all existing
>> interfaces and having a global batadv_hardif_list at all is also not
>> very nice, but this will be addressed separately, as changing it will
>> require more refactoring.
>>
>>   net/batman-adv/hard-interface.c | 37 ++++++++++++++++++++-------------
>>   1 file changed, 22 insertions(+), 15 deletions(-)
>>
>> diff --git a/net/batman-adv/hard-interface.c b/net/batman-adv/hard-interface.c
>> index f145f9662653..07b436626afb 100644
>> --- a/net/batman-adv/hard-interface.c
>> +++ b/net/batman-adv/hard-interface.c
>> @@ -506,28 +506,34 @@ batadv_hardif_is_iface_up(const struct batadv_hard_iface *hard_iface)
>>   	return false;
>>   }
>>   
>> -static void batadv_check_known_mac_addr(const struct net_device *net_dev)
>> +static void batadv_check_known_mac_addr(const struct batadv_hard_iface *hard_iface)
>>   {
>> -	const struct batadv_hard_iface *hard_iface;
>> +	const struct net_device *mesh_iface = hard_iface->mesh_iface;
>> +	const struct batadv_hard_iface *tmp_hard_iface;
>>   
>> -	rcu_read_lock();
>> -	list_for_each_entry_rcu(hard_iface, &batadv_hardif_list, list) {
>> -		if (hard_iface->if_status != BATADV_IF_ACTIVE &&
>> -		    hard_iface->if_status != BATADV_IF_TO_BE_ACTIVATED)
>> +	if (!mesh_iface)
>> +		return;
>> +
>> +	list_for_each_entry(tmp_hard_iface, &batadv_hardif_list, list) {
>> +		if (tmp_hard_iface == hard_iface)
>> +			continue;
>> +
>> +		if (tmp_hard_iface->mesh_iface != mesh_iface)
>>   			continue;
>>   
>> -		if (hard_iface->net_dev == net_dev)
>> +		if (tmp_hard_iface->if_status == BATADV_IF_NOT_IN_USE)
>>   			continue;
>>   
>> -		if (!batadv_compare_eth(hard_iface->net_dev->dev_addr,
>> -					net_dev->dev_addr))
>> +		if (!batadv_compare_eth(tmp_hard_iface->net_dev->dev_addr,
>> +					hard_iface->net_dev->dev_addr))
>>   			continue;
>>   
>> -		pr_warn("The newly added mac address (%pM) already exists on: %s\n",
>> -			net_dev->dev_addr, hard_iface->net_dev->name);
>> -		pr_warn("It is strongly recommended to keep mac addresses unique to avoid problems!\n");
>> +		netdev_warn(hard_iface->net_dev,
>> +			    "The newly added mac address (%pM) already exists on: %s\n",
>> +			    hard_iface->net_dev->dev_addr, tmp_hard_iface->net_dev->name);
>> +		netdev_warn(hard_iface->net_dev,
>> +			    "It is strongly recommended to keep mac addresses unique to avoid problems!\n");
>>   	}
>> -	rcu_read_unlock();
>>   }
> 
> I feel like the above code mixes unnecessarily fix and refactor
> (variable rename, different print helper usage).
> 
> I think the fix should be minimal, the refactor should land in a
> different patch for next.

Okay. I'll remove the print helper change for now.

I think the variable rename should be kept, as we now have two 
batadv_hard_iface* vars, so we need to introduce a second name. Naming the 
interface we're working on hard_iface and using tmp_hard_iface for a loop 
variable matches similar code that already exists in batman-adv.

Best,
Matthias

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH net] batman-adv: fix duplicate MAC address check

[Jakub Kicinski]

On Thu, 10 Apr 2025 17:38:30 +0200 Matthias Schiffer wrote:
> >> Fixes: c6c8fea29769 ("net: Add batman-adv meshing protocol")
> >> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>  
> > 
> > Even if marked for net I assume this will eventually go first via the
> > batman tree.  
> 
> Yes. Should I have marked this differently?

Yes, let's reserve net for patches which will go directly into:
https://web.git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/
Similar for net-next.

The tree name is supposed to tell the maintainer who you expect
to take the patch.

The batman tree (according to MAINTAINERS) is called linux-merge
so let's ignore that and use batman and batman-next?