Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1377906 > unrolled thread

[PATCH v2 0/2] vfs: Define new syscall getumask.

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/2] vfs: Define new syscall getumask. "Richard W.M. Jones" <rjones@redhat.com> - 2016-04-13 15:00 +0200
    [PATCH v2 2/2] x86: Wire up new getumask system call on x86. "Richard W.M. Jones" <rjones@redhat.com> - 2016-04-13 15:00 +0200
    Re: [PATCH v2 0/2] vfs: Define new syscall getumask. Greg KH <greg@kroah.com> - 2016-04-13 16:10 +0200
      Re: [PATCH v2 0/2] vfs: Define new syscall getumask. Steven Rostedt <rostedt@goodmis.org> - 2016-04-14 05:50 +0200
        Re: [PATCH v2 0/2] vfs: Define new syscall getumask. Greg KH <greg@kroah.com> - 2016-04-14 21:30 +0200
    Re: [PATCH v2 0/2] vfs: Define new syscall getumask. Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-04-13 17:40 +0200
      Re: [PATCH v2 0/2] vfs: Define new syscall getumask. Mathieu Desnoyers <mathieu.desnoyers@efficios.com> - 2016-04-13 23:10 +0200
        Re: [PATCH v2 0/2] vfs: Define new syscall getumask. Theodore Ts'o <tytso@mit.edu> - 2016-04-14 04:20 +0200
        Re: [PATCH v2 0/2] vfs: Define new syscall getumask. Andy Lutomirski <luto@amacapital.net> - 2016-04-14 20:00 +0200
    Re: [PATCH v2 0/2] vfs: Define new syscall getumask. Colin Walters <walters@verbum.org> - 2016-04-13 17:50 +0200
      Re: [PATCH v2 0/2] vfs: Define new syscall getumask. "Richard W.M. Jones" <rjones@redhat.com> - 2016-04-13 18:10 +0200

#1377906 — [PATCH v2 0/2] vfs: Define new syscall getumask.

v1 -> v2:

 - Use current_umask() instead of current->fs->umask.

 - Retested it.

----------------------------------------------------------------------

It's not possible to read the process umask without also modifying it,
which is what umask(2) does.  A library cannot read umask safely,
especially if the main program might be multithreaded.

This patch series adds a trivial system call "getumask" which returns
the umask of the current process.

Another approach to this has been attempted before, adding something
to /proc, although it didn't go anywhere.  See:

  http://comments.gmane.org/gmane.linux.kernel/1292109

Another way to solve this would be to add a thread-safe getumask to
glibc.  Since glibc could own the mutex, this would permit libraries
linked to this glibc to read umask safely.

I should also note that man-pages documents getumask(3), but no
version of glibc has ever implemented it.

Typical test script:

#include <stdio.h>
#include <stdlib.h>
#include <linux/unistd.h>
#include <sys/syscall.h>

int main(int argc, char *argv[])
{
  int r = syscall(329);
  if (r == -1) {
    perror("getumask");
    exit(1);
  }
  printf("umask = %o\n", r);
  exit(0);
}

$ ./getumask 
umask = 22

Rich.

[toc] | [next] | [standalone]

#1377912 — [PATCH v2 2/2] x86: Wire up new getumask system call on x86.

Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
---
 arch/x86/entry/syscalls/syscall_32.tbl | 1 +
 arch/x86/entry/syscalls/syscall_64.tbl | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index b30dd81..af0a032 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -386,3 +386,4 @@
 377	i386	copy_file_range		sys_copy_file_range
 378	i386	preadv2			sys_preadv2
 379	i386	pwritev2		sys_pwritev2
+380	i386	getumask		sys_getumask
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index cac6d17..47c1579 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -335,6 +335,7 @@
 326	common	copy_file_range		sys_copy_file_range
 327	64	preadv2			sys_preadv2
 328	64	pwritev2		sys_pwritev2
+329	common	getumask		sys_getumask
 
 #
 # x32-specific system call numbers start at 512 to avoid cache impact
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1377973

