[dm-devel] [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 at 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