[PATCH v4 0/3] vfs: Define new syscall umask2 [formerly getumask]

[PATCH v4 0/3] vfs: Define new syscall umask2 [formerly getumask]

[Richard W.M. Jones]

v3 -> v4:

 - Rename the syscall: getumask becomes umask2.

 - Add flags parameter, with one flag (UMASK_GET_MASK).

 - Expand the rationale for this change in the first commit message.

 - Add a selftest.

 - Retest everything.

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

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 new system call "umask2".  This adds a flags
parameter.  Specifying flags=UMASK_GET_MASK allows the umask of the
current process to be read without modifying it.

This leaves open the possibility in future of adding a per-thread
umask, set or read with other flags.  This is not implemented.

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.

Rich.

[PATCH v4 1/3] vfs: Define new syscall umask2.

[Richard W.M. Jones]

Before this change it was not possible to read the umask of the
current process from a shared library in a way that is safe if the
process uses multiple threads.

Define a variation of the umask system call with a flags parameter.
If flags=0 it behaves like the old umask call.  If flags=UMASK_GET_MASK
it reads the umask of the current process without modifying it.

Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
---
 include/linux/syscalls.h          |  1 +
 include/uapi/asm-generic/fcntl.h  |  2 ++
 include/uapi/asm-generic/unistd.h |  4 +++-
 kernel/sys.c                      | 18 +++++++++++++++++-
 4 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index d795472..939e969 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -659,6 +659,7 @@ asmlinkage long sys_prlimit64(pid_t pid, unsigned int resource,
 				struct rlimit64 __user *old_rlim);
 asmlinkage long sys_getrusage(int who, struct rusage __user *ru);
 asmlinkage long sys_umask(int mask);