On Wed, Apr 13, 2016 at 01:57:50PM +0100, Richard W.M. Jones wrote:
> v1 -> v2:
> 
>  - Use current_umask() instead of current->fs->umask.
> 
>  - Retested it.
> 
> ----------------------------------------------------------------------
> 
> It's not possible to read the process umask without also modifying it,
> which is what umask(2) does.  A library cannot read umask safely,
> especially if the main program might be multithreaded.
> 
> This patch series adds a trivial system call "getumask" which returns
> the umask of the current process.
> 
> Another approach to this has been attempted before, adding something
> to /proc, although it didn't go anywhere.  See:
> 
>   http://comments.gmane.org/gmane.linux.kernel/1292109
> 
> Another way to solve this would be to add a thread-safe getumask to
> glibc.  Since glibc could own the mutex, this would permit libraries
> linked to this glibc to read umask safely.
> 
> I should also note that man-pages documents getumask(3), but no
> version of glibc has ever implemented it.
> 
> Typical test script:
> 
> #include <stdio.h>
> #include <stdlib.h>
> #include <linux/unistd.h>
> #include <sys/syscall.h>
> 
> int main(int argc, char *argv[])
> {
>   int r = syscall(329);
>   if (r == -1) {
>     perror("getumask");
>     exit(1);
>   }
>   printf("umask = %o\n", r);
>   exit(0);
> }

Why not add this to the ktest infrastructure, we strongly encourage that
for new syscalls, along with a man page patch.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1378462

On Wed, 13 Apr 2016 06:59:50 -0700
Greg KH <greg@kroah.com> wrote:


> Why not add this to the ktest infrastructure, we strongly encourage that
> for new syscalls, along with a man page patch.

Do you mean the "selftest infrastructure"? As I don't see how this
could be used with ktest.

See tools/testing/selftests/

-- Steve

[toc] | [prev] | [next] | [standalone]

#1379235

On Wed, Apr 13, 2016 at 11:47:03PM -0400, Steven Rostedt wrote:
> On Wed, 13 Apr 2016 06:59:50 -0700
> Greg KH <greg@kroah.com> wrote:
> 
> 
> > Why not add this to the ktest infrastructure, we strongly encourage that
> > for new syscalls, along with a man page patch.
> 
> Do you mean the "selftest infrastructure"? As I don't see how this
> could be used with ktest.
> 
> See tools/testing/selftests/

Sorry, yes, I meant selftest, not ktest, sorry for the confusion.

greg k-h

[toc] | [prev] | [next] | [standalone]

#1378053

----- On Apr 13, 2016, at 8:57 AM, Richard W.M. Jones rjones@redhat.com wrote:

> v1 -> v2:
> 
> - Use current_umask() instead of current->fs->umask.
> 
> - Retested it.
> 
> ----------------------------------------------------------------------
> 
> It's not possible to read the process umask without also modifying it,
> which is what umask(2) does.  A library cannot read umask safely,
> especially if the main program might be multithreaded.
> 
> This patch series adds a trivial system call "getumask" which returns
> the umask of the current process.

