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.
> ---
>
> 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
