Re: [PATCH batadv 2/5] batman-adv: only create hardif while a netdev is part of a mesh

[Sven Eckelmann <sven@narfation.org>]

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

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

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