[CRIU] [PATCH 2/2 v4] fsnotify: Always provide the path for inotify watchees

[Pavel Emelyanov]

On 10/19/2015 01:29 PM, Cyrill Gorcunov wrote:
> On Mon, Oct 19, 2015 at 01:23:21PM +0300, Pavel Emelyanov wrote:
>>>
>>> Yeah, exactly!
>>
>> OK. Can we then split common path from tmpfs-path then? Like -- do regular
>> open_handle(), then (if opened) get the fstype and IFF it's tmpfs go on a
>> loop to find the accessible path?
>>
>> Another issue I've found while re-reading the patch:
>>
>>> +		openable_fd = openat(mntfd, __path, O_PATH);
>>> +		if (openable_fd >= 0) {
>>> +			close(openable_fd);
>>> +
>>> +			pr_debug("\t\t\topenable as %s\n", __path);
>>> +			path = xstrdup(buf);
>>>  			if (path == NULL)
>>>  				goto err;
>>>  
>>>  			f_handle->has_mnt_id = true;
>>> -			f_handle->mnt_id = mi->mnt_id;
>>> -
>>> +			f_handle->mnt_id = m->mnt_id;
>>>  			goto out;
>>> -		}
>>> -
>>> -		if (!opts.force_irmap)
>>> -			/*
>>> -			 * If we're not forced to do irmap, then
>>> -			 * say we have no path for watch. Otherwise
>>> -			 * do irmap scan even if the handle is
>>> -			 * working.
>>> -			 *
>>> -			 * FIXME -- no need to open-by-handle if
>>> -			 * we are in force-irmap and not on tempfs
>>> -			 */
>>> -			goto out_nopath;
>>> +		} else
>>> +			pr_debug("\t\t\tnot openable as %s (%m)\n", __path);
>>>  	}
>>
>> After you've managed to open something by some name you should check that
>> the file opened is the same as referenced by fd from open_by_handle().
> 
> Well, I'll try to split it but I fear this would be more messy. Simply take
> a look not on the diff but the complete function instead
> 
> int check_open_handle(unsigned int s_dev, unsigned long i_ino,
> 		FhEntry *f_handle)
> {
> 	struct mount_info *m;
> 	fh_t handle;
> 	int fd = -1;
> 	char *path;
> 
> 	decode_handle(&handle, f_handle);
> 
> 	/*
> 	 * We gonna try to open the handle and then
> 	 * depending on command line options and type
> 	 * of the filesystem (tmpfs/devtmpfs do not
> 	 * preserve their inodes between mounts) we
> 	 * might need to find out an openable path
> 	 * get used on restore as a watch destination.
> 	 */
> 	for (m = mntinfo; m; m = m->next) {
> 		char buf[PATH_MAX], *__path;
> 		int mntfd, openable_fd;
> 
> 		if (m->s_dev != s_dev)
> 			continue;
> 
> 		mntfd = __open_mountpoint(m, -1);
> 		pr_debug("\t\tTrying via mntid %d root %s ns_mountpoint @%s (%d)\n",
> 			 m->mnt_id, m->root, m->ns_mountpoint, mntfd);
> 		if (mntfd < 0)
> 			goto cant_open;
> 
> 		fd = userns_call(open_by_handle, UNS_FDOUT, &handle,
> 				 sizeof(handle), mntfd);
> 		close(mntfd);
> 		if (fd < 0)
> 			goto cant_open;
> 
> 		/*
> 		 * On tmpfs/devtmps we have to always fetch
> 		 * openable path, in turn on all others
> 		 * it depends on command line option: if
> 		 * we're requested to use irmap lets fetch
> 		 * the path, otherwise simply save the bare
> 		 * handler and that's it.
> 		 */
> 		if ((m->fstype->code != FSTYPE__TMPFS) &&
> 		    (m->fstype->code != FSTYPE__DEVTMPFS)) {
> 			if (!opts.force_irmap)
> 				goto out_nopath;
> 			else
> 				goto force_irmap;
> 		}
> 
> 		if (read_fd_link(fd, buf, sizeof(buf)) < 0) {
> 			close_safe(&fd);
> 			goto err;
> 		}
> 		close_safe(&fd);
> 
> 		/*
> 		 * Convert into a relative path.
> 		 */
> 		__path = (buf[1] != '\0') ? buf + 1 : ".";
> 		pr_debug("\t\t\tlink as %s\n", __path);
> 
> 		mntfd = mntns_get_root_by_mnt_id(m->mnt_id);
> 		if (mntfd < 0)
> 			goto err;
> 
> 		openable_fd = openat(mntfd, __path, O_PATH);
> 		if (openable_fd >= 0) {
> 			close(openable_fd);
> 
> 			pr_debug("\t\t\topenable as %s\n", __path);
> 			path = xstrdup(buf);
> 			if (path == NULL)
> 				goto err;
> 
> 			f_handle->has_mnt_id = true;
> 			f_handle->mnt_id = m->mnt_id;
> 			goto out;
> 		} else
> 			pr_debug("\t\t\tnot openable as %s (%m)\n", __path);
> 	}
> 
> cant_open:
> 	pr_warn("\tHandle 0x%x:0x%lx cannot be opened\n", s_dev, i_ino);
> force_irmap:
> 	path = irmap_lookup(s_dev, i_ino);
> 	if (!path) {
> 		pr_err("\tCan't dump that handle\n");
> 		return -1;
> 	}
> out:
> 	pr_debug("\tDumping %s as path for handle\n", path);
> 	f_handle->path = path;
> out_nopath:
> 	close_safe(&fd);
> 	return 0;
> err:
> 	close_safe(&fd);
> 	return -1;
> }
> 
> As to path check -- we don't care which path we obtain,
> the inode referenced by the path is the same.

Where does the guarantee for that come from? I can agree with devices
being the same, OK (since you look up the mountpoint by device), but
the inodes match MUST be there, as you open some derived path at 
different location.

-- Pavel