NFSv3 has no concept of a umask on the server side: The client applies
the umask locally, and sends the effective permissions to the server.
This behavior is wrong when files are created in a directory that has
a default ACL. In this case, the umask is supposed to be ignored, and
only the default ACL determines the file's effective permissions.
Usually its the server's task to conditionally apply the umask. But
since the server knows nothing about the umask, we have to do it on the
client side. This patch tries to fetch the parent directory's default
ACL before creating a new file, computes the appropriate create mode to
send to the server, and finally sets the new file's access and default
acl appropriately.
Many thanks to Buck Huppmann <[email protected]> for sending the initial
version of this patch, as well as for arguing why we need this change.
Signed-off-by: Andreas Gruenbacher <[email protected]>
Acked-by: Olaf Kirch <[email protected]>
Index: linux-2.6.11-rc2/fs/nfs/dir.c
===================================================================
--- linux-2.6.11-rc2.orig/fs/nfs/dir.c
+++ linux-2.6.11-rc2/fs/nfs/dir.c
@@ -31,6 +31,7 @@
#include <linux/pagemap.h>
#include <linux/smp_lock.h>
#include <linux/namei.h>
+#include <linux/posix_acl.h>
#include "delegation.h"
@@ -976,6 +977,38 @@ out_err:
return error;
}
+static int nfs_set_default_acl(struct inode *dir, struct inode *inode,
+ mode_t mode)
+{
+#ifdef CONFIG_NFS_ACL
+ struct posix_acl *dfacl, *acl;
+ int error = 0;
+
+ dfacl = NFS_PROTO(dir)->getacl(dir, ACL_TYPE_DEFAULT);
+ if (IS_ERR(dfacl)) {
+ error = PTR_ERR(dfacl);
+ return (error == -EOPNOTSUPP) ? 0 : error;
+ }
+ if (!dfacl)
+ return 0;
+ acl = posix_acl_clone(dfacl, GFP_KERNEL);
+ error = -ENOMEM;
+ if (!acl)
+ goto out;
+ error = posix_acl_create_masq(acl, &mode);
+ if (error < 0)
+ goto out;
+ error = NFS_PROTO(inode)->setacls(inode, acl, S_ISDIR(inode->i_mode) ?
+ dfacl : NULL);
+out:
+ posix_acl_release(acl);
+ posix_acl_release(dfacl);
+ return error;
+#else
+ return 0;
+#endif
+}
+
/*
* Following a failed create operation, we drop the dentry rather
* than retain a negative dentry. This avoids a problem in the event
@@ -993,7 +1026,7 @@ static int nfs_create(struct inode *dir,
dfprintk(VFS, "NFS: create(%s/%ld, %s\n", dir->i_sb->s_id,
dir->i_ino, dentry->d_name.name);
- attr.ia_mode = mode;
+ attr.ia_mode = mode & ~current->fs->umask;
attr.ia_valid = ATTR_MODE;
if (nd && (nd->flags & LOOKUP_CREATE))
@@ -1007,7 +1040,7 @@ static int nfs_create(struct inode *dir,
d_instantiate(dentry, inode);
nfs_renew_times(dentry);
nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
- error = 0;
+ error = nfs_set_default_acl(dir, inode, mode);
} else {
error = PTR_ERR(inode);
d_drop(dentry);
@@ -1033,7 +1066,7 @@ nfs_mknod(struct inode *dir, struct dent
if (!new_valid_dev(rdev))
return -EINVAL;
- attr.ia_mode = mode;
+ attr.ia_mode = mode & ~current->fs->umask;
attr.ia_valid = ATTR_MODE;
lock_kernel();
@@ -1045,6 +1078,8 @@ nfs_mknod(struct inode *dir, struct dent
error = nfs_instantiate(dentry, &fhandle, &fattr);
else
d_drop(dentry);
+ if (!error)
+ error = nfs_set_default_acl(dir, dentry->d_inode, mode);
unlock_kernel();
return error;
}
@@ -1063,7 +1098,7 @@ static int nfs_mkdir(struct inode *dir,
dir->i_ino, dentry->d_name.name);
attr.ia_valid = ATTR_MODE;
- attr.ia_mode = mode | S_IFDIR;
+ attr.ia_mode = (mode & ~current->fs->umask) | S_IFDIR;
lock_kernel();
#if 0
@@ -1083,6 +1118,8 @@ static int nfs_mkdir(struct inode *dir,
error = nfs_instantiate(dentry, &fhandle, &fattr);
else
d_drop(dentry);
+ if (!error)
+ error = nfs_set_default_acl(dir, dentry->d_inode, mode);
unlock_kernel();
return error;
}
Index: linux-2.6.11-rc2/fs/nfs/inode.c
===================================================================
--- linux-2.6.11-rc2.orig/fs/nfs/inode.c
+++ linux-2.6.11-rc2/fs/nfs/inode.c
@@ -1494,6 +1494,8 @@ static struct super_block *nfs_get_sb(st
return ERR_PTR(error);
}
s->s_flags |= MS_ACTIVE;
+ /* The nfs client applies the umask itself when needed. */
+ s->s_flags |= MS_POSIXACL;
return s;
}
--
Andreas Gruenbacher <[email protected]>
SUSE Labs, SUSE LINUX PRODUCTS GMBH
Hello,
this patch has an NFSv2 problem that I haven't tripped over until today. The
fix is this:
------- 8< -------
Fix NFSv2 null pointer access
With NFSv2 we would try to follow a NULL getacl and setacl function
pointer here. Add the missing checks.
Signed-off-by: Andreas Gruenbacher <[email protected]>
Index: linux-2.6.10/fs/nfs/dir.c
===================================================================
--- linux-2.6.10.orig/fs/nfs/dir.c
+++ linux-2.6.10/fs/nfs/dir.c
@@ -984,6 +984,9 @@ static int nfs_set_default_acl(struct in
struct posix_acl *dfacl, *acl;
int error = 0;
+ if (NFS_PROTO(inode)->version != 3 ||
+ !NFS_PROTO(dir)->getacl || !NFS_PROTO(inode)->setacls)
+ return 0;
dfacl = NFS_PROTO(dir)->getacl(dir, ACL_TYPE_DEFAULT);
if (IS_ERR(dfacl)) {
error = PTR_ERR(dfacl);
Regards,
--
Andreas Gruenbacher <[email protected]>
SUSE Labs, SUSE LINUX PRODUCTS GMBH
lau den 22.01.2005 Klokka 21:34 (+0100) skreiv Andreas Gruenbacher:
> vanlig tekstdokument vedlegg (patches.suse)
> NFSv3 has no concept of a umask on the server side: The client applies
> the umask locally, and sends the effective permissions to the server.
> This behavior is wrong when files are created in a directory that has
> a default ACL. In this case, the umask is supposed to be ignored, and
> only the default ACL determines the file's effective permissions.
>
> Usually its the server's task to conditionally apply the umask. But
> since the server knows nothing about the umask, we have to do it on the
> client side. This patch tries to fetch the parent directory's default
> ACL before creating a new file, computes the appropriate create mode to
> send to the server, and finally sets the new file's access and default
> acl appropriately.
Firstly, this sort of code belongs in the NFSv3-specific code. POSIX
acls have no business whatsoever in the generic NFS code.
Secondly, what is the point of doing all this *after* you have created
the file with the wrong permissions? How are you avoiding races?
Cheers,
Trond
--
Trond Myklebust <[email protected]>
On Tue, 2005-02-15 at 19:04, Trond Myklebust wrote:
> lau den 22.01.2005 Klokka 21:34 (+0100) skreiv Andreas Gruenbacher:
> > vanlig tekstdokument vedlegg (patches.suse)
> > NFSv3 has no concept of a umask on the server side: The client applies
> > the umask locally, and sends the effective permissions to the server.
> > This behavior is wrong when files are created in a directory that has
> > a default ACL. In this case, the umask is supposed to be ignored, and
> > only the default ACL determines the file's effective permissions.
> >
> > Usually its the server's task to conditionally apply the umask. But
> > since the server knows nothing about the umask, we have to do it on the
> > client side. This patch tries to fetch the parent directory's default
> > ACL before creating a new file, computes the appropriate create mode to
> > send to the server, and finally sets the new file's access and default
> > acl appropriately.
>
> Firstly, this sort of code belongs in the NFSv3-specific code. POSIX
> acls have no business whatsoever in the generic NFS code.
See attached patch.
NOTE:
During testing I noticed that without
nfsacl-cache-acls-on-the-nfs-client-side.patch, no directories or
devices can be created. It's probably a problem with
nfs_set_default_acl(). I'll have to debug this tomorrow.
> Secondly, what is the point of doing all this *after* you have created
> the file with the wrong permissions? How are you avoiding races?
Well, everything but the umask is always correct; that is guaranteed by
the server. The initial create sets permissions that may be more
restrictive than necessary, and then the SETACL RPC sets up the final,
correct permissions. I don't believe that a race-free solution is
possible.
Cheers,
--
Andreas Gruenbacher <[email protected]>
SUSE Labs, SUSE LINUX GMBH
fs/nfs/dir.c | 20 +++++++++-----------
fs/nfs/nfs3proc.c | 23 +++++++++++++----------
fs/nfs/nfs4proc.c | 25 +++++++++++++------------
fs/nfs/proc.c | 24 ++++++++++++++----------
include/linux/nfs_fs.h | 2 ++
include/linux/nfs_xdr.h | 4 ++--
6 files changed, 53 insertions(+), 45 deletions(-)
Index: linux-2.6.11-rc4/fs/nfs/nfs3proc.c
===================================================================
--- linux-2.6.11-rc4.orig/fs/nfs/nfs3proc.c
+++ linux-2.6.11-rc4/fs/nfs/nfs3proc.c
@@ -639,23 +639,24 @@ nfs3_proc_readdir(struct dentry *dentry,
}
static int
-nfs3_proc_mknod(struct inode *dir, struct qstr *name, struct iattr *sattr,
- dev_t rdev, struct nfs_fh *fh, struct nfs_fattr *fattr)
+nfs3_proc_mknod(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
+ dev_t rdev)
{
- struct nfs_fattr dir_attr;
+ struct nfs_fh fh;
+ struct nfs_fattr fattr, dir_attr;
struct nfs3_mknodargs arg = {
.fh = NFS_FH(dir),
- .name = name->name,
- .len = name->len,
+ .name = dentry->d_name.name,
+ .len = dentry->name.len,
.sattr = sattr,
.rdev = rdev
};
struct nfs3_diropres res = {
.dir_attr = &dir_attr,
- .fh = fh,
- .fattr = fattr
+ .fh = &fh,
+ .fattr = &fattr
};
- int status;
+ int status;
switch (sattr->ia_mode & S_IFMT) {
case S_IFBLK: arg.type = NF3BLK; break;
@@ -665,12 +666,14 @@ nfs3_proc_mknod(struct inode *dir, struc
default: return -EINVAL;
}
- dprintk("NFS call mknod %s %u:%u\n", name->name,
+ dprintk("NFS call mknod %s %u:%u\n", dentry->d_name.name,
MAJOR(rdev), MINOR(rdev));
dir_attr.valid = 0;
- fattr->valid = 0;
+ fattr.valid = 0;
status = rpc_call(NFS_CLIENT(dir), NFS3PROC_MKNOD, &arg, &res, 0);
nfs_refresh_inode(dir, &dir_attr);
+ if (status == 0)
+ status = nfs_instantiate(dentry, &fh, &fattr);
dprintk("NFS reply mknod: %d\n", status);
return status;
}
Index: linux-2.6.11-rc4/fs/nfs/nfs4proc.c
===================================================================
--- linux-2.6.11-rc4.orig/fs/nfs/nfs4proc.c
+++ linux-2.6.11-rc4/fs/nfs/nfs4proc.c
@@ -1630,22 +1630,23 @@ static int nfs4_proc_readdir(struct dent
return err;
}
-static int _nfs4_proc_mknod(struct inode *dir, struct qstr *name,
- struct iattr *sattr, dev_t rdev, struct nfs_fh *fh,
- struct nfs_fattr *fattr)
+static int _nfs4_proc_mknod(struct inode *dir, struct dentry *dentry,
+ struct iattr *sattr, dev_t rdev)
{
struct nfs_server *server = NFS_SERVER(dir);
+ struct nfs_fh fh;
+ struct nfs_fattr fattr;
struct nfs4_create_arg arg = {
.dir_fh = NFS_FH(dir),
.server = server,
- .name = name,
+ .name = &dentry->d_name,
.attrs = sattr,
.bitmask = server->attr_bitmask,
};
struct nfs4_create_res res = {
.server = server,
- .fh = fh,
- .fattr = fattr,
+ .fh = &fh,
+ .fattr = &fattr,
};
struct rpc_message msg = {
.rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_CREATE],
@@ -1655,7 +1656,7 @@ static int _nfs4_proc_mknod(struct inode
int status;
int mode = sattr->ia_mode;
- fattr->valid = 0;
+ fattr.valid = 0;
BUG_ON(!(sattr->ia_valid & ATTR_MODE));
BUG_ON(!S_ISFIFO(mode) && !S_ISBLK(mode) && !S_ISCHR(mode) && !S_ISSOCK(mode));
@@ -1677,19 +1678,19 @@ static int _nfs4_proc_mknod(struct inode
status = rpc_call_sync(NFS_CLIENT(dir), &msg, 0);
if (!status)
update_changeattr(dir, &res.dir_cinfo);
+ if (status == 0)
+ status = nfs_instantiate(dentry, &fh, &fattr);
return status;
}
-static int nfs4_proc_mknod(struct inode *dir, struct qstr *name,
- struct iattr *sattr, dev_t rdev, struct nfs_fh *fh,
- struct nfs_fattr *fattr)
+static int nfs4_proc_mknod(struct inode *dir, struct dentry *dentry,
+ struct iattr *sattr, dev_t rdev)
{
struct nfs4_exception exception = { };
int err;
do {
err = nfs4_handle_exception(NFS_SERVER(dir),
- _nfs4_proc_mknod(dir, name, sattr, rdev,
- fh, fattr),
+ _nfs4_proc_mknod(dir, dentry, sattr, rdev),
&exception);
} while (exception.retry);
return err;
Index: linux-2.6.11-rc4/fs/nfs/proc.c
===================================================================
--- linux-2.6.11-rc4.orig/fs/nfs/proc.c
+++ linux-2.6.11-rc4/fs/nfs/proc.c
@@ -248,22 +248,24 @@ nfs_proc_create(struct inode *dir, struc
* In NFSv2, mknod is grafted onto the create call.
*/
static int
-nfs_proc_mknod(struct inode *dir, struct qstr *name, struct iattr *sattr,
- dev_t rdev, struct nfs_fh *fhandle, struct nfs_fattr *fattr)
+nfs_proc_mknod(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
+ dev_t rdev)
{
+ struct nfs_fh fhandle;
+ struct nfs_fattr fattr;
struct nfs_createargs arg = {
.fh = NFS_FH(dir),
- .name = name->name,
- .len = name->len,
+ .name = dentry->d_name.name,
+ .len = dentry->d_name.len,
.sattr = sattr
};
struct nfs_diropok res = {
- .fh = fhandle,
- .fattr = fattr
+ .fh = &fhandle,
+ .fattr = &fattr
};
- int status, mode;
+ int status, mode;
- dprintk("NFS call mknod %s\n", name->name);
+ dprintk("NFS call mknod %s\n", dentry->d_name.name);
mode = sattr->ia_mode;
if (S_ISFIFO(mode)) {
@@ -274,14 +276,16 @@ nfs_proc_mknod(struct inode *dir, struct
sattr->ia_size = new_encode_dev(rdev);/* get out your barf bag */
}
- fattr->valid = 0;
+ fattr.valid = 0;
status = rpc_call(NFS_CLIENT(dir), NFSPROC_CREATE, &arg, &res, 0);
if (status == -EINVAL && S_ISFIFO(mode)) {
sattr->ia_mode = mode;
- fattr->valid = 0;
+ fattr.valid = 0;
status = rpc_call(NFS_CLIENT(dir), NFSPROC_CREATE, &arg, &res, 0);
}
+ if (status == 0)
+ status = nfs_instantiate(dentry, &fhandle, &fattr);
dprintk("NFS reply mknod: %d\n", status);
return status;
}
Index: linux-2.6.11-rc4/fs/nfs/dir.c
===================================================================
--- linux-2.6.11-rc4.orig/fs/nfs/dir.c
+++ linux-2.6.11-rc4/fs/nfs/dir.c
@@ -938,7 +938,7 @@ static struct dentry *nfs_readdir_lookup
/*
* Code common to create, mkdir, and mknod.
*/
-static int nfs_instantiate(struct dentry *dentry, struct nfs_fh *fhandle,
+int nfs_instantiate(struct dentry *dentry, struct nfs_fh *fhandle,
struct nfs_fattr *fattr)
{
struct inode *inode;
@@ -1019,9 +1019,7 @@ static int
nfs_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t rdev)
{
struct iattr attr;
- struct nfs_fattr fattr;
- struct nfs_fh fhandle;
- int error;
+ int status;
dfprintk(VFS, "NFS: mknod(%s/%ld, %s\n", dir->i_sb->s_id,
dir->i_ino, dentry->d_name.name);
@@ -1034,15 +1032,15 @@ nfs_mknod(struct inode *dir, struct dent
lock_kernel();
nfs_begin_data_update(dir);
- error = NFS_PROTO(dir)->mknod(dir, &dentry->d_name, &attr, rdev,
- &fhandle, &fattr);
+ status = NFS_PROTO(dir)->mknod(dir, dentry, &attr, rdev);
nfs_end_data_update(dir);
- if (!error)
- error = nfs_instantiate(dentry, &fhandle, &fattr);
- else
- d_drop(dentry);
unlock_kernel();
- return error;
+ if (status != 0)
+ goto out_err;
+ return 0;
+out_err:
+ d_drop(dentry);
+ return status;
}
/*
Index: linux-2.6.11-rc4/include/linux/nfs_xdr.h
===================================================================
--- linux-2.6.11-rc4.orig/include/linux/nfs_xdr.h
+++ linux-2.6.11-rc4/include/linux/nfs_xdr.h
@@ -698,8 +698,8 @@ struct nfs_rpc_ops {
int (*rmdir) (struct inode *, struct qstr *);
int (*readdir) (struct dentry *, struct rpc_cred *,
u64, struct page *, unsigned int, int);
- int (*mknod) (struct inode *, struct qstr *, struct iattr *,
- dev_t, struct nfs_fh *, struct nfs_fattr *);
+ int (*mknod) (struct inode *, struct dentry *, struct iattr *,
+ dev_t);
int (*statfs) (struct nfs_server *, struct nfs_fh *,
struct nfs_fsstat *);
int (*fsinfo) (struct nfs_server *, struct nfs_fh *,
Index: linux-2.6.11-rc4/include/linux/nfs_fs.h
===================================================================
--- linux-2.6.11-rc4.orig/include/linux/nfs_fs.h
+++ linux-2.6.11-rc4/include/linux/nfs_fs.h
@@ -345,6 +345,8 @@ extern struct inode_operations nfs_dir_i
extern struct file_operations nfs_dir_operations;
extern struct dentry_operations nfs_dentry_operations;
+extern int nfs_instantiate(struct dentry *dentry, struct nfs_fh *fh, struct nfs_fattr *fattr);
+
/*
* linux/fs/nfs/symlink.c
*/