[Devel] [PATCH vz7] ploop: properly restore old delta kobject on replace error

[Pavel Tikhomirov]

On 22.05.2023 23:19, Alexander Atanasov wrote:
> Current code removes the old_delta kobject before the new delta
> is ready to be used in case there is an error while reading BAT,
> the "too short BAT" error we have seen.
> The new delta and its objects are destroyed properly but the original
> delta kobject is never added back, so pdelta sysfs dir stay empty and
> userspace tools get confused since they consult this files before
> performing operations on the ploop device. At later point when ploop
> device is destroyed a second kobject_del will be called and it can lead
> to a crash or corrupted memory.
> 
> To fix this instead of deleting the delta early rename it with prefix
> and move the deletion at the end, while on error rename back to original
> name. This way if something goes wrong it will be visible in sysfs as
> old_N, where N is the delta level.
> 
> Extract code to rename kobject into a function and cleanup its users.
> 
> Fixes:
> commit 5f3ee110e6f4 ("ploop: Repopulate holes_bitmap on changing delta")
> 
> https://jira.vzint.dev/browse/PSBM-146797
> Signed-off-by: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
> ---
>   drivers/block/ploop/dev.c | 47 ++++++++++++++++++++++-----------------
>   1 file changed, 26 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
> index 6eb22168b5fe..0ed63f9da1cb 100644
> --- a/drivers/block/ploop/dev.c
> +++ b/drivers/block/ploop/dev.c
> @@ -3557,6 +3557,16 @@ static int ploop_add_delta(struct ploop_device * plo, unsigned long arg)
>   	return err;
>   }
>   
> +/*
> + * Have to implement own version of kobject_rename since it is GPL only symbol
> + */
> +static int ploop_rename_delta(struct ploop_delta *delta, int level, char *pref)
> +{
> +	kobject_del(&delta->kobj);
> +	return KOBJECT_ADD(&delta->kobj, &delta->plo->kobj,
> +			  "%s%d", pref ? : "", level);
> +}
> +
>   static int ploop_replace_delta(struct ploop_device * plo, unsigned long arg)
>   {
>   	int err;
> @@ -3594,15 +3604,9 @@ static int ploop_replace_delta(struct ploop_device * plo, unsigned long arg)
>   	if (err)
>   		goto out_destroy;
>   
> -	kobject_del(&old_delta->kobj);
> -
> -	err = KOBJECT_ADD(&delta->kobj, kobject_get(&plo->kobj),
> -			  "%d", delta->level);
> -	/* _put below is a pair for _get for OLD delta */
> -	kobject_put(&plo->kobj);
> -
> +	err = ploop_rename_delta(old_delta, old_delta->level, "old_");
>   	if (err < 0) {
> -		kobject_put(&plo->kobj);
> +		PL_WARN(plo, "Failed to rename old delta kobj\n");
>   		goto out_close;

I don't like this error path, if ploop_rename_delta failed it means that 
we did kobject_del(&old_delta->kobj); but didn't finish corresponding 
KOBJECT_ADD, and in out_close, we would do ploop_rename_delta again 
where we would do second kobject_del(&old_delta->jobj) and corresponding 
KOBJECT_ADD. This would do double 
kobject_del->kobj_kset_leave->kset_put, not sure if it would break 
something or not though.

I mean, maybe it's better to preserve original out_close, and add a new 
label out_restore_rename.

>   	}
>   
> @@ -3611,10 +3615,17 @@ static int ploop_replace_delta(struct ploop_device * plo, unsigned long arg)
>   		err = delta->ops->replace_delta(delta);
>   		if (err) {
>   			ploop_relax(plo);
> -			goto out_kobj_del;
> +			goto out_close;
>   		}
>   	}
>   
> +	err = KOBJECT_ADD(&delta->kobj, kobject_get(&plo->kobj),
> +			  "%d", delta->level);
> +	if (err < 0) {
> +		/* _put for failed _ADD */
> +		kobject_put(&plo->kobj);
> +		goto out_close;
> +	}
>   	ploop_map_destroy(&plo->map);
>   	list_replace_init(&old_delta->list, &delta->list);
>   	ploop_delta_list_changed(plo);
> @@ -3631,13 +3642,15 @@ static int ploop_replace_delta(struct ploop_device * plo, unsigned long arg)
>   
>   	old_delta->ops->stop(old_delta);
>   	old_delta->ops->destroy(old_delta);
> +	kobject_del(&old_delta->kobj);
>   	kobject_put(&old_delta->kobj);
> +	kobject_put(&plo->kobj);
>   	return 0;
>   
> -out_kobj_del:
> -	kobject_del(&delta->kobj);
> -	kobject_put(&plo->kobj);
>   out_close:
> +	err = ploop_rename_delta(old_delta, old_delta->level, NULL);
> +	if (err < 0)
> +		PL_ERR(plo, "Failed to restore old delta kobj name\n");
>   	delta->ops->stop(delta);
>   out_destroy:
>   	delta->ops->destroy(delta);
> @@ -3974,15 +3987,7 @@ static void rename_deltas(struct ploop_device * plo, int level)
>   
>   		if (delta->level < level)
>   			continue;
> -#if 0
> -		/* Oops, kobject_rename() is not exported! */
> -		sprintf(nname, "%d", delta->level);
> -		err = kobject_rename(&delta->kobj, nname);
> -#else
> -		kobject_del(&delta->kobj);
> -		err = KOBJECT_ADD(&delta->kobj, &plo->kobj,
> -				  "%d", delta->level);
> -#endif
> +		err = ploop_rename_delta(delta, delta->level, NULL);
>   		if (err)
>   			PL_WARN(plo, "rename_deltas: %d %d %d\n", err, level, delta->level);
>   	}

-- 
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.