[PATCH v2] libselinux: use /proc/thread-self when available

[PATCH v2] libselinux: use /proc/thread-self when available

[Stephen Smalley]

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

Re: [PATCH v2] libselinux: use /proc/thread-self when available

[Eric Paris]

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;
>  }

Re: [PATCH v2] libselinux: use /proc/thread-self when available

[Stephen Smalley]

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;
>>  }
> 
> 

Re: [PATCH v2] libselinux: use /proc/thread-self when available

[Eric Paris]

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;
> > >  }
> > 
> > 
> 

Re: [PATCH v2] libselinux: use /proc/thread-self when available

[Stephen Smalley]

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?