From c35ce1d918c12900375a60165c908de47856900d Mon Sep 17 00:00:00 2001 From: Ian Rogers Date: Mon, 17 Apr 2023 18:51:57 -0300 Subject: perf namespaces: Add reference count checking Add reference count checking controlled by REFCNT_CHECKING ifdef. The reference count checking interposes an allocated pointer between the reference counted struct on a get and frees the pointer on a put. Accesses after a put cause faults and use after free, missed puts are caughts as leaks and double puts are double frees. This checking helped resolve a memory leak and use after free: https://lore.kernel.org/linux-perf-users/CAP-5=fWZH20L4kv-BwVtGLwR=Em3AOOT+Q4QGivvQuYn5AsPRg@mail.gmail.com/ Signed-off-by: Ian Rogers Cc: Adrian Hunter Cc: Alexey Bayduraev Cc: Dmitriy Vyukov Cc: Jiri Olsa Cc: Namhyung Kim Cc: Riccardo Mancini Cc: Stephane Eranian Cc: Stephen Brennan Link: https://lore.kernel.org/lkml/20230407230405.2931830-4-irogers@google.com [ Extracted from a larger patch ] Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/namespaces.c | 132 +++++++++++++++++++++++++------------------ 1 file changed, 76 insertions(+), 56 deletions(-) (limited to 'tools/perf/util/namespaces.c') diff --git a/tools/perf/util/namespaces.c b/tools/perf/util/namespaces.c index dd536220cdb9..8a3b7bd27b19 100644 --- a/tools/perf/util/namespaces.c +++ b/tools/perf/util/namespaces.c @@ -60,7 +60,7 @@ void namespaces__free(struct namespaces *namespaces) free(namespaces); } -static int nsinfo__get_nspid(struct nsinfo *nsi, const char *path) +static int nsinfo__get_nspid(pid_t *tgid, pid_t *nstgid, bool *in_pidns, const char *path) { FILE *f = NULL; char *statln = NULL; @@ -74,19 +74,18 @@ static int nsinfo__get_nspid(struct nsinfo *nsi, const char *path) while (getline(&statln, &linesz, f) != -1) { /* Use tgid if CONFIG_PID_NS is not defined. */ if (strstr(statln, "Tgid:") != NULL) { - nsi->tgid = (pid_t)strtol(strrchr(statln, '\t'), - NULL, 10); - nsi->nstgid = nsinfo__tgid(nsi); + *tgid = (pid_t)strtol(strrchr(statln, '\t'), NULL, 10); + *nstgid = *tgid; } if (strstr(statln, "NStgid:") != NULL) { nspid = strrchr(statln, '\t'); - nsi->nstgid = (pid_t)strtol(nspid, NULL, 10); + *nstgid = (pid_t)strtol(nspid, NULL, 10); /* * If innermost tgid is not the first, process is in a different * PID namespace. */ - nsi->in_pidns = (statln + sizeof("NStgid:") - 1) != nspid; + *in_pidns = (statln + sizeof("NStgid:") - 1) != nspid; break; } } @@ -121,8 +120,8 @@ int nsinfo__init(struct nsinfo *nsi) * want to switch as part of looking up dso/map data. */ if (old_stat.st_ino != new_stat.st_ino) { - nsi->need_setns = true; - nsi->mntns_path = newns; + RC_CHK_ACCESS(nsi)->need_setns = true; + RC_CHK_ACCESS(nsi)->mntns_path = newns; newns = NULL; } @@ -132,13 +131,26 @@ int nsinfo__init(struct nsinfo *nsi) if (snprintf(spath, PATH_MAX, "/proc/%d/status", nsinfo__pid(nsi)) >= PATH_MAX) goto out; - rv = nsinfo__get_nspid(nsi, spath); + rv = nsinfo__get_nspid(&RC_CHK_ACCESS(nsi)->tgid, &RC_CHK_ACCESS(nsi)->nstgid, + &RC_CHK_ACCESS(nsi)->in_pidns, spath); out: free(newns); return rv; } +static struct nsinfo *nsinfo__alloc(void) +{ + struct nsinfo *res; + RC_STRUCT(nsinfo) *nsi; + + nsi = calloc(1, sizeof(*nsi)); + if (ADD_RC_CHK(res, nsi)) + refcount_set(&nsi->refcnt, 1); + + return res; +} + struct nsinfo *nsinfo__new(pid_t pid) { struct nsinfo *nsi; @@ -146,22 +158,21 @@ struct nsinfo *nsinfo__new(pid_t pid) if (pid == 0) return NULL; - nsi = calloc(1, sizeof(*nsi)); - if (nsi != NULL) { - nsi->pid = pid; - nsi->tgid = pid; - nsi->nstgid = pid; - nsi->need_setns = false; - nsi->in_pidns = false; - /* Init may fail if the process exits while we're trying to look - * at its proc information. In that case, save the pid but - * don't try to enter the namespace. - */ - if (nsinfo__init(nsi) == -1) - nsi->need_setns = false; + nsi = nsinfo__alloc(); + if (!nsi) + return NULL; - refcount_set(&nsi->refcnt, 1); - } + RC_CHK_ACCESS(nsi)->pid = pid; + RC_CHK_ACCESS(nsi)->tgid = pid; + RC_CHK_ACCESS(nsi)->nstgid = pid; + RC_CHK_ACCESS(nsi)->need_setns = false; + RC_CHK_ACCESS(nsi)->in_pidns = false; + /* Init may fail if the process exits while we're trying to look at its + * proc information. In that case, save the pid but don't try to enter + * the namespace. + */ + if (nsinfo__init(nsi) == -1) + RC_CHK_ACCESS(nsi)->need_setns = false; return nsi; } @@ -173,21 +184,21 @@ struct nsinfo *nsinfo__copy(const struct nsinfo *nsi) if (nsi == NULL) return NULL; - nnsi = calloc(1, sizeof(*nnsi)); - if (nnsi != NULL) { - nnsi->pid = nsinfo__pid(nsi); - nnsi->tgid = nsinfo__tgid(nsi); - nnsi->nstgid = nsinfo__nstgid(nsi); - nnsi->need_setns = nsinfo__need_setns(nsi); - nnsi->in_pidns = nsinfo__in_pidns(nsi); - if (nsi->mntns_path) { - nnsi->mntns_path = strdup(nsi->mntns_path); - if (!nnsi->mntns_path) { - free(nnsi); - return NULL; - } + nnsi = nsinfo__alloc(); + if (!nnsi) + return NULL; + + RC_CHK_ACCESS(nnsi)->pid = nsinfo__pid(nsi); + RC_CHK_ACCESS(nnsi)->tgid = nsinfo__tgid(nsi); + RC_CHK_ACCESS(nnsi)->nstgid = nsinfo__nstgid(nsi); + RC_CHK_ACCESS(nnsi)->need_setns = nsinfo__need_setns(nsi); + RC_CHK_ACCESS(nnsi)->in_pidns = nsinfo__in_pidns(nsi); + if (RC_CHK_ACCESS(nsi)->mntns_path) { + RC_CHK_ACCESS(nnsi)->mntns_path = strdup(RC_CHK_ACCESS(nsi)->mntns_path); + if (!RC_CHK_ACCESS(nnsi)->mntns_path) { + nsinfo__put(nnsi); + return NULL; } - refcount_set(&nnsi->refcnt, 1); } return nnsi; @@ -195,51 +206,60 @@ struct nsinfo *nsinfo__copy(const struct nsinfo *nsi) static void nsinfo__delete(struct nsinfo *nsi) { - zfree(&nsi->mntns_path); - free(nsi); + if (nsi) { + WARN_ONCE(refcount_read(&RC_CHK_ACCESS(nsi)->refcnt) != 0, + "nsinfo refcnt unbalanced\n"); + zfree(&RC_CHK_ACCESS(nsi)->mntns_path); + RC_CHK_FREE(nsi); + } } struct nsinfo *nsinfo__get(struct nsinfo *nsi) { - if (nsi) - refcount_inc(&nsi->refcnt); - return nsi; + struct nsinfo *result; + + if (RC_CHK_GET(result, nsi)) + refcount_inc(&RC_CHK_ACCESS(nsi)->refcnt); + + return result; } void nsinfo__put(struct nsinfo *nsi) { - if (nsi && refcount_dec_and_test(&nsi->refcnt)) + if (nsi && refcount_dec_and_test(&RC_CHK_ACCESS(nsi)->refcnt)) nsinfo__delete(nsi); + else + RC_CHK_PUT(nsi); } bool nsinfo__need_setns(const struct nsinfo *nsi) { - return nsi->need_setns; + return RC_CHK_ACCESS(nsi)->need_setns; } void nsinfo__clear_need_setns(struct nsinfo *nsi) { - nsi->need_setns = false; + RC_CHK_ACCESS(nsi)->need_setns = false; } pid_t nsinfo__tgid(const struct nsinfo *nsi) { - return nsi->tgid; + return RC_CHK_ACCESS(nsi)->tgid; } pid_t nsinfo__nstgid(const struct nsinfo *nsi) { - return nsi->nstgid; + return RC_CHK_ACCESS(nsi)->nstgid; } pid_t nsinfo__pid(const struct nsinfo *nsi) { - return nsi->pid; + return RC_CHK_ACCESS(nsi)->pid; } pid_t nsinfo__in_pidns(const struct nsinfo *nsi) { - return nsi->in_pidns; + return RC_CHK_ACCESS(nsi)->in_pidns; } void nsinfo__mountns_enter(struct nsinfo *nsi, @@ -256,7 +276,7 @@ void nsinfo__mountns_enter(struct nsinfo *nsi, nc->oldns = -1; nc->newns = -1; - if (!nsi || !nsi->need_setns) + if (!nsi || !RC_CHK_ACCESS(nsi)->need_setns) return; if (snprintf(curpath, PATH_MAX, "/proc/self/ns/mnt") >= PATH_MAX) @@ -270,7 +290,7 @@ void nsinfo__mountns_enter(struct nsinfo *nsi, if (oldns < 0) goto errout; - newns = open(nsi->mntns_path, O_RDONLY); + newns = open(RC_CHK_ACCESS(nsi)->mntns_path, O_RDONLY); if (newns < 0) goto errout; @@ -339,9 +359,9 @@ int nsinfo__stat(const char *filename, struct stat *st, struct nsinfo *nsi) bool nsinfo__is_in_root_namespace(void) { - struct nsinfo nsi; + pid_t tgid = 0, nstgid = 0; + bool in_pidns = false; - memset(&nsi, 0x0, sizeof(nsi)); - nsinfo__get_nspid(&nsi, "/proc/self/status"); - return !nsi.in_pidns; + nsinfo__get_nspid(&tgid, &nstgid, &in_pidns, "/proc/self/status"); + return !in_pidns; } -- cgit From 4d623903f1ed63a06e469c4ce45231440d1be5b6 Mon Sep 17 00:00:00 2001 From: Arnaldo Carvalho de Melo Date: Mon, 17 Apr 2023 22:11:58 -0300 Subject: perf namespaces: Use the need_setns() accessors instead of accessing ->need_setns directly This uses pre-existing accessors and reduces the use of RC_CHK_ACCESS(nsi). Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/namespaces.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'tools/perf/util/namespaces.c') diff --git a/tools/perf/util/namespaces.c b/tools/perf/util/namespaces.c index 8a3b7bd27b19..214a8391e07c 100644 --- a/tools/perf/util/namespaces.c +++ b/tools/perf/util/namespaces.c @@ -165,14 +165,14 @@ struct nsinfo *nsinfo__new(pid_t pid) RC_CHK_ACCESS(nsi)->pid = pid; RC_CHK_ACCESS(nsi)->tgid = pid; RC_CHK_ACCESS(nsi)->nstgid = pid; - RC_CHK_ACCESS(nsi)->need_setns = false; + nsinfo__clear_need_setns(nsi); RC_CHK_ACCESS(nsi)->in_pidns = false; /* Init may fail if the process exits while we're trying to look at its * proc information. In that case, save the pid but don't try to enter * the namespace. */ if (nsinfo__init(nsi) == -1) - RC_CHK_ACCESS(nsi)->need_setns = false; + nsinfo__clear_need_setns(nsi); return nsi; } @@ -276,7 +276,7 @@ void nsinfo__mountns_enter(struct nsinfo *nsi, nc->oldns = -1; nc->newns = -1; - if (!nsi || !RC_CHK_ACCESS(nsi)->need_setns) + if (!nsi || !nsinfo__need_setns(nsi)) return; if (snprintf(curpath, PATH_MAX, "/proc/self/ns/mnt") >= PATH_MAX) -- cgit From f94c21dfd02e98aa0fcb9b453a1198e76ede60e7 Mon Sep 17 00:00:00 2001 From: Arnaldo Carvalho de Melo Date: Mon, 17 Apr 2023 22:11:58 -0300 Subject: perf namespaces: Introduce nsinfo__refcnt() accessor to avoid accessing ->refcnt directly To reduces the use of RC_CHK_ACCESS(nsi). Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/namespaces.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) (limited to 'tools/perf/util/namespaces.c') diff --git a/tools/perf/util/namespaces.c b/tools/perf/util/namespaces.c index 214a8391e07c..2f9fb1e2769c 100644 --- a/tools/perf/util/namespaces.c +++ b/tools/perf/util/namespaces.c @@ -204,11 +204,15 @@ struct nsinfo *nsinfo__copy(const struct nsinfo *nsi) return nnsi; } +static refcount_t *nsinfo__refcnt(struct nsinfo *nsi) +{ + return &RC_CHK_ACCESS(nsi)->refcnt; +} + static void nsinfo__delete(struct nsinfo *nsi) { if (nsi) { - WARN_ONCE(refcount_read(&RC_CHK_ACCESS(nsi)->refcnt) != 0, - "nsinfo refcnt unbalanced\n"); + WARN_ONCE(refcount_read(nsinfo__refcnt(nsi)) != 0, "nsinfo refcnt unbalanced\n"); zfree(&RC_CHK_ACCESS(nsi)->mntns_path); RC_CHK_FREE(nsi); } @@ -219,14 +223,14 @@ struct nsinfo *nsinfo__get(struct nsinfo *nsi) struct nsinfo *result; if (RC_CHK_GET(result, nsi)) - refcount_inc(&RC_CHK_ACCESS(nsi)->refcnt); + refcount_inc(nsinfo__refcnt(nsi)); return result; } void nsinfo__put(struct nsinfo *nsi) { - if (nsi && refcount_dec_and_test(&RC_CHK_ACCESS(nsi)->refcnt)) + if (nsi && refcount_dec_and_test(nsinfo__refcnt(nsi))) nsinfo__delete(nsi); else RC_CHK_PUT(nsi); -- cgit From 2d1acd3f10baed49329c1b201cafb060aeb377e0 Mon Sep 17 00:00:00 2001 From: Arnaldo Carvalho de Melo Date: Mon, 17 Apr 2023 22:11:58 -0300 Subject: perf namespaces: Introduce nsinfo__mntns_path() accessor to avoid accessing ->mntns_path directly To reduce the use of RC_CHK_ACCESS(nsi). Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/namespaces.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) (limited to 'tools/perf/util/namespaces.c') diff --git a/tools/perf/util/namespaces.c b/tools/perf/util/namespaces.c index 2f9fb1e2769c..cb185c5659d6 100644 --- a/tools/perf/util/namespaces.c +++ b/tools/perf/util/namespaces.c @@ -177,6 +177,11 @@ struct nsinfo *nsinfo__new(pid_t pid) return nsi; } +static const char *nsinfo__mntns_path(const struct nsinfo *nsi) +{ + return RC_CHK_ACCESS(nsi)->mntns_path; +} + struct nsinfo *nsinfo__copy(const struct nsinfo *nsi) { struct nsinfo *nnsi; @@ -193,8 +198,8 @@ struct nsinfo *nsinfo__copy(const struct nsinfo *nsi) RC_CHK_ACCESS(nnsi)->nstgid = nsinfo__nstgid(nsi); RC_CHK_ACCESS(nnsi)->need_setns = nsinfo__need_setns(nsi); RC_CHK_ACCESS(nnsi)->in_pidns = nsinfo__in_pidns(nsi); - if (RC_CHK_ACCESS(nsi)->mntns_path) { - RC_CHK_ACCESS(nnsi)->mntns_path = strdup(RC_CHK_ACCESS(nsi)->mntns_path); + if (nsinfo__mntns_path(nsi)) { + RC_CHK_ACCESS(nnsi)->mntns_path = strdup(nsinfo__mntns_path(nsi)); if (!RC_CHK_ACCESS(nnsi)->mntns_path) { nsinfo__put(nnsi); return NULL; @@ -294,7 +299,7 @@ void nsinfo__mountns_enter(struct nsinfo *nsi, if (oldns < 0) goto errout; - newns = open(RC_CHK_ACCESS(nsi)->mntns_path, O_RDONLY); + newns = open(nsinfo__mntns_path(nsi), O_RDONLY); if (newns < 0) goto errout; -- cgit