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

[Andrew Vagin]

On Fri, Feb 17, 2017 at 05:05:45PM +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.

An error isn't a problem because we skip it and try to open via another
mount.

Can you show where it hangs for fifo?


> 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
> 
> Signed-off-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>
> ---
>  criu/fsnotify.c      |  4 +++-
>  criu/include/mount.h |  4 +++-
>  criu/mount.c         | 41 ++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 44 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..5195c3e 100644
> --- a/criu/include/mount.h
> +++ b/criu/include/mount.h
> @@ -30,6 +30,7 @@ struct mount_info {
>  	 */
>  	char			*mountpoint;
>  	char			*ns_mountpoint;
> +	int			isdir;
>  	int			fd;
>  	unsigned		flags;
>  	unsigned		sb_flags;
> @@ -87,6 +88,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 +99,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 8f66b4e..7f0b4b0 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)
>  {
>  	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))
> +			continue;
>  		if (m->s_dev == s_dev)
>  			return m;
> +	}
>  
>  	return NULL;
>  }
> @@ -957,6 +960,38 @@ static struct mount_info *mnt_build_tree(struct mount_info *list,
>  	return tree;
>  }
>  
> +typedef enum {
> +	MNT_UNDEF	= 0,
> +	MNT_DIR		= 1,
> +	MNT_NOT_DIR	= 2,
> +} mountpoint_type;
> +
> +int notdir_mountpoint(struct mount_info *pm)
> +{
> +	int mntns_root;
> +	struct stat st;
> +
> +	if (pm->isdir == MNT_UNDEF) {
> +		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;

0 means MNT_DIR
> +		}
> +
> +		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))
> +			pm->isdir = MNT_DIR;
> +		else
> +			pm->isdir = MNT_NOT_DIR;
> +	}
> +
> +	return pm->isdir - 1;
> +}
> +
>  /*
>   * 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 +1052,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);

lookup_mnt_sdev(s_dev, MNT_DIR) - looks better

>  	if (!m)
>  		return -ENOENT;
>  
> -- 
> 2.9.3
>