Don't fail to apply umask for O_TMPFILE files #8998

kusumi · 2019-07-07T08:08:29Z

Motivation and Context

Description

Apply umask to mode which will eventually be applied to inode.
This is needed since VFS doesn't apply umask for O_TMPFILE files.

(Note that zpl_init_acl() applies ip->i_mode &= ~current_umask();
only when POSIX ACL is used.)

How Has This Been Tested?

With the code pasted in #8997.

# gcc -Wall -g ./tmpfile1.c
# zpool create p1 sdb
# cd /p1
# ~/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

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
All new and existing tests passed.
All commit messages are properly formatted and contain Signed-off-by.

codecov · 2019-07-07T16:50:22Z

Codecov Report

Merging #8998 into master will decrease coverage by 0.18%.
The diff coverage is 4.76%.

@@            Coverage Diff             @@
##           master    #8998      +/-   ##
==========================================
- Coverage   79.52%   79.33%   -0.19%     
==========================================
  Files         418      419       +1     
  Lines      123544   123586      +42     
==========================================
- Hits        98247    98047     -200     
- Misses      25297    25539     +242

Flag	Coverage Δ
#kernel	`80.02% <100%> (-0.13%)`	⬇️
#user	`66.98% <0%> (-0.4%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e69bb31...bb1daae. Read the comment docs.

behlendorf · 2019-07-08T19:52:06Z

module/zfs/zpl_inode.c

