LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: NeilBrown <neilb@suse.de>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: nfs@lists.sourceforge.net, linux-kernel@vger.kernel.org
Subject: [PATCH 008 of 9] knfsd: nfsd4: acls: avoid unnecessary denies
Date: Tue, 13 Feb 2007 10:45:01 +1100	[thread overview]
Message-ID: <1070212234501.29343@suse.de> (raw)
In-Reply-To: <20070213103941.28958.patches@notabene>


From: J. Bruce Fields <bfields@snoopy.citi.umich.edu>
We're inserting deny's between some ACEs in order to enforce posix draft
acl semantics which prevent permissions from accumulating across entries
in an acl.

That's fine, but we're doing that by inserting a deny after *every* allow,
which is overkill.  We shouldn't be adding them in places where they
actually make no difference.

Also replaced some helper functions for creating acl entries; I prefer
just assigning directly to the struct fields--it takes a few more lines,
but the field names provide some documentation that I think makes the
result easier understand.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/nfs4acl.c |  190 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 145 insertions(+), 45 deletions(-)

diff .prev/fs/nfsd/nfs4acl.c ./fs/nfsd/nfs4acl.c
--- .prev/fs/nfsd/nfs4acl.c	2007-02-13 10:37:27.000000000 +1100
+++ ./fs/nfsd/nfs4acl.c	2007-02-13 10:38:09.000000000 +1100
@@ -89,12 +89,19 @@ mask_from_posix(unsigned short perm, uns
 }
 
 static u32
-deny_mask(u32 allow_mask, unsigned int flags)
+deny_mask_from_posix(unsigned short perm, u32 flags)
 {
-	u32 ret = ~allow_mask & ~NFS4_MASK_UNSUPP;
-	if (!(flags & NFS4_ACL_DIR))
-		ret &= ~NFS4_ACE_DELETE_CHILD;
-	return ret;
+	u32 mask = 0;
+
+	if (perm & ACL_READ)
+		mask |= NFS4_READ_MODE;
+	if (perm & ACL_WRITE)
+		mask |= NFS4_WRITE_MODE;
+	if ((perm & ACL_WRITE) && (flags & NFS4_ACL_DIR))
+		mask |= NFS4_ACE_DELETE_CHILD;
+	if (perm & ACL_EXECUTE)
+		mask |= NFS4_EXECUTE_MODE;
+	return mask;
 }
 
 /* XXX: modify functions to return NFS errors; they're only ever
@@ -164,14 +171,51 @@ nfs4_acl_posix_to_nfsv4(struct posix_acl
 	return acl;
 }
 
+struct posix_acl_summary {
+	unsigned short owner;
+	unsigned short users;
+	unsigned short group;
+	unsigned short groups;
+	unsigned short other;
+	unsigned short mask;
+};
+
 static void
-nfs4_acl_add_pair(struct nfs4_acl *acl, int eflag, u32 mask, int whotype,
-		uid_t owner, unsigned int flags)
+summarize_posix_acl(struct posix_acl *acl, struct posix_acl_summary *pas)
 {
-	nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE,
-				 eflag, mask, whotype, owner);
-	nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
-				eflag, deny_mask(mask, flags), whotype, owner);
+	struct posix_acl_entry *pa, *pe;
+	pas->users = 0;
+	pas->groups = 0;
+	pas->mask = 07;
+
+	pe = acl->a_entries + acl->a_count;
+
+	FOREACH_ACL_ENTRY(pa, acl, pe) {
+		switch (pa->e_tag) {
+			case ACL_USER_OBJ:
+				pas->owner = pa->e_perm;
+				break;
+			case ACL_GROUP_OBJ:
+				pas->group = pa->e_perm;
+				break;
+			case ACL_USER:
+				pas->users |= pa->e_perm;
+				break;
+			case ACL_GROUP:
+				pas->groups |= pa->e_perm;
+				break;
+			case ACL_OTHER:
+				pas->other = pa->e_perm;
+				break;
+			case ACL_MASK:
+				pas->mask = pa->e_perm;
+				break;
+		}
+	}
+	/* We'll only care about effective permissions: */
+	pas->users &= pas->mask;
+	pas->group &= pas->mask;
+	pas->groups &= pas->mask;
 }
 
 /* We assume the acl has been verified with posix_acl_valid. */
