| 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 |
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);
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 > . >
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
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.
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
Applied
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(-)