[RFC] Apply the umask in VFS optionally (also POSIX ACL kernel infrastructure)

[RFC] Apply the umask in VFS optionally (also POSIX ACL kernel infrastructure)

[Andreas Gruenbacher]

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

Hello,

[This is a copy of a previous posting to linux-kernel.]

The umask(2) is usually applied to the mode parameters to open(2), mkdir(2),
creat(2) and mknod(2) in the VFS. With POSIX Access Control Lists the umask
must only be applied in some situations, and must have no effect in others.
Currently there is no way for a file system to find out the original mode
parameter passed to the system calls.

There are two ways to solve this problem, namely, to (1) move the code that
applies the umask into the file systems, or to (2) apply the umask in the VFS
optionally only. Option (1) is intrusive on existing file systems, and might
introduce bugs, while (2) slightly complicates the VFS, but leaves file
systems unaffected.

I believe that (2) is the more reasonable choice in this case, so I propose
this patch, which adds the MS_NOUMASK mount option. The flag is set by the
file system, if the file system does not want the VFS to apply the umask,
after which the file system itself is responsible for applying the umask
where appropriate.

Finally, I have a question related to this. We had a bug with kernel tasks,
which don't have a umask associated with them (nfsd in particular). Should
kernel tasks that create files be required to have a valid fs_struct (which
includes the umask), or should this be special cased in file systems?

Regards,
Andreas.

------------------------------------------------------------------
 Andreas Gruenbacher                                SuSE Linux AG
 mailto:agruen@suse.de                     Deutschherrnstr. 15-19
 http://www.suse.de/                   D-90429 Nuernberg, Germany

