Re: [PATCH batadv 2/5] batman-adv: only create hardif while a netdev is part of a mesh
From: Sven Eckelmann <sven@narfation.org>
Date: 2025-05-31 09:16:46
Also in:
batman, lkml
On Monday, 19 May 2025 22:46:29 CEST Matthias Schiffer wrote:
quoted hunk ↗ jump to hunk
@@ -734,9 +768,6 @@ int batadv_hardif_enable_interface(struct batadv_hard_iface *hard_iface, if (ret < 0) goto err_upper; - hard_iface->if_status = BATADV_IF_INACTIVE; - - kref_get(&hard_iface->refcount); hard_iface->batman_adv_ptype.type = ethertype; hard_iface->batman_adv_ptype.func = batadv_batman_skb_recv
This is confusing. You remove the reference for the batman_adv_ptype but kept the `batadv_hardif_put(hard_iface);` after `dev_remove_pack(&hard_iface->batman_adv_ptype);`. I think this should be added again and instead following code should receive a `batadv_hardif_put(hard_iface);` after the `list_del_rcu(&hard_iface->list);`:
quoted hunk ↗ jump to hunk
@@ -818,11 +849,16 @@ void batadv_hardif_disable_interface(struct batadv_hard_iface *hard_iface) struct batadv_priv *bat_priv = netdev_priv(hard_iface->mesh_iface); struct batadv_hard_iface *primary_if = NULL; + ASSERT_RTNL(); + batadv_hardif_deactivate_interface(hard_iface); if (hard_iface->if_status != BATADV_IF_INACTIVE) goto out; + list_del_rcu(&hard_iface->list); + batadv_hardif_generation++; + batadv_info(hard_iface->mesh_iface, "Removing interface: %s\n", hard_iface->net_dev->name); dev_remove_pack(&hard_iface->batman_adv_ptype);
And yes, this means that this needs to be removed in PATCH 3 again - together with the `kref_get` from this chunk (from PATCH 3): On Monday, 19 May 2025 22:46:31 CEST Matthias Schiffer wrote:
quoted hunk ↗ jump to hunk
@@ -738,8 +735,6 @@ int batadv_hardif_enable_interface(struct net_device *net_dev, batadv_v_hardif_init(hard_iface); kref_get(&hard_iface->refcount); - list_add_tail_rcu(&hard_iface->list, &batadv_hardif_list); - batadv_hardif_generation++; hardif_mtu = READ_ONCE(hard_iface->net_dev->mtu); required_mtu = READ_ONCE(mesh_iface->mtu) + max_header_len;
Just a question about this part (you don't really need to change it - I am just interested). Why did you move this MTU check to such a late position in the code?
- hardif_mtu = READ_ONCE(hard_iface->net_dev->mtu);
- required_mtu = READ_ONCE(mesh_iface->mtu) + max_header_len;
+ ASSERT_RTNL();
- if (hardif_mtu < ETH_MIN_MTU + max_header_len)
+ if (!batadv_is_valid_iface(net_dev))
return -EINVAL;
[...]
+ hard_iface = kzalloc(sizeof(*hard_iface), GFP_ATOMIC); + if (!hard_iface) + return -ENOMEM; + + netdev_hold(net_dev, &hard_iface->dev_tracker, GFP_ATOMIC); + hard_iface->net_dev = net_dev;
[...]
+ hardif_mtu = READ_ONCE(hard_iface->net_dev->mtu);
+ required_mtu = READ_ONCE(mesh_iface->mtu) + max_header_len;
+
+ if (hardif_mtu < ETH_MIN_MTU + max_header_len) {
+ ret = -EINVAL;
+ goto err_put;
+ }It made the error handling more complicated. And at the moment, I don't see the reason. For me, It would have been been more logical to just a a minimal invasive change like:
- hardif_mtu = READ_ONCE(hard_iface->net_dev->mtu); + hardif_mtu = READ_ONCE(net_dev->mtu);
Thanks, Sven
Attachments
- signature.asc [application/pgp-signature] 228 bytes