proc_parse: fix vma file open mode recognition

Submitted by Stanislav Kinsburskiy on June 30, 2016, 4:24 p.m.

Details

Message ID 20160630162433.518367.71030.stgit@skinsbursky-vz7-gold.qa.sw.ru
State Rejected
Series "proc_parse: fix vma file open mode recognition"
Headers show

Commit Message

Stanislav Kinsburskiy June 30, 2016, 4:24 p.m.
The correct place to get fd open flags for file mappings is
/proc/<pid>/map_files.
An attempt tp speculate on "shared" and "maywrite" bits doesn't garantee,
that file will be opened with correct permissions on restore.
Here is an example:

Process mapping (read/write):

# cat /proc/481943/maps | grep 7f7108077000-7f7108078000
7f7108077000-7f7108078000 rw-p 00001000 00:35 7 <snip>

1) Before suspend:

# ls -l /proc/481427/map_files/7f7108077000-7f7108078000
lrw------- <snip> /proc/481427/map_files/7f7108077000-7f7108078000 -> <snip>

2) After restore:

# ls -l /proc/481943/map_files/7f7108077000-7f7108078000
lr-------- <snip> /proc/481943/map_files/7f7108077000-7f7108078000 -> <snip>

Write bit is lost.

This patch set vma->e->fdflags as /proc/<pid>/map_files/<vma> open mode.

Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
---
 criu/proc_parse.c |   51 ++++++++++++++++++++++++++++++++++-----------------
 criu/util.c       |    3 ++-
 2 files changed, 36 insertions(+), 18 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/proc_parse.c b/criu/proc_parse.c
index 5210226..8e36916 100644
--- a/criu/proc_parse.c
+++ b/criu/proc_parse.c
@@ -82,8 +82,6 @@  static bool is_vma_range_fmt(char *line)
 static int parse_vmflags(char *buf, struct vma_area *vma_area)
 {
 	char *tok;
-	bool shared = false;
-	bool maywrite = false;
 
 	if (!buf[0])
 		return 0;
@@ -95,12 +93,6 @@  static int parse_vmflags(char *buf, struct vma_area *vma_area)
 #define _vmflag_match(_t, _s) (_t[0] == _s[0] && _t[1] == _s[1])
 
 	do {
-		/* open() block */
-		if (_vmflag_match(tok, "sh"))
-			shared = true;
-		else if (_vmflag_match(tok, "mw"))
-			maywrite = true;
-
 		/* mmap() block */
 		if (_vmflag_match(tok, "gd"))
 			vma_area->e->flags |= MAP_GROWSDOWN;
@@ -144,12 +136,6 @@  static int parse_vmflags(char *buf, struct vma_area *vma_area)
 
 #undef _vmflag_match
 
-	if (shared && maywrite)
-		vma_area->e->fdflags = O_RDWR;
-	else
-		vma_area->e->fdflags = O_RDONLY;
-	vma_area->e->has_fdflags = true;
-
 	if (vma_area->e->madv)
 		vma_area->e->has_madv = true;
 
@@ -175,12 +161,46 @@  static inline int vfi_equal(struct vma_file_info *a, struct vma_file_info *b)
 			(a->dev_min ^ b->dev_min)) == 0;
 }
 