@@ -231,6 +231,7 @@ zpl_tmpfile(struct inode *dir, struct dentry *dentry, zpl_umode_t mode)
 	cookie = spl_fstrans_mark();
 	error = -zfs_tmpfile(dir, vap, 0, mode, &ip, cr, 0, NULL);
 	if (error == 0) {
+		ip->i_mode &= ~current_umask();


As I understand it, the core issue here is that unlike .create we can't rely of the VFS apply the umask for .tmpfile, so we need to handle it ourselves. This fix makes sense but I think we should apply the mask prior to the zpl_vap_init() call, this should work and is more consistent with all other callers. How about something like this (untested).

/* * The VFS does not apply the umask to the provided mode. * Therefore, it must be applied at the filesystem layer. */ mode &= ~current_umask(); zpl_vap_init(vap, dir, mode, cr);

As I understand it, the core issue here is that unlike .create we can't rely of the VFS apply the umask for .tmpfile, so we need to handle it ourselves.

yes, and even if VFS supports it at some point, I don't think this change (mode &= ~umask) will bother that.

This fix makes sense but I think we should apply the mask prior to the zpl_vap_init() call, this should work and is more consistent with all other callers. How about something like this (untested).

ok, that might be better.

Updated + rebased + tested.

Added test case under functional/tmpfile.

behlendorf

Thanks for adding the test case.

behlendorf · 2019-07-09T20:04:41Z

module/zfs/zpl_inode.c

@@ -226,6 +226,11 @@ zpl_tmpfile(struct inode *dir, struct dentry *dentry, zpl_umode_t mode)

 	crhold(cr);
 	vap = kmem_zalloc(sizeof (vattr_t), KM_SLEEP);
+	/*
+	 * The VFS does not apply the umask to the provided mode as of 5.2.
+	 * Therefore, it must be applied at the filesystem layer.


Was this really a change in the 5.2 kernel, or does it predate it? That shouldn't matter for the fix itself, but I'm curious.

This has nothing to do with version 5.2, this bug is a lot older, and may even be older than O_TMPFILE itself.

btw. the rest of the Linux kernel applies the umask only if there's no ACL, e.g.:
https://github.com/torvalds/linux/blob/v5.2/fs/posix_acl.c#L596-L597
https://github.com/torvalds/linux/blob/v5.2/fs/namei.c#L3176-L3177
This PR applies it unconditionally.

Was this really a change in the 5.2 kernel, or does it predate it?

I meant to say it has been there and still not supported by VFS in 5.2.

Thanks for pointing out that the kernel only applies this when there is no ACL. @kusumi what do you think about the follow modification.

index 3f3b2e2..9854791 100644 --- a/module/zfs/zpl_inode.c +++ b/module/zfs/zpl_inode.c @@ -226,6 +226,15 @@ zpl_tmpfile(struct inode *dir, struct dentry *dentry, zpl_u crhold(cr); vap = kmem_zalloc(sizeof (vattr_t), KM_SLEEP); + /* + * The VFS does not apply the umask, therefore it is applied here + * when POSIX ACLs are not enabled. + */ + if (!IS_POSIXACL(dir->d_inode)) + mode &= ~current_umask(); + zpl_vap_init(vap, dir, mode, cr); cookie = spl_fstrans_mark();

Then I guess we also want to change ip->i_mode &= ~current_umask(); in zpl_init_acl() side too ? It sounds like ZoL was doing opposite of what VFS expects.

zfs seems to be ignoring umask if files created with O_TMPFILE unless acltype=posixacl

I'll check my diff and Linux VFS code again soon. If needed I'll make the change and rebase again.

Applied !IS_POSIXACL() conditional + rebased.

tonyhutter · 2019-10-17T15:49:56Z

If you can rebase this on master, I can take a look at it.

kusumi · 2019-10-26T00:12:02Z

Thanks, I will in next week or so.

kusumi · 2019-11-27T15:23:57Z

Rebased. Haven't ran tests/zfs-tests/tests/functional/tmpfile/tmpfile_stat_mode.c on my local after rebase, but let's see how it goes.

Apply umask to `mode` which will eventually be applied to inode. This is needed since VFS doesn't apply umask for O_TMPFILE files. (Note that zpl_init_acl() applies `ip->i_mode &= ~current_umask();` only when POSIX ACL is used.) Signed-off-by: Tomohiro Kusumi <[email protected]>

kusumi · 2019-12-07T01:57:43Z

@behlendorf Looks like tests are seeing ENOSPC this time, whereas in previous rebase on Nov 28 tests went fine IIRC.

behlendorf · 2019-12-07T02:37:48Z

@kusumi probably a CI related hiccup, I've resubmitted the those builds.

kusumi · 2019-12-08T01:39:42Z

@behlendorf Thanks. Looks to me this is ready to merge.

behlendorf

Thanks for updating this, looks good. @tonyhutter can you please take a look at this too.

Apply umask to `mode` which will eventually be applied to inode. This is needed since VFS doesn't apply umask for O_TMPFILE files. (Note that zpl_init_acl() applies `ip->i_mode &= ~current_umask();` only when POSIX ACL is used.) Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Tony Hutter <[email protected]> Signed-off-by: Tomohiro Kusumi <[email protected]> Closes openzfs#8997 Closes openzfs#8998

Apply umask to `mode` which will eventually be applied to inode. This is needed since VFS doesn't apply umask for O_TMPFILE files. (Note that zpl_init_acl() applies `ip->i_mode &= ~current_umask();` only when POSIX ACL is used.) Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Tony Hutter <[email protected]> Signed-off-by: Tomohiro Kusumi <[email protected]> Closes #8997 Closes #8998

kusumi force-pushed the acl1 branch from 104b9d2 to eb58787 Compare July 7, 2019 08:25

behlendorf requested changes Jul 8, 2019

View reviewed changes

behlendorf added the Status: Code Review Needed Ready for review and testing label Jul 8, 2019

kusumi force-pushed the acl1 branch from eb58787 to a14d19d Compare July 9, 2019 08:23

kusumi changed the title ~~Don't fail to umask for O_TMPFILE files~~ Don't fail to apply umask for O_TMPFILE files Jul 9, 2019

kusumi force-pushed the acl1 branch 4 times, most recently from 944b19f to 0782977 Compare July 9, 2019 16:13

behlendorf approved these changes Jul 9, 2019

View reviewed changes

kusumi force-pushed the acl1 branch from 0782977 to 0e7bbe9 Compare July 16, 2019 14:24

behlendorf requested a review from tonyhutter July 30, 2019 23:03

behlendorf added the Status: Revision Needed Changes are required for the PR to be accepted label Nov 13, 2019

kusumi force-pushed the acl1 branch from 0e7bbe9 to 2e26e90 Compare November 27, 2019 15:21

behlendorf removed the Status: Revision Needed Changes are required for the PR to be accepted label Nov 28, 2019

kusumi force-pushed the acl1 branch from 2e26e90 to bb1daae Compare December 3, 2019 17:11

behlendorf approved these changes Dec 9, 2019

View reviewed changes

tonyhutter approved these changes Dec 13, 2019

View reviewed changes

behlendorf merged commit ddb4e69 into openzfs:master Dec 13, 2019

behlendorf mentioned this pull request Oct 6, 2021

zfs ignores umask for nfs4.2 mounted files unless acltype=posixacl #10504

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't fail to apply umask for O_TMPFILE files #8998

Don't fail to apply umask for O_TMPFILE files #8998

kusumi commented Jul 7, 2019 •

edited

Loading

codecov bot commented Jul 7, 2019 •

edited

Loading

behlendorf Jul 8, 2019

kusumi Jul 9, 2019

kusumi Jul 9, 2019

kusumi Jul 9, 2019

behlendorf left a comment

behlendorf Jul 9, 2019

MaxKellermann Jul 9, 2019

MaxKellermann Jul 9, 2019

kusumi Jul 10, 2019

behlendorf Jul 26, 2019

kusumi Jul 27, 2019

kusumi Nov 28, 2019

kusumi Dec 3, 2019

tonyhutter commented Oct 17, 2019

kusumi commented Oct 26, 2019

kusumi commented Nov 27, 2019

kusumi commented Dec 7, 2019

behlendorf commented Dec 7, 2019

kusumi commented Dec 8, 2019

behlendorf left a comment

Don't fail to apply umask for O_TMPFILE files #8998

Don't fail to apply umask for O_TMPFILE files #8998

Conversation

kusumi commented Jul 7, 2019 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

codecov bot commented Jul 7, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behlendorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyhutter commented Oct 17, 2019

kusumi commented Oct 26, 2019

kusumi commented Nov 27, 2019

kusumi commented Dec 7, 2019

behlendorf commented Dec 7, 2019

kusumi commented Dec 8, 2019

behlendorf left a comment

Choose a reason for hiding this comment

kusumi commented Jul 7, 2019 •

edited

Loading

codecov bot commented Jul 7, 2019 •

edited

Loading