Re: [PATCH batadv 4/5] batman-adv: remove global hardif list

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 4204 bytes --]

On Saturday, 31 May 2025 11:56:34 CEST Sven Eckelmann wrote:
> 
> Signature created on Samstag, 31. Mai 2025 11:56:35 Mitteleuropäische Sommerzeit with certificate: Sven Eckelmann <sven@narfation.org> (EC37 1482 9567 81AF)
> The signature is valid and the certificate's validity is ultimately trusted.
> On Monday, 19 May 2025 22:46:31 CEST Matthias Schiffer wrote:
> >  struct batadv_hard_iface *
> > -batadv_hardif_get_by_netdev(const struct net_device *net_dev)
> > +batadv_hardif_get_by_netdev(struct net_device *net_dev)
> >  {
> >         struct batadv_hard_iface *hard_iface;
> > +       struct net_device *mesh_iface;
> >  
> > -       rcu_read_lock();
> > -       list_for_each_entry_rcu(hard_iface, &batadv_hardif_list, list) {
> > -               if (hard_iface->net_dev == net_dev &&
> > -                   kref_get_unless_zero(&hard_iface->refcount))
> > -                       goto out;
> > -       }
> > +       mesh_iface = netdev_master_upper_dev_get(net_dev);
> > +       if (!mesh_iface || !batadv_meshif_is_valid(mesh_iface))
> > +               return NULL;
> >  
> > -       hard_iface = NULL;
> > +       hard_iface = netdev_lower_dev_get_private(mesh_iface, net_dev);
> > +       if (!kref_get_unless_zero(&hard_iface->refcount))
> > +               return NULL;
> >  
> > -out:
> > -       rcu_read_unlock();
> >         return hard_iface;
> >  }
> 
> This code is now relying on rtnl_lock() (see `ASSERT_RTNL` in 
> `netdev_master_upper_dev_get` and most likely some comments somwhere about the 
> lists used by `netdev_lower_dev_get_private`). But `batadv_tt_local_add` is 
> using this function without holding this lock all the time. For example during
> packet processing.
> 
> See for example `batadv_tt_local_add` calls in `batadv_interface_tx`. This 
> will happen when `skb->skb_iif` is not 0 (so it was forwarded).

I am currently gathering the changes in an RFC branch 
https://git.open-mesh.org/pub/ecsv/batman-adv.git/log/?h=b4/drop-hardif-list 
and will post the result after the v2 version of the net-next.git PR was 
(first submitted and then) merged.

I came to the conclusion that the batadv_tt_local_add() part must already be 
handled in "batman-adv: only create hardif while a netdev is part of a mesh"
(or earlier). And since we have an rhashtable for it, I would propose 
something like:

diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index 25acbb346a17297a558a021a717a2cfc1753ae91..2abf0e35e2eafe3d19f37ebd566c926deca082b5 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -636,20 +636,22 @@ bool batadv_tt_local_add(struct net_device *mesh_iface, const u8 *addr,
 	struct net *net = dev_net(mesh_iface);
 	struct batadv_meshif_vlan *vlan;
 	struct net_device *in_dev = NULL;
-	struct batadv_hard_iface *in_hardif = NULL;
 	struct hlist_head *head;
 	struct batadv_tt_orig_list_entry *orig_entry;
 	int hash_added, table_size, packet_size_max;
 	bool ret = false;
 	bool roamed_back = false;
+	bool iif_is_wifi = false;
 	u8 remote_flags;
 	u32 match_mark;
 
 	if (ifindex != BATADV_NULL_IFINDEX)
 		in_dev = dev_get_by_index(net, ifindex);
 
-	if (in_dev)
-		in_hardif = batadv_hardif_get_by_netdev(in_dev);
+	if (in_dev) {
+		u32 wifi_flags = batadv_netdev_get_wifi_flags(in_dev);
+		iif_is_wifi = batadv_is_wifi(wifi_flags);
+	}
 
 	tt_local = batadv_tt_local_hash_find(bat_priv, addr, vid);
 
@@ -724,7 +726,7 @@ bool batadv_tt_local_add(struct net_device *mesh_iface, const u8 *addr,
 	 */
 	tt_local->common.flags = BATADV_TT_CLIENT_NEW;
 	tt_local->common.vid = vid;
-	if (batadv_is_wifi_hardif(in_hardif))
+	if (iif_is_wifi)
 		tt_local->common.flags |= BATADV_TT_CLIENT_WIFI;
 	kref_init(&tt_local->common.refcount);
 	tt_local->last_seen = jiffies;



It is still required to adjust the netlink code to not run into rtnl_lock 
problems when running things like batadv_pre_doit() -> ... -> 
batadv_netlink_get_hardif_from_ifindex() -> batadv_hardif_get_by_netdev().
Most likely by a simple rtnl_lock() in
batadv_netlink_get_hardif_from_ifindex().

Regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]