+static int vma_get_mapfile_flags(struct vma_area *vma, DIR *mfd, char *path)
+{
+	struct stat stat;
+
+	if (fstatat(dirfd(mfd), path, &stat, AT_SYMLINK_NOFOLLOW) < 0) {
+		if (errno == ENOENT) {
+			/* Just mapping w/o map_files link */
+			return 0;
+		}
+		pr_perror("Failed fstatat on map %"PRIx64"", vma->e->start);
+		return -1;
+	}
+
+	switch(stat.st_mode & 0600) {
+		case 0200:
+			vma->e->fdflags = O_WRONLY;
+			break;
+		case 0400:
+			vma->e->fdflags = O_RDONLY;
+			break;
+		case 0600:
+			vma->e->fdflags = O_RDWR;
+			break;
+	}
+	vma->e->has_fdflags = true;
+	return 0;
+}
+
 static int vma_get_mapfile(char *fname, struct vma_area *vma, DIR *mfd,
 		struct vma_file_info *vfi, struct vma_file_info *prev_vfi)
 {
 	char path[32];
 	int flags;
 
+	/* Figure out if it's file mapping */
+	snprintf(path, sizeof(path), "%"PRIx64"-%"PRIx64, vma->e->start, vma->e->end);
+
+	if (vma_get_mapfile_flags(vma, mfd, path))
+		return -1;
+
 	if (prev_vfi->vma && vfi_equal(vfi, prev_vfi)) {
 		struct vma_area *prev = prev_vfi->vma;
 
@@ -209,9 +229,6 @@  static int vma_get_mapfile(char *fname, struct vma_area *vma, DIR *mfd,
 		return 0;
 	}
 
-	/* Figure out if it's file mapping */
-	snprintf(path, sizeof(path), "%"PRIx64"-%"PRIx64, vma->e->start, vma->e->end);
-
 	/*
 	 * Note that we "open" it in dumper process space
 	 * so later we might refer to it via /proc/self/fd/vm_file_fd
diff --git a/criu/util.c b/criu/util.c
index e8ebe61..07bc16e 100644
--- a/criu/util.c
+++ b/criu/util.c
@@ -161,12 +161,13 @@  void pr_vma(unsigned int loglevel, const struct vma_area *vma_area)
 		return;
 
 	vma_opt_str(vma_area, opt);
-	print_on_level(loglevel, "%#"PRIx64"-%#"PRIx64" (%"PRIi64"K) prot %#x flags %#x st %#x off %#"PRIx64" "
+	print_on_level(loglevel, "%#"PRIx64"-%#"PRIx64" (%"PRIi64"K) prot %#x flags %#x fdflags %#o st %#x off %#"PRIx64" "
 			"%s shmid: %#"PRIx64"\n",
 			vma_area->e->start, vma_area->e->end,
 			KBYTES(vma_area_len(vma_area)),
 			vma_area->e->prot,
 			vma_area->e->flags,
+			vma_area->e->fdflags,
 			vma_area->e->status,
 			vma_area->e->pgoff,
 			opt, vma_area->e->shmid);

Comments

Pavel Emelianov July 4, 2016, 6:07 p.m.
On 06/30/2016 07:24 PM, Stanislav Kinsburskiy wrote:
> The correct place to get fd open flags for file mappings is
> /proc/<pid>/map_files.
> An attempt tp speculate on "shared" and "maywrite" bits doesn't garantee,
> that file will be opened with correct permissions on restore.
> Here is an example:
> 
> Process mapping (read/write):
> 
> # cat /proc/481943/maps | grep 7f7108077000-7f7108078000
> 7f7108077000-7f7108078000 rw-p 00001000 00:35 7 <snip>
> 
> 1) Before suspend:
> 
> # ls -l /proc/481427/map_files/7f7108077000-7f7108078000
> lrw------- <snip> /proc/481427/map_files/7f7108077000-7f7108078000 -> <snip>
> 
> 2) After restore:
> 
> # ls -l /proc/481943/map_files/7f7108077000-7f7108078000
> lr-------- <snip> /proc/481943/map_files/7f7108077000-7f7108078000 -> <snip>
> 
> Write bit is lost.
> 
> This patch set vma->e->fdflags as /proc/<pid>/map_files/<vma> open mode.
> 
> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
> ---
>  criu/proc_parse.c |   51 ++++++++++++++++++++++++++++++++++-----------------
>  criu/util.c       |    3 ++-
>  2 files changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/criu/proc_parse.c b/criu/proc_parse.c
> index 5210226..8e36916 100644
> --- a/criu/proc_parse.c
> +++ b/criu/proc_parse.c
> @@ -82,8 +82,6 @@ static bool is_vma_range_fmt(char *line)
>  static int parse_vmflags(char *buf, struct vma_area *vma_area)
>  {
>  	char *tok;
> -	bool shared = false;
> -	bool maywrite = false;
>  
>  	if (!buf[0])
>  		return 0;
> @@ -95,12 +93,6 @@ static int parse_vmflags(char *buf, struct vma_area *vma_area)
>  #define _vmflag_match(_t, _s) (_t[0] == _s[0] && _t[1] == _s[1])
>  
>  	do {
> -		/* open() block */
> -		if (_vmflag_match(tok, "sh"))
> -			shared = true;
> -		else if (_vmflag_match(tok, "mw"))
> -			maywrite = true;
> -
>  		/* mmap() block */
>  		if (_vmflag_match(tok, "gd"))
>  			vma_area->e->flags |= MAP_GROWSDOWN;
> @@ -144,12 +136,6 @@ static int parse_vmflags(char *buf, struct vma_area *vma_area)
>  
>  #undef _vmflag_match
>  
> -	if (shared && maywrite)
> -		vma_area->e->fdflags = O_RDWR;
> -	else
> -		vma_area->e->fdflags = O_RDONLY;
> -	vma_area->e->has_fdflags = true;
> -
>  	if (vma_area->e->madv)
>  		vma_area->e->has_madv = true;
>  
> @@ -175,12 +161,46 @@ static inline int vfi_equal(struct vma_file_info *a, struct vma_file_info *b)
>  			(a->dev_min ^ b->dev_min)) == 0;
>  }
>  
> +static int vma_get_mapfile_flags(struct vma_area *vma, DIR *mfd, char *path)
> +{
> +	struct stat stat;
> +
> +	if (fstatat(dirfd(mfd), path, &stat, AT_SYMLINK_NOFOLLOW) < 0) {
> +		if (errno == ENOENT) {
> +			/* Just mapping w/o map_files link */
> +			return 0;
> +		}
> +		pr_perror("Failed fstatat on map %"PRIx64"", vma->e->start);
> +		return -1;
> +	}
> +
> +	switch(stat.st_mode & 0600) {
> +		case 0200:
> +			vma->e->fdflags = O_WRONLY;
> +			break;
> +		case 0400:
> +			vma->e->fdflags = O_RDONLY;
> +			break;
> +		case 0600:
> +			vma->e->fdflags = O_RDWR;
> +			break;
> +	}
> +	vma->e->has_fdflags = true;
> +	return 0;
> +}
> +
>  static int vma_get_mapfile(char *fname, struct vma_area *vma, DIR *mfd,
>  		struct vma_file_info *vfi, struct vma_file_info *prev_vfi)
>  {
>  	char path[32];
>  	int flags;
>  
> +	/* Figure out if it's file mapping */
> +	snprintf(path, sizeof(path), "%"PRIx64"-%"PRIx64, vma->e->start, vma->e->end);
> +
> +	if (vma_get_mapfile_flags(vma, mfd, path))
> +		return -1;

Can we re-use the vmfd borrowing logic for fdflags too?

> +
>  	if (prev_vfi->vma && vfi_equal(vfi, prev_vfi)) {
>  		struct vma_area *prev = prev_vfi->vma;
>  
> @@ -209,9 +229,6 @@ static int vma_get_mapfile(char *fname, struct vma_area *vma, DIR *mfd,
>  		return 0;
>  	}
>  
> -	/* Figure out if it's file mapping */
> -	snprintf(path, sizeof(path), "%"PRIx64"-%"PRIx64, vma->e->start, vma->e->end);
> -
>  	/*
>  	 * Note that we "open" it in dumper process space
>  	 * so later we might refer to it via /proc/self/fd/vm_file_fd
> diff --git a/criu/util.c b/criu/util.c
> index e8ebe61..07bc16e 100644
> --- a/criu/util.c
> +++ b/criu/util.c
> @@ -161,12 +161,13 @@ void pr_vma(unsigned int loglevel, const struct vma_area *vma_area)
>  		return;
>  
>  	vma_opt_str(vma_area, opt);
> -	print_on_level(loglevel, "%#"PRIx64"-%#"PRIx64" (%"PRIi64"K) prot %#x flags %#x st %#x off %#"PRIx64" "
> +	print_on_level(loglevel, "%#"PRIx64"-%#"PRIx64" (%"PRIi64"K) prot %#x flags %#x fdflags %#o st %#x off %#"PRIx64" "
>  			"%s shmid: %#"PRIx64"\n",
>  			vma_area->e->start, vma_area->e->end,
>  			KBYTES(vma_area_len(vma_area)),
>  			vma_area->e->prot,
>  			vma_area->e->flags,
> +			vma_area->e->fdflags,
>  			vma_area->e->status,
>  			vma_area->e->pgoff,
>  			opt, vma_area->e->shmid);
> 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
> .
>
Andrey Vagin July 7, 2016, 1:10 a.m.
On Thu, Jun 30, 2016 at 07:24:45PM +0300, Stanislav Kinsburskiy wrote:
> The correct place to get fd open flags for file mappings is
> /proc/<pid>/map_files.
> An attempt tp speculate on "shared" and "maywrite" bits doesn't garantee,
> that file will be opened with correct permissions on restore.
> Here is an example:
> 
> Process mapping (read/write):
> 
> # cat /proc/481943/maps | grep 7f7108077000-7f7108078000
> 7f7108077000-7f7108078000 rw-p 00001000 00:35 7 <snip>
> 
> 1) Before suspend:
> 
> # ls -l /proc/481427/map_files/7f7108077000-7f7108078000
> lrw------- <snip> /proc/481427/map_files/7f7108077000-7f7108078000 -> <snip>
> 
> 2) After restore:
> 
> # ls -l /proc/481943/map_files/7f7108077000-7f7108078000
> lr-------- <snip> /proc/481943/map_files/7f7108077000-7f7108078000 -> <snip>
> 
> Write bit is lost.

Could you add this check into zdtm.py?

> 
> This patch set vma->e->fdflags as /proc/<pid>/map_files/<vma> open mode.
> 
> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
> ---
>  criu/proc_parse.c |   51 ++++++++++++++++++++++++++++++++++-----------------
>  criu/util.c       |    3 ++-
>  2 files changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/criu/proc_parse.c b/criu/proc_parse.c
> index 5210226..8e36916 100644
> --- a/criu/proc_parse.c
> +++ b/criu/proc_parse.c
> @@ -82,8 +82,6 @@ static bool is_vma_range_fmt(char *line)
>  static int parse_vmflags(char *buf, struct vma_area *vma_area)
>  {
>  	char *tok;
> -	bool shared = false;
> -	bool maywrite = false;
>  
>  	if (!buf[0])
>  		return 0;
> @@ -95,12 +93,6 @@ static int parse_vmflags(char *buf, struct vma_area *vma_area)
>  #define _vmflag_match(_t, _s) (_t[0] == _s[0] && _t[1] == _s[1])
>  
>  	do {
> -		/* open() block */
> -		if (_vmflag_match(tok, "sh"))
> -			shared = true;
> -		else if (_vmflag_match(tok, "mw"))
> -			maywrite = true;
> -
>  		/* mmap() block */
>  		if (_vmflag_match(tok, "gd"))
>  			vma_area->e->flags |= MAP_GROWSDOWN;
> @@ -144,12 +136,6 @@ static int parse_vmflags(char *buf, struct vma_area *vma_area)
>  
>  #undef _vmflag_match
>  
> -	if (shared && maywrite)
> -		vma_area->e->fdflags = O_RDWR;
> -	else
> -		vma_area->e->fdflags = O_RDONLY;
> -	vma_area->e->has_fdflags = true;
> -
>  	if (vma_area->e->madv)
>  		vma_area->e->has_madv = true;
>  
> @@ -175,12 +161,46 @@ static inline int vfi_equal(struct vma_file_info *a, struct vma_file_info *b)
>  			(a->dev_min ^ b->dev_min)) == 0;
>  }
>  
> +static int vma_get_mapfile_flags(struct vma_area *vma, DIR *mfd, char *path)
> +{
> +	struct stat stat;
> +
> +	if (fstatat(dirfd(mfd), path, &stat, AT_SYMLINK_NOFOLLOW) < 0) {
> +		if (errno == ENOENT) {
> +			/* Just mapping w/o map_files link */
> +			return 0;
> +		}
> +		pr_perror("Failed fstatat on map %"PRIx64"", vma->e->start);
> +		return -1;
> +	}
> +
> +	switch(stat.st_mode & 0600) {
> +		case 0200:
> +			vma->e->fdflags = O_WRONLY;
> +			break;
> +		case 0400:
> +			vma->e->fdflags = O_RDONLY;
> +			break;
> +		case 0600:
> +			vma->e->fdflags = O_RDWR;
> +			break;
> +	}
> +	vma->e->has_fdflags = true;
> +	return 0;
> +}
> +
>  static int vma_get_mapfile(char *fname, struct vma_area *vma, DIR *mfd,
>  		struct vma_file_info *vfi, struct vma_file_info *prev_vfi)
>  {
>  	char path[32];
>  	int flags;
>  
> +	/* Figure out if it's file mapping */
> +	snprintf(path, sizeof(path), "%"PRIx64"-%"PRIx64, vma->e->start, vma->e->end);
> +
> +	if (vma_get_mapfile_flags(vma, mfd, path))
> +		return -1;
> +
>  	if (prev_vfi->vma && vfi_equal(vfi, prev_vfi)) {
>  		struct vma_area *prev = prev_vfi->vma;
>  
> @@ -209,9 +229,6 @@ static int vma_get_mapfile(char *fname, struct vma_area *vma, DIR *mfd,
>  		return 0;
>  	}
>  
> -	/* Figure out if it's file mapping */
> -	snprintf(path, sizeof(path), "%"PRIx64"-%"PRIx64, vma->e->start, vma->e->end);
> -
>  	/*
>  	 * Note that we "open" it in dumper process space
>  	 * so later we might refer to it via /proc/self/fd/vm_file_fd
> diff --git a/criu/util.c b/criu/util.c
> index e8ebe61..07bc16e 100644
> --- a/criu/util.c
> +++ b/criu/util.c
> @@ -161,12 +161,13 @@ void pr_vma(unsigned int loglevel, const struct vma_area *vma_area)
>  		return;
>  
>  	vma_opt_str(vma_area, opt);
> -	print_on_level(loglevel, "%#"PRIx64"-%#"PRIx64" (%"PRIi64"K) prot %#x flags %#x st %#x off %#"PRIx64" "
> +	print_on_level(loglevel, "%#"PRIx64"-%#"PRIx64" (%"PRIi64"K) prot %#x flags %#x fdflags %#o st %#x off %#"PRIx64" "
>  			"%s shmid: %#"PRIx64"\n",
>  			vma_area->e->start, vma_area->e->end,
>  			KBYTES(vma_area_len(vma_area)),
>  			vma_area->e->prot,
>  			vma_area->e->flags,
> +			vma_area->e->fdflags,
>  			vma_area->e->status,
>  			vma_area->e->pgoff,
>  			opt, vma_area->e->shmid);
> 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
Stanislav Kinsburskiy July 7, 2016, 9:43 a.m.
07.07.2016 03:10, Andrew Vagin пишет:
> On Thu, Jun 30, 2016 at 07:24:45PM +0300, Stanislav Kinsburskiy wrote:
>> The correct place to get fd open flags for file mappings is
>> /proc/<pid>/map_files.
>> An attempt tp speculate on "shared" and "maywrite" bits doesn't garantee,
>> that file will be opened with correct permissions on restore.
>> Here is an example:
>>
>> Process mapping (read/write):
>>
>> # cat /proc/481943/maps | grep 7f7108077000-7f7108078000
>> 7f7108077000-7f7108078000 rw-p 00001000 00:35 7 <snip>
>>
>> 1) Before suspend:
>>
>> # ls -l /proc/481427/map_files/7f7108077000-7f7108078000
>> lrw------- <snip> /proc/481427/map_files/7f7108077000-7f7108078000 -> <snip>
>>
>> 2) After restore:
>>
>> # ls -l /proc/481943/map_files/7f7108077000-7f7108078000
>> lr-------- <snip> /proc/481943/map_files/7f7108077000-7f7108078000 -> <snip>
>>
>> Write bit is lost.
> Could you add this check into zdtm.py?
>

I'm sorry, but I have no idea where and how to do this.
Stanislav Kinsburskiy July 13, 2016, 1:11 p.m.
10.07.2016 20:47, Andrew Vagin пишет:
> On Sun, Jul 10, 2016 at 10:10:33AM -0700, Andrew Vagin wrote:
>> On Sun, Jul 10, 2016 at 08:33:34AM -0700, Andrew Vagin wrote:
>>> On Thu, Jul 07, 2016 at 11:43:59AM +0200, Stanislav Kinsburskiy wrote:
>>>>
>>>> 07.07.2016 03:10, Andrew Vagin пишет:
>>>>> On Thu, Jun 30, 2016 at 07:24:45PM +0300, Stanislav Kinsburskiy wrote:
>>>>>> The correct place to get fd open flags for file mappings is
>>>>>> /proc/<pid>/map_files.
>>>>>> An attempt tp speculate on "shared" and "maywrite" bits doesn't garantee,
>>>>>> that file will be opened with correct permissions on restore.
>>>>>> Here is an example:
>>>>>>
>>>>>> Process mapping (read/write):
>>>>>>
>>>>>> # cat /proc/481943/maps | grep 7f7108077000-7f7108078000
>>>>>> 7f7108077000-7f7108078000 rw-p 00001000 00:35 7 <snip>
>>>>>>
>>>>>> 1) Before suspend:
>>>>>>
>>>>>> # ls -l /proc/481427/map_files/7f7108077000-7f7108078000
>>>>>> lrw------- <snip> /proc/481427/map_files/7f7108077000-7f7108078000 -> <snip>
>>>>>>
>>>>>> 2) After restore:
>>>>>>
>>>>>> # ls -l /proc/481943/map_files/7f7108077000-7f7108078000
>>>>>> lr-------- <snip> /proc/481943/map_files/7f7108077000-7f7108078000 -> <snip>
>>>>>>
>>>>>> Write bit is lost.
>>>>> Could you add this check into zdtm.py?
>>>>>
>>>> I'm sorry, but I have no idea where and how to do this.
>>>>
>>> I'm sorry, but we accept patches only with tests;).
>>>
>>> You need to add this check into get_visible_state() and
>>> check_visible_state().
>> I think it should be something like this:
>>
>> diff --git a/test/zdtm.py b/test/zdtm.py
>> index 7072b62..23559fd 100755
>> --- a/test/zdtm.py
>> +++ b/test/zdtm.py
>> @@ -899,6 +899,12 @@ def get_visible_state(test):
>>                  last = 0
>>                  for mp in open("/proc/%s/root/proc/%s/maps" % (test.getpid(), pid)):
>>                          m = map(lambda x: int('0x' + x, 0), mp.split()[0].split('-'))
>> +
>> +                        f = "/proc/%s/root/proc/%s/map_files/%s" % (test.getpid(), pid, mp.split()[0])
>> +                        if os.access(f, os.F_OK):
>> +                                st = os.stat(f)
>> +                                m.append(st.st_mode)
>> +
>>                          if cmaps[last][1] == m[0]:
>>                                  cmaps[last][1] = m[1]
>>                          else:
>                                  cmaps.append(m)
>                                  last += 1
> -               maps[pid] = set(map(lambda x: '%x-%x' % (x[0], x[1]), cmaps))
> +               maps[pid] = set(map(lambda x: '%x-%x %s' % (x[0], x[1], x[2:]), cmaps))
>
> But even with this fix it doesn't catch your problem and I can't reproduce it by hand.

Please, try static/unlink_mmap00 test.

>> but zdtm.py doesn't detect any errors with this patch,
>> so you need to fix it to detect your issue.
>> _______________________________________________
>> CRIU mailing list
>> CRIU@openvz.org
>> https://lists.openvz.org/mailman/listinfo/criu
Pavel Emelianov July 13, 2016, 1:43 p.m.
Applied