fsnotify: skip non-direcory mounts

Submitted by Pavel Tikhomirov on Feb. 17, 2017, 2:05 p.m.

Details

Message ID 20170217140545.11418-1-ptikhomirov@virtuozzo.com
State New
Series "fsnotify: skip non-direcory mounts"
Headers show

Commit Message

Pavel Tikhomirov Feb. 17, 2017, 2:05 p.m.
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.
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@virtuozzo.com>
---
 criu/fsnotify.c      |  4 +++-
 criu/include/mount.h |  4 +++-
 criu/mount.c         | 41 ++++++++++++++++++++++++++++++++++++++---
 3 files changed, 44 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

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;
+		}
+
+		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);
 	if (!m)
 		return -ENOENT;
 

Comments

Andrey Vagin Feb. 21, 2017, 9:38 p.m.
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@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
>