Re: [PATCH] batman-adv: fix panic during interface removal

[Remi Pommarel <repk@triplefau.lt>]

Hi Andrew,

On Fri, Jan 10, 2025 at 01:02:19AM -0800, Andrew Strohman wrote:
> > I would prefer when you would call cancel_work_sync when metric stuff should
> > be stopped. I was expecting to see this somewhere around
> > batadv_v_elp_iface_disable after the cancel_work_sync but it seems like it is
> > missing there (or in a similar place)
> >
> 
> I tried this:
> 
> diff --git a/net/batman-adv/bat_v_elp.c b/net/batman-adv/bat_v_elp.c
> index 1d704574..b35ded79 100644
> --- a/net/batman-adv/bat_v_elp.c
> +++ b/net/batman-adv/bat_v_elp.c
> @@ -387,8 +387,20 @@ int batadv_v_elp_iface_enable(struct
> batadv_hard_iface *hard_iface)
>   */
>  void batadv_v_elp_iface_disable(struct batadv_hard_iface *hard_iface)
>  {
> +       struct batadv_hardif_neigh_node *hardif_neigh;
> +
>         cancel_delayed_work_sync(&hard_iface->bat_v.elp_wq);
> 
> +       rcu_read_lock();
> +       hlist_for_each_entry_rcu(hardif_neigh,
> +                                &hard_iface->neigh_list, list) {
> +               if (!kref_get_unless_zero(&hardif_neigh->refcount))
> +                       continue;
> +               cancel_work_sync(&hardif_neigh->bat_v.metric_work);
> +               batadv_hardif_neigh_put(hardif_neigh);
> +       }
> +       rcu_read_unlock();
> +
>         dev_kfree_skb(hard_iface->bat_v.elp_skb);
>         hard_iface->bat_v.elp_skb = NULL;
>  }
> 
> But it seems to cause a hang on reboot every once in a while. When the hang
> happens, I'm not able to trigger sysrq over serial.

Quickly looking at that I think that metric_work may need to sleep so
calling cancel_work_sync() on this work does not seem safe while in rcu
protected context.

I would try to frame the cancel_work_sync() call with
rcu_read_unlock()/rcu_read_lock() as below:

  rcu_read_unlock();
  cancel_work_sync(...);
  rcu_read_lock();
  batadv_hardif_neigh_put(...);

But be careful as batadv_hardif_neigh_put() could modify the list you
are currently traversing. At first glance it seems safe to realease
rcu read constraint to call cancel_work_sync() as long as you take it
back before calling batadv_hardif_neigh_put(), but this could need more
though on that.

-- 
Remi