In addition to this system call, we could extend a variation of my
thread_local_abi system call (https://lkml.org/lkml/2016/4/4/455)
(could be without features flags, or an entirely new system call
specifically for a umask cache) to register a "current umask" cache
located in a TLS area.

Basically, reading the current umask value would be a simple load from
a TLS variable. This could also allow quickly blocking and unblocking
signal delivery from user-space by storing a mask to this TLS area.

The kernel could then look into the signal mask in this TLS area whenever
it needs to deliver a signal (assuming this code path can take
user-space faults), in addition to the mask kept within the
task struct.

This "tls cache" idea could also apply to setting a CPU affinity to the
currently running CPU for short user-space critical sections.

The benefit here is to get _very_ fast operations on the thread umask
and cpu affinity.

Are those ideas too far-fetched ?

Thanks,

Mathieu

> 
> Another approach to this has been attempted before, adding something
> to /proc, although it didn't go anywhere.  See:
> 
>  http://comments.gmane.org/gmane.linux.kernel/1292109
> 
> Another way to solve this would be to add a thread-safe getumask to
> glibc.  Since glibc could own the mutex, this would permit libraries
> linked to this glibc to read umask safely.
> 
> I should also note that man-pages documents getumask(3), but no
> version of glibc has ever implemented it.
> 
> Typical test script:
> 
> #include <stdio.h>
> #include <stdlib.h>
> #include <linux/unistd.h>
> #include <sys/syscall.h>
> 
> int main(int argc, char *argv[])
> {
>  int r = syscall(329);
>  if (r == -1) {
>    perror("getumask");
>    exit(1);
>  }
>  printf("umask = %o\n", r);
>  exit(0);
> }
> 
> $ ./getumask
> umask = 22
> 
> Rich.

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

[toc] | [prev] | [next] | [standalone]

#1378295

----- On Apr 13, 2016, at 11:39 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:

> ----- On Apr 13, 2016, at 8:57 AM, Richard W.M. Jones rjones@redhat.com wrote:
> 
>> v1 -> v2:
>> 
>> - Use current_umask() instead of current->fs->umask.
>> 
>> - Retested it.
>> 
>> ----------------------------------------------------------------------
>> 
>> It's not possible to read the process umask without also modifying it,
>> which is what umask(2) does.  A library cannot read umask safely,
>> especially if the main program might be multithreaded.
>> 
>> This patch series adds a trivial system call "getumask" which returns
>> the umask of the current process.
> 
> In addition to this system call, we could extend a variation of my
> thread_local_abi system call (https://lkml.org/lkml/2016/4/4/455)
> (could be without features flags, or an entirely new system call
> specifically for a umask cache) to register a "current umask" cache
> located in a TLS area.
> 
> Basically, reading the current umask value would be a simple load from
> a TLS variable.

I'm actually discussing 3 separate things here: the umask, sigmask, and
cpu affinity mask.

Not sure if caching the umask in a TLS would be that useful, though.
The caching idea seems to make more sense for signal mask and cpu
affinity mask.

Thanks,

Mathieu

> This could also allow quickly blocking and unblocking
> signal delivery from user-space by storing a mask to this TLS area.
> 
> The kernel could then look into the signal mask in this TLS area whenever
> it needs to deliver a signal (assuming this code path can take
> user-space faults), in addition to the mask kept within the
> task struct.
> 
> This "tls cache" idea could also apply to setting a CPU affinity to the
> currently running CPU for short user-space critical sections.
> 
> The benefit here is to get _very_ fast operations on the thread umask
> and cpu affinity.
> 
> Are those ideas too far-fetched ?
> 
> Thanks,
> 
> Mathieu
> 
>> 
>> Another approach to this has been attempted before, adding something
>> to /proc, although it didn't go anywhere.  See:
>> 
>>  http://comments.gmane.org/gmane.linux.kernel/1292109
>> 
>> Another way to solve this would be to add a thread-safe getumask to
>> glibc.  Since glibc could own the mutex, this would permit libraries
>> linked to this glibc to read umask safely.
>> 
>> I should also note that man-pages documents getumask(3), but no
>> version of glibc has ever implemented it.
>> 
>> Typical test script:
>> 
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <linux/unistd.h>
>> #include <sys/syscall.h>
>> 
>> int main(int argc, char *argv[])
>> {
>>  int r = syscall(329);
>>  if (r == -1) {
>>    perror("getumask");
>>    exit(1);
>>  }
>>  printf("umask = %o\n", r);
>>  exit(0);
>> }
>> 
>> $ ./getumask
>> umask = 22
>> 
>> Rich.
> 
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

[toc] | [prev] | [next] | [standalone]

#1378404

On Wed, Apr 13, 2016 at 09:01:25PM +0000, Mathieu Desnoyers wrote:
> I'm actually discussing 3 separate things here: the umask, sigmask, and
> cpu affinity mask.

The last two are available in /proc/<pid>/status --- which brings up
the question why not just add umask to /proc/<pid>/status?

That way the shared library can read it via /proc/self/status, but
this way it would be possible to look at other process's umask values
this as well.

> >> Another approach to this has been attempted before, adding something
> >> to /proc, although it didn't go anywhere.  See:
> >> 
> >>  http://comments.gmane.org/gmane.linux.kernel/1292109

... and indeed that's what I suggested.  It looks like from the thread
that it petered out due to apathy instead of people not liking the
idea.

One other reason to suggest using a /proc file is that you're not at
the mercy of the glibc folks to wire up a new system call.  (Glibc has
been refusing to wire up getrandom(2), for example.   Grrrr.....)

     	      	      	 	       	   	      - Ted

[toc] | [prev] | [next] | [standalone]

#1379182

On Apr 13, 2016 2:01 PM, "Mathieu Desnoyers"
<mathieu.desnoyers@efficios.com> wrote:
>
> ----- On Apr 13, 2016, at 11:39 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
>
> > ----- On Apr 13, 2016, at 8:57 AM, Richard W.M. Jones rjones@redhat.com wrote:
> >
> >> v1 -> v2:
> >>
> >> - Use current_umask() instead of current->fs->umask.
> >>
> >> - Retested it.
> >>
> >> ----------------------------------------------------------------------
> >>
> >> It's not possible to read the process umask without also modifying it,
> >> which is what umask(2) does.  A library cannot read umask safely,
> >> especially if the main program might be multithreaded.
> >>
> >> This patch series adds a trivial system call "getumask" which returns
> >> the umask of the current process.
> >
> > In addition to this system call, we could extend a variation of my
> > thread_local_abi system call (https://lkml.org/lkml/2016/4/4/455)
> > (could be without features flags, or an entirely new system call
> > specifically for a umask cache) to register a "current umask" cache
> > located in a TLS area.
> >
> > Basically, reading the current umask value would be a simple load from
> > a TLS variable.
>
> I'm actually discussing 3 separate things here: the umask, sigmask, and
> cpu affinity mask.
>
> Not sure if caching the umask in a TLS would be that useful, though.
> The caching idea seems to make more sense for signal mask and cpu
> affinity mask.
>

I think this is of questionable value.

Keep in mind that every feature like this adds overhead to lots of
code paths as well as additional complexity.  Such features should be
justified by big performance benefits.

Given that processes can easily track their own umasks and signal
masks if they care, I don't see why the kernel would want to help.

--Andy

[toc] | [prev] | [next] | [standalone]

#1378059

On Wed, Apr 13, 2016, at 08:57 AM, Richard W.M. Jones wrote:

> It's not possible to read the process umask without also modifying it,
> which is what umask(2) does.  A library cannot read umask safely,
> especially if the main program might be multithreaded.

I assume you just want to do this from a shared library so you can
determine whether or not you need to call fchown() after making files
and the like?  If that's the case it'd be good to note it in the commit
message.

BTW...it might be a good idea to add a flags argument:
https://lwn.net/Articles/585415/

Did you consider calling this `umask2`, having the initial version only support
retrieving it via a UMASK_GET flag, and lay the groundwork to support
setting a threadsafe umask with a UMASK_SET_THREAD flag?

[toc] | [prev] | [next] | [standalone]

#1378081

On Wed, Apr 13, 2016 at 11:41:45AM -0400, Colin Walters wrote:
> On Wed, Apr 13, 2016, at 08:57 AM, Richard W.M. Jones wrote:
> 
> > It's not possible to read the process umask without also modifying it,
> > which is what umask(2) does.  A library cannot read umask safely,
> > especially if the main program might be multithreaded.
> 
> I assume you just want to do this from a shared library so you can
> determine whether or not you need to call fchown() after making files
> and the like?  If that's the case it'd be good to note it in the commit
> message.

Yes, the use case is something like that.  I write a shared library
(libguestfs) and we get bug reports that turn out to be caused by odd
umask settings.  Of course we fix these on a case-by-case basis, but
we also want to include the current umask in debug output so that we
can identify the problem quickly in future reports.

Actually I wrote a rather involved getumask substitute:

  https://github.com/libguestfs/libguestfs/blob/master/src/launch.c#L477

It works by creating a temporary directory, writing a file inside that
directory with mode 0777, then calling fstat(2) to work out what mode
the kernel gave it.

It turns out this code is not even correct.  It was pointed out to me
that there is a filesystem umask mount option (and fmask, dmask too)
which stops this from working properly.

So it's a lot of work to read umask safely inside a shared library.

I will update the commit comment with a brief summary of the above.

> BTW...it might be a good idea to add a flags argument:
> https://lwn.net/Articles/585415/
>
> Did you consider calling this `umask2`, having the initial version
> only support retrieving it via a UMASK_GET flag, and lay the
> groundwork to support setting a threadsafe umask with a
> UMASK_SET_THREAD flag?

Can certainly do it like this if that is preferable.

For my needs, getumask as implemented now is sufficient.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web