We've gotten complaints that the effect of NFSv4 ACL inheritance is
often overridden by common umask settings.
Options:
- Do nothing, make sure the behavior's documented. I don't
think this will make people happy, but I haven't tried to
figure out exactly what any of them are trying to do.
- Do as in NFSv3 and teach the client to skip the umask in the
case the parent directory has inheritable ACEs. The following
patch is a proof-of-concept with some bugs. It requires
documenting what we mean by "parent directory has interitable
ACEs"--I'm taking it to mean that the ACL has some ACE with
inheritance bits set, as that's simple to explain. Drawbacks
include: requires fetching and caching an ACL on create;
there's a race between checking and creating; creates may fail
just because reading the ACL fails.
- New protocol: e.g. add a new attribute representing the
un-umasked mode, send both that and the regular mode on
create, let the server sort it out.
- Something else??: adopt some convention that uses redundant
information in a compound (multiple SETATTRs?) to communicate
umask to "in the know" servers without changing behavior on
existing servers. Provide an "ignore the umask" mount option.
Maybe I'm missing a clever trick.
Any ideas?
--b.
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 6e62155abf26..e3b4b6250693 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1503,7 +1503,7 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
if (open_flags & O_CREAT) {
attr.ia_valid |= ATTR_MODE;
- attr.ia_mode = mode & ~current_umask();
+ attr.ia_mode = mode;
}
if (open_flags & O_TRUNC) {
attr.ia_valid |= ATTR_SIZE;
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 69dc20a743f9..efa2589f9242 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2793,6 +2793,81 @@ out:
return status;
}
+static bool nfs4_ace_inheritable(u32 flags)
+{
+ return flags & (NFS4_ACE_FILE_INHERIT_ACE |
+ NFS4_ACE_DIRECTORY_INHERIT_ACE);
+
+}
+
+/* XXX: one-off by-hand xdr parsing: */
+static int nfs4_acl_has_inheritable_aces(void *buf, int len)
+{
+ __be32 *p = buf;
+ int naces, i;
+ u32 word;
+
+ if (len < 4)
+ goto overflow;
+ len -= 4;
+ naces = be32_to_cpup(p++);
+ for (i=0; i < naces; i++) {
+ if (len < 4*4)
+ goto overflow;
+ len -= 4 * 4;
+ p++; /* skip type */
+ word = be32_to_cpup(p++); /* flags */
+ if (nfs4_ace_inheritable(word))
+ return 1;
+ p++; /* skip access mask; */
+ /* skip name: */
+ word = be32_to_cpup(p++);
+ if (len < (XDR_QUADLEN(word) << 2))
+ goto overflow;
+ len -= XDR_QUADLEN(word) << 2;
+ p += XDR_QUADLEN(word);
+ }
+ return 0;
+overflow:
+ /* XXX: what should we do with too-long ACLs? */
+ return -EIO;
+}
+
+static ssize_t nfs4_proc_get_acl(struct inode *inode, void *buf, size_t buflen);
+
+/*
+ * Apply the umask iff the directory lacks inheritable ACLs.
+ *
+ * XXX: Callers are ignoring the error cases.
+ */
+static int nfs4_apply_umask(struct inode *dir, unsigned short *mode)
+{
+ struct page *page;
+ void *buf;
+ int ret;
+
+ /* XXX: We probably need to handle longer ACLs: */
+ page = alloc_page(GFP_KERNEL);
+ if (!page)
+ return -ENOMEM;
+ buf = page_address(page);
+
+ ret = nfs4_proc_get_acl(dir, buf, PAGE_SIZE);
+ if (ret < 0)
+ goto out_free;
+ ret = nfs4_acl_has_inheritable_aces(buf, PAGE_SIZE);
+ if (ret < 0)
+ goto out_free;
+ if (ret) {
+ ret = 0;
+ goto out_free;
+ }
+ *mode &= ~current_umask();
+out_free:
+ __free_page(page);
+ return ret;
+}
+
static struct inode *
nfs4_atomic_open(struct inode *dir, struct nfs_open_context *ctx,
int open_flags, struct iattr *attr, int *opened)
@@ -2800,6 +2875,9 @@ nfs4_atomic_open(struct inode *dir, struct nfs_open_context *ctx,
struct nfs4_state *state;
struct nfs4_label l = {0, 0, 0, NULL}, *label = NULL;
+ if (open_flags & O_CREAT)
+ nfs4_apply_umask(dir, &attr->ia_mode);
+
label = nfs4_label_init_security(dir, ctx->dentry, attr, &l);
/* Protect against concurrent sillydeletes */
@@ -3507,7 +3585,7 @@ nfs4_proc_create(struct inode *dir, struct dentry *dentry, struct iattr *sattr,
ilabel = nfs4_label_init_security(dir, dentry, sattr, &l);
- sattr->ia_mode &= ~current_umask();
+ nfs4_apply_umask(dir, &sattr->ia_mode);
state = nfs4_do_open(dir, ctx, flags, sattr, ilabel, &opened);
if (IS_ERR(state)) {
status = PTR_ERR(state);
@@ -3818,7 +3896,7 @@ static int nfs4_proc_mkdir(struct inode *dir, struct dentry *dentry,
label = nfs4_label_init_security(dir, dentry, sattr, &l);
- sattr->ia_mode &= ~current_umask();
+ nfs4_apply_umask(dir, &sattr->ia_mode);
do {
err = _nfs4_proc_mkdir(dir, dentry, sattr, label);
trace_nfs4_mkdir(dir, &dentry->d_name, err);
@@ -3927,7 +4005,7 @@ static int nfs4_proc_mknod(struct inode *dir, struct dentry *dentry,
label = nfs4_label_init_security(dir, dentry, sattr, &l);
- sattr->ia_mode &= ~current_umask();
+ nfs4_apply_umask(dir, &sattr->ia_mode);
do {
err = _nfs4_proc_mknod(dir, dentry, sattr, label, rdev);
trace_nfs4_mknod(dir, &dentry->d_name, err);
On Wed, Dec 10, 2014 at 1:31 PM, J. Bruce Fields <[email protected]> wrote:
> We've gotten complaints that the effect of NFSv4 ACL inheritance is
> often overridden by common umask settings.
>
> Options:
>
> - Do nothing, make sure the behavior's documented. I don't
> think this will make people happy, but I haven't tried to
> figure out exactly what any of them are trying to do.
POSIX documents exactly how open(O_CREATE) works. I'd say that if
anything, we'd need to document how inherited ACLs are supposed to
work in this situation.
>
> - Do as in NFSv3 and teach the client to skip the umask in the
> case the parent directory has inheritable ACEs. The following
> patch is a proof-of-concept with some bugs. It requires
> documenting what we mean by "parent directory has interitable
> ACEs"--I'm taking it to mean that the ACL has some ACE with
> inheritance bits set, as that's simple to explain. Drawbacks
> include: requires fetching and caching an ACL on create;
> there's a race between checking and creating; creates may fail
> just because reading the ACL fails.
Yeah, this is a non-starter.
> - New protocol: e.g. add a new attribute representing the
> un-umasked mode, send both that and the regular mode on
> create, let the server sort it out.
>
> - Something else??: adopt some convention that uses redundant
> information in a compound (multiple SETATTRs?) to communicate
> umask to "in the know" servers without changing behavior on
> existing servers. Provide an "ignore the umask" mount option.
> Maybe I'm missing a clever trick.
>
> Any ideas?
As I indicated above, I'd like to see a little more debate around how
we expect this whole ACL inheritance thing to work in practice. The
NFSv4 spec is all about enabling Windows-like ACL behaviour, but that
isn't necessarily the right thing for Linux applications.
IOW: before we discuss fixes, I'd like to understand how the current
behaviour is broken.
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]