[dm-devel] [PATCH 1/3] kpartx: open /dev/loop-control only once

[dm-devel] [PATCH 1/3] kpartx: open /dev/loop-control only once

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Opening the same file repeatedly in a loop seems wrong.

For unknown reason, this patch caused gcc to diagnose a possible buffer
overflow for the device name, and I had to increase the buffer by
one byte.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/lopart.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/kpartx/lopart.c b/kpartx/lopart.c
index 9b65255..2661940 100644
--- a/kpartx/lopart.c
+++ b/kpartx/lopart.c
@@ -159,26 +159,28 @@ char *find_loop_by_file(const char *filename)
 
 char *find_unused_loop_device(void)
 {
-	char dev[20], *next_loop_dev = NULL;
+	char dev[21], *next_loop_dev = NULL;
 	int fd, next_loop = 0, somedev = 0, someloop = 0, loop_known = 0;
+	int next_loop_fd;
 	struct stat statbuf;
 	struct loop_info loopinfo;
 	FILE *procdev;
 
+	next_loop_fd = open("/dev/loop-control", O_RDWR);
+	if (next_loop_fd < 0)
+		return NULL;
+
+	if (!(fstat(next_loop_fd, &statbuf) == 0 && S_ISCHR(statbuf.st_mode))) {
+		close(next_loop_fd);
+		return NULL;
+	}
+
 	while (next_loop_dev == NULL) {
-		if (stat("/dev/loop-control", &statbuf) == 0 &&
-		    S_ISCHR(statbuf.st_mode)) {
-			int next_loop_fd;
-
-			next_loop_fd = open("/dev/loop-control", O_RDWR);
-			if (next_loop_fd < 0)
-				return NULL;
-			next_loop = ioctl(next_loop_fd, LOOP_CTL_GET_FREE);
+		next_loop = ioctl(next_loop_fd, LOOP_CTL_GET_FREE);
+		if (next_loop < 0) {
 			close(next_loop_fd);
-			if (next_loop < 0)
-				return NULL;
+			return NULL;
 		}
-
 		sprintf(dev, "/dev/loop%d", next_loop);
 
 		fd = open (dev, O_RDONLY);
@@ -199,6 +201,9 @@ char *find_unused_loop_device(void)
 		}
 		break;
 	}
+
+	close(next_loop_fd);
+
 	if (next_loop_dev)
 		return next_loop_dev;
 
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 2/3] kpartx: use opened loop device immediately

[mwilck]

From: Martin Wilck <mwilck@suse.com>

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 <mwilck@suse.com>
---
 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
@@ -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 *);
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 2/3] kpartx: use opened loop device immediately

[Martin Wilck]

On Mon, 2021-11-15 at 21:22 +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> 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 <mwilck@suse.com>
> ---
>  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
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 3/3] kpartx: find_unused_loop_device(): add newlines

[mwilck]

From: Martin Wilck <mwilck@suse.com>

... to avoid these messages being joined with the error message
from the caller.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/lopart.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kpartx/lopart.c b/kpartx/lopart.c
index fccb522..ba9f2c0 100644
--- a/kpartx/lopart.c
+++ b/kpartx/lopart.c
@@ -209,26 +209,26 @@ nothing_found:
 	}
 
 	if (!somedev)
-		fprintf(stderr, "mount: could not find any device /dev/loop#");
+		fprintf(stderr, "mount: could not find any device /dev/loop#\n");
 
 	else if (!someloop) {
 		if (loop_known == 1)
 			fprintf(stderr,
 				"mount: Could not find any loop device.\n"
-				"       Maybe /dev/loop# has a wrong major number?");
+				"       Maybe /dev/loop# has a wrong major number?\n");
 		else if (loop_known == -1)
 			fprintf(stderr,
 				"mount: Could not find any loop device, and, according to %s,\n"
 				"       this kernel does not know about the loop device.\n"
-				"       (If so, then recompile or `modprobe loop'.)",
+				"       (If so, then recompile or `modprobe loop'.)\n",
 				PROC_DEVICES);
 		else
 			fprintf(stderr,
 				"mount: Could not find any loop device. Maybe this kernel does not know\n"
 				"       about the loop device (then recompile or `modprobe loop'), or\n"
-				"       maybe /dev/loop# has the wrong major number?");
+				"       maybe /dev/loop# has the wrong major number?\n");
 	} else
-		fprintf(stderr, "mount: could not find any free loop device");
+		fprintf(stderr, "mount: could not find any free loop device\n");
 	return NULL;
 }
 
-- 
2.33.1


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel