From: Aneesh Kumar K. V on
On Sat, 3 Jul 2010 00:13:31 +0530, "Aneesh Kumar K.V" <aneesh.kumar(a)linux.vnet.ibm.com> wrote:
> Hi,
>
> The following set of patches implements a new acl model for linux. Rich ACLs
> are an implementation of NFSv4 ACLs, extended by file masks to fit into the
> standard POSIX file permission model. They are designed to work seamlessly
> locally as well as across the NFSv4 and CIFS/SMB2 network file system protocols.
>
> Since posting the patches the previous time [0], Andreas Gruenbacher and I have
> worked on cleaning up the code, splitting things up into smaller pieces, and
> adding more documentation.
>
> [0] http://article.gmane.org/gmane.comp.file-systems.ext4/17414
>
> The patch set consists of three parts.
>
> The first set of patches, posted as a follow up, contains the Rich ACL model
> and Ext4 implementation. The second set [1] contains mapping of Rich ACL to
> NFSv4 ACL (how to apply file mask to access mask) and implementation of
> Richacl ACL for NFS server and client. The third set [2] contains POSIX ACL
> to Rich ACL mapping and its ext4 usage.
>
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/agruen/linux-2.6-richacl.git richacl-upstream
> [2] git://git.kernel.org/pub/scm/linux/kernel/git/agruen/linux-2.6-richacl.git richacl-fullset
>
> A user-space utility for displaying and changing richacls is available at [3]
> (a number of examples can be found at http://acl.bestbits.at/richacl/examples.html).
>
> [3] git://git.kernel.org/pub/scm/linux/kernel/git/agruen/richacl.git master
>
> To test richacl on ext4 use -o richacl mount option. This mount option may later be
> dropped in favour of a feature flag.
>
> More details regarding richacl can be found at
> http://acl.bestbits.at/richacl/
>
> Changes from V1:
> 1) Split the patches into smaller patches
> 2) Added extensive documentation to the patches.
>

We also need the below patch set to make sure the permission check
function does the right thing. This patch is a part of second patch set
[1] pointed above. We add it as a part of the later series because it is
better explained with isolate owner and group patchset.

commit 01a3a0c12dc4b87cdee11cd101b3ceefaa331e41
Author: Andreas Gruenbacher <agruen(a)suse.de>
Date: Mon Jun 28 21:32:24 2010 +0530

richacl: Restrict access check algorithm

We want to avoid applying the file masks to an acl when changing the
file permission bits or performing an access check. On the other hand,
when we *do* apply the file masks to the acl, we want the resulting acl
to produce the same access check results with the standard nfs4 access
check algorithm as the richacl access check algorithm with the original
acl. This is already the case, except in the following scenario:

With file masks equivalent to file mode 0600, the following acl would
grant the owner rw access if the owner is in the owning group:

group@:rw::allow

There is no way to express this in an nfs4 acl; the result is always a
more restrictive acl. There are two approaches to deal with this
difference: either accept that it exists and that applying the file
masks is imperfect, or change the richacl access check algorithm so that
such accesses are denied.

This patch denies such accesses and makes sure that the richacl access
check algorithm grants the same accesses as the nfsv4 acl with the file
masks applied.

Signed-off-by: Andreas Gruenbacher <agruen(a)suse.de>
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar(a)linux.vnet.ibm.com>

diff --git a/fs/richacl_base.c b/fs/richacl_base.c
index b368b51..79fb00f 100644
--- a/fs/richacl_base.c
+++ b/fs/richacl_base.c
@@ -424,6 +424,16 @@ richacl_permission(struct inode *inode, const struct richacl *acl,
} else
goto is_everyone;

+ /*
+ * Apply the group file mask to entries other than OWNER@ and
+ * EVERYONE@. This is not required for correct access checking
+ * but ensures that we grant the same permissions as the acl
+ * computed by richacl_apply_masks() would grant. See
+ * richacl_apply_masks() for a more detailed explanation.
+ */
+ if (richace_is_allow(ace))
+ ace_mask &= acl->a_group_mask;
+
is_owner:
/* The process is in the owner or group file class. */
in_owner_or_group_class = 1;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andreas Gruenbacher on
Aneesh,

here is a patch on top of the richacl-upstream queue.

So far, we were assuming that acls are always masked, and the file masks
could be set so that they would not mask any permissions. This is fine
for permission checking, but richacl_apply_masks() didn't recognize when
the file masks were set to mask nothing, and was still transforming such
acls unnecessarily. This could lead to correct but surprising results.

Instead of using a flag here we could check if the file masks are set to
"ineffective" values, but that would require the same computation as
richacl_compute_max_masks(), which can be a slow on large acls. It is a
lot easier to instead remember if the file masks are "effective".

(We still need to compute the file masks in nfsd where no file masks are
supplied so that the file permission bits will be set to a reasonable
value. user space will always pass valid file masks in, so we are covered
in that side.)

Thanks,
Andreas

--------------------------------------------------------------------------------

[PATCH] richacl: Allow acls to be masked or unmasked

Use a flag to indicate when the file masks can mask something
(e.g., after a chmod). richacl_apply_masks() can skip transforming acls
without this flag, which speeds things up and avoids modifying those
acls unnecessarily.

Signed-off-by: Andreas Gruenbacher <agruen(a)suse.de>
---
fs/richacl_base.c | 46 +++++++++++++++++++++++++++++++---------------
fs/richacl_compat.c | 31 ++++++++++++++++++-------------
fs/richacl_inode.c | 1 +
include/linux/richacl.h | 1 +
4 files changed, 51 insertions(+), 28 deletions(-)

diff --git a/fs/richacl_base.c b/fs/richacl_base.c
index 79fb00f..baaecf2 100644
--- a/fs/richacl_base.c
+++ b/fs/richacl_base.c
@@ -329,6 +329,8 @@ restart:
}
}
}
+
+ acl->a_flags &= ~ACL4_MASKED;
}
EXPORT_SYMBOL_GPL(richacl_compute_max_masks);

@@ -354,7 +356,8 @@ richacl_chmod(struct richacl *acl, mode_t mode)
if (acl->a_owner_mask == owner_mask &&
acl->a_group_mask == group_mask &&
acl->a_other_mask == other_mask &&
- (!richacl_is_auto_inherit(acl) || richacl_is_protected(acl)))
+ (!richacl_is_auto_inherit(acl) || richacl_is_protected(acl)) &&
+ (acl->a_flags & ACL4_MASKED))
return acl;

clone = richacl_clone(acl);
@@ -362,6 +365,7 @@ richacl_chmod(struct richacl *acl, mode_t mode)
if (!clone)
return ERR_PTR(-ENOMEM);

+ clone->a_flags |= ACL4_MASKED;
clone->a_owner_mask = owner_mask;
clone->a_group_mask = group_mask;
clone->a_other_mask = other_mask;
@@ -385,11 +389,18 @@ richacl_permission(struct inode *inode, const struct richacl *acl,
unsigned int mask)
{
const struct richace *ace;
- unsigned int file_mask, requested = mask, denied = 0;
+ unsigned int requested = mask, denied = 0;
int in_owning_group = in_group_p(inode->i_gid);
int in_owner_or_group_class = in_owning_group;

/*
+ * We don't need to know which class the process is in when the acl is
+ * not masked.
+ */
+ if (!(acl->a_flags & ACL4_MASKED))
+ in_owner_or_group_class = 1;
+
+ /*
* A process is
* - in the owner file class if it owns the file,
* - in the group file class if it is in the file's owning group or
@@ -431,7 +442,7 @@ richacl_permission(struct inode *inode, const struct richacl *acl,
* computed by richacl_apply_masks() would grant. See
* richacl_apply_masks() for a more detailed explanation.
*/
- if (richace_is_allow(ace))
+ if ((acl->a_flags & ACL4_MASKED) && richace_is_allow(ace))
ace_mask &= acl->a_group_mask;

is_owner:
@@ -453,17 +464,22 @@ is_everyone:
}
denied |= mask;

- /*
- * The file class a process is in determines which file mask applies.
- * Check if that file mask also grants the requested access.
- */
- if (current_fsuid() == inode->i_uid)
- file_mask = acl->a_owner_mask;
- else if (in_owner_or_group_class)
- file_mask = acl->a_group_mask;
- else
- file_mask = acl->a_other_mask;
- denied |= requested & ~file_mask;
+ if (acl->a_flags & ACL4_MASKED) {
+ unsigned int file_mask;
+
+ /*
+ * The file class a process is in determines which file mask
+ * applies. Check if that file mask also grants the requested
+ * access.
+ */
+ if (current_fsuid() == inode->i_uid)
+ file_mask = acl->a_owner_mask;
+ else if (in_owner_or_group_class)
+ file_mask = acl->a_group_mask;
+ else
+ file_mask = acl->a_other_mask;
+ denied |= requested & ~file_mask;
+ }

