[dm-devel] [PATCH 16/72] libmultipath: make path_discovery() pthread_cancel()-safe

[Benjamin Marzinski]

On Sat, Oct 12, 2019 at 09:27:57PM +0000, Martin Wilck wrote:
> From: Martin Wilck <mwilck at suse.com>
> 
> The udev_enumerate and udev_device refs wouldn't be released
> if the thread was cancelled. Fix it.
> 
> Signed-off-by: Martin Wilck <mwilck at suse.com>
> ---
>  libmultipath/discovery.c | 51 +++++++++++++++++++++++++++++++---------
>  1 file changed, 40 insertions(+), 11 deletions(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index e68b0e9f..d217ca92 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -140,19 +140,47 @@ path_discover (vector pathvec, struct config * conf,
>  	return pathinfo(pp, conf, flag);
>  }
>  
> +static void cleanup_udev_enumerate_ptr(void *arg)
> +{
> +	struct udev_enumerate *ue;
> +
> +	if (!arg)
> +		return;
> +	ue = *((struct udev_enumerate**) arg);
> +	if (ue)
> +		(void)udev_enumerate_unref(ue);
> +}
> +
> +static void cleanup_udev_device_ptr(void *arg)
> +{
> +	struct udev_device *ud;
> +
> +	if (!arg)
> +		return;
> +	ud = *((struct udev_device**) arg);
> +	if (ud)
> +		(void)udev_device_unref(ud);
> +}
> +
>  int
>  path_discovery (vector pathvec, int flag)
>  {
> -	struct udev_enumerate *udev_iter;
> +	struct udev_enumerate *udev_iter = NULL;
>  	struct udev_list_entry *entry;
> -	struct udev_device *udevice;
> +	struct udev_device *udevice = NULL;
>  	struct config *conf;
> -	const char *devpath;
>  	int num_paths = 0, total_paths = 0, ret;
>  
> +	pthread_cleanup_push(cleanup_udev_enumerate_ptr, &udev_iter);
> +	pthread_cleanup_push(cleanup_udev_device_ptr, &udevice);
> +	conf = get_multipath_config();
> +	pthread_cleanup_push(put_multipath_config, conf);
> +
>  	udev_iter = udev_enumerate_new(udev);
> -	if (!udev_iter)
> -		return -ENOMEM;
> +	if (!udev_iter) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
>  
>  	if (udev_enumerate_add_match_subsystem(udev_iter, "block") < 0 ||
>  	    udev_enumerate_add_match_is_initialized(udev_iter) < 0 ||
> @@ -165,6 +193,8 @@ path_discovery (vector pathvec, int flag)
>  	udev_list_entry_foreach(entry,
>  				udev_enumerate_get_list_entry(udev_iter)) {
>  		const char *devtype;
> +		const char *devpath;
> +
>  		devpath = udev_list_entry_get_name(entry);
>  		condlog(4, "Discover device %s", devpath);
>  		udevice = udev_device_new_from_syspath(udev, devpath);
> @@ -175,19 +205,18 @@ path_discovery (vector pathvec, int flag)
>  		devtype = udev_device_get_devtype(udevice);
>  		if(devtype && !strncmp(devtype, "disk", 4)) {
>  			total_paths++;
> -			conf = get_multipath_config();
> -			pthread_cleanup_push(put_multipath_config, conf);

Why move grabbing the config RCU lock out of the loop? All things being
equal, it seems like we'd rather hold this for less time, and
rcu_read_lock() is designed to be lightweight, so calling it more times
shouldn't be an issue. 

-Ben

>  			if (path_discover(pathvec, conf,
>  					  udevice, flag) == PATHINFO_OK)
>  				num_paths++;
> -			pthread_cleanup_pop(1);
>  		}
> -		udev_device_unref(udevice);
> +		udevice = udev_device_unref(udevice);
>  	}
>  	ret = total_paths - num_paths;
> -out:
> -	udev_enumerate_unref(udev_iter);
>  	condlog(4, "Discovered %d/%d paths", num_paths, total_paths);
> +out:
> +	pthread_cleanup_pop(1);
> +	pthread_cleanup_pop(1);
> +	pthread_cleanup_pop(1);
>  	return ret;
>  }
>  
> -- 
> 2.23.0