perf tools: Restore proper cwd on return from mnt ns

PATCH Applied Jiri Olsa 4 participants 11 messages Nov 1, 2018 – Nov 21, 2018 perf events

Share & tools

Jiri Olsa

author reviewer

2018-11-01 17:00 UTC #0 lore raw

PATCH 1/1 perf tools: Restore proper cwd on return from mnt ns

When reporting on 'record' server we try to retrieve/use the mnt namespace of the profiled tasks. We use following API with cookie to hold the return namespace, roughly:

nsinfo__mountns_enter(struct nsinfo *nsi, struct nscookie *nc)

    setns(newns, 0);
  ...
  new ns related open..
  ...
  nsinfo__mountns_exit(struct nscookie *nc)
    setns(nc->oldns)

Once finished we setns to old namespace, which also sets the current working directory (cwd) to "/", trashing the cwd we had.

This is mostly fine, because we use absolute paths almost everywhere, but it screws up perf diff:

  # perf diff
  failed to open perf.data: No such file or directory  (try 'perf record' first)
  ...

Adding the current working directory to be part of the cookie and restoring it in the nsinfo__mountns_exit call.

Cc Krister Johansen <[email protected]>

Fixes 843ff37bb59e ("perf symbols: Find symbols in different mount namespace")

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/namespaces.c | 18 ++++++++++++++++--
 tools/perf/util/namespaces.h |  1 +
 2 files changed, 17 insertions(+), 2 deletions(-)

tools/perf/util/namespaces.c +16 -2

index cf8bd123cf73..fb0458b7e6aa 100644
@@ -18,6 +18,7 @@
 #include <stdio.h>
 #include <string.h>
 #include <unistd.h>