+asmlinkage long sys_umask2(int mask, int flags);
 
 asmlinkage long sys_msgget(key_t key, int msgflg);
 asmlinkage long sys_msgsnd(int msqid, struct msgbuf __user *msgp,
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index e063eff..f6c5e5a 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -217,4 +217,6 @@ struct flock64 {
 };
 #endif
 
+#define UMASK_GET_MASK 1	/* umask2 should only return current umask */
+
 #endif /* _ASM_GENERIC_FCNTL_H */
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 2622b33..8a61471 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -717,9 +717,11 @@ __SYSCALL(__NR_membarrier, sys_membarrier)
 __SYSCALL(__NR_mlock2, sys_mlock2)
 #define __NR_copy_file_range 285
 __SYSCALL(__NR_copy_file_range, sys_copy_file_range)
+#define __NR_umask2 286
+__SYSCALL(__NR_umask2, sys_umask2)
 
 #undef __NR_syscalls
-#define __NR_syscalls 286
+#define __NR_syscalls 287
 
 /*
  * All syscalls below here should go away really,
diff --git a/kernel/sys.c b/kernel/sys.c
index cf8ba54..cf60091 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1643,12 +1643,28 @@ COMPAT_SYSCALL_DEFINE2(getrusage, int, who, struct compat_rusage __user *, ru)
 }
 #endif
 
-SYSCALL_DEFINE1(umask, int, mask)
+static int do_umask(int mask, int flags)
 {
+	if (flags & ~UMASK_GET_MASK)
+		return -EINVAL;
+
+	if (flags & UMASK_GET_MASK)
+		return current_umask();
+
 	mask = xchg(&current->fs->umask, mask & S_IRWXUGO);
 	return mask;
 }
 
+SYSCALL_DEFINE1(umask, int, mask)
+{
+	return do_umask(mask, 0);
+}
+
+SYSCALL_DEFINE2(umask2, int, mask, int, flags)
+{
+	return do_umask(mask, flags);
+}
+
 static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
 {
 	struct fd exe;
-- 
2.7.4

[PATCH v4 2/3] x86: Wire up new umask2 system call on x86.

[Richard W.M. Jones]

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..ccf75ba 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	umask2			sys_umask2
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index cac6d17..e566c64 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	umask2			sys_umask2
 
 #
 # x32-specific system call numbers start at 512 to avoid cache impact
-- 
2.7.4

Re: [PATCH v4 2/3] x86: Wire up new umask2 system call on x86.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1415 bytes --]

Hi Richard,

[auto build test WARNING on v4.6-rc3]
[also build test WARNING on next-20160413]
[cannot apply to tip/x86/core]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Richard-W-M-Jones/vfs-Define-new-syscall-umask2-formerly-getumask/20160414-030818
config: xtensa-allyesconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=xtensa 

All warnings (new ones prefixed by >>):

   <stdin>:1298:2: warning: #warning syscall userfaultfd not implemented [-Wcpp]
   <stdin>:1301:2: warning: #warning syscall membarrier not implemented [-Wcpp]
   <stdin>:1304:2: warning: #warning syscall mlock2 not implemented [-Wcpp]
   <stdin>:1307:2: warning: #warning syscall copy_file_range not implemented [-Wcpp]
   <stdin>:1310:2: warning: #warning syscall preadv2 not implemented [-Wcpp]
   <stdin>:1313:2: warning: #warning syscall pwritev2 not implemented [-Wcpp]
>> <stdin>:1316:2: warning: #warning syscall umask2 not implemented [-Wcpp]

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 44878 bytes --]

[PATCH v4 3/3] vfs: selftests: Add test for umask2 system call.

[Richard W.M. Jones]

Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
---
 tools/testing/selftests/Makefile          |  1 +
 tools/testing/selftests/fs/.gitignore     |  1 +
 tools/testing/selftests/fs/Makefile       |  9 ++++
 tools/testing/selftests/fs/umask2-tests.c | 77 +++++++++++++++++++++++++++++++
 4 files changed, 88 insertions(+)
 create mode 100644 tools/testing/selftests/fs/.gitignore
 create mode 100644 tools/testing/selftests/fs/Makefile
 create mode 100644 tools/testing/selftests/fs/umask2-tests.c

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index b04afc3..9e2eb24 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -4,6 +4,7 @@ TARGETS += cpu-hotplug
 TARGETS += efivarfs
 TARGETS += exec
 TARGETS += firmware
+TARGETS += fs
 TARGETS += ftrace
 TARGETS += futex
 TARGETS += ipc
diff --git a/tools/testing/selftests/fs/.gitignore b/tools/testing/selftests/fs/.gitignore
new file mode 100644
index 0000000..057dced
--- /dev/null
+++ b/tools/testing/selftests/fs/.gitignore
@@ -0,0 +1 @@
+umask2-tests
diff --git a/tools/testing/selftests/fs/Makefile b/tools/testing/selftests/fs/Makefile
new file mode 100644
index 0000000..6f231d7
--- /dev/null
+++ b/tools/testing/selftests/fs/Makefile
@@ -0,0 +1,9 @@
+BINARIES := umask2-tests
+
+all: $(BINARIES)
+
+clean:
+	$(RM) $(BINARIES)
+
+include ../lib.mk
+
diff --git a/tools/testing/selftests/fs/umask2-tests.c b/tools/testing/selftests/fs/umask2-tests.c
new file mode 100644
index 0000000..3e01575
--- /dev/null
+++ b/tools/testing/selftests/fs/umask2-tests.c
@@ -0,0 +1,77 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <syscall.h>
+#include <linux/unistd.h>
+
+#ifndef UMASK_GET_MASK
+#define UMASK_GET_MASK 1
+#endif
+
+static int umask2_(int mask, int flags)
+{
+#ifdef __NR_umask2
+	return syscall(__NR_umask2, mask, flags);
+#else
+	errno = ENOSYS;
+	return -1;
+#endif
+}
+
+int main(int argc, char **argv)
+{
+	int r;
+
+	/* umask2 available in current kernel? */
+	r = umask2_(0, UMASK_GET_MASK);
+	if (r == -1 && errno == ENOSYS) {
+		fprintf(stderr,
+			"umask2 not available in current kernel or headers, skipping test\n");
+		exit(0);
+	}
+
+	/* Check that old umask still works. */
+	r = umask(022);
+	if (r == -1) {
+		perror("umask");
+		exit(1);
+	}
+
+	/* Call umask2 to emulate old umask. */
+	r = umask2_(023, 0);
+	if (r == -1) {
+		perror("umask2");
+		exit(1);
+	}
+	if (r != 022) {
+		fprintf(stderr, "umask2: expected %o, got %o\n", 022, r);
+		exit(1);
+	}
+
+	/* Call umask2 to read current umask without modifying it. */
+	r = umask2_(0777, UMASK_GET_MASK);
+	if (r == -1) {
+		perror("umask2");
+		exit(1);
+	}
+	if (r != 023) {
+		fprintf(stderr, "umask2: expected %o, got %o\n", 023, r);
+		exit(1);
+	}
+
+	/* Call it again to make sure we didn't modify umask. */
+	r = umask2_(0777, UMASK_GET_MASK);
+	if (r == -1) {
+		perror("umask2");
+		exit(1);
+	}
+	if (r != 023) {
+		fprintf(stderr, "umask2: expected %o, got %o\n", 023, r);
+		exit(1);
+	}
+
+	exit(0);
+}
-- 
2.7.4

