[2/3] kpartx: use opened loop device immediately

Commit Message

Martin Wilck Nov. 15, 2021, 8:22 p.m. UTC

From: Martin Wilck <[e-mail]>

The code in find_unused_loop_device() goes through circles to
get an unused device, but it takes no care not to race with a different
process opening the same loop device. Use the once-opened
loop device for setup immediately instead of closing and re-opening it.

While at it, simplify the code somewhat.

Signed-off-by: Martin Wilck <[e-mail]>
---
 kpartx/kpartx.c |  4 +--
 kpartx/lopart.c | 72 +++++++++++++++++++------------------------------
 kpartx/lopart.h |  3 +--
 3 files changed, 29 insertions(+), 50 deletions(-)

Comments

Martin Wilck Nov. 16, 2021, 9:34 p.m. UTC | #1

On Mon, 2021-11-15 at 21:22 +0100, [e-mail] wrote:
> From: Martin Wilck <[e-mail]>
> 
> The code in find_unused_loop_device() goes through circles to
> get an unused device, but it takes no care not to race with a
> different
> process opening the same loop device. Use the once-opened
> loop device for setup immediately instead of closing and re-opening
> it.
> 
> While at it, simplify the code somewhat.
> 
> Signed-off-by: Martin Wilck <[e-mail]>
> ---
>  kpartx/kpartx.c |  4 +--
>  kpartx/lopart.c | 72 +++++++++++++++++++----------------------------
> --
>  kpartx/lopart.h |  3 +--
>  3 files changed, 29 insertions(+), 50 deletions(-)
> 
> diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
> index 7bc6454..3c49999 100644
> --- a/kpartx/kpartx.c
> +++ b/kpartx/kpartx.c
>  
> @@ -266,11 +250,9 @@ int set_loop(const char *device, const char
> *file, int offset, int *loopro)
>                 }
>         }
>  
> -       if ((fd = open (device, mode)) < 0) {
> -               close(ffd);
> -               perror (device);
> +       *device = find_unused_loop_device(mode, &fd);
> +       if (!*device)
>                 return 1;

This leaks the file descriptor ffd. I'll re-post.

Regards
Martin


--
dm-devel mailing list
[e-mail]
https://listman.redhat.com/mailman/listinfo/dm-devel

Patch

diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 7bc6454..3c49999 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -359,9 +359,7 @@  main(int argc, char **argv){
 			exit (0);
 
 		if (!loopdev) {
-			loopdev = find_unused_loop_device();
-
-			if (set_loop(loopdev, rpath, 0, &ro)) {
+			if (set_loop(&loopdev, rpath, 0, &ro)) {
 				fprintf(stderr, "can't set up loop\n");
 				exit (1);
 			}
diff --git a/kpartx/lopart.c b/kpartx/lopart.c
index 2661940..fccb522 100644
--- a/kpartx/lopart.c
+++ b/kpartx/lopart.c
@@ -39,24 +39,6 @@ 
 #define LOOP_CTL_GET_FREE       0x4C82
 #endif
 
-static char *
-xstrdup (const char *s)
-{
-	char *t;
-
-	if (s == NULL)
-		return NULL;
-
-	t = strdup (s);
-
-	if (t == NULL) {
-		fprintf(stderr, "not enough memory");
-		exit(1);
-	}
-
-	return t;
-}
-
 #define SIZE(a) (sizeof(a)/sizeof(a[0]))
 
 char *find_loop_by_file(const char *filename)
@@ -157,9 +139,9 @@  char *find_loop_by_file(const char *filename)
 	return found;
 }
 
-char *find_unused_loop_device(void)
+static char *find_unused_loop_device(int mode, int *loop_fd)
 {
-	char dev[21], *next_loop_dev = NULL;
+	char dev[21];
 	int fd, next_loop = 0, somedev = 0, someloop = 0, loop_known = 0;
 	int next_loop_fd;
 	struct stat statbuf;
@@ -168,45 +150,47 @@  char *find_unused_loop_device(void)
 
 	next_loop_fd = open("/dev/loop-control", O_RDWR);
 	if (next_loop_fd < 0)
-		return NULL;
+		goto nothing_found;
 
-	if (!(fstat(next_loop_fd, &statbuf) == 0 && S_ISCHR(statbuf.st_mode))) {
-		close(next_loop_fd);
-		return NULL;
-	}
+	if (!(fstat(next_loop_fd, &statbuf) == 0 && S_ISCHR(statbuf.st_mode)))
+		goto nothing_found;
 
-	while (next_loop_dev == NULL) {
+	for (;;) {
 		next_loop = ioctl(next_loop_fd, LOOP_CTL_GET_FREE);
-		if (next_loop < 0) {
-			close(next_loop_fd);
-			return NULL;
-		}
+		if (next_loop < 0)
+			goto nothing_found;
+
 		sprintf(dev, "/dev/loop%d", next_loop);
 
-		fd = open (dev, O_RDONLY);
+		fd = open (dev, mode);
 		if (fd >= 0) {
 			if (fstat (fd, &statbuf) == 0 &&
 			    S_ISBLK(statbuf.st_mode)) {
 				somedev++;
 				if(ioctl (fd, LOOP_GET_STATUS, &loopinfo) == 0)
 					someloop++;		/* in use */
-				else if (errno == ENXIO)
-					next_loop_dev = xstrdup(dev);
+				else if (errno == ENXIO) {
+					char *name = strdup(dev);
+
+					if (name == NULL)
+						close(fd);
+					else
+						*loop_fd = fd;
+					close(next_loop_fd);
+					return name;
+				}
 
 			}
 			close (fd);
 
 			/* continue trying as long as devices exist */
-			continue;
-		}
-		break;
+		} else
+			break;
 	}
 
+nothing_found:
 	close(next_loop_fd);
 
-	if (next_loop_dev)
-		return next_loop_dev;
-
 	/* Nothing found. Why not? */
 	if ((procdev = fopen(PROC_DEVICES, "r")) != NULL) {
 		char line[100];
@@ -248,10 +232,10 @@  char *find_unused_loop_device(void)
 	return NULL;
 }
 
-int set_loop(const char *device, const char *file, int offset, int *loopro)
+int set_loop(char **device, const char *file, int offset, int *loopro)
 {
 	struct loop_info loopinfo;
-	int fd, ffd, mode;
+	int fd = -1, ffd, mode;
 
 	mode = (*loopro ? O_RDONLY : O_RDWR);
 
@@ -266,11 +250,9 @@  int set_loop(const char *device, const char *file, int offset, int *loopro)
 		}
 	}
 
-	if ((fd = open (device, mode)) < 0) {
-		close(ffd);
-		perror (device);
+	*device = find_unused_loop_device(mode, &fd);
+	if (!*device)
 		return 1;
-	}
 
 	*loopro = (mode == O_RDONLY);
 	memset (&loopinfo, 0, sizeof (loopinfo));
diff --git a/kpartx/lopart.h b/kpartx/lopart.h
index d3bad10..c73ab23 100644
--- a/kpartx/lopart.h
+++ b/kpartx/lopart.h
@@ -1,5 +1,4 @@ 
 extern int verbose;
-extern int set_loop (const char *, const char *, int, int *);
+extern int set_loop (char **, const char *, int, int *);
 extern int del_loop (const char *);
-extern char * find_unused_loop_device (void);
 extern char * find_loop_by_file (const char *);