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
2018-08-29T06:56:47Z

[-- 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 --]