+#include <asm/bug.h>
 
 struct namespaces *namespaces__new(struct namespaces_event *event)
 {

@@ -186,6 +187,7 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	char curpath[PATH_MAX];
 	int oldns = -1;
 	int newns = -1;
+	char *oldcwd = NULL;
 
 	if (nc == NULL)
 		return;

@@ -199,9 +201,13 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (snprintf(curpath, PATH_MAX, "/proc/self/ns/mnt") >= PATH_MAX)
 		return;
 
+	oldcwd = get_current_dir_name();
+	if (!oldcwd)
+		return;
+
 	oldns = open(curpath, O_RDONLY);
 	if (oldns < 0)
-		return;
+		goto errout;
 
 	newns = open(nsi->mntns_path, O_RDONLY);
 	if (newns < 0)

@@ -210,11 +216,14 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (setns(newns, CLONE_NEWNS) < 0)
 		goto errout;
 
+	nc->oldcwd = oldcwd;
 	nc->oldns = oldns;
 	nc->newns = newns;
 	return;
 
 errout:
+	if (oldcwd)
+		free(oldcwd);
 	if (oldns > -1)
 		close(oldns);
 	if (newns > -1)

@@ -223,11 +232,16 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 
 void nsinfo__mountns_exit(struct nscookie *nc)
 {
-	if (nc == NULL || nc->oldns == -1 || nc->newns == -1)
+	if (nc == NULL || nc->oldns == -1 || nc->newns == -1 || !nc->oldcwd)
 		return;
 
 	setns(nc->oldns, CLONE_NEWNS);
 
+	if (nc->oldcwd) {
+		WARN_ON_ONCE(chdir(nc->oldcwd));
+		free(nc->oldcwd);
+	}
+
 	if (nc->oldns > -1) {
 		close(nc->oldns);
 		nc->oldns = -1;

tools/perf/util/namespaces.h +1 -0

index cae1a9a39722..d5f46c09ea31 100644
@@ -38,6 +38,7 @@ struct nsinfo {
 struct nscookie {
 	int			oldns;
 	int			newns;
+	char			*oldcwd;
 };
 
 int nsinfo__init(struct nsinfo *nsi);

Signature · 1 line

2.17.2

Jiri Olsa

2018-11-16 13:03 UTC #1 lore raw

ping

jirka

Quoted 117 lines · Jiri Olsa

When reporting on 'record' server we try to retrieve/use the mnt namespace of the profiled tasks. We use following API with cookie to hold the return namespace, roughly:

nsinfo__mountns_enter(struct nsinfo *nsi, struct nscookie *nc)

    setns(newns, 0);
  ...
  new ns related open..
  ...
  nsinfo__mountns_exit(struct nscookie *nc)
    setns(nc->oldns)

Once finished we setns to old namespace, which also sets the current working directory (cwd) to "/", trashing the cwd we had.

This is mostly fine, because we use absolute paths almost everywhere, but it screws up perf diff:

  # perf diff
  failed to open perf.data: No such file or directory  (try 'perf record' first)
  ...

Adding the current working directory to be part of the cookie and restoring it in the nsinfo__mountns_exit call.

Cc Krister Johansen <[email protected]>

Fixes 843ff37bb59e ("perf symbols: Find symbols in different mount namespace")

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/namespaces.c | 18 ++++++++++++++++--
 tools/perf/util/namespaces.h |  1 +
 2 files changed, 17 insertions(+), 2 deletions(-)

tools/perf/util/namespaces.c +16 -2

index cf8bd123cf73..fb0458b7e6aa 100644
@@ -18,6 +18,7 @@
 #include <stdio.h>
 #include <string.h>
 #include <unistd.h>
+#include <asm/bug.h>
 
 struct namespaces *namespaces__new(struct namespaces_event *event)
 {

@@ -186,6 +187,7 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	char curpath[PATH_MAX];
 	int oldns = -1;
 	int newns = -1;
+	char *oldcwd = NULL;
 
 	if (nc == NULL)
 		return;

@@ -199,9 +201,13 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (snprintf(curpath, PATH_MAX, "/proc/self/ns/mnt") >= PATH_MAX)
 		return;
 
+	oldcwd = get_current_dir_name();
+	if (!oldcwd)
+		return;
+
 	oldns = open(curpath, O_RDONLY);
 	if (oldns < 0)
-		return;
+		goto errout;
 
 	newns = open(nsi->mntns_path, O_RDONLY);
 	if (newns < 0)

@@ -210,11 +216,14 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (setns(newns, CLONE_NEWNS) < 0)
 		goto errout;
 
+	nc->oldcwd = oldcwd;
 	nc->oldns = oldns;
 	nc->newns = newns;
 	return;
 
 errout:
+	if (oldcwd)
+		free(oldcwd);
 	if (oldns > -1)
 		close(oldns);
 	if (newns > -1)

@@ -223,11 +232,16 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 
 void nsinfo__mountns_exit(struct nscookie *nc)
 {
-	if (nc == NULL || nc->oldns == -1 || nc->newns == -1)
+	if (nc == NULL || nc->oldns == -1 || nc->newns == -1 || !nc->oldcwd)
 		return;
 
 	setns(nc->oldns, CLONE_NEWNS);
 
+	if (nc->oldcwd) {
+		WARN_ON_ONCE(chdir(nc->oldcwd));
+		free(nc->oldcwd);
+	}
+
 	if (nc->oldns > -1) {
 		close(nc->oldns);
 		nc->oldns = -1;

tools/perf/util/namespaces.h +1 -0

index cae1a9a39722..d5f46c09ea31 100644
@@ -38,6 +38,7 @@ struct nsinfo {
 struct nscookie {
 	int			oldns;
 	int			newns;
+	char			*oldcwd;
 };
 
 int nsinfo__init(struct nsinfo *nsi);

Signature · 1 line

2.17.2

Arnaldo Carvalho de Melo

maintainer Applied

2018-11-16 18:31 UTC #2 lore raw

In reply to Jiri Olsa

Em Thu, Nov 01, 2018 at 06:00:01PM +0100, Jiri Olsa escreveu:

Quoted 71 lines

When reporting on 'record' server we try to retrieve/use the mnt namespace of the profiled tasks. We use following API with cookie to hold the return namespace, roughly:
nsinfo__mountns_enter(struct nsinfo *nsi, struct nscookie *nc)
    setns(newns, 0);
  ...
  new ns related open..
  ...
  nsinfo__mountns_exit(struct nscookie *nc)
    setns(nc->oldns)
Once finished we setns to old namespace, which also sets the current working directory (cwd) to "/", trashing the cwd we had.
This is mostly fine, because we use absolute paths almost everywhere, but it screws up perf diff:
  # perf diff
  failed to open perf.data: No such file or directory  (try 'perf record' first)
  ...
Adding the current working directory to be part of the cookie and restoring it in the nsinfo__mountns_exit call.
Cc Krister Johansen <[email protected]>
Fixes 843ff37bb59e ("perf symbols: Find symbols in different mount namespace")
Link http://lkml.kernel.org/n/[email protected] open here
Signed-off-by Jiri Olsa <[email protected]>
---
 tools/perf/util/namespaces.c | 18 ++++++++++++++++--
 tools/perf/util/namespaces.h |  1 +
 2 files changed, 17 insertions(+), 2 deletions(-)
tools/perf/util/namespaces.c +7 -1
index cf8bd123cf73..fb0458b7e6aa 100644
@@ -18,6 +18,7 @@
 #include <stdio.h>
 #include <string.h>
 #include <unistd.h>
+#include <asm/bug.h>
 
 struct namespaces *namespaces__new(struct namespaces_event *event)
 {
@@ -186,6 +187,7 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	char curpath[PATH_MAX];
 	int oldns = -1;
 	int newns = -1;
+	char *oldcwd = NULL;
 
 	if (nc == NULL)
 		return;
@@ -199,9 +201,13 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (snprintf(curpath, PATH_MAX, "/proc/self/ns/mnt") >= PATH_MAX)
 		return;
 
+	oldcwd = get_current_dir_name();
+	if (!oldcwd)
+		return;
+
 	oldns = open(curpath, O_RDONLY);
 	if (oldns < 0)
-		return;
+		goto errout;
 
 	newns = open(nsi->mntns_path, O_RDONLY);
 	if (newns < 0)
@@ -210,11 +216,14 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (setns(newns, CLONE_NEWNS) < 0)

tools/perf/util/namespaces.c void nsinfo__mountns_enter(struct nsinfo *nsi, · line 226

Quoted 10 lines

tools/perf/util/namespaces.c +3 -0

 		goto errout;
 
+	nc->oldcwd = oldcwd;
 	nc->oldns = oldns;
 	nc->newns = newns;
 	return;
 
 errout:
+	if (oldcwd)
+		free(oldcwd);

Applied, and while at it I removed the needless if in the above two lines.

Quoted 35 lines

tools/perf/util/namespaces.c +6 -1

 	if (oldns > -1)
 		close(oldns);
 	if (newns > -1)

@@ -223,11 +232,16 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 
 void nsinfo__mountns_exit(struct nscookie *nc)
 {
-	if (nc == NULL || nc->oldns == -1 || nc->newns == -1)
+	if (nc == NULL || nc->oldns == -1 || nc->newns == -1 || !nc->oldcwd)
 		return;
 
 	setns(nc->oldns, CLONE_NEWNS);
 
+	if (nc->oldcwd) {
+		WARN_ON_ONCE(chdir(nc->oldcwd));
+		free(nc->oldcwd);
+	}
+
 	if (nc->oldns > -1) {
 		close(nc->oldns);
 		nc->oldns = -1;

tools/perf/util/namespaces.h +1 -0

index cae1a9a39722..d5f46c09ea31 100644
@@ -38,6 +38,7 @@ struct nsinfo {
 struct nscookie {
 	int			oldns;
 	int			newns;
+	char			*oldcwd;
 };
 
 int nsinfo__init(struct nsinfo *nsi);

Signature · 1 line

2.17.2

Arnaldo Carvalho de Melo

maintainer

2018-11-16 18:42 UTC #3 lore raw

Em Fri, Nov 16, 2018 at 10:31:43AM -0800, Arnaldo Carvalho de Melo escreveu:

Quoted 97 lines

Em Thu, Nov 01, 2018 at 06:00:01PM +0100, Jiri Olsa escreveu:
Quoted 71 lines
When reporting on 'record' server we try to retrieve/use the mnt namespace of the profiled tasks. We use following API with cookie to hold the return namespace, roughly:
nsinfo__mountns_enter(struct nsinfo *nsi, struct nscookie *nc)
    setns(newns, 0);
  ...
  new ns related open..
  ...
  nsinfo__mountns_exit(struct nscookie *nc)
    setns(nc->oldns)
Once finished we setns to old namespace, which also sets the current working directory (cwd) to "/", trashing the cwd we had.
This is mostly fine, because we use absolute paths almost everywhere, but it screws up perf diff:
  # perf diff
  failed to open perf.data: No such file or directory  (try 'perf record' first)
  ...
Adding the current working directory to be part of the cookie and restoring it in the nsinfo__mountns_exit call.
Cc Krister Johansen <[email protected]>
Fixes 843ff37bb59e ("perf symbols: Find symbols in different mount namespace")
Link http://lkml.kernel.org/n/[email protected] open here
Signed-off-by Jiri Olsa <[email protected]>
---
 tools/perf/util/namespaces.c | 18 ++++++++++++++++--
 tools/perf/util/namespaces.h |  1 +
 2 files changed, 17 insertions(+), 2 deletions(-)
tools/perf/util/namespaces.c +7 -1
index cf8bd123cf73..fb0458b7e6aa 100644
@@ -18,6 +18,7 @@
 #include <stdio.h>
 #include <string.h>
 #include <unistd.h>
+#include <asm/bug.h>
 
 struct namespaces *namespaces__new(struct namespaces_event *event)
 {
@@ -186,6 +187,7 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	char curpath[PATH_MAX];
 	int oldns = -1;
 	int newns = -1;
+	char *oldcwd = NULL;
 
 	if (nc == NULL)
 		return;
@@ -199,9 +201,13 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (snprintf(curpath, PATH_MAX, "/proc/self/ns/mnt") >= PATH_MAX)
 		return;
 
+	oldcwd = get_current_dir_name();
+	if (!oldcwd)
+		return;
+
 	oldns = open(curpath, O_RDONLY);
 	if (oldns < 0)
-		return;
+		goto errout;
 
 	newns = open(nsi->mntns_path, O_RDONLY);
 	if (newns < 0)
@@ -210,11 +216,14 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (setns(newns, CLONE_NEWNS) < 0)
tools/perf/util/namespaces.c void nsinfo__mountns_enter(struct nsinfo *nsi, · line 226
Quoted 10 lines
tools/perf/util/namespaces.c +3 -0
 		goto errout;
 
+	nc->oldcwd = oldcwd;
 	nc->oldns = oldns;
 	nc->newns = newns;
 	return;
 
 errout:
+	if (oldcwd)
+		free(oldcwd);
Applied, and while at it I removed the needless if in the above two lines.
Quoted 11 lines
tools/perf/util/namespaces.c +1 -1
 	if (oldns > -1)
 		close(oldns);
 	if (newns > -1)
@@ -223,11 +232,16 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 
 void nsinfo__mountns_exit(struct nscookie *nc)
 {
-	if (nc == NULL || nc->oldns == -1 || nc->newns == -1)
+	if (nc == NULL || nc->oldns == -1 || nc->newns == -1 || !nc->oldcwd)
 		return;
 

Quoted 7 lines

tools/perf/util/namespaces.c void nsinfo__mountns_enter(struct nsinfo *nsi, · line 244
Quoted 7 lines
tools/perf/util/namespaces.c +5 -0
 	setns(nc->oldns, CLONE_NEWNS);
 
+	if (nc->oldcwd) {
+		WARN_ON_ONCE(chdir(nc->oldcwd));
+		free(nc->oldcwd);
+	}
+

Also changed the above to zfree(&nc->oldcwd) as this is in a struct member, that is not freed by the freeing caller (nsinfo__mountns_exit()), to avoid later possible use-after-free (I haven't checked if it _actually_ happens with the current codebase, but it could) also to match the following block, where the resource is released and then the handler is invalidated.

- Arnaldo

Quoted 3 lines

Quoted 3 lines
tools/perf/util/namespaces.c +0 -0
 	if (nc->oldns > -1) {
 		close(nc->oldns);
 		nc->oldns = -1;

Jiri Olsa

2018-11-17 09:00 UTC #4 lore raw

Quoted 75 lines · Arnaldo Carvalho de Melo

Em Fri, Nov 16, 2018 at 10:31:43AM -0800, Arnaldo Carvalho de Melo escreveu:
Quoted 74 lines
Em Thu, Nov 01, 2018 at 06:00:01PM +0100, Jiri Olsa escreveu:
Quoted 73 lines
When reporting on 'record' server we try to retrieve/use the mnt namespace of the profiled tasks. We use following API with cookie to hold the return namespace, roughly:
nsinfo__mountns_enter(struct nsinfo *nsi, struct nscookie *nc)
    setns(newns, 0);
  ...
  new ns related open..
  ...
  nsinfo__mountns_exit(struct nscookie *nc)
    setns(nc->oldns)
Once finished we setns to old namespace, which also sets the current working directory (cwd) to "/", trashing the cwd we had.
This is mostly fine, because we use absolute paths almost everywhere, but it screws up perf diff:
  # perf diff
  failed to open perf.data: No such file or directory  (try 'perf record' first)
  ...
Adding the current working directory to be part of the cookie and restoring it in the nsinfo__mountns_exit call.
Cc Krister Johansen <[email protected]>
Fixes 843ff37bb59e ("perf symbols: Find symbols in different mount namespace")
Link http://lkml.kernel.org/n/[email protected] open here
Signed-off-by Jiri Olsa <[email protected]>
---
 tools/perf/util/namespaces.c | 18 ++++++++++++++++--
 tools/perf/util/namespaces.h |  1 +
 2 files changed, 17 insertions(+), 2 deletions(-)
tools/perf/util/namespaces.c +7 -1
index cf8bd123cf73..fb0458b7e6aa 100644
@@ -18,6 +18,7 @@
 #include <stdio.h>
 #include <string.h>
 #include <unistd.h>
+#include <asm/bug.h>
 
 struct namespaces *namespaces__new(struct namespaces_event *event)
 {
@@ -186,6 +187,7 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	char curpath[PATH_MAX];
 	int oldns = -1;
 	int newns = -1;
+	char *oldcwd = NULL;
 
 	if (nc == NULL)
 		return;
@@ -199,9 +201,13 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (snprintf(curpath, PATH_MAX, "/proc/self/ns/mnt") >= PATH_MAX)
 		return;
 
+	oldcwd = get_current_dir_name();
+	if (!oldcwd)
+		return;
+
 	oldns = open(curpath, O_RDONLY);
 	if (oldns < 0)
-		return;
+		goto errout;
 
 	newns = open(nsi->mntns_path, O_RDONLY);
 	if (newns < 0)
@@ -210,11 +216,14 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (setns(newns, CLONE_NEWNS) < 0)
 		goto errout;
 

Quoted 8 lines

Quoted 8 lines
tools/perf/util/namespaces.c void nsinfo__mountns_enter(struct nsinfo *nsi, · line 226
Quoted 8 lines
tools/perf/util/namespaces.c +3 -0
+	nc->oldcwd = oldcwd;
 	nc->oldns = oldns;
 	nc->newns = newns;
 	return;
 
 errout:
+	if (oldcwd)
+		free(oldcwd);

right, ok

Quoted 17 lines

Applied, and while at it I removed the needless if in the above two lines.

Quoted 13 lines

tools/perf/util/namespaces.c +1 -1

 	if (oldns > -1)
 		close(oldns);
 	if (newns > -1)

@@ -223,11 +232,16 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 
 void nsinfo__mountns_exit(struct nscookie *nc)
 {
-	if (nc == NULL || nc->oldns == -1 || nc->newns == -1)
+	if (nc == NULL || nc->oldns == -1 || nc->newns == -1 || !nc->oldcwd)
 		return;
 
 	setns(nc->oldns, CLONE_NEWNS);

Quoted 12 lines

Quoted 5 lines
tools/perf/util/namespaces.c void nsinfo__mountns_enter(struct nsinfo *nsi, · line 244
Quoted 5 lines
tools/perf/util/namespaces.c +5 -0
+	if (nc->oldcwd) {
+		WARN_ON_ONCE(chdir(nc->oldcwd));
+		free(nc->oldcwd);
+	}
+
Also changed the above to zfree(&nc->oldcwd) as this is in a struct member, that is not freed by the freeing caller (nsinfo__mountns_exit()), to avoid later possible use-after-free (I haven't checked if it _actually_ happens with the current codebase, but it could) also to match the following block, where the resource is released and then the handler is invalidated.

great, thanks

jirka

Arnaldo Carvalho de Melo

maintainer

2018-11-19 16:10 UTC #5 lore raw

In reply to Jiri Olsa

Em Thu, Nov 01, 2018 at 06:00:01PM +0100, Jiri Olsa escreveu:

tools/perf/util/namespaces.c void nsinfo__mountns_enter(struct nsinfo *nsi, · line 204

Quoted 13 lines

Adding the current working directory to be part of the cookie and restoring it in the nsinfo__mountns_exit call.
tools/perf/util/namespaces.c +1 -0
index cf8bd123cf73..fb0458b7e6aa 100644
@@ -199,9 +201,13 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (snprintf(curpath, PATH_MAX, "/proc/self/ns/mnt") >= PATH_MAX)
 		return;
 
+	oldcwd = get_current_dir_name();

Breaks the build with bionic (Android), where get_current_dir_name() is not available:

/tmp/build/perf/libperf.a(libperf-in.o):hist.c:function nsinfo__mountns_enter: error: undefined reference to 'get_current_dir_name'
collect2: error: ld returned 1 exit status
Makefile.perf:534: recipe for target '/tmp/build/perf/perf' failed
make[2]: *** [/tmp/build/perf/perf] Error 1
Makefile.perf:206: recipe for target 'sub-make' failed
make[1]: *** [sub-make] Error 2
Makefile:69: recipe for target 'all' failed
make: *** [all] Error 2
make: Leaving directory '/git/linux/tools/perf'
[root@seventh ~]#

The container builds are ongoing, its possoble that this becomes an issue in uCLibc, wasn't on musl libc (Alpine Linux).

- Arnaldo

Jiri Olsa

2018-11-20 10:56 UTC #6 lore raw

Quoted 29 lines · Arnaldo Carvalho de Melo

Em Thu, Nov 01, 2018 at 06:00:01PM +0100, Jiri Olsa escreveu:
tools/perf/util/namespaces.c void nsinfo__mountns_enter(struct nsinfo *nsi, · line 204
Quoted 13 lines
Adding the current working directory to be part of the cookie and restoring it in the nsinfo__mountns_exit call.
tools/perf/util/namespaces.c +1 -0
index cf8bd123cf73..fb0458b7e6aa 100644
@@ -199,9 +201,13 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (snprintf(curpath, PATH_MAX, "/proc/self/ns/mnt") >= PATH_MAX)
 		return;
 
+	oldcwd = get_current_dir_name();
Breaks the build with bionic (Android), where get_current_dir_name() is not available:
/tmp/build/perf/libperf.a(libperf-in.o):hist.c:function nsinfo__mountns_enter: error: undefined reference to 'get_current_dir_name'
collect2: error: ld returned 1 exit status
Makefile.perf:534: recipe for target '/tmp/build/perf/perf' failed
make[2]: *** [/tmp/build/perf/perf] Error 1
Makefile.perf:206: recipe for target 'sub-make' failed
make[1]: *** [sub-make] Error 2
Makefile:69: recipe for target 'all' failed
make: *** [all] Error 2
make: Leaving directory '/git/linux/tools/perf'
[root@seventh ~]#

Quoted 2 lines

The container builds are ongoing, its possoble that this becomes an issue in uCLibc, wasn't on musl libc (Alpine Linux).

will check, thanks

jirka

Jiri Olsa

2018-11-20 11:24 UTC #7 lore raw

PATCH v2 1/1 perf tools: Restore proper cwd on return from mnt ns

Quoted 30 lines · Jiri Olsa

Quoted 29 lines · Arnaldo Carvalho de Melo
Em Thu, Nov 01, 2018 at 06:00:01PM +0100, Jiri Olsa escreveu:
tools/perf/util/namespaces.c void nsinfo__mountns_enter(struct nsinfo *nsi, · line 204
Quoted 13 lines
Adding the current working directory to be part of the cookie and restoring it in the nsinfo__mountns_exit call.
tools/perf/util/namespaces.c +1 -0
index cf8bd123cf73..fb0458b7e6aa 100644
@@ -199,9 +201,13 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (snprintf(curpath, PATH_MAX, "/proc/self/ns/mnt") >= PATH_MAX)
 		return;
 
+	oldcwd = get_current_dir_name();
Breaks the build with bionic (Android), where get_current_dir_name() is not available:
/tmp/build/perf/libperf.a(libperf-in.o):hist.c:function nsinfo__mountns_enter: error: undefined reference to 'get_current_dir_name'
collect2: error: ld returned 1 exit status
Makefile.perf:534: recipe for target '/tmp/build/perf/perf' failed
make[2]: *** [/tmp/build/perf/perf] Error 1
Makefile.perf:206: recipe for target 'sub-make' failed
make[1]: *** [sub-make] Error 2
Makefile:69: recipe for target 'all' failed
make: *** [all] Error 2
make: Leaving directory '/git/linux/tools/perf'
[root@seventh ~]#

Quoted 4 lines

Quoted 2 lines
The container builds are ongoing, its possoble that this becomes an issue in uCLibc, wasn't on musl libc (Alpine Linux).
will check, thanks

how about this one?

thanks,
jirka

---
When reporting on 'record' server we try to retrieve/use the mnt namespace of the profiled tasks. We use following API with cookie to hold the return namespace, roughly:

nsinfo__mountns_enter(struct nsinfo *nsi, struct nscookie *nc)

    setns(newns, 0);
  ...
  new ns related open..
  ...
  nsinfo__mountns_exit(struct nscookie *nc)
    setns(nc->oldns)

Once finished we setns to old namespace, which also sets the current working directory (cwd) to "/", trashing the cwd we had.

This is mostly fine, because we use absolute paths almost everywhere, but it screws up perf diff:

  # perf diff
  failed to open perf.data: No such file or directory  (try 'perf record' first)
  ...

Adding the current working directory to be part of the cookie and restoring it in the nsinfo__mountns_exit call.

v2: get rid of _GNU_SOURCE get_current_dir_name call [acme]

Cc Krister Johansen <[email protected]>

Fixes 843ff37bb59e ("perf symbols: Find symbols in different mount namespace")

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/namespaces.c | 25 +++++++++++++++++++++++--
 tools/perf/util/namespaces.h |  1 +
 2 files changed, 24 insertions(+), 2 deletions(-)

tools/perf/util/namespaces.c +23 -2

index cf8bd123cf73..b759a7ad87e7 100644
@@ -18,6 +18,7 @@
 #include <stdio.h>
 #include <string.h>
 #include <unistd.h>
+#include <asm/bug.h>
 
 struct namespaces *namespaces__new(struct namespaces_event *event)
 {

@@ -180,12 +181,20 @@ void nsinfo__put(struct nsinfo *nsi)
 		nsinfo__delete(nsi);
 }
 
+static char *get_current_dir(void)
+{
+	char path[PATH_MAX];
+
+	return getcwd(path, PATH_MAX) ? strdup(path) : NULL;
+}
+
 void nsinfo__mountns_enter(struct nsinfo *nsi,
 				  struct nscookie *nc)
 {
 	char curpath[PATH_MAX];
 	int oldns = -1;
 	int newns = -1;
+	char *oldcwd = NULL;
 
 	if (nc == NULL)
 		return;

@@ -199,9 +208,13 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (snprintf(curpath, PATH_MAX, "/proc/self/ns/mnt") >= PATH_MAX)
 		return;
 
+	oldcwd = get_current_dir();
+	if (!oldcwd)
+		return;
+
 	oldns = open(curpath, O_RDONLY);
 	if (oldns < 0)
-		return;
+		goto errout;
 
 	newns = open(nsi->mntns_path, O_RDONLY);
 	if (newns < 0)

@@ -210,11 +223,14 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (setns(newns, CLONE_NEWNS) < 0)
 		goto errout;
 
+	nc->oldcwd = oldcwd;
 	nc->oldns = oldns;
 	nc->newns = newns;
 	return;
 
 errout:
+	if (oldcwd)
+		free(oldcwd);
 	if (oldns > -1)
 		close(oldns);
 	if (newns > -1)

@@ -223,11 +239,16 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 
 void nsinfo__mountns_exit(struct nscookie *nc)
 {
-	if (nc == NULL || nc->oldns == -1 || nc->newns == -1)
+	if (nc == NULL || nc->oldns == -1 || nc->newns == -1 || !nc->oldcwd)
 		return;
 
 	setns(nc->oldns, CLONE_NEWNS);
 
+	if (nc->oldcwd) {
+		WARN_ON_ONCE(chdir(nc->oldcwd));
+		free(nc->oldcwd);
+	}
+
 	if (nc->oldns > -1) {
 		close(nc->oldns);
 		nc->oldns = -1;

tools/perf/util/namespaces.h +1 -0

index cae1a9a39722..d5f46c09ea31 100644
@@ -38,6 +38,7 @@ struct nsinfo {
 struct nscookie {
 	int			oldns;
 	int			newns;
+	char			*oldcwd;
 };
 
 int nsinfo__init(struct nsinfo *nsi);

Signature · 1 line

2.17.2

Arnaldo Carvalho de Melo

maintainer

2018-11-20 15:56 UTC #8 lore raw

In reply to Jiri Olsa

Em Tue, Nov 20, 2018 at 11:56:18AM +0100, Jiri Olsa escreveu:

Quoted 30 lines · Arnaldo Carvalho de Melo

Quoted 29 lines
Em Thu, Nov 01, 2018 at 06:00:01PM +0100, Jiri Olsa escreveu:
tools/perf/util/namespaces.c void nsinfo__mountns_enter(struct nsinfo *nsi, · line 204
Quoted 13 lines
Adding the current working directory to be part of the cookie and restoring it in the nsinfo__mountns_exit call.
tools/perf/util/namespaces.c +1 -0
index cf8bd123cf73..fb0458b7e6aa 100644
@@ -199,9 +201,13 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (snprintf(curpath, PATH_MAX, "/proc/self/ns/mnt") >= PATH_MAX)
 		return;
 
+	oldcwd = get_current_dir_name();
Breaks the build with bionic (Android), where get_current_dir_name() is not available:
/tmp/build/perf/libperf.a(libperf-in.o):hist.c:function nsinfo__mountns_enter: error: undefined reference to 'get_current_dir_name'
collect2: error: ld returned 1 exit status
Makefile.perf:534: recipe for target '/tmp/build/perf/perf' failed
make[2]: *** [/tmp/build/perf/perf] Error 1
Makefile.perf:206: recipe for target 'sub-make' failed
make[1]: *** [sub-make] Error 2
Makefile:69: recipe for target 'all' failed
make: *** [all] Error 2
make: Leaving directory '/git/linux/tools/perf'
[root@seventh ~]#

Quoted 4 lines

Quoted 2 lines
The container builds are ongoing, its possoble that this becomes an issue in uCLibc, wasn't on musl libc (Alpine Linux).
will check, thanks

I fixed this already, by adding a test for that get_current_dir_name() function and a simple implementation based on strdup(getcwd(bf)), take a look at my perf/urgent branch

- Arnaldo

Jiri Olsa

2018-11-20 17:42 UTC #9 lore raw

Quoted 31 lines · Arnaldo Carvalho de Melo

Em Tue, Nov 20, 2018 at 11:56:18AM +0100, Jiri Olsa escreveu:
Quoted 30 lines · Arnaldo Carvalho de Melo
Quoted 29 lines
Em Thu, Nov 01, 2018 at 06:00:01PM +0100, Jiri Olsa escreveu:
tools/perf/util/namespaces.c void nsinfo__mountns_enter(struct nsinfo *nsi, · line 204
Quoted 13 lines
Adding the current working directory to be part of the cookie and restoring it in the nsinfo__mountns_exit call.
tools/perf/util/namespaces.c +1 -0
index cf8bd123cf73..fb0458b7e6aa 100644
@@ -199,9 +201,13 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (snprintf(curpath, PATH_MAX, "/proc/self/ns/mnt") >= PATH_MAX)
 		return;
 
+	oldcwd = get_current_dir_name();
Breaks the build with bionic (Android), where get_current_dir_name() is not available:
/tmp/build/perf/libperf.a(libperf-in.o):hist.c:function nsinfo__mountns_enter: error: undefined reference to 'get_current_dir_name'
collect2: error: ld returned 1 exit status
Makefile.perf:534: recipe for target '/tmp/build/perf/perf' failed
make[2]: *** [/tmp/build/perf/perf] Error 1
Makefile.perf:206: recipe for target 'sub-make' failed
make[1]: *** [sub-make] Error 2
Makefile:69: recipe for target 'all' failed
make: *** [all] Error 2
make: Leaving directory '/git/linux/tools/perf'
[root@seventh ~]#

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
The container builds are ongoing, its possoble that this becomes an issue in uCLibc, wasn't on musl libc (Alpine Linux).
will check, thanks
I fixed this already, by adding a test for that get_current_dir_name() function and a simple implementation based on strdup(getcwd(bf)), take a look at my perf/urgent branch

ok, I saw it.. but it seems little too much for a single function to me

jirka

tip-bot for Jiri Olsa

merge bot

2018-11-21 15:02 UTC #10 lore raw

[tip:perf/urgent] perf tools: Restore proper cwd on return from mnt namespace

In reply to Jiri Olsa

perf tools: Restore proper cwd on return from mnt namespace

Commit-ID:  b01c1f69c8660eaeab7d365cd570103c5c073a02
Gitweb:     https://git.kernel.org/tip/b01c1f69c8660eaeab7d365cd570103c5c073a02
Author:     Jiri Olsa <[email protected]>
AuthorDate: Thu, 1 Nov 2018 18:00:01 +0100
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Mon, 19 Nov 2018 12:12:26 -0800

perf tools: Restore proper cwd on return from mnt namespace

When reporting on 'record' server we try to retrieve/use the mnt namespace of the profiled tasks. We use following API with cookie to hold the return namespace, roughly:

nsinfo__mountns_enter(struct nsinfo *nsi, struct nscookie *nc)

    setns(newns, 0);
  ...
  new ns related open..
  ...
  nsinfo__mountns_exit(struct nscookie *nc)
    setns(nc->oldns)

Once finished we setns to old namespace, which also sets the current working directory (cwd) to "/", trashing the cwd we had.

This is mostly fine, because we use absolute paths almost everywhere, but it screws up 'perf diff':

  # perf diff
  failed to open perf.data: No such file or directory  (try 'perf record' first)
  ...

Adding the current working directory to be part of the cookie and restoring it in the nsinfo__mountns_exit call.

Signed-off-by Jiri Olsa <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Krister Johansen <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Peter Zijlstra <[email protected]>

Fixes 843ff37bb59e ("perf symbols: Find symbols in different mount namespace")

Link http://lkml.kernel.org/r/[email protected] open here

[ No need to check for NULL args for free(), use zfree() for struct members ]

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/util/namespaces.c | 17 +++++++++++++++--
 tools/perf/util/namespaces.h |  1 +
 2 files changed, 16 insertions(+), 2 deletions(-)

tools/perf/util/namespaces.c +15 -2

index cf8bd123cf73..aed170bd4384 100644
@@ -18,6 +18,7 @@
 #include <stdio.h>
 #include <string.h>
 #include <unistd.h>
+#include <asm/bug.h>
 
 struct namespaces *namespaces__new(struct namespaces_event *event)
 {

@@ -186,6 +187,7 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	char curpath[PATH_MAX];
 	int oldns = -1;
 	int newns = -1;
+	char *oldcwd = NULL;
 
 	if (nc == NULL)
 		return;

@@ -199,9 +201,13 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (snprintf(curpath, PATH_MAX, "/proc/self/ns/mnt") >= PATH_MAX)
 		return;
 
+	oldcwd = get_current_dir_name();
+	if (!oldcwd)
+		return;
+
 	oldns = open(curpath, O_RDONLY);
 	if (oldns < 0)
-		return;
+		goto errout;
 
 	newns = open(nsi->mntns_path, O_RDONLY);
 	if (newns < 0)

@@ -210,11 +216,13 @@ void nsinfo__mountns_enter(struct nsinfo *nsi,
 	if (setns(newns, CLONE_NEWNS) < 0)
 		goto errout;
 
+	nc->oldcwd = oldcwd;
 	nc->oldns = oldns;
 	nc->newns = newns;
 	return;
 
 errout:
+	free(oldcwd);
 	if (oldns > -1)
 		close(oldns);
 	if (newns > -1)

@@ -223,11 +231,16 @@ errout:
 
 void nsinfo__mountns_exit(struct nscookie *nc)
 {
-	if (nc == NULL || nc->oldns == -1 || nc->newns == -1)
+	if (nc == NULL || nc->oldns == -1 || nc->newns == -1 || !nc->oldcwd)
 		return;
 
 	setns(nc->oldns, CLONE_NEWNS);
 
+	if (nc->oldcwd) {
+		WARN_ON_ONCE(chdir(nc->oldcwd));
+		zfree(&nc->oldcwd);
+	}
+
 	if (nc->oldns > -1) {
 		close(nc->oldns);
 		nc->oldns = -1;

tools/perf/util/namespaces.h +1 -0

index cae1a9a39722..d5f46c09ea31 100644
@@ -38,6 +38,7 @@ struct nsinfo {
 struct nscookie {
 	int			oldns;
 	int			newns;
+	char			*oldcwd;
 };
 
 int nsinfo__init(struct nsinfo *nsi);