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

[Sven Eckelmann <sven@narfation.org>]

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

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

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