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

[Pavel Tikhomirov]

On 09.06.2023 00: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 above sentence seems incomplete, as in an opposite case where there 
was no error, old_delta kobject would have been removed before new delta 
is ready anyway =)

> 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 move it at the end,
> on error restore back original.
> 
> Extract code to rename kobject into a function and cleanup its users.
> Since kobject_add can fail, get an extra reference on error so later
> kobject_del/kobject_put would not destroy it unexpectedly. Object
> should be intact except that it would not be visible in sysfs.
> 
> Fixes:
> 5f3ee110e6f4 (ploop: Repopulate holes_bitmap on changing delta, 2019-04-17)
> 
> https://jira.vzint.dev/browse/PSBM-146797
> Signed-off-by: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
> ---
>   drivers/block/ploop/dev.c | 63 +++++++++++++++++++++++----------------
>   1 file changed, 37 insertions(+), 26 deletions(-)
> 
> v1->v2: Addressing review comments. Removed the rename to minimize
> possibility of errors. added getting extra refs after failed add.
> 
> diff --git a/drivers/block/ploop/dev.c b/drivers/block/ploop/dev.c
> index 6eb22168b5fe..2833865b087f 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);
> +}
> +

It would probably be cleaner to put this function just before the 
function using it (rename_deltas).

>   static int ploop_replace_delta(struct ploop_device * plo, unsigned long arg)
>   {
>   	int err;
> @@ -3594,27 +3604,25 @@ 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);
> -
> -	if (err < 0) {
> -		kobject_put(&plo->kobj);
> -		goto out_close;
> -	}
>   

^ double newline after code removal

>   	ploop_quiesce(plo);
>   	if (delta->ops->replace_delta) {
>   		err = delta->ops->replace_delta(delta);
>   		if (err) {
>   			ploop_relax(plo);
> -			goto out_kobj_del;
> +			goto out_close;
>   		}
>   	}
>   
> +	/* Remove old delta kobj to avoid name collision with the new one */
> +	kobject_del(&old_delta->kobj);
> +	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_kobj_restore;
> +	}
>   	ploop_map_destroy(&plo->map);
>   	list_replace_init(&old_delta->list, &delta->list);
>   	ploop_delta_list_changed(plo);
> @@ -3632,11 +3640,19 @@ 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_put(&old_delta->kobj);
> -	return 0;
> -
> -out_kobj_del:
> -	kobject_del(&delta->kobj);
>   	kobject_put(&plo->kobj);
> +	return 0;
> +out_kobj_restore:
> +	/* we haven't dropped our plo->kobj ref just add back */
> +	err = KOBJECT_ADD(&old_delta->kobj, &plo->kobj, "%d", old_delta->level);
> +	if (err < 0) {
> +		PL_ERR(plo, "Failed to restore old delta kobject\n");
> +		/*
> +		 * Get an extra ref to parent object as kobject_add does, so it
> +		 * later kobject_del doesn't destory it
> +		 */
> +		kobject_get(&plo->kobj);

Later kobject_del would not see parent object as first kobject_del did 
"kobj->parent = NULL;", so in this aspect you should not be getting 
reference here.

note: Problem I was talking in my previous message was about kobj->kset put.

> +	}
>   out_close:
>   	delta->ops->stop(delta);
>   out_destroy:
> @@ -3974,17 +3990,12 @@ 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
> -		if (err)
> +		err = ploop_rename_delta(delta, delta->level, NULL);
You change from "KOBJECT_ADD(&delta->kobj, &plo->kobj," to 
"KOBJECT_ADD(&delta->kobj, &delta->plo->kobj,", are we sure that plo 
argument always equal to delta->plo here? May be we can to add a BUG_ON 
comparison to be sure?

> +		if (err) {
>   			PL_WARN(plo, "rename_deltas: %d %d %d\n", err, level, delta->level);
> +			/* Add failed - get an extra ref to parent to match later  _put */
> +			kobject_get(&delta->plo->kobj); > +		}
>   	}
>   }
>   

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