Re: [B.A.T.M.A.N.] [PATCH v2 1/7] batman-adv: tp_meter - prevent concurrent tp_meter sessions by using workqueue

Hi,

looks like you have reference counting issues here:

On Freitag, 18. Mai 2018 09:47:48 CEST Marek Lindner wrote:

-static void batadv_tp_start_kthread(struct batadv_tp_vars *tp_vars) +static void batadv_tp_start_work(struct batadv_tp_vars *tp_vars) {

•   struct task_struct *kthread;
  

•   struct batadv_priv *bat_priv = tp_vars->bat_priv;
  

•   u32 session_cookie;
  

•   kref_get(&tp_vars->refcount);
  

•   kthread = kthread_create(batadv_tp_send, tp_vars, "kbatadv_tp_meter");
  

•   if (IS_ERR(kthread)) {
  

•           session_cookie = batadv_tp_session_cookie(tp_vars->session,
  

•                                                     tp_vars->icmp_uid);
  

•           pr_err("batadv: cannot create tp meter kthread\n");
  

•           batadv_tp_batctl_error_notify(BATADV_TP_REASON_MEMORY_ERROR,
  

•                                         tp_vars->other_end,
  

•                                         bat_priv, session_cookie);
  

•           /* drop reserved reference for kthread */
  

•           batadv_tp_vars_put(tp_vars);
  

•           /* cleanup of failed tp meter variables */
  

•           batadv_tp_sender_cleanup(bat_priv, tp_vars);
  

•           return;
  

•   }
  

•   wake_up_process(kthread);
  

•   /* init work item that will actually execute the test and schedule it */
  

•   INIT_WORK(&tp_vars->test_work, batadv_tp_send);
  

•   queue_work(batadv_tp_meter_queue, &tp_vars->test_work);
  

}

You completely ignore here that queue_work can fail. And then you have an item with a reference counter > 0 in memory and no one cares about it.

/** @@ -1053,13 +1035,10 @@ void batadv_tp_start(struct batadv_priv *bat_priv, const u8 *dst, /* init work item for finished tp tests */ INIT_DELAYED_WORK(&tp_vars->finish_work, batadv_tp_sender_finish);

•   /* start tp kthread. This way the write() call issued from userspace can
  

•    * happily return and avoid to block
  

•   /* schedule the tp worker. This way the write() call issued from
  

•    * userspace can happily return and avoid to block
     */
  

•   batadv_tp_start_kthread(tp_vars);
  

•   /* don't return reference to new tp_vars */
  

•   batadv_tp_vars_put(tp_vars);
  

•   batadv_tp_start_work(tp_vars);
  

}

Just from the context of this function, it isn't clear what happened with the reference.

Kind regards, Sven