@@ -179,30 +223,63 @@ static void
 _posix_to_nfsv4_one(struct posix_acl *pacl, struct nfs4_acl *acl,
 						unsigned int flags)
 {
-	struct posix_acl_entry *pa, *pe, *group_owner_entry;
-	u32 mask;
-	unsigned short mask_mask;
+	struct posix_acl_entry *pa, *group_owner_entry;
+	struct nfs4_ace *ace;
+	struct posix_acl_summary pas;
+	unsigned short deny;
 	int eflag = ((flags & NFS4_ACL_TYPE_DEFAULT) ?
 					NFS4_INHERITANCE_FLAGS : 0);
 
 	BUG_ON(pacl->a_count < 3);
-	pe = pacl->a_entries + pacl->a_count;
-	pa = pe - 2; /* if mask entry exists, it's second from the last. */
-	if (pa->e_tag == ACL_MASK)
-		mask_mask = pa->e_perm;
-	else
-		mask_mask = S_IRWXO;
+	summarize_posix_acl(pacl, &pas);
 
 	pa = pacl->a_entries;
-	BUG_ON(pa->e_tag != ACL_USER_OBJ);
-	mask = mask_from_posix(pa->e_perm, flags | NFS4_ACL_OWNER);
-	nfs4_acl_add_pair(acl, eflag, mask, NFS4_ACL_WHO_OWNER, 0, flags);
+	ace = acl->aces + acl->naces;
+
+	/* We could deny everything not granted by the owner: */
+	deny = ~pas.owner;
+	/*
+	 * but it is equivalent (and simpler) to deny only what is not
+	 * granted by later entries:
+	 */
+	deny &= pas.users | pas.group | pas.groups | pas.other;
+	if (deny) {
+		ace->type = NFS4_ACE_ACCESS_DENIED_ACE_TYPE;
+		ace->flag = eflag;
+		ace->access_mask = deny_mask_from_posix(deny, flags);
+		ace->whotype = NFS4_ACL_WHO_OWNER;
+		ace++;
+		acl->naces++;
+	}
+
+	ace->type = NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE;
+	ace->flag = eflag;
+	ace->access_mask = mask_from_posix(pa->e_perm, flags | NFS4_ACL_OWNER);
+	ace->whotype = NFS4_ACL_WHO_OWNER;
+	ace++;
+	acl->naces++;
 	pa++;
 
 	while (pa->e_tag == ACL_USER) {
-		mask = mask_from_posix(pa->e_perm & mask_mask, flags);
-		nfs4_acl_add_pair(acl, eflag, mask,
-				NFS4_ACL_WHO_NAMED, pa->e_id, flags);
+		deny = ~(pa->e_perm & pas.mask);
+		deny &= pas.groups | pas.group | pas.other;
+		if (deny) {
+			ace->type = NFS4_ACE_ACCESS_DENIED_ACE_TYPE;
+			ace->flag = eflag;
+			ace->access_mask = deny_mask_from_posix(deny, flags);
+			ace->whotype = NFS4_ACL_WHO_NAMED;
+			ace->who = pa->e_id;
+			ace++;
+			acl->naces++;
+		}
+		ace->type = NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE;
+		ace->flag = eflag;
+		ace->access_mask = mask_from_posix(pa->e_perm & pas.mask,
+						   flags);
+		ace->whotype = NFS4_ACL_WHO_NAMED;
+		ace->who = pa->e_id;
+		ace++;
+		acl->naces++;
 		pa++;
 	}
 
@@ -212,41 +289,64 @@ _posix_to_nfsv4_one(struct posix_acl *pa
 	/* allow ACEs */
 
 	group_owner_entry = pa;
-	mask = mask_from_posix(pa->e_perm & mask_mask, flags);
-	nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE,
-			NFS4_ACE_IDENTIFIER_GROUP | eflag, mask,
-			NFS4_ACL_WHO_GROUP, 0);
+
+	ace->type = NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE;
+	ace->flag = eflag;
+	ace->access_mask = mask_from_posix(pas.group, flags);
+	ace->whotype = NFS4_ACL_WHO_GROUP;
+	ace++;
+	acl->naces++;
 	pa++;
 
 	while (pa->e_tag == ACL_GROUP) {
-		mask = mask_from_posix(pa->e_perm & mask_mask, flags);
-		nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE,
-		    		NFS4_ACE_IDENTIFIER_GROUP | eflag, mask,
-		    		NFS4_ACL_WHO_NAMED, pa->e_id);
+		ace->type = NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE;
+		ace->flag = eflag | NFS4_ACE_IDENTIFIER_GROUP;
+		ace->access_mask = mask_from_posix(pa->e_perm & pas.mask,
+						   flags);
+		ace->whotype = NFS4_ACL_WHO_NAMED;
+		ace->who = pa->e_id;
+		ace++;
+		acl->naces++;
 		pa++;
 	}
 
 	/* deny ACEs */
 
 	pa = group_owner_entry;
