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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7760123.MhkbZ0Pkbq@sven-desktop>
Date: Sat, 31 May 2025 11:16:30 +0200
From: Sven Eckelmann <sven@...fation.org>
To: Marek Lindner <marek.lindner@...lbox.org>,
 Simon Wunderlich <sw@...onwunderlich.de>,
 Antonio Quartulli <antonio@...delbit.com>,
 Matthias Schiffer <mschiffer@...verse-factory.net>
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
 b.a.t.m.a.n@...ts.open-mesh.org, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject:
 Re: [PATCH batadv 2/5] batman-adv: only create hardif while a netdev is part
 of a mesh

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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)