[dm-devel] [PATCH v2] libmultipath: setup_map(): don't break multipath attributes

[Benjamin Marzinski]

On Thu, Sep 10, 2020 at 09:56:11PM +0200, mwilck at suse.com wrote:
> From: Martin Wilck <mwilck at suse.com>
> 
> setup_map() is called both for new maps (e.g. from coalesce_paths())
> and existing maps (e.g. from reload_map(), resize_map()). In the former
> case, the map will be removed from global data structures, so incomplete
> initialization is not an issue. But In the latter case, removal isn't
> generally possible. We expect that mpp->features, mpp->hwhandler,
> mpp->selector have been initialized and are are non-NULL. We must make sure
> not to break this assumption because of an error in this setup_map()
> invocation. As these properties aren't likely to change during an update
> operation, saving and restoring them is better than leaving the map
> improperly initialized.
> 
Reviewed-by: Benjamin Marzinski <bmarzins at redhat.com>
> Signed-off-by: Martin Wilck <mwilck at suse.com>
> ---
> 
> v1->v2: forgot to remove the call to free_multipath_attributes().
> 
> This is supposed to be applied on top of lixiaokeng's patch
> "libmultipath: check whether mpp->features is NUll in setup_map".
> 
>  libmultipath/configure.c | 29 +++++++++++++++++++++++++----
>  libmultipath/util.h      |  7 +++++++
>  2 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 5d5d941..8fd12df 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -298,6 +298,7 @@ int setup_map(struct multipath *mpp, char *params, int params_size,
>  	struct pathgroup * pgp;
>  	struct config *conf;
>  	int i, n_paths, marginal_pathgroups;
> +	char *save_attr;
>  
>  	/*
>  	 * don't bother if devmap size is unknown
> @@ -307,10 +308,6 @@ int setup_map(struct multipath *mpp, char *params, int params_size,
>  		return 1;
>  	}
>  
> -	/*
> -	 * free features, selector, and hwhandler properties if they are being reused
> -	 */
> -	free_multipath_attributes(mpp);
>  	if (mpp->disable_queueing && VECTOR_SIZE(mpp->paths) != 0)
>  		mpp->disable_queueing = 0;
>  
> @@ -328,11 +325,35 @@ int setup_map(struct multipath *mpp, char *params, int params_size,
>  
>  	select_pgfailback(conf, mpp);
>  	select_pgpolicy(conf, mpp);
> +
> +	/*
> +	 * If setup_map() is called from e.g. from reload_map() or resize_map(),
> +	 * make sure that we don't corrupt attributes.
> +	 */
> +	save_attr = steal_ptr(mpp->selector);
>  	select_selector(conf, mpp);
> +	if (!mpp->selector)
> +		mpp->selector = save_attr;
> +	else
> +		free(save_attr);
> +
>  	select_no_path_retry(conf, mpp);
>  	select_retain_hwhandler(conf, mpp);
> +
> +	save_attr = steal_ptr(mpp->features);
>  	select_features(conf, mpp);
> +	if (!mpp->features)
> +		mpp->features = save_attr;
> +	else
> +		free(save_attr);
> +
> +	save_attr = steal_ptr(mpp->hwhandler);
>  	select_hwhandler(conf, mpp);
> +	if (!mpp->hwhandler)
> +		mpp->hwhandler = save_attr;
> +	else
> +		free(save_attr);
> +
>  	select_rr_weight(conf, mpp);
>  	select_minio(conf, mpp);
>  	select_mode(conf, mpp);
> diff --git a/libmultipath/util.h b/libmultipath/util.h
> index 52aa559..045bbee 100644
> --- a/libmultipath/util.h
> +++ b/libmultipath/util.h
> @@ -110,4 +110,11 @@ static inline void clear_bit_in_bitfield(unsigned int bit, struct bitfield *bf)
>  	bf->bits[bit / bits_per_slot] &= ~(1ULL << (bit % bits_per_slot));
>  }
>  
> +#define steal_ptr(x)		       \
> +	({			       \
> +		void *___p = x;	       \
> +		x = NULL;	       \
> +		___p;		       \
> +	})
> +
>  #endif /* _UTIL_H */
> -- 
> 2.28.0