-	mask = mask_from_posix(pa->e_perm, flags);
-	nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
-			NFS4_ACE_IDENTIFIER_GROUP | eflag,
-			deny_mask(mask, flags), NFS4_ACL_WHO_GROUP, 0);
+
+	deny = ~pas.group & pas.other;
+	if (deny) {
+		ace->type = NFS4_ACE_ACCESS_DENIED_ACE_TYPE;
+		ace->flag = eflag | NFS4_ACE_IDENTIFIER_GROUP;
+		ace->access_mask = deny_mask_from_posix(deny, flags);
+		ace->whotype = NFS4_ACL_WHO_GROUP;
+		ace++;
+		acl->naces++;
+	}
 	pa++;
+
 	while (pa->e_tag == ACL_GROUP) {
-		mask = mask_from_posix(pa->e_perm, flags);
-		nfs4_acl_add_ace(acl, NFS4_ACE_ACCESS_DENIED_ACE_TYPE,
-		    		NFS4_ACE_IDENTIFIER_GROUP | eflag,
-		    		deny_mask(mask, flags), NFS4_ACL_WHO_NAMED, pa->e_id);
+		deny = ~(pa->e_perm & pas.mask);
+		deny &= pas.other;
+		if (deny) {
+			ace->type = NFS4_ACE_ACCESS_DENIED_ACE_TYPE;
+			ace->flag = eflag | NFS4_ACE_IDENTIFIER_GROUP;
+			ace->access_mask = mask_from_posix(deny, flags);
+			ace->whotype = NFS4_ACL_WHO_NAMED;
+			ace->who = pa->e_id;
+			ace++;
+			acl->naces++;
+		}
 		pa++;
 	}
 
 	if (pa->e_tag == ACL_MASK)
 		pa++;
-	BUG_ON(pa->e_tag != ACL_OTHER);
-	mask = mask_from_posix(pa->e_perm, flags);
-	nfs4_acl_add_pair(acl, eflag, mask, NFS4_ACL_WHO_EVERYONE, 0, flags);
+	ace->type = NFS4_ACE_ACCESS_ALLOWED_ACE_TYPE;
+	ace->flag = eflag;
+	ace->access_mask = mask_from_posix(pa->e_perm, flags);
+	ace->whotype = NFS4_ACL_WHO_EVERYONE;
+	acl->naces++;
 }
 
 static void

  parent reply	other threads:[~2007-02-12 23:46 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-02-12 23:43 [PATCH 000 of 9] knfsd: NFSv4 ACL improvements and a couple of bug fixes NeilBrown
2007-02-12 23:44 ` [PATCH 001 of 9] knfsd: nfsd4: fix non-terminated string NeilBrown
2007-02-14 13:00   ` [NFS] " Ming Zhang
2007-02-14 17:55     ` Chuck Lever
2007-02-14 18:04       ` Ming Zhang
2007-02-12 23:44 ` [PATCH 002 of 9] knfsd: nfsd4: relax checking of ACL inheritance bits NeilBrown
2007-02-12 23:44 ` [PATCH 003 of 9] knfsd: nfsd4: simplify nfsv4->posix translation NeilBrown
2007-02-12 23:44 ` [PATCH 004 of 9] knfsd: nfsd4: represent nfsv4 acl with array instead of linked list NeilBrown
2007-02-12 23:44 ` [PATCH 005 of 9] knfsd: nfsd4: fix memory leak on kmalloc failure in savemem NeilBrown
2007-02-12 23:44 ` [PATCH 006 of 9] knfsd: nfsd4: fix error return on unsupported acl NeilBrown
2007-02-12 23:44 ` [PATCH 007 of 9] knfsd: nfsd4: acls: don't return explicit mask NeilBrown
2007-02-12 23:45 ` NeilBrown [this message]
2007-02-12 23:45 ` [PATCH 009 of 9] knfsd: nfsd4: fix handling of directories without default ACLs NeilBrown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1070212234501.29343@suse.de \
    --to=neilb@suse.de \
    --cc=akpm@linux-foundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nfs@lists.sourceforge.net \
    --subject='Re: [PATCH 008 of 9] knfsd: nfsd4: acls: avoid unnecessary denies' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).