[-- Attachment #2: linux-2.5.30-ms_noumask.diff --]
[-- Type: text/x-diff, Size: 2152 bytes --]

Apply the umask in VFS optionally

This patch adds the MS_NOUMASK mount option. This mount option is set by the file system, if the file system does not want the VFS to apply the umask.

diff -Nur linux-2.5.30/include/linux/fs.h linux-2.5.30.patch/include/linux/fs.h
--- linux-2.5.30/include/linux/fs.h	Thu Aug  1 23:16:15 2002
+++ linux-2.5.30.patch/include/linux/fs.h	Fri Aug  2 15:21:29 2002
@@ -110,6 +110,7 @@
 #define MS_MOVE		8192
 #define MS_REC		16384
 #define MS_VERBOSE	32768
+#define MS_NOUMASK	(1<<16) /* VFS does not apply the umask */
 #define MS_ACTIVE	(1<<30)
 #define MS_NOUSER	(1<<31)
 
@@ -164,6 +165,7 @@
 #define IS_IMMUTABLE(inode)	((inode)->i_flags & S_IMMUTABLE)
 #define IS_NOATIME(inode)	(__IS_FLG(inode, MS_NOATIME) || ((inode)->i_flags & S_NOATIME))
 #define IS_NODIRATIME(inode)	__IS_FLG(inode, MS_NODIRATIME)
+#define IS_NOUMASK(inode)	__IS_FLG(inode, MS_NOUMASK)
 
 #define IS_DEADDIR(inode)	((inode)->i_flags & S_DEAD)
 
diff -Nur linux-2.5.30/fs/namei.c linux-2.5.30.patch/fs/namei.c
--- linux-2.5.30/fs/namei.c	Thu Aug  1 23:16:18 2002
+++ linux-2.5.30.patch/fs/namei.c	Fri Aug  2 15:35:10 2002
@@ -1279,8 +1279,9 @@
 
 	/* Negative dentry, just create the file */
 	if (!dentry->d_inode) {
-		error = vfs_create(dir->d_inode, dentry,
-				   mode & ~current->fs->umask);
+		if (!IS_NOUMASK(dir->d_inode))
+			mode &= ~current->fs->umask;
+		error = vfs_create(dir->d_inode, dentry, mode);
 		up(&dir->d_inode->i_sem);
 		dput(nd->dentry);
 		nd->dentry = dentry;
@@ -1442,7 +1443,8 @@
 	dentry = lookup_create(&nd, 0);
 	error = PTR_ERR(dentry);
 
-	mode &= ~current->fs->umask;
+	if (!IS_NOUMASK(nd.dentry->d_inode))
+		mode &= ~current->fs->umask;
 	if (!IS_ERR(dentry)) {
 		switch (mode & S_IFMT) {
 		case 0: case S_IFREG:
@@ -1508,8 +1510,9 @@
 		dentry = lookup_create(&nd, 1);
 		error = PTR_ERR(dentry);
 		if (!IS_ERR(dentry)) {
-			error = vfs_mkdir(nd.dentry->d_inode, dentry,
-					  mode & ~current->fs->umask);
+			if (!IS_NOUMASK(nd.dentry->d_inode))
+				mode &= ~current->fs->umask;
+			error = vfs_mkdir(nd.dentry->d_inode, dentry, mode);
 			dput(dentry);
 		}
 		up(&nd.dentry->d_inode->i_sem);

Re: [RFC] Apply the umask in VFS optionally (also POSIX ACL kernel infrastructure)

[Christoph Hellwig]

On Sun, Aug 04, 2002 at 03:46:43PM +0200, Andreas Gruenbacher wrote:
> I believe that (2) is the more reasonable choice in this case, so I propose
> this patch, which adds the MS_NOUMASK mount option. The flag is set by the
> file system, if the file system does not want the VFS to apply the umask,
> after which the file system itself is responsible for applying the umask
> where appropriate.

In the current XFS trees we have that flag as IS_POSIXACL() and S_POSIXACL
inod flag. (And I think some of your 2.4 patches do the same).  After some
thinking a per-superblock flag looks best to me, but instead of naming it
MS_NOUMASK I'd really, really prefer MS_POSIXACL.  Rationale:

(1) there is not much other reasoning why we shouldn't apply the umask
(2) this way lowlevel filesystem drivers can enabled/disable acl
    per-mountpoint or even on-the-fly (if remount implements it)

> Finally, I have a question related to this. We had a bug with kernel tasks,
> which don't have a umask associated with them (nfsd in particular). Should
> kernel tasks that create files be required to have a valid fs_struct (which
> includes the umask), or should this be special cased in file systems?

Anything that deals with files shall have a valid fs_struct.  The number
of in-kernel threads that are supposed to deal with files is extremly
low (only in-kernel fileservers like nfsd, tux or the now dead khttpd)
and not worth workarounds in filesystem code.  Not to mention it is much
more elegant.

Re: [RFC] Apply the umask in VFS optionally (also POSIX ACL kernel infrastructure)

[Andreas Gruenbacher]

On Sunday 04 August 2002 16:42, Christoph Hellwig wrote:
> On Sun, Aug 04, 2002 at 03:46:43PM +0200, Andreas Gruenbacher wrote:
> > I believe that (2) is the more reasonable choice in this case, so I
> > propose this patch, which adds the MS_NOUMASK mount option. The flag is
> > set by the file system, if the file system does not want the VFS to apply
> > the umask, after which the file system itself is responsible for applying
> > the umask where appropriate.
>
> In the current XFS trees we have that flag as IS_POSIXACL() and S_POSIXACL
> inode flag. (And I think some of your 2.4 patches do the same).  After some
> thinking a per-superblock flag looks best to me, but instead of naming it
> MS_NOUMASK I'd really, really prefer MS_POSIXACL.

Well, the previous macro was IS_POSIX_ACL(inode); it was testing a super block 
flag. We had an additional s_xattr_flags in the super block:


--- linux/include/linux/fs.h.orig	Thu Aug  1 11:17:45 2002
+++ linux/include/linux/fs.h	Thu Aug  1 11:32:22 2002
@@ -117,6 +117,21 @@
 #define MS_NOUSER	(1<<31)
 
 /*
+ * These are the super block s_ext_attr_flags
+ */
+#define XATTR_MNT_FLAG_ALL		1	/* Extended attributes */
+#define XATTR_MNT_FLAG_USER		1	/* Extended user attributes */
+#define XATTR_MNT_FLAG_POSIX_ACL	2	/* Access Control Lists */
+
+#define __IS_XATTR_FLG(inode,flg) \
+	((inode)->i_sb && \
+	 (inode)->i_sb->s_xattr_flags & (XATTR_MNT_FLAG_ ## flg))
+
+#define IS_XATTR(inode)		__IS_XATTR_FLG(inode, ALL)
+#define IS_XATTR_USER(inode)	__IS_XATTR_FLG(inode, XATTR_USER)
+#define IS_POSIX_ACL(inode)	__IS_XATTR_FLG(inode, POSIX_ACL)
+
+/*
  * Superblock flags that can be altered by MS_REMOUNT
  */
 #define MS_RMT_MASK	(MS_RDONLY|MS_SYNCHRONOUS|MS_MANDLOCK|MS_NOATIME|\
@@ -785,6 +800,7 @@
 	struct super_operations	*s_op;
 	struct dquot_operations	*dq_op;
 	unsigned long		s_flags;
+	unsigned long		s_xattr_flags;
 	unsigned long		s_magic;
 	struct dentry		*s_root;
 	struct rw_semaphore	s_umount;


The MS_NOUMASK flag is in s_flags, and so doesn't require an additinal field. 
In the Ext2/Ext3 patch that I'm working on, there is a per-filesystem mount 
flag for POSIX ACLs (and also for USER EAs), which is being synchronized with 
MS_NOUMASK.

> Rationale:
>
> (1) there is not much other reasoning why we shouldn't apply the umask
> (2) this way lowlevel filesystem drivers can enabled/disable acl
>     per-mountpoint or even on-the-fly (if remount implements it)

>From the point of the VFS it's just `don't apply the umask'; the VFS doesn't 
need to care about POSIX ACLs otherwise. (And who knows if there won't be 
another case where this is needed?) The low-level filesystem can still 
enable/disable ACLs per mount point or even per inode, it only needs to take 
care of the umask itself.

> > Finally, I have a question related to this. We had a bug with kernel
> > tasks, which don't have a umask associated with them (nfsd in
> > particular). Should kernel tasks that create files be required to have a
> > valid fs_struct (which includes the umask), or should this be special
> > cased in file systems?
>
> Anything that deals with files shall have a valid fs_struct.  The number
> of in-kernel threads that are supposed to deal with files is extremly
> low (only in-kernel fileservers like nfsd, tux or the now dead khttpd)
> and not worth workarounds in filesystem code.  Not to mention it is much
> more elegant.

That sounds very reasonable.

Regards,
Andreas.

------------------------------------------------------------------
 Andreas Gruenbacher                                SuSE Linux AG
 mailto:agruen@suse.de                     Deutschherrnstr. 15-19
 http://www.suse.de/                   D-90429 Nuernberg, Germany

Re: [RFC] Apply the umask in VFS optionally (also POSIX ACL kernel infrastructure)

[Christoph Hellwig]

On Mon, Aug 05, 2002 at 01:41:28PM +0200, Andreas Gruenbacher wrote:
> Well, the previous macro was IS_POSIX_ACL(inode); it was testing a super block 
> flag. We had an additional s_xattr_flags in the super block:

XFS tree is currently a little different:

--- 2.4.19-xfs/include/linux/fs.h
+++ 2.4.19-xfs/include/linux/fs.h
@@ -135,6 +135,7 @@
 #define S_IMMUTABLE	16	/* Immutable file */
 #define S_DEAD		32	/* removed, but still open directory */
 #define S_NOQUOTA	64	/* Inode is not counted to quota */
+#define S_POSIXACL	128	/* Defer applying umask to mode bits */
 
 /*
  * Note that nosuid etc flags are inode-specific: setting some file-system
@@ -163,6 +164,7 @@
 #define IS_NODIRATIME(inode)	__IS_FLG(inode, MS_NODIRATIME)
 
 #define IS_DEADDIR(inode)	((inode)->i_flags & S_DEAD)
+#define IS_POSIXACL(inode)	((inode)->i_flags & S_POSIXACL)
 
 /* the read-only stuff doesn't really belong here, but any other place is
    probably as bad and I don't want to create yet another include file. */
 

Id on't like the per-inode thinkgy, though.  A per-superblock flag
(without s_xattr_flags) is most reasonable in my eyes.