return denied ? -EACCES : 0;
}
@@ -563,7 +579,7 @@ richacl_equiv_mode(const struct richacl *acl, mode_t *mode_p)
mode_t mode;

if (acl->a_count != 1 ||
- acl->a_flags ||
+ acl->a_flags != ACL4_MASKED ||
!richace_is_everyone(ace) ||
!richace_is_allow(ace) ||
ace->e_flags & ~ACE4_SPECIAL_WHO)
diff --git a/fs/richacl_compat.c b/fs/richacl_compat.c
index 9adbd62..4aa0b7d 100644
--- a/fs/richacl_compat.c
+++ b/fs/richacl_compat.c
@@ -699,20 +699,25 @@ __richacl_apply_masks(struct richacl_alloc *x)
int
richacl_apply_masks(struct richacl **acl)
{
- struct richacl_alloc x = {
- .acl = *acl,
- .count = (*acl)->a_count,
- };
int retval = 0;

- if (richacl_move_everyone_aces_down(&x) ||
- richacl_propagate_everyone(&x) ||
- __richacl_apply_masks(&x) ||
- richacl_isolate_owner_class(&x) ||
- richacl_isolate_group_class(&x))
- retval = -ENOMEM;
+ if ((*acl)->a_flags & ACL4_MASKED) {
+ struct richacl_alloc x = {
+ .acl = *acl,
+ .count = (*acl)->a_count,
+ };
+
+ if (richacl_move_everyone_aces_down(&x) ||
+ richacl_propagate_everyone(&x) ||
+ __richacl_apply_masks(&x) ||
+ richacl_isolate_owner_class(&x) ||
+ richacl_isolate_group_class(&x))
+ retval = -ENOMEM;
+
+ x.acl->a_flags &= ~ACL4_MASKED;
+ *acl = x.acl;
+ }

- *acl = x.acl;
return retval;
}
EXPORT_SYMBOL_GPL(richacl_apply_masks);
@@ -730,12 +735,12 @@ richacl_from_mode(mode_t mode)
acl = richacl_alloc(1);
if (!acl)
return NULL;
- ace = acl->a_entries;
-
+ acl->a_flags = ACL4_MASKED;
acl->a_owner_mask = richacl_mode_to_mask(mode >> 6);
acl->a_group_mask = richacl_mode_to_mask(mode >> 3);
acl->a_other_mask = richacl_mode_to_mask(mode);

