[CRIU] [PATCH v2] fsnotify: skip non-direcory mounts

[Andrew Vagin]

On Thu, Mar 02, 2017 at 03:43:09PM +0300, Pavel Tikhomirov wrote:
> To restore fsnotify's watches on files we need to find paths for each
> of them using handle we have in /proc/<pid>/fdinfo/<fsnotifyfd>.
> These handle is valid to open the file with open_by_handle_at if
> you have mount fd where the file lays. So we try open_by_handle_at
> for all possible mount fds we have.
> 
> But we can not do so for 'file' bind-mounts, as the way we open
> mount fd opens file instead and can hang on fifos or fail on sockets.
> (see check_open_handle->open_handle->open_mount code path, imagine
> lookup_mnt_sdev() found 'file' bind-mount, open_mount() failed(hanged)
> in __open_mountpoint() and if irmap_lookup() also was not successful the
> whole dump fails too)
> 
> So if we have file bindmount of fifo file, and we restore some
> inotify on other file on other mount with same s_dev, we hang forever
> on open.
> 
> So just skip non-directory mounts from inotify search we will find
> path for them on other mount(e.g. non-bindmount) with same s_dev.
> 
> https://jira.sw.ru/browse/PSBM-57362
> 
> v2: remove isdir hashing, improve commit message
> Signed-off-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>
> ---
>  criu/fsnotify.c      |  4 +++-
>  criu/include/mount.h |  3 ++-
>  criu/mount.c         | 30 +++++++++++++++++++++++++++---
>  3 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/criu/fsnotify.c b/criu/fsnotify.c
> index 0dcb07c..08fd56d 100644
> --- a/criu/fsnotify.c
> +++ b/criu/fsnotify.c
> @@ -132,6 +132,8 @@ static char *alloc_openable(unsigned int s_dev, unsigned long i_ino, FhEntry *f_
>  
>  		if (m->s_dev != s_dev)
>  			continue;
> +		if (notdir_mountpoint(m))
> +			continue;
>  
>  		mntfd = __open_mountpoint(m, -1);
>  		pr_debug("\t\tTrying via mntid %d root %s ns_mountpoint @%s (%d)\n",
> @@ -238,7 +240,7 @@ int check_open_handle(unsigned int s_dev, unsigned long i_ino,
>  
>  		pr_debug("\tHandle 0x%x:0x%lx is openable\n", s_dev, i_ino);
>  
> -		mi = lookup_mnt_sdev(s_dev);
> +		mi = lookup_mnt_sdev(s_dev, 1);
>  		if (mi == NULL) {
>  			pr_err("Unable to lookup a mount by dev 0x%x\n", s_dev);
>  			goto err;
> diff --git a/criu/include/mount.h b/criu/include/mount.h
> index a692b55..9b27237 100644
> --- a/criu/include/mount.h
> +++ b/criu/include/mount.h
> @@ -87,6 +87,7 @@ extern struct ns_id *lookup_nsid_by_mnt_id(int mnt_id);
>  
>  extern int open_mount(unsigned int s_dev);
>  extern int __open_mountpoint(struct mount_info *pm, int mnt_fd);
> +extern int notdir_mountpoint(struct mount_info *pm);
>  extern int open_mountpoint(struct mount_info *pm);
>  
>  extern struct mount_info *collect_mntinfo(struct ns_id *ns, bool for_dump);
> @@ -97,7 +98,7 @@ extern int pivot_root(const char *new_root, const char *put_old);
>  extern struct mount_info *lookup_overlayfs(char *rpath, unsigned int s_dev,
>  					   unsigned int st_ino, unsigned int mnt_id);
>  extern struct mount_info *lookup_mnt_id(unsigned int id);
> -extern struct mount_info *lookup_mnt_sdev(unsigned int s_dev);
> +extern struct mount_info *lookup_mnt_sdev(unsigned int s_dev, bool only_dir);
>  
>  extern dev_t phys_stat_resolve_dev(struct ns_id *, dev_t st_dev, const char *path);
>  extern bool phys_stat_dev_match(dev_t st_dev, dev_t phys_dev,
> diff --git a/criu/mount.c b/criu/mount.c
> index 2973d27..b1a16ea 100644
> --- a/criu/mount.c
> +++ b/criu/mount.c
> @@ -223,13 +223,16 @@ struct mount_info *lookup_mnt_id(unsigned int id)
>  	return __lookup_mnt_id(mntinfo, id);
>  }
>  
> -struct mount_info *lookup_mnt_sdev(unsigned int s_dev)
> +struct mount_info *lookup_mnt_sdev(unsigned int s_dev, bool only_dir)

Nobody uses this function to open a file mounts, so I don't think that
we need this extra argument now. I suggest to add a comment why we
seatch only non-files mounts and dont' check arguments.
>  {
>  	struct mount_info *m;
>  
> -	for (m = mntinfo; m != NULL; m = m->next)
> +	for (m = mntinfo; m != NULL; m = m->next) {
> +		if (only_dir && notdir_mountpoint(m))

this check has to be the after the next one, becase it call system
calls.

> +			continue;
>  		if (m->s_dev == s_dev)
>  			return m;
> +	}
>  
>  	return NULL;
>  }
> @@ -957,6 +960,27 @@ static struct mount_info *mnt_build_tree(struct mount_info *list,
>  	return tree;
>  }
>  
> +int notdir_mountpoint(struct mount_info *pm)

bool mnt_is_dir(struct mount_info *pm)

> +{
> +	int mntns_root;
> +	struct stat st;
> +
> +	mntns_root = mntns_get_root_fd(pm->nsid);
> +	if (mntns_root < 0) {
> +		pr_perror("Can't get root fd of mntns for %d", pm->mnt_id);
> +		return 0;
> +	}
> +
> +	if (fstatat(mntns_root, pm->ns_mountpoint, &st, 0)) {
> +		pr_perror("Can't fstatat on %s", pm->ns_mountpoint);
> +		return 0;
> +	}
> +
> +	if (!S_ISDIR(st.st_mode))
> +		return 1;
> +	return 0;
> +}
> +
>  /*
>   * mnt_fd is a file descriptor on the mountpoint, which is closed in an error case.
>   * If mnt_fd is -1, the mountpoint will be opened by this function.
> @@ -1017,7 +1041,7 @@ int open_mount(unsigned int s_dev)
>  {
>  	struct mount_info *m;
>  
> -	m = lookup_mnt_sdev(s_dev);
> +	m = lookup_mnt_sdev(s_dev, 1);
>  	if (!m)
>  		return -ENOENT;
>  
> -- 
> 2.9.3
>