umask2 man page (was: Re: [PATCH v4 0/3] vfs: Define new syscall umask2 [formerly getumask])

[Richard W.M. Jones]

[-- Attachment #1: umask2.2.txt --]
[-- Type: text/plain, Size: 3627 bytes --]

UMASK(2)                   Linux Programmer's Manual                  UMASK(2)



NAME
       umask, umask2 - get and set file mode creation mask

SYNOPSIS
       #include <sys/types.h>
       #include <sys/stat.h>

       mode_t umask(mode_t mask);

       #define _GNU_SOURCE
       #include <fcntl.h>
       #include <sys/types.h>
       #include <sys/stat.h>

       mode_t umask2(mode_t mask, int flags);

DESCRIPTION
       umask()  sets  the calling process's file mode creation mask (umask) to
       mask & 0777 (i.e., only the file permission bits of mask are used), and
       returns the previous value of the mask.

       If flags is 0, then umask2() is the same as umask().

       If flags is UMASK_GET_MASK then umask2() ignores the mask parameter and
       returns the process's current umask.  The process's current mask is not
       modified in this case.

       The  umask  is  used  by open(2), mkdir(2), and other system calls that
       create files to modify the permissions placed on newly created files or
       directories.   Specifically,  permissions  in  the umask are turned off
       from the mode argument to open(2) and mkdir(2).

       Alternatively, if the parent directory has a default ACL (see  acl(5)),
       the umask is ignored, the default ACL is inherited, the permission bits
       are set based on the inherited ACL, and permission bits absent  in  the
       mode  argument  are turned off.  For example, the following default ACL
       is equivalent to a umask of 022:

           u::rwx,g::r-x,o::r-x

       Combining the effect of this default ACL with a mode argument  of  0666
       (rw-rw-rw-), the resulting file permissions would be 0644 (rw-r--r--).

       The  constants  that should be used to specify mask are described under
       stat(2).

       The typical default value for the process  umask  is  S_IWGRP | S_IWOTH
       (octal  022).   In the usual case where the mode argument to open(2) is
       specified as:

           S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH

       (octal 0666) when creating a new file, the permissions on the resulting
       file will be:

           S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH

       (because 0666 & ~022 = 0644; i.e., rw-r--r--).

RETURN VALUE
       The  umask()  system call always succeeds and the previous value of the
       mask is returned.

       The umask2() system call returns the process's current  umask  on  suc‐
       cess.  On error it returns -1 and sets errno appropriately.

CONFORMING TO
       SVr4, 4.3BSD, POSIX.1-2001.

NOTES
       A  child  process created via fork(2) inherits its parent's umask.  The
       umask is left unchanged by execve(2).

       The umask setting also affects the permissions assigned  to  POSIX  IPC
       objects  (mq_open(3), sem_open(3), shm_open(3)), FIFOs (mkfifo(3)), and
       UNIX domain sockets (unix(7)) created by the process.  The  umask  does
       not  affect the permissions assigned to System V IPC objects created by
       the process (using msgget(2), semget(2), shmget(2)).

SEE ALSO
       chmod(2), mkdir(2), open(2), stat(2), acl(5)

COLOPHON
       This page is part of release 4.00 of the Linux  man-pages  project.   A
       description  of  the project, information about reporting bugs, and the
       latest    version    of    this    page,    can     be     found     at
       http://www.kernel.org/doc/man-pages/.



Linux                             2016-04-13                          UMASK(2)

Re: [PATCH v4 0/3] vfs: Define new syscall umask2 [formerly getumask]

[Stephen Rothwell]

Hi Richard,

On Wed, 13 Apr 2016 20:05:33 +0100 "Richard W.M. Jones" <rjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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 was wondering if you really need to read the umask, or would just a
"ignore umask" flag to open{,at} do what you want?

-- 
Cheers,
Stephen Rothwell

Re: [PATCH v4 0/3] vfs: Define new syscall umask2 [formerly getumask]

[Richard W.M. Jones]

On Thu, Apr 14, 2016 at 09:09:38AM +1000, Stephen Rothwell wrote:
> Hi Richard,
> 
> On Wed, 13 Apr 2016 20:05:33 +0100 "Richard W.M. Jones" <rjones@redhat.com> 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 was wondering if you really need to read the umask, or would just a
> "ignore umask" flag to open{,at} do what you want?

This would be very useful, although I think being able to read umask
is also useful.

---

FWIW I am currently developing a patch to add umask to
/proc/PID/status.  Will post it shortly once I've tested it properly.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

Re: [PATCH v4 0/3] vfs: Define new syscall umask2 [formerly getumask]

[H. Peter Anvin]

On 04/13/16 12:05, Richard W.M. Jones wrote:
> v3 -> v4:
>
>   - Rename the syscall: getumask becomes umask2.
>
>   - Add flags parameter, with one flag (UMASK_GET_MASK).
>
>   - Expand the rationale for this change in the first commit message.
>
>
> 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 wouldn't say "if"; that is the case when it matters.

I have to say I'm skeptic to the need for umask2() as opposed to 
getumask().  I would also like to be able to get the umask of another 
process, which would argue for adding it to /proc anyway.

	-hpa

Re: [PATCH v4 0/3] vfs: Define new syscall umask2 [formerly getumask]

[Florian Weimer]

* H. Peter Anvin:

> I have to say I'm skeptic to the need for umask2() as opposed to
> getumask().

I find the extension with a set-the-thread umask somewhat unlikely.
How would a potential per-thread umask interact with CLONE_FS?
Have a per-thread umask that, when active, overrides the global
one, similar to what uselocale provides?  That seems rather messy,
and I'm not aware of any precedent.

Re: [PATCH v4 0/3] vfs: Define new syscall umask2 [formerly getumask]

[Richard W.M. Jones]

On Wed, Apr 13, 2016 at 10:45:05PM +0200, Florian Weimer wrote:
> * H. Peter Anvin:
> 
> > I have to say I'm skeptic to the need for umask2() as opposed to
> > getumask().
> 
> I find the extension with a set-the-thread umask somewhat unlikely.
> How would a potential per-thread umask interact with CLONE_FS?
> Have a per-thread umask that, when active, overrides the global
> one, similar to what uselocale provides?  That seems rather messy,
> and I'm not aware of any precedent.

The flags parameter is for extensions we can't envisage now ...

... although since umask has been around since Unix V6 in 1978, we
might be waiting a long time.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org