* [PATCH v2] libselinux: use /proc/thread-self when available @ 2015-07-13 13:23 Stephen Smalley 2015-07-13 13:34 ` Eric Paris 0 siblings, 1 reply; 5+ messages in thread From: Stephen Smalley @ 2015-07-13 13:23 UTC (permalink / raw) To: selinux; +Cc: Stephen Smalley, eparis Linux 3.17 introduced a /proc/thread-self symlink that can be used to reference the proc files of the current thread without needing to use gettid(2). Use this symlink when it exists, falling back to using gettid(2) when it does not. This is generally beneficial, but was specifically motivated by https://github.com/systemd/systemd/issues/475. Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> --- Only change is the patch description: /proc/thread-self was introduced in Linux 3.17, not Linux 3.16. libselinux/src/procattr.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c index f990350..e444571 100644 --- a/libselinux/src/procattr.c +++ b/libselinux/src/procattr.c @@ -95,6 +95,12 @@ static int openattr(pid_t pid, const char *attr, int flags) if (pid > 0) rc = asprintf(&path, "/proc/%d/attr/%s", pid, attr); else { + rc = asprintf(&path, "/proc/thread-self/attr/%s", attr); + if (rc < 0) + return -1; + fd = open(path, flags | O_CLOEXEC); + if (fd >= 0 || errno != ENOENT) + goto out; if (!tid) tid = gettid(); rc = asprintf(&path, "/proc/self/task/%d/attr/%s", tid, attr); @@ -103,6 +109,7 @@ static int openattr(pid_t pid, const char *attr, int flags) return -1; fd = open(path, flags | O_CLOEXEC); +out: free(path); return fd; } -- 2.1.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] libselinux: use /proc/thread-self when available 2015-07-13 13:23 [PATCH v2] libselinux: use /proc/thread-self when available Stephen Smalley @ 2015-07-13 13:34 ` Eric Paris 2015-07-13 13:56 ` Stephen Smalley 0 siblings, 1 reply; 5+ messages in thread From: Eric Paris @ 2015-07-13 13:34 UTC (permalink / raw) To: Stephen Smalley, selinux Looks good to me. That issue also brings up problems with glibc pid caching, right? Do we need to rip out my *con cache entirely? I remember there was a measurable perf win with the cache, but it has proved a PITA multiple times. Keeping the cache and fixing/working around the glibc pid caching would mean a tradeoff using getpid(2) instead of getpid(3), even with this patch. Right? Or maybe storing a ctid instead of cpid and using gettid(2)... But this patch is LGTM. On Mon, 2015-07-13 at 09:23 -0400, Stephen Smalley wrote: > Linux 3.17 introduced a /proc/thread-self symlink that can be used > to reference the proc files of the current thread without needing to > use gettid(2). Use this symlink when it exists, falling back to > using gettid(2) when it does not. This is generally beneficial, but > was specifically motivated by > https://github.com/systemd/systemd/issues/475. > > Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> > --- > Only change is the patch description: /proc/thread-self was > introduced in > Linux 3.17, not Linux 3.16. > > libselinux/src/procattr.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c > index f990350..e444571 100644 > --- a/libselinux/src/procattr.c > +++ b/libselinux/src/procattr.c > @@ -95,6 +95,12 @@ static int openattr(pid_t pid, const char *attr, > int flags) > if (pid > 0) > rc = asprintf(&path, "/proc/%d/attr/%s", pid, attr); > else { > + rc = asprintf(&path, "/proc/thread-self/attr/%s", > attr); > + if (rc < 0) > + return -1; > + fd = open(path, flags | O_CLOEXEC); > + if (fd >= 0 || errno != ENOENT) > + goto out; > if (!tid) > tid = gettid(); > rc = asprintf(&path, "/proc/self/task/%d/attr/%s", > tid, attr); > @@ -103,6 +109,7 @@ static int openattr(pid_t pid, const char *attr, > int flags) > return -1; > > fd = open(path, flags | O_CLOEXEC); > +out: > free(path); > return fd; > } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] libselinux: use /proc/thread-self when available 2015-07-13 13:34 ` Eric Paris @ 2015-07-13 13:56 ` Stephen Smalley 2015-07-13 19:20 ` Eric Paris 0 siblings, 1 reply; 5+ messages in thread From: Stephen Smalley @ 2015-07-13 13:56 UTC (permalink / raw) To: Eric Paris, selinux On 07/13/2015 09:34 AM, Eric Paris wrote: > Looks good to me. > > That issue also brings up problems with glibc pid caching, right? Do we > need to rip out my *con cache entirely? I remember there was a > measurable perf win with the cache, but it has proved a PITA multiple > times. Keeping the cache and fixing/working around the glibc pid > caching would mean a tradeoff using getpid(2) instead of getpid(3), > even with this patch. Right? Or maybe storing a ctid instead of cpid > and using gettid(2)... > > But this patch is LGTM. >From the discussion in that issue, it sounded like switching to using /proc/thread-self would avoid the problem without needing to switch to getpid(2), but I have not tested systemd-nspawn --selinux-context after the change to confirm this fact. Certainly it ensures that we always open the right path on Linux 3.17 and later. I would love to rip out the cache, but I didn't think we were free to do so without first reworking coreutils in some manner to avoid the repeated getfscreatecon()...setfscreatecon() calls made when copying directory trees. > > On Mon, 2015-07-13 at 09:23 -0400, Stephen Smalley wrote: >> Linux 3.17 introduced a /proc/thread-self symlink that can be used >> to reference the proc files of the current thread without needing to >> use gettid(2). Use this symlink when it exists, falling back to >> using gettid(2) when it does not. This is generally beneficial, but >> was specifically motivated by >> https://github.com/systemd/systemd/issues/475. >> >> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> >> --- >> Only change is the patch description: /proc/thread-self was >> introduced in >> Linux 3.17, not Linux 3.16. >> >> libselinux/src/procattr.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c >> index f990350..e444571 100644 >> --- a/libselinux/src/procattr.c >> +++ b/libselinux/src/procattr.c >> @@ -95,6 +95,12 @@ static int openattr(pid_t pid, const char *attr, >> int flags) >> if (pid > 0) >> rc = asprintf(&path, "/proc/%d/attr/%s", pid, attr); >> else { >> + rc = asprintf(&path, "/proc/thread-self/attr/%s", >> attr); >> + if (rc < 0) >> + return -1; >> + fd = open(path, flags | O_CLOEXEC); >> + if (fd >= 0 || errno != ENOENT) >> + goto out; >> if (!tid) >> tid = gettid(); >> rc = asprintf(&path, "/proc/self/task/%d/attr/%s", >> tid, attr); >> @@ -103,6 +109,7 @@ static int openattr(pid_t pid, const char *attr, >> int flags) >> return -1; >> >> fd = open(path, flags | O_CLOEXEC); >> +out: >> free(path); >> return fd; >> } > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] libselinux: use /proc/thread-self when available 2015-07-13 13:56 ` Stephen Smalley @ 2015-07-13 19:20 ` Eric Paris 2015-07-13 19:42 ` Stephen Smalley 0 siblings, 1 reply; 5+ messages in thread From: Eric Paris @ 2015-07-13 19:20 UTC (permalink / raw) To: Stephen Smalley, selinux On Mon, 2015-07-13 at 09:56 -0400, Stephen Smalley wrote: > On 07/13/2015 09:34 AM, Eric Paris wrote: > > Looks good to me. > > > > That issue also brings up problems with glibc pid caching, right? > > Do we > > need to rip out my *con cache entirely? I remember there was a > > measurable perf win with the cache, but it has proved a PITA > > multiple > > times. Keeping the cache and fixing/working around the glibc pid > > caching would mean a tradeoff using getpid(2) instead of getpid(3), > > even with this patch. Right? Or maybe storing a ctid instead of > > cpid > > and using gettid(2)... > > > > But this patch is LGTM. > > From the discussion in that issue, it sounded like switching to using > /proc/thread-self would avoid the problem without needing to switch > to > getpid(2), but I have not tested systemd-nspawn --selinux-context > after > the change to confirm this fact. Certainly it ensures that we always > open the right path on Linux 3.17 and later. > > I would love to rip out the cache, but I didn't think we were free to > do > so without first reworking coreutils in some manner to avoid the > repeated getfscreatecon()...setfscreatecon() calls made when copying > directory trees. You might be right, but I'm not so sure. After a direct call to clone the cache has both a cpid and tid saved. So the (incorrect) cached tid you just fixed. Certainly possible this was the only problem systemd was having. But the {get,set}procattrcon_raw() functions also have that cached cpid. Which is wrong after clone. Which means they will use the cached values instead of even getting to openattr()... So maybe systemd hasn't caused anything to get cached and it will work by change, but the library is still busted against some users of clone... I was going to suggest telling systemd to call free_procattr() after they call their own clone. But even that is busted since it uses getpid() and libc is going to cache that result. Seems like we need to s/getpid(3)/getpid(2)/ in that whole file to make it right... > > > > > On Mon, 2015-07-13 at 09:23 -0400, Stephen Smalley wrote: > > > Linux 3.17 introduced a /proc/thread-self symlink that can be > > > used > > > to reference the proc files of the current thread without needing > > > to > > > use gettid(2). Use this symlink when it exists, falling back to > > > using gettid(2) when it does not. This is generally beneficial, > > > but > > > was specifically motivated by > > > https://github.com/systemd/systemd/issues/475. > > > > > > Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov> > > > --- > > > Only change is the patch description: /proc/thread-self was > > > introduced in > > > Linux 3.17, not Linux 3.16. > > > > > > libselinux/src/procattr.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/libselinux/src/procattr.c > > > b/libselinux/src/procattr.c > > > index f990350..e444571 100644 > > > --- a/libselinux/src/procattr.c > > > +++ b/libselinux/src/procattr.c > > > @@ -95,6 +95,12 @@ static int openattr(pid_t pid, const char > > > *attr, > > > int flags) > > > if (pid > 0) > > > rc = asprintf(&path, "/proc/%d/attr/%s", pid, > > > attr); > > > else { > > > + rc = asprintf(&path, "/proc/thread > > > -self/attr/%s", > > > attr); > > > + if (rc < 0) > > > + return -1; > > > + fd = open(path, flags | O_CLOEXEC); > > > + if (fd >= 0 || errno != ENOENT) > > > + goto out; > > > if (!tid) > > > tid = gettid(); > > > rc = asprintf(&path, > > > "/proc/self/task/%d/attr/%s", > > > tid, attr); > > > @@ -103,6 +109,7 @@ static int openattr(pid_t pid, const char > > > *attr, > > > int flags) > > > return -1; > > > > > > fd = open(path, flags | O_CLOEXEC); > > > +out: > > > free(path); > > > return fd; > > > } > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] libselinux: use /proc/thread-self when available 2015-07-13 19:20 ` Eric Paris @ 2015-07-13 19:42 ` Stephen Smalley 0 siblings, 0 replies; 5+ messages in thread From: Stephen Smalley @ 2015-07-13 19:42 UTC (permalink / raw) To: Eric Paris, selinux On 07/13/2015 03:20 PM, Eric Paris wrote: > On Mon, 2015-07-13 at 09:56 -0400, Stephen Smalley wrote: >> On 07/13/2015 09:34 AM, Eric Paris wrote: >>> Looks good to me. >>> >>> That issue also brings up problems with glibc pid caching, right? >>> Do we >>> need to rip out my *con cache entirely? I remember there was a >>> measurable perf win with the cache, but it has proved a PITA >>> multiple >>> times. Keeping the cache and fixing/working around the glibc pid >>> caching would mean a tradeoff using getpid(2) instead of getpid(3), >>> even with this patch. Right? Or maybe storing a ctid instead of >>> cpid >>> and using gettid(2)... >>> >>> But this patch is LGTM. >> >> From the discussion in that issue, it sounded like switching to using >> /proc/thread-self would avoid the problem without needing to switch >> to >> getpid(2), but I have not tested systemd-nspawn --selinux-context >> after >> the change to confirm this fact. Certainly it ensures that we always >> open the right path on Linux 3.17 and later. >> >> I would love to rip out the cache, but I didn't think we were free to >> do >> so without first reworking coreutils in some manner to avoid the >> repeated getfscreatecon()...setfscreatecon() calls made when copying >> directory trees. > > You might be right, but I'm not so sure. After a direct call to clone > the cache has both a cpid and tid saved. So the (incorrect) cached tid > you just fixed. Certainly possible this was the only problem systemd > was having. > > But the {get,set}procattrcon_raw() functions also have that cached > cpid. Which is wrong after clone. Which means they will use the cached > values instead of even getting to openattr()... So maybe systemd > hasn't caused anything to get cached and it will work by change, but > the library is still busted against some users of clone... > > I was going to suggest telling systemd to call free_procattr() after > they call their own clone. But even that is busted since it uses > getpid() and libc is going to cache that result. > > Seems like we need to s/getpid(3)/getpid(2)/ in that whole file to make > it right... None of the /proc/self/attr attributes change on fork, only on exec. So why do we need to flush the cache then? ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-07-13 19:42 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-07-13 13:23 [PATCH v2] libselinux: use /proc/thread-self when available Stephen Smalley 2015-07-13 13:34 ` Eric Paris 2015-07-13 13:56 ` Stephen Smalley 2015-07-13 19:20 ` Eric Paris 2015-07-13 19:42 ` Stephen Smalley
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.