[PATCH] kpartx: Improve finding loopback device by file

[PATCH] kpartx: Improve finding loopback device by file

[Julian Andres Klode]

Commit 9bdfa3eb8e24b668e6c2bb882cddb0ccfe23ed5b changed kpartx
to lookup files by absolute path, using realpath(). This introduced
a regression when part of the filename where symbolic links. While
the kernel stores the absolute path to the backing file, it does not
resolve symbolic links, and hence kpartx would fail to find the loopback
device because it resolves symbolic links when deleting.

This introduces two workarounds in find_loop_by_file()

(1) We match against the specified file name as is as well
(2) We canonicalize the loopinfo.lo_name and match the canonicalized filename
    against that.

Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/multipath-tools/+bug/1747044
Signed-off-by: Julian Andres Klode <julian.klode@canonical.com>
---
 kpartx/kpartx.c |  8 +-------
 kpartx/lopart.c | 12 +++++++++++-
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 442b6bd9..c1af1c5e 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -327,13 +327,7 @@ main(int argc, char **argv){
 
 	if (S_ISREG (buf.st_mode)) {
 		/* already looped file ? */
-		char rpath[PATH_MAX];
-		if (realpath(device, rpath) == NULL) {
-			fprintf(stderr, "Error: %s: %s\n", device,
-				strerror(errno));
-			exit (1);
-		}
-		loopdev = find_loop_by_file(rpath);
+		loopdev = find_loop_by_file(device);
 
 		if (!loopdev && what == DELETE)
 			exit (0);
diff --git a/kpartx/lopart.c b/kpartx/lopart.c
index 02b29e8c..26b2678c 100644
--- a/kpartx/lopart.c
+++ b/kpartx/lopart.c
@@ -69,6 +69,13 @@ char *find_loop_by_file(const char *filename)
 	struct loop_info loopinfo;
 	const char VIRT_BLOCK[] = "/sys/devices/virtual/block";
 	char path[PATH_MAX];
+	char rfilename[PATH_MAX];
+	char rloopfilename[PATH_MAX];
+	if (realpath(filename, rfilename) == NULL) {
+		fprintf(stderr, "Error: %s: %s\n", filename,
+				strerror(errno));
+		exit (1);
+	}
 
 	dir = opendir(VIRT_BLOCK);
 	if (!dir)
@@ -117,7 +124,10 @@ char *find_loop_by_file(const char *filename)
 
 		close (fd);
 
-		if (0 == strcmp(filename, loopinfo.lo_name)) {
+		if (0 == strcmp(filename, loopinfo.lo_name) ||
+		    0 == strcmp(rfilename, loopinfo.lo_name) ||
+		    (realpath(loopinfo.lo_name, rloopfilename) &&
+		     0 == strcmp(rfilename, rloopfilename))) {
 			found = realpath(path, NULL);
 			break;
 		}
-- 
2.15.1

Re: [PATCH] kpartx: Improve finding loopback device by file

[Martin Wilck]

On Mon, 2018-02-05 at 09:58 +0100, Julian Andres Klode wrote:
> Commit 9bdfa3eb8e24b668e6c2bb882cddb0ccfe23ed5b changed kpartx
> to lookup files by absolute path, using realpath(). This introduced
> a regression when part of the filename where symbolic links. While
> the kernel stores the absolute path to the backing file, it does not
> resolve symbolic links, and hence kpartx would fail to find the
> loopback
> device because it resolves symbolic links when deleting.
> 
> This introduces two workarounds in find_loop_by_file()
> 
> (1) We match against the specified file name as is as well
> (2) We canonicalize the loopinfo.lo_name and match the canonicalized
> filename
>     against that.
> 
> Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/multipath-tools
> /+bug/1747044
> Signed-off-by: Julian Andres Klode <julian.klode@canonical.com>
> ---
>  kpartx/kpartx.c |  8 +-------
>  kpartx/lopart.c | 12 +++++++++++-
>  2 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
> index 442b6bd9..c1af1c5e 100644
> --- a/kpartx/kpartx.c
> +++ b/kpartx/kpartx.c
> @@ -327,13 +327,7 @@ main(int argc, char **argv){
>  
>  	if (S_ISREG (buf.st_mode)) {
>  		/* already looped file ? */
> -		char rpath[PATH_MAX];
> -		if (realpath(device, rpath) == NULL) {
> -			fprintf(stderr, "Error: %s: %s\n", device,
> -				strerror(errno));
> -			exit (1);
> -		}
> -		loopdev = find_loop_by_file(rpath);
> +		loopdev = find_loop_by_file(device);
>  
>  		if (!loopdev && what == DELETE)
>  			exit (0);
> diff --git a/kpartx/lopart.c b/kpartx/lopart.c
> index 02b29e8c..26b2678c 100644
> --- a/kpartx/lopart.c
> +++ b/kpartx/lopart.c
> @@ -69,6 +69,13 @@ char *find_loop_by_file(const char *filename)
>  	struct loop_info loopinfo;
>  	const char VIRT_BLOCK[] = "/sys/devices/virtual/block";
>  	char path[PATH_MAX];
> +	char rfilename[PATH_MAX];
> +	char rloopfilename[PATH_MAX];
> +	if (realpath(filename, rfilename) == NULL) {
> +		fprintf(stderr, "Error: %s: %s\n", filename,
> +				strerror(errno));
> +		exit (1);
> +	}
>  
>  	dir = opendir(VIRT_BLOCK);
>  	if (!dir)
> @@ -117,7 +124,10 @@ char *find_loop_by_file(const char *filename)
>  
>  		close (fd);
>  
> -		if (0 == strcmp(filename, loopinfo.lo_name)) {
> +		if (0 == strcmp(filename, loopinfo.lo_name) ||
> +		    0 == strcmp(rfilename, loopinfo.lo_name) ||
> +		    (realpath(loopinfo.lo_name, rloopfilename) &&
> +		     0 == strcmp(rfilename, rloopfilename))) {
>  			found = realpath(path, NULL);
>  			break;
>  		}

That "if x matches y or x matches y' or x' matches y" feels like
guesswork. Can't we simply call realpath() on both loopinfo.lo_name and
filename, and compare the two? 

Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

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

Re: [PATCH] kpartx: Improve finding loopback device by file

[Julian Andres Klode]

On Mon, Feb 05, 2018 at 11:32:13AM +0100, Martin Wilck wrote:
> On Mon, 2018-02-05 at 09:58 +0100, Julian Andres Klode wrote:
> > Commit 9bdfa3eb8e24b668e6c2bb882cddb0ccfe23ed5b changed kpartx
> > to lookup files by absolute path, using realpath(). This introduced
> > a regression when part of the filename where symbolic links. While
> > the kernel stores the absolute path to the backing file, it does not
> > resolve symbolic links, and hence kpartx would fail to find the
> > loopback
> > device because it resolves symbolic links when deleting.
> > 
> > This introduces two workarounds in find_loop_by_file()
> > 
> > (1) We match against the specified file name as is as well
> > (2) We canonicalize the loopinfo.lo_name and match the canonicalized
> > filename
> >     against that.
> > 
> > Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/multipath-tools
> > /+bug/1747044
> > Signed-off-by: Julian Andres Klode <julian.klode@canonical.com>
> > ---
> >  kpartx/kpartx.c |  8 +-------
> >  kpartx/lopart.c | 12 +++++++++++-
> >  2 files changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
> > index 442b6bd9..c1af1c5e 100644
> > --- a/kpartx/kpartx.c
> > +++ b/kpartx/kpartx.c
> > @@ -327,13 +327,7 @@ main(int argc, char **argv){
> >  
> >  	if (S_ISREG (buf.st_mode)) {
> >  		/* already looped file ? */
> > -		char rpath[PATH_MAX];
> > -		if (realpath(device, rpath) == NULL) {
> > -			fprintf(stderr, "Error: %s: %s\n", device,
> > -				strerror(errno));
> > -			exit (1);
> > -		}
> > -		loopdev = find_loop_by_file(rpath);
> > +		loopdev = find_loop_by_file(device);
> >  
> >  		if (!loopdev && what == DELETE)
> >  			exit (0);
> > diff --git a/kpartx/lopart.c b/kpartx/lopart.c
> > index 02b29e8c..26b2678c 100644
> > --- a/kpartx/lopart.c
> > +++ b/kpartx/lopart.c
> > @@ -69,6 +69,13 @@ char *find_loop_by_file(const char *filename)
> >  	struct loop_info loopinfo;
> >  	const char VIRT_BLOCK[] = "/sys/devices/virtual/block";
> >  	char path[PATH_MAX];
> > +	char rfilename[PATH_MAX];
> > +	char rloopfilename[PATH_MAX];
> > +	if (realpath(filename, rfilename) == NULL) {
> > +		fprintf(stderr, "Error: %s: %s\n", filename,
> > +				strerror(errno));
> > +		exit (1);
> > +	}
> >  
> >  	dir = opendir(VIRT_BLOCK);
> >  	if (!dir)
> > @@ -117,7 +124,10 @@ char *find_loop_by_file(const char *filename)
> >  
> >  		close (fd);
> >  
> > -		if (0 == strcmp(filename, loopinfo.lo_name)) {
> > +		if (0 == strcmp(filename, loopinfo.lo_name) ||
> > +		    0 == strcmp(rfilename, loopinfo.lo_name) ||
> > +		    (realpath(loopinfo.lo_name, rloopfilename) &&
> > +		     0 == strcmp(rfilename, rloopfilename))) {
> >  			found = realpath(path, NULL);
> >  			break;
> >  		}
> 
> That "if x matches y or x matches y' or x' matches y" feels like
> guesswork. Can't we simply call realpath() on both loopinfo.lo_name and
> filename, and compare the two? 

Probably yes. That said there might be some corner cases:

(1) What happens if the backing file of a loopback is deleted - realpath
    can fail with ENOENT.
(2) A path could be longer than PATH_MAX and it would fail with ENAMETOOLONG
    - this is a "problem" with the initial realpath as well, but less so, because
    the user can control that.

I guess the 0 == strcmp(rfilename, loopinfo.lo_name) is not likely to happen, but
in the end, I did not want to think that much about it and go with something that
definitely works as best as possible.

-- 
debian developer - deb.li/jak | jak-linux.org - free software dev
ubuntu core developer                              i speak de, en

Re: [PATCH] kpartx: Improve finding loopback device by file

[Martin Wilck]

On Mon, 2018-02-05 at 11:45 +0100, Julian Andres Klode wrote:
> On Mon, Feb 05, 2018 at 11:32:13AM +0100, Martin Wilck wrote:
> > On Mon, 2018-02-05 at 09:58 +0100, Julian Andres Klode wrote:
> > > C
> > >  		close (fd);
> > >  
> > > -		if (0 == strcmp(filename, loopinfo.lo_name)) {
> > > +		if (0 == strcmp(filename, loopinfo.lo_name) ||
> > > +		    0 == strcmp(rfilename, loopinfo.lo_name) ||
> > > +		    (realpath(loopinfo.lo_name, rloopfilename)
> > > &&
> > > +		     0 == strcmp(rfilename, rloopfilename))) {
> > >  			found = realpath(path, NULL);
> > >  			break;
> > >  		}
> > 
> > That "if x matches y or x matches y' or x' matches y" feels like
> > guesswork. Can't we simply call realpath() on both loopinfo.lo_name
> > and
> > filename, and compare the two? 
> 
> Probably yes. That said there might be some corner cases:
> 
> (1) What happens if the backing file of a loopback is deleted -
> realpath
>     can fail with ENOENT.
> (2) A path could be longer than PATH_MAX and it would fail with
> ENAMETOOLONG
>     - this is a "problem" with the initial realpath as well, but less
> so, because
>     the user can control that.

Both are scenarios in which kpartx would have good reason to fail (with
a meaningful error message). That's better than guessing, IMO.

kpartx' functionality to work with loop file names (as opposed to just
block devices) is a convenience feature anyway. I'd rather be certain
to avoid deleting stuff we aren't supposed to.

Regards
Martin

> 
> I guess the 0 == strcmp(rfilename, loopinfo.lo_name) is not likely to
> happen, but
> in the end, I did not want to think that much about it and go with
> something that
> definitely works as best as possible.
> 

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

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

Re: [PATCH] kpartx: Improve finding loopback device by file

[Julian Andres Klode]

On Mon, Feb 05, 2018 at 02:53:07PM +0100, Martin Wilck wrote:
> On Mon, 2018-02-05 at 11:45 +0100, Julian Andres Klode wrote:
> > On Mon, Feb 05, 2018 at 11:32:13AM +0100, Martin Wilck wrote:
> > > On Mon, 2018-02-05 at 09:58 +0100, Julian Andres Klode wrote:
> > > > C
> > > >  		close (fd);
> > > >  
> > > > -		if (0 == strcmp(filename, loopinfo.lo_name)) {
> > > > +		if (0 == strcmp(filename, loopinfo.lo_name) ||
> > > > +		    0 == strcmp(rfilename, loopinfo.lo_name) ||
> > > > +		    (realpath(loopinfo.lo_name, rloopfilename)
> > > > &&
> > > > +		     0 == strcmp(rfilename, rloopfilename))) {
> > > >  			found = realpath(path, NULL);
> > > >  			break;
> > > >  		}
> > > 
> > > That "if x matches y or x matches y' or x' matches y" feels like
> > > guesswork. Can't we simply call realpath() on both loopinfo.lo_name
> > > and
> > > filename, and compare the two? 
> > 
> > Probably yes. That said there might be some corner cases:
> > 
> > (1) What happens if the backing file of a loopback is deleted -
> > realpath
> >     can fail with ENOENT.
> > (2) A path could be longer than PATH_MAX and it would fail with
> > ENAMETOOLONG
> >     - this is a "problem" with the initial realpath as well, but less
> > so, because
> >     the user can control that.
> 
> Both are scenarios in which kpartx would have good reason to fail (with
> a meaningful error message). That's better than guessing, IMO.

(1) Definitely not. Just because one of your (potentially other) loopback devices
has a deleted file you should not fail. (2) I don't really know.

But the problem here is that you are looking at loopback devices created by
stuff other than kpartx, and they might not be in a "good" state. But you
don't want to abort just because some _other_ loopback device is broken.

-- 
debian developer - deb.li/jak | jak-linux.org - free software dev
ubuntu core developer                              i speak de, en

Re: [PATCH] kpartx: Improve finding loopback device by file

[Martin Wilck]

On Mon, 2018-02-05 at 15:41 +0100, Julian Andres Klode wrote:
> On Mon, Feb 05, 2018 at 02:53:07PM +0100, Martin Wilck wrote:
> > On Mon, 2018-02-05 at 11:45 +0100, Julian Andres Klode wrote:
> > > On Mon, Feb 05, 2018 at 11:32:13AM +0100, Martin Wilck wrote:
> > > > On Mon, 2018-02-05 at 09:58 +0100, Julian Andres Klode wrote:
> > > > > C
> > > > >  		close (fd);
> > > > >  
> > > > > -		if (0 == strcmp(filename, loopinfo.lo_name))
> > > > > {
> > > > > +		if (0 == strcmp(filename, loopinfo.lo_name)
> > > > > ||
> > > > > +		    0 == strcmp(rfilename, loopinfo.lo_name)
> > > > > ||
> > > > > +		    (realpath(loopinfo.lo_name,
> > > > > rloopfilename)
> > > > > &&
> > > > > +		     0 == strcmp(rfilename, rloopfilename)))
> > > > > {
> > > > >  			found = realpath(path, NULL);
> > > > >  			break;
> > > > >  		}
> > > > 
> > > > That "if x matches y or x matches y' or x' matches y" feels
> > > > like
> > > > guesswork. Can't we simply call realpath() on both
> > > > loopinfo.lo_name
> > > > and
> > > > filename, and compare the two? 
> > > 
> > > Probably yes. That said there might be some corner cases:
> > > 
> > > (1) What happens if the backing file of a loopback is deleted -
> > > realpath
> > >     can fail with ENOENT.
> > > (2) A path could be longer than PATH_MAX and it would fail with
> > > ENAMETOOLONG
> > >     - this is a "problem" with the initial realpath as well, but
> > > less
> > > so, because
> > >     the user can control that.
> > 
> > Both are scenarios in which kpartx would have good reason to fail
> > (with
> > a meaningful error message). That's better than guessing, IMO.
> 
> (1) Definitely not. Just because one of your (potentially other)
> loopback devices
> has a deleted file you should not fail.

Sorry for being imprecise. You're right, aborting here while scanning
for a matching device would of course be wrong. And deleting such a
broken loop device can't be a big mistake.

>  (2) I don't really know.
> 
> But the problem here is that you are looking at loopback devices
> created by
> stuff other than kpartx, and they might not be in a "good" state. But
> you
> don't want to abort just because some _other_ loopback device is
> broken.

Can we agree on the following:

 1 if realpath (filename) results in error, abort
 2 if realpath(lo_name) results in ENODEV and filename matches lo_name,
remove loop device
 3 if realpath(lo_name) results in another error code, skip it
 4 remove if realpath(filename) matches realpath(lo_name)

???

Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

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

Re: [PATCH] kpartx: Improve finding loopback device by file

[Julian Andres Klode]

On Mon, Feb 05, 2018 at 04:12:00PM +0100, Martin Wilck wrote:
> On Mon, 2018-02-05 at 15:41 +0100, Julian Andres Klode wrote:
> > On Mon, Feb 05, 2018 at 02:53:07PM +0100, Martin Wilck wrote:
> > > On Mon, 2018-02-05 at 11:45 +0100, Julian Andres Klode wrote:
> > > > On Mon, Feb 05, 2018 at 11:32:13AM +0100, Martin Wilck wrote:
> > > > > On Mon, 2018-02-05 at 09:58 +0100, Julian Andres Klode wrote:
> > > > > > C
> > > > > >  		close (fd);
> > > > > >  
> > > > > > -		if (0 == strcmp(filename, loopinfo.lo_name))
> > > > > > {
> > > > > > +		if (0 == strcmp(filename, loopinfo.lo_name)
> > > > > > ||
> > > > > > +		    0 == strcmp(rfilename, loopinfo.lo_name)
> > > > > > ||
> > > > > > +		    (realpath(loopinfo.lo_name,
> > > > > > rloopfilename)
> > > > > > &&
> > > > > > +		     0 == strcmp(rfilename, rloopfilename)))
> > > > > > {
> > > > > >  			found = realpath(path, NULL);
> > > > > >  			break;
> > > > > >  		}
> > > > > 
> > > > > That "if x matches y or x matches y' or x' matches y" feels
> > > > > like
> > > > > guesswork. Can't we simply call realpath() on both
> > > > > loopinfo.lo_name
> > > > > and
> > > > > filename, and compare the two? 
> > > > 
> > > > Probably yes. That said there might be some corner cases:
> > > > 
> > > > (1) What happens if the backing file of a loopback is deleted -
> > > > realpath
> > > >     can fail with ENOENT.
> > > > (2) A path could be longer than PATH_MAX and it would fail with
> > > > ENAMETOOLONG
> > > >     - this is a "problem" with the initial realpath as well, but
> > > > less
> > > > so, because
> > > >     the user can control that.
> > > 
> > > Both are scenarios in which kpartx would have good reason to fail
> > > (with
> > > a meaningful error message). That's better than guessing, IMO.
> > 
> > (1) Definitely not. Just because one of your (potentially other)
> > loopback devices
> > has a deleted file you should not fail.
> 
> Sorry for being imprecise. You're right, aborting here while scanning
> for a matching device would of course be wrong. And deleting such a
> broken loop device can't be a big mistake.
> 
> >  (2) I don't really know.
> > 
> > But the problem here is that you are looking at loopback devices
> > created by
> > stuff other than kpartx, and they might not be in a "good" state. But
> > you
> > don't want to abort just because some _other_ loopback device is
> > broken.
> 
> Can we agree on the following:
> 
>  1 if realpath (filename) results in error, abort

OK


>  2 if realpath(lo_name) results in ENODEV and filename matches lo_name,
> remove loop device
>  3 if realpath(lo_name) results in another error code, skip it
>  4 remove if realpath(filename) matches realpath(lo_name)


This seems like a lot of ifs. It might be easier to just path if
realpath(path) fails (as realpath(1) does), something like:

		
		char *loopname = loopinfo.lo_name;
		if (realpath(loopinfo.lo_name, rloopfilename) != NULL)
			loopname = rloopfilename;

		if (0 == strcmp(filename, loopinfo.lo_name) ||
		    0 == strcmp(rfilename, loopname))) {
			found = realpath(path, NULL);
			break;
		}


That should solve all problems and produce useful results with not
a lot of logic.


We could also abstract that away into:

  const char *safe_realpath(const char *path, char *resolved_path)
  {
    char *real = realpath(path, resolved_path);

    return real != NULL ? real : path;
  }


(this has strange behavior compared to realpath, because it does not
 always return resolved_path; I guess, you could strncpy it there, but
 ugh, seems complicated).


-- 
debian developer - deb.li/jak | jak-linux.org - free software dev
ubuntu core developer                              i speak de, en

Re: [PATCH] kpartx: Improve finding loopback device by file

[Martin Wilck]

On Mon, 2018-02-05 at 16:46 +0100, Julian Andres Klode wrote:
> On Mon, Feb 05, 2018 at 04:12:00PM +0100, Martin Wilck wrote:
> > 
> > Can we agree on the following:
> > 
> >  1 if realpath (filename) results in error, abort
> 
> OK
> 
> 
> >  2 if realpath(lo_name) results in ENODEV and filename matches
> > lo_name,
> > remove loop device
> >  3 if realpath(lo_name) results in another error code, skip it
> >  4 remove if realpath(filename) matches realpath(lo_name)
> 
> 
> This seems like a lot of ifs. It might be easier to just path if
> realpath(path) fails (as realpath(1) does), something like:
> 
> 		
> 		char *loopname = loopinfo.lo_name;
> 		if (realpath(loopinfo.lo_name, rloopfilename) != NULL)
> 			loopname = rloopfilename;
> 
> 		if (0 == strcmp(filename, loopinfo.lo_name) ||my
		    0 == strcmp(rfilename, loopname))) {
> 			found = realpath(path, NULL);
> 			break;
> 		}
> 
> 
> That should solve all problems and produce useful results with not
> a lot of logic.

I've reproduced your issue. I can see that the current behavior is
wrong. 

This may be paranoid, but I really want to avoid false positives -
kpartx removing mappings it isn't supposed to remove. Therefore I'd
like to avoid compare by "filename" and "loopname". I think it's
possible.

IMO it would be better to have kpartx use the realpath while *creating*
the partition mapping already:

--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -341,7 +341,7 @@ main(int argc, char **argv){
                if (!loopdev) {
                        loopdev = find_unused_loop_device();
 
-                       if (set_loop(loopdev, device, 0, &ro)) {
+                       if (set_loop(loopdev, rpath, 0, &ro)) {
                                fprintf(stderr, "can't set up loop\n");
                                exit (1);
                        }


That would make sure that kpartx can delete loop devices created by
itself, which is what we want. IMO it's sufficient to solve your issue.

The -ENODEV case is more tricky, your patch doesn't solve it. If you do

kpartx -a /some/image
rm -f /some/image
kpartx -d /some/image

kpartx -d fails, and that's unrelated to the realpath code (it fails in
stat(device) already). Honestly, I don't think we need to support this
scenario. Setting up a loop device and then deleting the backing file
is shooting oneself in the foot.

Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

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

Re: [PATCH] kpartx: Improve finding loopback device by file

[Julian Andres Klode]

On Mon, Feb 05, 2018 at 06:07:13PM +0100, Martin Wilck wrote:
> On Mon, 2018-02-05 at 16:46 +0100, Julian Andres Klode wrote:
> > On Mon, Feb 05, 2018 at 04:12:00PM +0100, Martin Wilck wrote:
> > > 
> > > Can we agree on the following:
> > > 
> > >  1 if realpath (filename) results in error, abort
> > 
> > OK
> > 
> > 
> > >  2 if realpath(lo_name) results in ENODEV and filename matches
> > > lo_name,
> > > remove loop device
> > >  3 if realpath(lo_name) results in another error code, skip it
> > >  4 remove if realpath(filename) matches realpath(lo_name)
> > 
> > 
> > This seems like a lot of ifs. It might be easier to just path if
> > realpath(path) fails (as realpath(1) does), something like:
> > 
> > 		
> > 		char *loopname = loopinfo.lo_name;
> > 		if (realpath(loopinfo.lo_name, rloopfilename) != NULL)
> > 			loopname = rloopfilename;
> > 
> > 		if (0 == strcmp(filename, loopinfo.lo_name) ||my
> 		    0 == strcmp(rfilename, loopname))) {
> > 			found = realpath(path, NULL);
> > 			break;
> > 		}
> > 
> > 
> > That should solve all problems and produce useful results with not
> > a lot of logic.
> 
> I've reproduced your issue. I can see that the current behavior is
> wrong. 
> 
> This may be paranoid, but I really want to avoid false positives -
> kpartx removing mappings it isn't supposed to remove. Therefore I'd
> like to avoid compare by "filename" and "loopname". I think it's
> possible.
> 
> IMO it would be better to have kpartx use the realpath while *creating*
> the partition mapping already:
> 
> --- a/kpartx/kpartx.c
> +++ b/kpartx/kpartx.c
> @@ -341,7 +341,7 @@ main(int argc, char **argv){
>                 if (!loopdev) {
>                         loopdev = find_unused_loop_device();
>  
> -                       if (set_loop(loopdev, device, 0, &ro)) {
> +                       if (set_loop(loopdev, rpath, 0, &ro)) {
>                                 fprintf(stderr, "can't set up loop\n");
>                                 exit (1);
>                         }
> 
> 
> That would make sure that kpartx can delete loop devices created by
> itself, which is what we want. IMO it's sufficient to solve your issue.

I was about to suggest that, but did not do it, as it's kind of weird. You
add /a/b/c.img (pointing to /x/y/z.img) and losetup then shows you /x/y/z.img
- I fear this might break some stuff that checks whether the binding was
actually created (or checks which loop device it was created at).
-- 
debian developer - deb.li/jak | jak-linux.org - free software dev
ubuntu core developer                              i speak de, en

Re: [PATCH] kpartx: Improve finding loopback device by file

[Martin Wilck]

On Tue, 2018-02-06 at 12:19 +0100, Julian Andres Klode wrote:
> On Mon, Feb 05, 2018 at 06:07:13PM +0100, Martin Wilck wrote:
> > On Mon, 2018-02-05 at 16:46 +0100, Julian Andres Klode wrote:
> > > On Mon, Feb 05, 2018 at 04:12:00PM +0100, Martin Wilck wrote:
> > > > 
> > > > Can we agree on the following:
> > > > 
> > > >  1 if realpath (filename) results in error, abort
> > > 
> > > OK
> > > 
> > > 
> > > >  2 if realpath(lo_name) results in ENODEV and filename matches
> > > > lo_name,
> > > > remove loop device
> > > >  3 if realpath(lo_name) results in another error code, skip it
> > > >  4 remove if realpath(filename) matches realpath(lo_name)
> > > 
> > > 
> > > This seems like a lot of ifs. It might be easier to just path if
> > > realpath(path) fails (as realpath(1) does), something like:
> > > 
> > > 		
> > > 		char *loopname = loopinfo.lo_name;
> > > 		if (realpath(loopinfo.lo_name, rloopfilename) != NULL)
> > > 			loopname = rloopfilename;
> > > 
> > > 		if (0 == strcmp(filename, loopinfo.lo_name) ||my
> > 
> > 		    0 == strcmp(rfilename, loopname))) {
> > > 			found = realpath(path, NULL);
> > > 			break;
> > > 		}
> > > 
> > > 
> > > That should solve all problems and produce useful results with
> > > not
> > > a lot of logic.
> > 
> > I've reproduced your issue. I can see that the current behavior is
> > wrong. 
> > 
> > This may be paranoid, but I really want to avoid false positives -
> > kpartx removing mappings it isn't supposed to remove. Therefore I'd
> > like to avoid compare by "filename" and "loopname". I think it's
> > possible.
> > 
> > IMO it would be better to have kpartx use the realpath while
> > *creating*
> > the partition mapping already:
> > 
> > --- a/kpartx/kpartx.c
> > +++ b/kpartx/kpartx.c
> > @@ -341,7 +341,7 @@ main(int argc, char **argv){
> >                 if (!loopdev) {
> >                         loopdev = find_unused_loop_device();
> >  
> > -                       if (set_loop(loopdev, device, 0, &ro)) {
> > +                       if (set_loop(loopdev, rpath, 0, &ro)) {
> >                                 fprintf(stderr, "can't set up
> > loop\n");
> >                                 exit (1);
> >                         }
> > 
> > 
> > That would make sure that kpartx can delete loop devices created by
> > itself, which is what we want. IMO it's sufficient to solve your
> > issue.
> 
> I was about to suggest that, but did not do it, as it's kind of
> weird. You
> add /a/b/c.img (pointing to /x/y/z.img) and losetup then shows you
> /x/y/z.img
> - I fear this might break some stuff that checks whether the binding
> was
> actually created (or checks which loop device it was created at).

IMO this is exactly the right thing to do. In order to identify loop
mappings, you shouldn't rely on something as volatile as arbitrary
symlinks. I'm aware that the "real path" isn't carved in stone either, 
but at least it uniquely identifies a file at a given point in time.
Internally, the loop driver refers to the inode anyway.

The scripts you mention, if they actually exist, would benefit if they
were fixed to work with real paths, too.

Regards
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

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