Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 686142 - sys-fs/zfs-kmod: O_TMPFILE option ignores umask for noacl filesystems
Summary: sys-fs/zfs-kmod: O_TMPFILE option ignores umask for noacl filesystems
Status: RESOLVED FIXED
Alias: None
Product: Gentoo Linux
Classification: Unclassified
Component: Current packages (show other bugs)
Hardware: All Linux
: Normal normal (vote)
Assignee: Georgy Yakovlev
URL: https://github.com/zfsonlinux/zfs/iss...
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-05-17 10:38 UTC by crocket
Modified: 2020-06-10 21:55 UTC (History)
2 users (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
emerge --info (emerge--info,5.74 KB, text/plain)
2019-07-05 10:52 UTC, Sergei Trofimovich
Details
config-4.19.52-gentoo kernel config (config-4.19.52-gentoo,95.95 KB, text/plain)
2019-07-05 10:52 UTC, Sergei Trofimovich
Details
emerge --info sys-fs/zfs sys-fs/zfs-kmod (emerge.info,13.71 KB, text/plain)
2019-07-06 07:11 UTC, crocket
Details

Note You need to log in before you can comment on or make changes to this bug.
Description crocket 2019-05-17 10:38:00 UTC
According to https://github.com/MusicPlayerDaemon/MPD/issues/558

MPD uses O_TMPFILE to create its database, and the file mode of the newly created database is 666. This means umask is ignored.

`man 2 open` doesn't state explicit support for umask from O_TMPFILE.

Is O_TMPFILE supposed to ignore umask?
Or, is it a bug for O_TMPFILE to ignore umask?
Comment 1 Sergei Trofimovich gentoo-dev 2019-05-18 09:18:57 UTC
1. Can you provide selfcontained c code that demostrates the problem?
2. Is it a regression or always been like that?
Comment 2 crocket 2019-05-20 03:54:36 UTC
1. I don't know how to write C code. I certainly don't know how to write a minimal C example demonstrating the issue. I tried and gave up.
2. I think it has been like that for at least years. MPD had the issue for years. I noticed the issue again recently and submitted it here.
Comment 3 Sergei Trofimovich gentoo-dev 2019-06-22 17:52:47 UTC
'man 2 open' says that umask should already have an effect for O_CREAT and O_TMPFILE.

O_TMPFILE creates nameless files (for example via mkstemp). How do you observe it's effect (fstat?) and why does it matter? Feel free to use the template below to create a reproducer that illustrates a problem.

$ cat a.c
// $ gcc a.c -o a && ./a

#define _GNU_SOURCE /* for O_TMPFILE */

#include <fcntl.h>
#include <unistd.h>

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

#include <errno.h>
#include <stdio.h>
#include <string.h>

static const char probe_path[] = "/tmp/probe-open-for-b686142";

static void probe_umask(void) {
    mode_t current_umask = umask(0);
    umask(current_umask);

    printf("current umask = %#o\n", (unsigned)current_umask);
}

static void probe_open(const char * test_desc, int flags, mode_t mode) {
    struct stat s;
    int fd;
    memset(&s, 0, sizeof(s));

    // create target file
    fd = open(probe_path, flags, mode);
    close (fd);

    // check the resulting mode
    int r = stat(probe_path, &s);
    int e = errno;

    // cleanup
    unlink(probe_path);

    if (r == 0)
        printf("%s: in_mode=%#o, resulting_mode=%#o\n", test_desc, (unsigned)mode, (unsigned)s.st_mode);
    else
        printf("%s: stat() failed as: %s\n", test_desc, strerror(e));
}

static void probe_open_modes(void) {
    probe_umask();
    probe_open("O_CREAT, 777", O_CREAT, 0777);
    probe_open("O_TMPFILE, 777", O_TMPFILE, 777);
    probe_open("O_CREAT|O_TMPFILE, 777", O_CREAT|O_TMPFILE, 777);
}

int main() {
    // test defaults
    probe_open_modes();

    // test restricted masks
    umask(0077);
    probe_open_modes();
}
$ gcc a.c -o a && ./a
current umask = 022
O_CREAT, 777: in_mode=0777, resulting_mode=0100755
O_TMPFILE, 777: stat() failed as: No such file or directory
O_CREAT|O_TMPFILE, 777: stat() failed as: No such file or directory
current umask = 077
O_CREAT, 777: in_mode=0777, resulting_mode=0100700
O_TMPFILE, 777: stat() failed as: No such file or directory
O_CREAT|O_TMPFILE, 777: stat() failed as: No such file or directory
Comment 4 Sergei Trofimovich gentoo-dev 2019-06-22 18:05:51 UTC
Oh, I see mpd uses linkat from '/proc/self/fd', that makes sense. Let me craft a shorter and less broken example.
Comment 5 Sergei Trofimovich gentoo-dev 2019-06-22 18:26:58 UTC
Here is the updated sample. Works as expected (respects umask) on tmpfs, btrfs, xfs and ext4.

$ cat a.c
// $ gcc a.c -o a && ./a

#define _GNU_SOURCE /* for O_TMPFILE */

#include <fcntl.h>
#include <unistd.h>

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

#include <errno.h>
#include <limits.h>
#include <stdio.h>
#include <string.h>

static char probe_name[] = "file-for-b686142";

static void probe_umask(void) {
    mode_t current_umask = umask(0);
    umask(current_umask);

    printf("current umask = %#o\n", (unsigned)current_umask);
}

static void probe_open(void) {
    struct stat fs;
    struct stat s;
    int fd;
    int e;
    char path[PATH_MAX];
    mode_t mode = 0777;

    memset(&fs, 0, sizeof(fs));
    memset(&s, 0, sizeof(s));

    // create target file
    fd = open(".", O_TMPFILE | O_RDWR, mode);
    fstat(fd, &fs);
    snprintf(path, PATH_MAX,  "/proc/self/fd/%d", fd);
    linkat(AT_FDCWD, path, AT_FDCWD, probe_name, AT_SYMLINK_FOLLOW);
    close (fd);

    stat(probe_name, &s);
    unlink(probe_name);

    printf("fstat(): in_mode=%#o, resulting_mode=%#o\n", (unsigned)mode, (unsigned)fs.st_mode);
    printf("stat():  in_mode=%#o, resulting_mode=%#o\n", (unsigned)mode, (unsigned)s.st_mode);
}

static void probe_open_modes(void) {
    probe_umask();
    probe_open();
}

int main() {
    // test defaults
    probe_open_modes();

    // test restricted masks
    umask(0077);
    probe_open_modes();
}

$ gcc a.c -o a && ./a
current umask = 022
fstat(): in_mode=0777, resulting_mode=0100755
stat():  in_mode=0777, resulting_mode=0100755
current umask = 077
fstat(): in_mode=0777, resulting_mode=0100700
stat():  in_mode=0777, resulting_mode=0100700
Comment 6 crocket 2019-07-04 09:43:39 UTC
By the way, I use mpd on zfs.

> zfs get xattr user-data/data
NAME            PROPERTY  VALUE  SOURCE
user-data/data  xattr     sa     local

> zfs get acltype user-data/data
NAME            PROPERTY  VALUE     SOURCE
user-data/data  acltype   posixacl  local
Comment 7 Sergei Trofimovich gentoo-dev 2019-07-04 21:04:19 UTC
What does test from #comment5 yield for you when ran on those filesystems?
Comment 8 crocket 2019-07-05 04:40:41 UTC
On ZFS, I get the following result.

> ./a
current umask = 022
fstat(): in_mode=0777, resulting_mode=0100777
stat():  in_mode=0777, resulting_mode=0100777
current umask = 077
fstat(): in_mode=0777, resulting_mode=0100777
stat():  in_mode=0777, resulting_mode=0100777
Comment 9 Sergei Trofimovich gentoo-dev 2019-07-05 07:43:35 UTC
Looks like you found a zfs-specific bug. CC-ing maintainers.

Tl;DR: test from #comment5 demonstrates deviation from shipped linux filesystems and causes O_TMPFILE files relinked into named files have permission mask too broad.

I would say it's a security bug.
Comment 10 Sergei Trofimovich gentoo-dev 2019-07-05 07:48:10 UTC
Please provide 'emerge --info sys-fs/zfs sys-fs/zfs-kmod'.
Comment 11 Sergei Trofimovich gentoo-dev 2019-07-05 10:50:09 UTC
Confirmed [1] on a virtual machine:
  FAILS: /zp (ZFS):fails on ZFS
  WORKS: /home/root (EXT4)

On https://github.com/MusicPlayerDaemon/MPD/issues/558 Max Kellermann found out it is a mix of ACL support presence in kernel and filesystem-specific handling of vnode cretion.

CC-ing kernel@ as well.

I'm leaving this bug to ZFS and kernel Gentoo maintainers. Please decide who is best to own this bug.

[1] test of #comment5 program:

localhost /zp # gcc a.c -o a && ./a
current umask = 022
fstat(): in_mode=0777, resulting_mode=0100777
stat():  in_mode=0777, resulting_mode=0100777
current umask = 077
fstat(): in_mode=0777, resulting_mode=0100777
stat():  in_mode=0777, resulting_mode=0100777

localhost /zp # cd
localhost ~ # /zp/a
current umask = 022
fstat(): in_mode=0777, resulting_mode=0100755
stat():  in_mode=0777, resulting_mode=0100755
current umask = 077
fstat(): in_mode=0777, resulting_mode=0100700
stat():  in_mode=0777, resulting_mode=0100700

localhost ~ # mount
/dev/sda1 on / type ext4 (rw,relatime)
zp on /zp type zfs (rw,xattr,noacl)
...
Comment 12 Sergei Trofimovich gentoo-dev 2019-07-05 10:52:01 UTC
Created attachment 581946 [details]
emerge --info
Comment 13 Sergei Trofimovich gentoo-dev 2019-07-05 10:52:32 UTC
Created attachment 581948 [details]
config-4.19.52-gentoo kernel config
Comment 14 crocket 2019-07-06 07:11:03 UTC
Created attachment 582012 [details]
emerge --info sys-fs/zfs sys-fs/zfs-kmod
Comment 15 Georgy Yakovlev gentoo-dev 2019-07-06 16:44:11 UTC
I'll keep looking with other settings, but I can't reproduce on zfs right now on my pool.

./a.out 
current umask = 022
fstat(): in_mode=0777, resulting_mode=0100755
stat():  in_mode=0777, resulting_mode=0100755
current umask = 077
fstat(): in_mode=0777, resulting_mode=0100700
stat():  in_mode=0777, resulting_mode=0100700


zroot/ROOT/root on / type zfs (rw,relatime,xattr,posixacl)
NAME   PROPERTY  VALUE     SOURCE
zroot  acltype   posixacl  local
zroot  xattr     sa     local
Comment 16 Sergei Trofimovich gentoo-dev 2019-07-06 20:31:28 UTC
(In reply to Georgy Yakovlev from comment #15)
> I'll keep looking with other settings, but I can't reproduce on zfs right
> now on my pool.
> 
> ./a.out 
> current umask = 022
> fstat(): in_mode=0777, resulting_mode=0100755
> stat():  in_mode=0777, resulting_mode=0100755
> current umask = 077
> fstat(): in_mode=0777, resulting_mode=0100700
> stat():  in_mode=0777, resulting_mode=0100700
> 
> 
> zroot/ROOT/root on / type zfs (rw,relatime,xattr,posixacl)
> NAME   PROPERTY  VALUE     SOURCE
> zroot  acltype   posixacl  local
> zroot  xattr     sa     local

I guess it's because difference in noacl/posixacl mount options. No idea how to influence it on zfs. Maybe set some filesystem property?
Comment 17 Sergei Trofimovich gentoo-dev 2019-07-06 20:48:17 UTC
> I guess it's because difference in noacl/posixacl mount options. No idea how
> to influence it on zfs. Maybe set some filesystem property?

Probably via 'zfs set acltype=noacl zroot/ROOT/root' (or better newly created fliesystem).
Comment 18 Sergei Trofimovich gentoo-dev 2019-07-06 20:50:18 UTC
Seems to trigger at least for me:

localhost /zp # zfs set acltype=posixacl zp
localhost /zp # gcc a.c -o a && ./a
current umask = 022
fstat(): in_mode=0777, resulting_mode=0100755
stat():  in_mode=0777, resulting_mode=0100755
current umask = 077
fstat(): in_mode=0777, resulting_mode=0100700
stat():  in_mode=0777, resulting_mode=0100700

localhost /zp # zfs set acltype=noacl zp
localhost /zp # gcc a.c -o a && ./a
current umask = 022
fstat(): in_mode=0777, resulting_mode=0100777
stat():  in_mode=0777, resulting_mode=0100777
current umask = 077
fstat(): in_mode=0777, resulting_mode=0100777
stat():  in_mode=0777, resulting_mode=0100777
Comment 19 Georgy Yakovlev gentoo-dev 2019-07-06 20:56:20 UTC
indeed, 

current umask = 022
fstat(): in_mode=0777, resulting_mode=0100777
stat():  in_mode=0777, resulting_mode=0100777
current umask = 077
fstat(): in_mode=0777, resulting_mode=0100777
stat():  in_mode=0777, resulting_mode=0100777

on freshly created pool with default settings

zfs get acltype tmpfile
NAME     PROPERTY  VALUE     SOURCE
tmpfile  acltype   off       default

tmpfile on /tmpfile type zfs (rw,xattr,noacl)
Comment 20 Sergei Trofimovich gentoo-dev 2019-07-07 08:40:01 UTC
Looks like there is a pull request against zfs code available:
    https://github.com/zfsonlinux/zfs/pull/8998
Reassigning to zfs maintainers.
Comment 21 Georgy Yakovlev gentoo-dev 2019-07-09 18:08:16 UTC
while the initial patch works for me, there's an ongoing discussion.
will backport this to 0.8.1 (and maybe 0.7.x) as soon as it gets merged to master  upstream after all review and buildbot testing.
Comment 22 Georgy Yakovlev gentoo-dev 2019-09-30 23:41:05 UTC
just checking in, work upstream still in progress
Comment 23 Larry the Git Cow gentoo-dev 2019-12-13 23:43:14 UTC
The bug has been referenced in the following commit(s):

https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=2b66902c411859f19d0ee76b38efcd162d6b3f8f

commit 2b66902c411859f19d0ee76b38efcd162d6b3f8f
Author:     Georgy Yakovlev <gyakovlev@gentoo.org>
AuthorDate: 2019-12-13 23:41:16 +0000
Commit:     Georgy Yakovlev <gyakovlev@gentoo.org>
CommitDate: 2019-12-13 23:42:23 +0000

    sys-fs/zfs-kmod: revbump 0.8.2, fix O_TMPFILE umask ignore
    
    Bug: https://bugs.gentoo.org/686142
    Package-Manager: Portage-2.3.79, Repoman-2.3.17
    Signed-off-by: Georgy Yakovlev <gyakovlev@gentoo.org>

 sys-fs/zfs-kmod/files/0.8.2-umask_O_TMPFILE.patch  | 34 ++++++++++++++++++++++
 ...od-0.8.2-r1.ebuild => zfs-kmod-0.8.2-r2.ebuild} |  2 ++
 2 files changed, 36 insertions(+)
Comment 24 crocket 2019-12-14 04:38:09 UTC
The upstream issue was fixed.
Comment 25 Georgy Yakovlev gentoo-dev 2019-12-14 19:56:27 UTC
Yeah, already in gentoo for 0.8.2

Do we need a backport to 0.7.x ? I haven’t looked yet if the bug is there and moved all my systems to modern branch.
Comment 26 Sergei Trofimovich gentoo-dev 2020-01-04 00:01:07 UTC
I'm a bit mixed on backporting (and not a zfs user myself). On one hand (no backport) it's obscure enough and not a regression. On the other hand (yes backport) it's a security hole.
Comment 27 Georgy Yakovlev gentoo-dev 2020-01-04 01:39:14 UTC
I'm also not a fan of going full redhat =D

but this both look simple enough change and quite important.

I just need to set up non 0.8x machine to test...
Comment 28 Larry the Git Cow gentoo-dev 2020-05-15 03:50:19 UTC
The bug has been referenced in the following commit(s):

https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=8f41bec723fda7d56c431ed14fda7cf2c9990da9

commit 8f41bec723fda7d56c431ed14fda7cf2c9990da9
Author:     Georgy Yakovlev <gyakovlev@gentoo.org>
AuthorDate: 2020-05-15 03:44:16 +0000
Commit:     Georgy Yakovlev <gyakovlev@gentoo.org>
CommitDate: 2020-05-15 03:49:41 +0000

    profiles/package.mask: mask zfs-0.7.x and co
    
    Bug: https://bugs.gentoo.org/686142
    Bug: https://bugs.gentoo.org/723120
    
    Signed-off-by: Georgy Yakovlev <gyakovlev@gentoo.org>

 profiles/package.mask | 8 ++++++++
 1 file changed, 8 insertions(+)
Comment 29 Georgy Yakovlev gentoo-dev 2020-05-15 03:51:43 UTC
I tried back-porting but never got to run a testsuite on old version with the patch.
anyway, time has come, masking 0.7.x releases
Comment 30 Georgy Yakovlev gentoo-dev 2020-06-10 21:55:26 UTC
old versions removed in https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=b34316d01d3730756778f0b15e7fe6b2c50d255f

closing.