On Monday, 19 May 2025 22:46:29 CEST Matthias Schiffer wrote: > @@ -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);`:
> @@ -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:
> @@ -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
signature.asc
Description: This is a digitally signed message part.
