Re: [LTP] [PATCH v18 1/3] lib: Add tst_attach_loopdev()

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> Calling tst_find_free_loopdev() followed by tst_attach_device() has
> a race window where another process can claim the loop device before
> LOOP_SET_FD is issued, causing attachment to fail with EBUSY.
> 
> Introduce tst_attach_loopdev() which finds an unused loop device and
> attaches a backing file to it, retrying the lookup with exponential
> backoff if the candidate device is claimed concurrently (EBUSY). In
> addition, only populate the caller's device path buffer on successful
> attachment, and clear it on failure to prevent accidental cleanup of
> unattached devices.
> 
> Update tst_acquire_loop_device() to use tst_attach_loopdev().
> 
> Suggested-by: Cyril Hrubis <chrubis@suse.cz>
> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
> v17->v18:
> - New patch in v18.
> 
>  include/tst_device.h | 13 +++++++
>  lib/tst_device.c     | 87 ++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 92 insertions(+), 8 deletions(-)
> 
> diff --git a/include/tst_device.h b/include/tst_device.h
> index 0989e4501..2e7dadd59 100644
> --- a/include/tst_device.h
> +++ b/include/tst_device.h
> @@ -110,6 +110,19 @@ int tst_find_free_loopdev(char *path, size_t path_len);
>   */
>  int tst_attach_device(const char *dev_path, const char *file_path);
>  
> +/**
> + * tst_attach_loopdev() - Find a free loop device and attach a file to it.
> + * @dev_path: If non-NULL, buffer filled with the attached loop device path.
> + * @dev_path_len: Size of @dev_path buffer.
> + * @file_path: Path to a file (e.g. disk.img).
> + *
> + * Finds an unused loop device and attaches @file_path to it, automatically
> + * retrying the lookup if the chosen device is claimed concurrently (EBUSY).
> + *
> + * Return: Zero on success, non-zero otherwise.
> + */
> +int tst_attach_loopdev(char *dev_path, size_t dev_path_len, const char *file_path);
> +
>  /**
>   * tst_get_device_size() - Get device size in megabytes.
>   * @dev_path: Device path.
> diff --git a/lib/tst_device.c b/lib/tst_device.c
> index 00304711b..199c14059 100644
> --- a/lib/tst_device.c
> +++ b/lib/tst_device.c
> @@ -156,7 +156,7 @@ int tst_find_free_loopdev(char *path, size_t path_len)
>  	return -1;
>  }
>  
> -int tst_attach_device(const char *dev, const char *file)
> +static int attach_device(const char *dev, const char *file, int warn_on_ebusy)
>  {
>  	int dev_fd, file_fd;
>  	struct loop_info loopinfo;
> @@ -168,7 +168,8 @@ int tst_attach_device(const char *dev, const char *file)
>  
>  	dev_fd = TST_RETRY_FN_EXP_BACKOFF(open(dev, O_RDWR), TST_RETVAL_GE0, 1);
>  	if (dev_fd < 0) {
> -		tst_resm(TWARN | TERRNO, "open('%s', O_RDWR) failed", dev);
> +		if (warn_on_ebusy || errno != EBUSY)
> +			tst_resm(TWARN | TERRNO, "open('%s', O_RDWR) failed", dev);
>  		return 1;
>  	}
>  
> @@ -180,10 +181,16 @@ int tst_attach_device(const char *dev, const char *file)
>  	}
>  
>  	if (ioctl(dev_fd, LOOP_SET_FD, file_fd) < 0) {
> +		int err = errno;
> +
>  		close(dev_fd);
>  		close(file_fd);
> -		tst_resm(TWARN | TERRNO, "ioctl(%s, LOOP_SET_FD, %s) failed",
> -			 dev, file);
> +		errno = err;
> +
> +		if (warn_on_ebusy || errno != EBUSY) {
> +			tst_resm(TWARN | TERRNO, "ioctl(%s, LOOP_SET_FD, %s) failed",
> +				 dev, file);
> +		}
>  		return 1;
>  	}
>  
> @@ -207,6 +214,73 @@ int tst_attach_device(const char *dev, const char *file)
>  	return 0;
>  }
>  
> +int tst_attach_device(const char *dev, const char *file)
> +{
> +	return attach_device(dev, file, 1);
> +}
> +
> +static int attach_loopdev(char *dev, size_t dev_len, const char *file)
> +{
> +	if (tst_find_free_loopdev(dev, dev_len) == -1)
> +		return -1;
> +
> +	if (attach_device(dev, file, 0) == 0)
> +		return 0;
> +
> +	if (errno == EBUSY)
> +		return 1;
> +
> +	return -1;
> +}
> +
> +int tst_attach_loopdev(char *dev_path, size_t dev_path_len, const char *file)
> +{
> +	char dev[PATH_MAX];
> +	unsigned int delay = 1;
> +	unsigned int max_delay = tst_multiply_timeout(1000000);
> +	int ret, fd;
> +
> +	if (strlen(file) >= LO_NAME_SIZE) {
> +		tst_brkm(TBROK, NULL, "Device name can't be longer than %u chars",
> +			 LO_NAME_SIZE);
> +	}
> +
> +	fd = open(file, O_RDWR);
> +	if (fd < 0) {
> +		tst_resm(TWARN | TERRNO, "open('%s', O_RDWR) failed", file);
> +		goto fail;
> +	}
> +	close(fd);

I would drop this part, it's making the API ugly. The attach loopdev
should ideally just take the file name as an parameter and return newly
allocated string with the path to the device it was attached to or NULL
in a case of a failure so that we can call it as:

	dev = tst_attach_loopdev(file);
	if (!dev)
		tst_brk();

Or even have:
	dev = SAFE_ATTACH_LOOPDEV(file);

> +	for (;;) {
> +		ret = attach_loopdev(dev, sizeof(dev), file);
> +		if (ret <= 0)
> +			break;
> +
> +		if (delay < max_delay) {
> +			usleep(delay);
> +			delay *= 2;
> +		} else {
> +			ret = -1;
> +			break;
> +		}
> +	}
> +
> +	if (ret == 0) {
> +		if (dev_path && dev_path_len) {
> +			strncpy(dev_path, dev, dev_path_len);
> +			dev_path[dev_path_len - 1] = '\0';
> +		}
> +		return 0;
> +	}
> +
> +fail:
> +	if (dev_path && dev_path_len)
> +		dev_path[0] = '\0';
> +
> +	return 1;
> +}
> +
>  uint64_t tst_get_device_size(const char *dev_path)
>  {
>  	int fd;
> @@ -390,10 +464,7 @@ const char *tst_acquire_loop_device(unsigned int size, const char *filename)
>  		return NULL;
>  	}
>  
> -	if (tst_find_free_loopdev(dev_path, sizeof(dev_path)) == -1)
> -		return NULL;
> -
> -	if (tst_attach_device(dev_path, filename))
> +	if (tst_attach_loopdev(dev_path, sizeof(dev_path), filename))
>  		return NULL;
>  
>  	return dev_path;
> -- 
> 2.55.0
> 

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp