Fix dumping of /proc folder

Submitted by Andrei Tuicu on June 28, 2016, 7:02 a.m.

Details

Message ID 1467097342-6667-1-git-send-email-andrei.tuicu@gmail.com
State Rejected
Series "Fix dumping of /proc folder"
Headers show

Commit Message

Andrei Tuicu June 28, 2016, 7:02 a.m.
CRIU fails to dump processes that have a file descriptor pointing
to the /proc folder, because check_path_remap returns error code
in that case. Fix: return 0 instead.

Signed-off-by: Andrei Tuicu <andrei.tuicu@gmail.com>
---
 criu/files-reg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/files-reg.c b/criu/files-reg.c
index 899dcf6..0fcef6d 100644
--- a/criu/files-reg.c
+++ b/criu/files-reg.c
@@ -975,8 +975,8 @@  static int check_path_remap(struct fd_link *link, const struct fd_parms *parms,
 		if (!start)
 			return -1;
 		start = strstr(start + 1, "/");
-		if (!start)
-			return -1;
+		if (!start) /* it's /proc */
+			return 0;
 		pid = strtol(start + 1, &end, 10);
 
 		/* If strtol didn't convert anything, then we are looking at

Comments

Cyrill Gorcunov June 28, 2016, 7:49 a.m.
On Tue, Jun 28, 2016 at 10:02:22AM +0300, Andrei Tuicu wrote:
> CRIU fails to dump processes that have a file descriptor pointing
> to the /proc folder, because check_path_remap returns error code
> in that case. Fix: return 0 instead.
> 
> Signed-off-by: Andrei Tuicu <andrei.tuicu@gmail.com>
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/criu/files-reg.c b/criu/files-reg.c
> index 899dcf6..0fcef6d 100644
> --- a/criu/files-reg.c
> +++ b/criu/files-reg.c
> @@ -975,8 +975,8 @@ static int check_path_remap(struct fd_link *link, const struct fd_parms *parms,
>  		if (!start)
>  			return -1;
>  		start = strstr(start + 1, "/");
> -		if (!start)
> -			return -1;
> +		if (!start) /* it's /proc */
> +			return 0;
>  		pid = strtol(start + 1, &end, 10);
>  
>  		/* If strtol didn't convert anything, then we are looking at

What if the process sitting in some mountnamespace with procfs mounted inside,
then instead of exiting early we've to fallthrough and check for ns root, ie
down to line mntns_root = mntns_get_root_fd(nsid);, if only I'm not missing
somthing obvious. avagin@?
Andrei Tuicu June 28, 2016, 8:30 a.m.
Hi, Cyrill,

I did not go that far... I looked at the other lines that handle files
under /proc and I didn't see anything special being done about those. The
only problem is that criu returns error code for the /proc folder itself,
due to what I think is a minor bug. As it is now, if a process does
fopen("/proc", "r"), criu will fail to dump it and I found that a little
odd since it has no problem with files/folders that are under /proc.
If I understand correctly what you are saying, then procfs would not be
mounted in /proc, but instead in some /path/to/proc, as seen by the
process, right? In that case then the code might need other patches to
handle that.

Thank you,
Andrei

2016-06-28 10:49 GMT+03:00 Cyrill Gorcunov <gorcunov@gmail.com>:

> On Tue, Jun 28, 2016 at 10:02:22AM +0300, Andrei Tuicu wrote:
> > CRIU fails to dump processes that have a file descriptor pointing
> > to the /proc folder, because check_path_remap returns error code
> > in that case. Fix: return 0 instead.
> >
> > Signed-off-by: Andrei Tuicu <andrei.tuicu@gmail.com>
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/criu/files-reg.c b/criu/files-reg.c
> > index 899dcf6..0fcef6d 100644
> > --- a/criu/files-reg.c
> > +++ b/criu/files-reg.c
> > @@ -975,8 +975,8 @@ static int check_path_remap(struct fd_link *link,
> const struct fd_parms *parms,
> >               if (!start)
> >                       return -1;
> >               start = strstr(start + 1, "/");
> > -             if (!start)
> > -                     return -1;
> > +             if (!start) /* it's /proc */
> > +                     return 0;
> >               pid = strtol(start + 1, &end, 10);
> >
> >               /* If strtol didn't convert anything, then we are looking
> at
>
> What if the process sitting in some mountnamespace with procfs mounted
> inside,
> then instead of exiting early we've to fallthrough and check for ns root,
> ie
> down to line mntns_root = mntns_get_root_fd(nsid);, if only I'm not missing
> somthing obvious. avagin@?
>
Cyrill Gorcunov June 28, 2016, 8:51 a.m.
On Tue, Jun 28, 2016 at 11:30:21AM +0300, Andrei Tuicu wrote:
>    Hi, Cyrill,
>    I did not go that far... I looked at the other lines that handle files
>    under /proc and I didn't see anything special being done about those. The
>    only problem is that criu returns error code for the /proc folder itself,
>    due to what I think is a minor bug. As it is now, if a process does
>    fopen("/proc", "r"), criu will fail to dump it and I found that a little
>    odd since it has no problem with files/folders that are under /proc.
>    If I understand correctly what you are saying, then procfs would not be
>    mounted in /proc, but instead in some /path/to/proc, as seen by the
>    process, right? In that case then the code might need other patches to
>    handle that.

I meant the case where completely new file tree created (say inside container)
and such processes lives in own /, /proc and etc.

Actually after thinking more I think the patch is correct. Thank you!
But please make more verbose changelog.
Andrei Tuicu June 28, 2016, 11:32 a.m.
Resent it with more verbose changelog. Thanks for the review! :)

Andrei

2016-06-28 11:51 GMT+03:00 Cyrill Gorcunov <gorcunov@gmail.com>:

> On Tue, Jun 28, 2016 at 11:30:21AM +0300, Andrei Tuicu wrote:
> >    Hi, Cyrill,
> >    I did not go that far... I looked at the other lines that handle files
> >    under /proc and I didn't see anything special being done about those.
> The
> >    only problem is that criu returns error code for the /proc folder
> itself,
> >    due to what I think is a minor bug. As it is now, if a process does
> >    fopen("/proc", "r"), criu will fail to dump it and I found that a
> little
> >    odd since it has no problem with files/folders that are under /proc.
> >    If I understand correctly what you are saying, then procfs would not
> be
> >    mounted in /proc, but instead in some /path/to/proc, as seen by the
> >    process, right? In that case then the code might need other patches to
> >    handle that.
>
> I meant the case where completely new file tree created (say inside
> container)
> and such processes lives in own /, /proc and etc.
>
> Actually after thinking more I think the patch is correct. Thank you!
> But please make more verbose changelog.
>
Pavel Emelianov June 28, 2016, 1:34 p.m.
Wow :) Applied, thanks!