* [dm-devel] [PATCH 1/3] kpartx: open /dev/loop-control only once
@ 2021-11-15 20:21 mwilck
2021-11-15 20:22 ` [dm-devel] [PATCH 2/3] kpartx: use opened loop device immediately mwilck
2021-11-15 20:22 ` [dm-devel] [PATCH 3/3] kpartx: find_unused_loop_device(): add newlines mwilck
0 siblings, 2 replies; 4+ messages in thread
From: mwilck @ 2021-11-15 20:21 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [dm-devel] [PATCH 2/3] kpartx: use opened loop device immediately
2021-11-15 20:21 [dm-devel] [PATCH 1/3] kpartx: open /dev/loop-control only once mwilck
@ 2021-11-15 20:22 ` mwilck
2021-11-16 21:34 ` Martin Wilck
2021-11-15 20:22 ` [dm-devel] [PATCH 3/3] kpartx: find_unused_loop_device(): add newlines mwilck
1 sibling, 1 reply; 4+ messages in thread
From: mwilck @ 2021-11-15 20:22 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [dm-devel] [PATCH 3/3] kpartx: find_unused_loop_device(): add newlines
2021-11-15 20:21 [dm-devel] [PATCH 1/3] kpartx: open /dev/loop-control only once mwilck
2021-11-15 20:22 ` [dm-devel] [PATCH 2/3] kpartx: use opened loop device immediately mwilck
@ 2021-11-15 20:22 ` mwilck
1 sibling, 0 replies; 4+ messages in thread
From: mwilck @ 2021-11-15 20:22 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [dm-devel] [PATCH 2/3] kpartx: use opened loop device immediately
2021-11-15 20:22 ` [dm-devel] [PATCH 2/3] kpartx: use opened loop device immediately mwilck
@ 2021-11-16 21:34 ` Martin Wilck
0 siblings, 0 replies; 4+ messages in thread
From: Martin Wilck @ 2021-11-16 21:34 UTC (permalink / raw)
To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-11-16 21:35 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-15 20:21 [dm-devel] [PATCH 1/3] kpartx: open /dev/loop-control only once mwilck
2021-11-15 20:22 ` [dm-devel] [PATCH 2/3] kpartx: use opened loop device immediately mwilck
2021-11-16 21:34 ` Martin Wilck
2021-11-15 20:22 ` [dm-devel] [PATCH 3/3] kpartx: find_unused_loop_device(): add newlines mwilck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.