+ ace = acl->a_entries;
ace->e_type = ACE4_ACCESS_ALLOWED_ACE_TYPE;
ace->e_flags = ACE4_SPECIAL_WHO;
ace->e_mask = ACE4_POSIX_MODE_ALL;
diff --git a/fs/richacl_inode.c b/fs/richacl_inode.c
index 1e9c901..6be821a 100644
--- a/fs/richacl_inode.c
+++ b/fs/richacl_inode.c
@@ -224,6 +224,7 @@ richacl_inherit_inode(const struct richacl *dir_acl, struct inode *inode)
* Ensure that the acl will not grant any permissions beyond
* the create mode.
*/
+ acl->a_flags |= ACL4_MASKED;
acl->a_owner_mask &= richacl_mode_to_mask(inode->i_mode >> 6);
acl->a_group_mask &= richacl_mode_to_mask(inode->i_mode >> 3);
acl->a_other_mask &= richacl_mode_to_mask(inode->i_mode);
diff --git a/include/linux/richacl.h b/include/linux/richacl.h
index 74b4c80..8ff317c 100644
--- a/include/linux/richacl.h
+++ b/include/linux/richacl.h
@@ -51,6 +51,7 @@ struct richacl {
#define ACL4_AUTO_INHERIT 0x01
#define ACL4_PROTECTED 0x02
/*#define ACL4_DEFAULTED 0x04*/
+#define ACL4_MASKED 0x08

#define ACL4_VALID_FLAGS ( \
ACL4_AUTO_INHERIT | \
--
1.7.1.rc2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Aneesh Kumar K. V on
On Mon, 19 Jul 2010 21:19:50 +0200, Andreas Gruenbacher <agruen(a)suse.de> wrote:
> Aneesh,
>
> here is a patch on top of the richacl-upstream queue.
>
> So far, we were assuming that acls are always masked, and the file masks
> could be set so that they would not mask any permissions. This is fine
> for permission checking, but richacl_apply_masks() didn't recognize when
> the file masks were set to mask nothing, and was still transforming such
> acls unnecessarily. This could lead to correct but surprising results.
>
> Instead of using a flag here we could check if the file masks are set to
> "ineffective" values, but that would require the same computation as
> richacl_compute_max_masks(), which can be a slow on large acls. It is a
> lot easier to instead remember if the file masks are "effective".
>
> (We still need to compute the file masks in nfsd where no file masks are
> supplied so that the file permission bits will be set to a reasonable
> value. user space will always pass valid file masks in, so we are covered
> in that side.)

We need to update ACL4_VALID_FLAGS to now consider ACL4_MASKED as a
valid flag. This is also needed for userspace. On a related note,
should we move ACL4_MASKED and ACL4_POSIX_MAPPED to be the higher
bits ? That would make sure we will be able to accomodate new flag
value NFSv4 define. Something like

diff --git a/include/linux/richacl.h b/include/linux/richacl.h
index 929cc32..ff3c12b 100644
--- a/include/linux/richacl.h
+++ b/include/linux/richacl.h
@@ -50,16 +50,17 @@ struct richacl {
/* a_flags values */
#define ACL4_AUTO_INHERIT 0x01
#define ACL4_PROTECTED 0x02
-/*#define ACL4_DEFAULTED 0x04*/
-#define ACL4_MASKED 0x08
-#define ACL4_POSIX_MAPPED 0x10
+#define ACL4_DEFAULTED 0x04
+/* flag value defined by Richacl */
+#define ACL4_MASKED 0x40
+#define ACL4_POSIX_MAPPED 0x80

#define ACL4_VALID_FLAGS ( \
ACL4_AUTO_INHERIT | \
ACL4_PROTECTED | \
+ ACL4_MASKED | \
ACL4_POSIX_MAPPED)

-
/* e_type values */
#define ACE4_ACCESS_ALLOWED_ACE_TYPE 0x0000
#define ACE4_ACCESS_DENIED_ACE_TYPE 0x0001



-aneesh


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andreas Gruenbacher on
On Tuesday 20 July 2010 11:31:07 Aneesh Kumar K. V wrote:
> We need to update ACL4_VALID_FLAGS to now consider ACL4_MASKED as a
> valid flag. This is also needed for userspace.

Good point, I missed that.

> On a related note, should we move ACL4_MASKED and ACL4_POSIX_MAPPED to
> be the higher bits ? That would make sure we will be able to accomodate
> new flag value NFSv4 define.

That makes sense, except that ACL4_POSIX_MAPPED hasn't entered the scene in
the patches posted here, and I'm still not convinced that we'll actually need
it.

Thanks,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Aneesh Kumar K. V on
On Tue, 20 Jul 2010 12:11:53 +0200, Andreas Gruenbacher <agruen(a)suse.de> wrote:
> On Tuesday 20 July 2010 11:31:07 Aneesh Kumar K. V wrote:
> > We need to update ACL4_VALID_FLAGS to now consider ACL4_MASKED as a
> > valid flag. This is also needed for userspace.
>
> Good point, I missed that.

I updated the patch and will push the change to korg after running some
test.

>
> > On a related note, should we move ACL4_MASKED and ACL4_POSIX_MAPPED to
> > be the higher bits ? That would make sure we will be able to accomodate
> > new flag value NFSv4 define.
>
> That makes sense, except that ACL4_POSIX_MAPPED hasn't entered the scene in
> the patches posted here, and I'm still not convinced that we'll actually need
> it.
>

The userspace change did result in a different output for the below ex:

richacl --set 'flags:a 101:w::deny 101:rw::allow 101:w:a:deny 101:rw:a:allow' f

this now gives

/mnt/d# richacl --get --numeric f
f:
flags:a
101:-w-----------::deny
101:rw-----------::allow
101:-w-----------:a:deny
101:rw-----------:a:allow

that 'w' in rw::allow is redundant, because we have a deny entry before.

-aneesh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/