Re: [Devel] [PATCH hci-8.1 v4 1/6] drivers/md/push_backup: correct initialization

[Konstantin Khorenko <khorenko@virtuozzo.com>]

On 11/3/26 11:03, Andrey Zhadchenko wrote:
> Allocate pool and htable after lock, waitqueue and deadline
> timer init so we are sure that pb_destroy() can be called
> from anywhere. Otherwise we risk timer_delete_sync_try on
> zeroed timer struct (currently luckily fine).

Hi Andrey,

The code change is fine. On the message:

- "from anywhere" is broader than the patch: it makes pb_destroy() safe
  on every error path of pb_ctr().
- "luckily fine" holds for release kernels only. Our debug config has
  CONFIG_DEBUG_OBJECTS_TIMERS=y, and there debug_assert_init() reports
  the zeroed timer with an ODEBUG warning.

> +	pb->pbio_pool = mempool_create_slab_pool(PUSH_BACKUP_POOL_SIZE,
> +						 pbio_cache);
> +	pb->pending_htable = kcalloc(PB_HASH_TABLE_SIZE,
> +				     sizeof(struct hlist_head),
> +				     GFP_KERNEL);
> +	if (!pb->pbio_pool || !pb->pending_htable)
> +		goto err;

Not introduced here, but one more error path of the same function is
broken. Further down:

	ret = dm_get_device(ti, argv[1], dm_table_get_mode(ti->table),
			    &pb->origin_dev);
	...
	pb->wq = alloc_ordered_workqueue("dm-" DM_MSG_PREFIX, WQ_MEM_RECLAIM);
	if (!pb->wq) {
		ti->error = "could not create workqueue for metadata object";
		goto err;
	}

ret is still 0 from dm_get_device(), so a failed
alloc_ordered_workqueue() frees pb and pb_ctr() returns success. The
table keeps ti->private pointing to freed memory, and pb_dtr() destroys
it a second time. It dates back to 62e07487348e ("dm: Add push backup
driver"). Could you set ret = -ENOMEM there, in this patch or a
separate one?

With the message adjusted:

Reviewed-by: Konstantin Khorenko <khorenko@virtuozzo.com>

-- 
Konstantin Khorenko,
Virtuozzo Linux Kernel Team

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel