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

[Matthias Schiffer]

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 <mschif...@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 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;

-       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 (tmp_hard_iface->if_status == BATADV_IF_NOT_IN_USE)
                        continue;

-                                       net_dev->dev_addr))
+               if (!batadv_compare_eth(tmp_hard_iface->net_dev->dev_addr,
+                                       hard_iface->net_dev->dev_addr))
                        continue;

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

-static void batadv_check_known_mac_addr(const struct net_device *net_dev) - rcu_read_lock(); - if (hard_iface->net_dev == net_dev) - if (!batadv_compare_eth(hard_iface->net_dev->dev_addr, - pr_warn("The newly added mac address (%pM) already exists on: %s\n",

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.



Best,
Matthias

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.

OpenPGP_signature.asc
Description: OpenPGP digital signature