[PATCH v8 00/41] Richacls

classic Classic list List threaded Threaded
68 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

[PATCH v8 04/41] vfs: Make the inode passed to inode_change_ok non-const

Andreas Gruenbacher-6
We will need to call iop->permission and iop->get_acl from
inode_change_ok() for additional permission checks, and both take a
non-const inode.

Signed-off-by: Andreas Gruenbacher <[hidden email]>
Reviewed-by: J. Bruce Fields <[hidden email]>
---
 fs/attr.c          | 2 +-
 include/linux/fs.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index 6530ced..328be71 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -28,7 +28,7 @@
  * Should be called as the first thing in ->setattr implementations,
  * possibly after taking additional locks.
  */
-int inode_change_ok(const struct inode *inode, struct iattr *attr)
+int inode_change_ok(struct inode *inode, struct iattr *attr)
 {
  unsigned int ia_valid = attr->ia_valid;
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 402acd7..aab32c8 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2871,7 +2871,7 @@ extern int buffer_migrate_page(struct address_space *,
 #define buffer_migrate_page NULL
 #endif
 
-extern int inode_change_ok(const struct inode *, struct iattr *);
+extern int inode_change_ok(struct inode *, struct iattr *);
 extern int inode_newsize_ok(const struct inode *, loff_t offset);
 extern void setattr_copy(struct inode *inode, const struct iattr *attr);
 
--
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

[PATCH v8 02/41] vfs: Add MAY_CREATE_FILE and MAY_CREATE_DIR permission flags

Andreas Gruenbacher-6
In reply to this post by Andreas Gruenbacher-6
Richacls distinguish between creating non-directories and directories. To
support that, add an isdir parameter to may_create(). When checking
inode_permission() for create permission, pass in an additional
MAY_CREATE_FILE or MAY_CREATE_DIR mask flag.

To allow checking for delete *and* create access when replacing an existing
file via vfs_rename(), add a replace parameter to may_delete().

Signed-off-by: Andreas Gruenbacher <[hidden email]>
Reviewed-by: J. Bruce Fields <[hidden email]>
---
 fs/namei.c         | 43 +++++++++++++++++++++++++------------------
 include/linux/fs.h |  2 ++
 2 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 48c2752..7c0f310 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -453,7 +453,9 @@ static int sb_permission(struct super_block *sb, struct inode *inode, int mask)
  * this, letting us set arbitrary permissions for filesystem access without
  * changing the "normal" UIDs which are used for other things.
  *
- * When checking for MAY_APPEND, MAY_WRITE must also be set in @mask.
+ * MAY_WRITE must be set in @mask whenever MAY_APPEND, MAY_CREATE_FILE, or
+ * MAY_CREATE_DIR are set.  That way, file systems that don't support these
+ * permissions will check for MAY_WRITE instead.
  */
 int inode_permission(struct inode *inode, int mask)
 {
@@ -2545,10 +2547,11 @@ EXPORT_SYMBOL(__check_sticky);
  * 10. We don't allow removal of NFS sillyrenamed files; it's handled by
  *     nfs_async_unlink().
  */
-static int may_delete(struct inode *dir, struct dentry *victim, bool isdir)
+static int may_delete(struct inode *dir, struct dentry *victim,
+      bool isdir, bool replace)
 {
  struct inode *inode = d_backing_inode(victim);
- int error;
+ int error, mask = MAY_WRITE | MAY_EXEC;
 
  if (d_is_negative(victim))
  return -ENOENT;
@@ -2557,7 +2560,9 @@ static int may_delete(struct inode *dir, struct dentry *victim, bool isdir)
  BUG_ON(victim->d_parent->d_inode != dir);
  audit_inode_child(dir, victim, AUDIT_TYPE_CHILD_DELETE);
 
- error = inode_permission(dir, MAY_WRITE | MAY_EXEC);
+ if (replace)
+ mask |= isdir ? MAY_CREATE_DIR : MAY_CREATE_FILE;
+ error = inode_permission(dir, mask);
  if (error)
  return error;
  if (IS_APPEND(dir))
@@ -2588,14 +2593,16 @@ static int may_delete(struct inode *dir, struct dentry *victim, bool isdir)
  *  3. We should have write and exec permissions on dir
  *  4. We can't do it if dir is immutable (done in permission())
  */
-static inline int may_create(struct inode *dir, struct dentry *child)
+static inline int may_create(struct inode *dir, struct dentry *child, bool isdir)
 {
+ int mask = isdir ? MAY_CREATE_DIR : MAY_CREATE_FILE;
+
  audit_inode_child(dir, child, AUDIT_TYPE_CHILD_CREATE);
  if (child->d_inode)
  return -EEXIST;
  if (IS_DEADDIR(dir))
  return -ENOENT;
- return inode_permission(dir, MAY_WRITE | MAY_EXEC);
+ return inode_permission(dir, MAY_WRITE | MAY_EXEC | mask);
 }
 
 /*
@@ -2645,7 +2652,7 @@ EXPORT_SYMBOL(unlock_rename);
 int vfs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
  bool want_excl)
 {
- int error = may_create(dir, dentry);
+ int error = may_create(dir, dentry, false);
  if (error)
  return error;
 
@@ -3490,7 +3497,7 @@ EXPORT_SYMBOL(user_path_create);
 
 int vfs_mknod(struct inode *dir, struct dentry *dentry, umode_t mode, dev_t dev)
 {
- int error = may_create(dir, dentry);
+ int error = may_create(dir, dentry, false);
 
  if (error)
  return error;
@@ -3582,7 +3589,7 @@ SYSCALL_DEFINE3(mknod, const char __user *, filename, umode_t, mode, unsigned, d
 
 int vfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
 {
- int error = may_create(dir, dentry);
+ int error = may_create(dir, dentry, true);
  unsigned max_links = dir->i_sb->s_max_links;
 
  if (error)
@@ -3663,7 +3670,7 @@ EXPORT_SYMBOL(dentry_unhash);
 
 int vfs_rmdir(struct inode *dir, struct dentry *dentry)
 {
- int error = may_delete(dir, dentry, 1);
+ int error = may_delete(dir, dentry, true, false);
 
  if (error)
  return error;
@@ -3785,7 +3792,7 @@ SYSCALL_DEFINE1(rmdir, const char __user *, pathname)
 int vfs_unlink(struct inode *dir, struct dentry *dentry, struct inode **delegated_inode)
 {
  struct inode *target = dentry->d_inode;
- int error = may_delete(dir, dentry, 0);
+ int error = may_delete(dir, dentry, false, false);
 
  if (error)
  return error;
@@ -3919,7 +3926,7 @@ SYSCALL_DEFINE1(unlink, const char __user *, pathname)
 
 int vfs_symlink(struct inode *dir, struct dentry *dentry, const char *oldname)
 {
- int error = may_create(dir, dentry);
+ int error = may_create(dir, dentry, false);
 
  if (error)
  return error;
@@ -4002,7 +4009,7 @@ int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_de
  if (!inode)
  return -ENOENT;
 
- error = may_create(dir, new_dentry);
+ error = may_create(dir, new_dentry, false);
  if (error)
  return error;
 
@@ -4190,19 +4197,19 @@ int vfs_rename(struct inode *old_dir, struct dentry *old_dentry,
  if (source == target)
  return 0;
 
- error = may_delete(old_dir, old_dentry, is_dir);
+ error = may_delete(old_dir, old_dentry, is_dir, false);
  if (error)
  return error;
 
  if (!target) {
- error = may_create(new_dir, new_dentry);
+ error = may_create(new_dir, new_dentry, is_dir);
  } else {
  new_is_dir = d_is_dir(new_dentry);
 
  if (!(flags & RENAME_EXCHANGE))
- error = may_delete(new_dir, new_dentry, is_dir);
+ error = may_delete(new_dir, new_dentry, is_dir, true);
  else
- error = may_delete(new_dir, new_dentry, new_is_dir);
+ error = may_delete(new_dir, new_dentry, new_is_dir, true);
  }
  if (error)
  return error;
@@ -4465,7 +4472,7 @@ SYSCALL_DEFINE2(rename, const char __user *, oldname, const char __user *, newna
 
 int vfs_whiteout(struct inode *dir, struct dentry *dentry)
 {
- int error = may_create(dir, dentry);
+ int error = may_create(dir, dentry, false);
  if (error)
  return error;
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 4efa435..d6e2330 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -82,6 +82,8 @@ typedef void (dax_iodone_t)(struct buffer_head *bh_map, int uptodate);
 #define MAY_CHDIR 0x00000040
 /* called from RCU mode, don't block */
 #define MAY_NOT_BLOCK 0x00000080
+#define MAY_CREATE_FILE 0x00000100
+#define MAY_CREATE_DIR 0x00000200
 
 /*
  * flags in file.f_mode.  Note that FMODE_READ and FMODE_WRITE must correspond
--
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v8 09/41] richacl: Update the file masks in chmod()

J. Bruce Fields
In reply to this post by Andreas Gruenbacher-6
On Mon, Sep 28, 2015 at 12:09:00AM +0200, Andreas Gruenbacher wrote:

> Doing a chmod() sets the file mode, which includes the file permission
> bits.  When a file has a richacl, the permissions that the richacl
> grants need to be limited to what the new file permission bits allow.
>
> This is done by setting the file masks in the richacl to what the file
> permission bits map to.  The richacl access check algorithm takes the
> file masks into account, which ensures that the richacl cannot grant too
> many permissions.
>
> It is possible to explicitly add permissions to the file masks which go
> beyond what the file permission bits can grant (like the
> RICHACE_WRITE_ACL permission).  The POSIX.1 standard calls this an
> alternate file access control mechanism.  A subsequent chmod() would
> ensure that those permissions are disabled again.
>
> Signed-off-by: Andreas Gruenbacher <[hidden email]>
> Reviewed-by: J. Bruce Fields <[hidden email]>
> ---
>  fs/richacl_base.c       | 40 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/richacl.h |  1 +
>  2 files changed, 41 insertions(+)
>
> diff --git a/fs/richacl_base.c b/fs/richacl_base.c
> index fc544f7..6c69234 100644
> --- a/fs/richacl_base.c
> +++ b/fs/richacl_base.c
> @@ -339,3 +339,43 @@ restart:
>   acl->a_flags &= ~(RICHACL_WRITE_THROUGH | RICHACL_MASKED);
>  }
>  EXPORT_SYMBOL_GPL(richacl_compute_max_masks);
> +
> +/**
> + * richacl_chmod  -  update the file masks to reflect the new mode
> + * @mode: new file permission bits including the file type
> + *
> + * Return a copy of @acl where the file masks have been replaced by the file
> + * masks corresponding to the file permission bits in @mode, or returns @acl
> + * itself if the file masks are already up to date.  Takes over a reference
> + * to @acl.
> + */
> +struct richacl *
> +richacl_chmod(struct richacl *acl, mode_t mode)
> +{
> + unsigned int x = S_ISDIR(mode) ? 0 : RICHACE_DELETE_CHILD;
> + unsigned int owner_mask, group_mask, other_mask;
> + struct richacl *clone;
> +
> + owner_mask = richacl_mode_to_mask(mode >> 6) & ~x;
> + group_mask = richacl_mode_to_mask(mode >> 3) & ~x;
> + other_mask = richacl_mode_to_mask(mode)      & ~x;
> +
> + if (acl->a_owner_mask == owner_mask &&
> +    acl->a_group_mask == group_mask &&
> +    acl->a_other_mask == other_mask &&
> +    (acl->a_flags & (RICHACL_WRITE_THROUGH | RICHACL_MASKED)))

I think you meant to require both of those bits to be set.

--b.

> + return acl;
> +
> + clone = richacl_clone(acl, GFP_KERNEL);
> + richacl_put(acl);
> + if (!clone)
> + return ERR_PTR(-ENOMEM);
> +
> + clone->a_flags |= (RICHACL_WRITE_THROUGH | RICHACL_MASKED);
> + clone->a_owner_mask = owner_mask;
> + clone->a_group_mask = group_mask;
> + clone->a_other_mask = other_mask;
> +
> + return clone;
> +}
> +EXPORT_SYMBOL_GPL(richacl_chmod);
> diff --git a/include/linux/richacl.h b/include/linux/richacl.h
> index e81144a..e00f313 100644
> --- a/include/linux/richacl.h
> +++ b/include/linux/richacl.h
> @@ -298,5 +298,6 @@ extern int richacl_masks_to_mode(const struct richacl *);
>  extern unsigned int richacl_mode_to_mask(mode_t);
>  extern unsigned int richacl_want_to_mask(unsigned int);
>  extern void richacl_compute_max_masks(struct richacl *);
> +extern struct richacl *richacl_chmod(struct richacl *, mode_t);
>  
>  #endif /* __RICHACL_H */
> --
> 2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v8 10/41] richacl: Permission check algorithm

J. Bruce Fields
In reply to this post by Andreas Gruenbacher-6
On Mon, Sep 28, 2015 at 12:09:01AM +0200, Andreas Gruenbacher wrote:

> A richacl roughly grants a requested access if the NFSv4 acl in the
> richacl grants the requested permissions according to the NFSv4
> permission check algorithm and the file mask that applies to the process
> includes the requested permissions.
>
> Signed-off-by: Andreas Gruenbacher <[hidden email]>
> Reviewed-by: "J. Bruce Fields" <[hidden email]>
> ---
>  fs/Makefile             |   2 +-
>  fs/richacl_inode.c      | 148 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/richacl.h |   3 +
>  3 files changed, 152 insertions(+), 1 deletion(-)
>  create mode 100644 fs/richacl_inode.c
>
> diff --git a/fs/Makefile b/fs/Makefile
> index fe3e9dd..ec665fd 100644
> --- a/fs/Makefile
> +++ b/fs/Makefile
> @@ -49,7 +49,7 @@ obj-$(CONFIG_SYSCTL) += drop_caches.o
>  
>  obj-$(CONFIG_FHANDLE) += fhandle.o
>  obj-$(CONFIG_FS_RICHACL) += richacl.o
> -richacl-y := richacl_base.o
> +richacl-y := richacl_base.o richacl_inode.o
>  
>  obj-y += quota/
>  
> diff --git a/fs/richacl_inode.c b/fs/richacl_inode.c
> new file mode 100644
> index 0000000..54e899d
> --- /dev/null
> +++ b/fs/richacl_inode.c
> @@ -0,0 +1,148 @@
> +/*
> + * Copyright (C) 2010  Novell, Inc.
> + * Copyright (C) 2015  Red Hat, Inc.
> + * Written by Andreas Gruenbacher <[hidden email]>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2, or (at your option) any
> + * later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <linux/sched.h>
> +#include <linux/module.h>
> +#include <linux/fs.h>
> +#include <linux/slab.h>
> +#include <linux/richacl.h>
> +
> +/**
> + * richacl_permission  -  richacl permission check algorithm
> + * @inode: inode to check
> + * @acl: rich acl of the inode
> + * @want: requested access (MAY_* flags)
> + *
> + * Checks if the current process is granted @mask flags in @acl.
> + */
> +int
> +richacl_permission(struct inode *inode, const struct richacl *acl,
> +   int want)
> +{
> + const struct richace *ace;
> + unsigned int mask = richacl_want_to_mask(want);
> + 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;
> +
> + /*
> + * 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
> + *     it matches any of the user or group entries, and
> + *   - in the other file class otherwise.
> + * The file class is only relevant for determining which file mask to
> + * apply, which only happens for masked acls.
> + */
> + if (acl->a_flags & RICHACL_MASKED) {
> + if ((acl->a_flags & RICHACL_WRITE_THROUGH) &&
> +    uid_eq(current_fsuid(), inode->i_uid)) {
> + denied = requested & ~acl->a_owner_mask;
> + goto out;
> + }
> + } else {
> + /*
> + * When the acl is not masked, there is no need to determine if
> + * the process is in the group class and we can break out
> + * earlier of the loop below.
> + */
> + in_owner_or_group_class = 1;
> + }
> +
> + /*
> + * Check if the acl grants the requested access and determine which
> + * file class the process is in.
> + */
> + richacl_for_each_entry(ace, acl) {
> + unsigned int ace_mask = ace->e_mask;
> +
> + if (richace_is_inherit_only(ace))
> + continue;
> + if (richace_is_owner(ace)) {
> + if (!uid_eq(current_fsuid(), inode->i_uid))
> + continue;
> + goto entry_matches_owner;
> + } else if (richace_is_group(ace)) {
> + if (!in_owning_group)
> + continue;
> + } else if (richace_is_unix_user(ace)) {
> + if (!uid_eq(current_fsuid(), ace->e_id.uid))
> + continue;
> + goto entry_matches_owner;
> + } else if (richace_is_unix_group(ace)) {
> + if (!in_group_p(ace->e_id.gid))
> + continue;
> + } else
> + goto entry_matches_everyone;
> +
> + /*
> + * Apply the group file mask to entries other than owner@ and
> + * everyone@ or user entries matching the owner.

The above also skips the following group_mask application on any unix
group.  But that's OK, since a non-owner matching a unix group will be
placed into the group class, so the RICHACL_MASKED clause after the loop
below will apply the group mask.  That logic is a little subtle, but OK.

--b.

> This ensures
> + * that we grant the same permissions as the acl computed by
> + * richacl_apply_masks().
> + *
> + * Without this restriction, the following richacl would grant
> + * rw access to processes which are both the owner and in the
> + * owning group, but not to other users in the owning group,
> + * which could not be represented without masks:
> + *
> + *  owner:rw::mask
> + *  group@:rw::allow
> + */
> + if ((acl->a_flags & RICHACL_MASKED) && richace_is_allow(ace))
> + ace_mask &= acl->a_group_mask;
> +
> +entry_matches_owner:
> + /* The process is in the owner or group file class. */
> + in_owner_or_group_class = 1;
> +
> +entry_matches_everyone:
> + /* Check which mask flags the ACE allows or denies. */
> + if (richace_is_deny(ace))
> + denied |= ace_mask & mask;
> + mask &= ~ace_mask;
> +
> + /*
> + * Keep going until we know which file class
> + * the process is in.
> + */
> + if (!mask && in_owner_or_group_class)
> + break;
> + }
> + denied |= mask;
> +
> + if (acl->a_flags & RICHACL_MASKED) {
> + /*
> + * The file class a process is in determines which file mask
> + * applies.  Check if that file mask also grants the requested
> + * access.
> + */
> + if (uid_eq(current_fsuid(), inode->i_uid))
> + denied |= requested & ~acl->a_owner_mask;
> + else if (in_owner_or_group_class)
> + denied |= requested & ~acl->a_group_mask;
> + else {
> + if (acl->a_flags & RICHACL_WRITE_THROUGH)
> + denied = requested & ~acl->a_other_mask;
> + else
> + denied |= requested & ~acl->a_other_mask;
> + }
> + }
> +
> +out:
> + return denied ? -EACCES : 0;
> +}
> +EXPORT_SYMBOL_GPL(richacl_permission);
> diff --git a/include/linux/richacl.h b/include/linux/richacl.h
> index e00f313..9768eeb 100644
> --- a/include/linux/richacl.h
> +++ b/include/linux/richacl.h
> @@ -300,4 +300,7 @@ extern unsigned int richacl_want_to_mask(unsigned int);
>  extern void richacl_compute_max_masks(struct richacl *);
>  extern struct richacl *richacl_chmod(struct richacl *, mode_t);
>  
> +/* richacl_inode.c */
> +extern int richacl_permission(struct inode *, const struct richacl *, int);
> +
>  #endif /* __RICHACL_H */
> --
> 2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v8 10/41] richacl: Permission check algorithm

Andreas Grünbacher
2015-09-28 18:08 GMT+02:00 J. Bruce Fields <[hidden email]>:

> On Mon, Sep 28, 2015 at 12:09:01AM +0200, Andreas Gruenbacher wrote:
>> +     /*
>> +      * Check if the acl grants the requested access and determine which
>> +      * file class the process is in.
>> +      */
>> +     richacl_for_each_entry(ace, acl) {
>> +             unsigned int ace_mask = ace->e_mask;
>> +
>> +             if (richace_is_inherit_only(ace))
>> +                     continue;
>> +             if (richace_is_owner(ace)) {
>> +                     if (!uid_eq(current_fsuid(), inode->i_uid))
>> +                             continue;
>> +                     goto entry_matches_owner;
>> +             } else if (richace_is_group(ace)) {
>> +                     if (!in_owning_group)
>> +                             continue;
>> +             } else if (richace_is_unix_user(ace)) {
>> +                     if (!uid_eq(current_fsuid(), ace->e_id.uid))
>> +                             continue;
>> +                     goto entry_matches_owner;
>> +             } else if (richace_is_unix_group(ace)) {
>> +                     if (!in_group_p(ace->e_id.gid))
>> +                             continue;
>> +             } else
>> +                     goto entry_matches_everyone;
>> +
>> +             /*
>> +              * Apply the group file mask to entries other than owner@ and
>> +              * everyone@ or user entries matching the owner.
>
> The above also skips the following group_mask application on any unix
> group.

Really? How does it do that?

Thanks,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v8 10/41] richacl: Permission check algorithm

J. Bruce Fields
On Mon, Sep 28, 2015 at 06:25:23PM +0200, Andreas Grünbacher wrote:

> 2015-09-28 18:08 GMT+02:00 J. Bruce Fields <[hidden email]>:
> > On Mon, Sep 28, 2015 at 12:09:01AM +0200, Andreas Gruenbacher wrote:
> >> +     /*
> >> +      * Check if the acl grants the requested access and determine which
> >> +      * file class the process is in.
> >> +      */
> >> +     richacl_for_each_entry(ace, acl) {
> >> +             unsigned int ace_mask = ace->e_mask;
> >> +
> >> +             if (richace_is_inherit_only(ace))
> >> +                     continue;
> >> +             if (richace_is_owner(ace)) {
> >> +                     if (!uid_eq(current_fsuid(), inode->i_uid))
> >> +                             continue;
> >> +                     goto entry_matches_owner;
> >> +             } else if (richace_is_group(ace)) {
> >> +                     if (!in_owning_group)
> >> +                             continue;
> >> +             } else if (richace_is_unix_user(ace)) {
> >> +                     if (!uid_eq(current_fsuid(), ace->e_id.uid))
> >> +                             continue;
> >> +                     goto entry_matches_owner;
> >> +             } else if (richace_is_unix_group(ace)) {
> >> +                     if (!in_group_p(ace->e_id.gid))
> >> +                             continue;
> >> +             } else
> >> +                     goto entry_matches_everyone;
> >> +
> >> +             /*
> >> +              * Apply the group file mask to entries other than owner@ and
> >> +              * everyone@ or user entries matching the owner.
> >
> > The above also skips the following group_mask application on any unix
> > group.
>
> Really? How does it do that?

Sorry, I meant "unix user", not "unix group"!

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v8 00/41] Richacls

J. Bruce Fields
In reply to this post by Andreas Gruenbacher-6
On Mon, Sep 28, 2015 at 12:08:51AM +0200, Andreas Gruenbacher wrote:
> here's another update of the richacl patch queue.  At this stage, I would
> like to ask for final feedback so that the core and ext4 code (patches
> 1-19) can be merged in the 4.4 merge window.  The nfsd and nfs code should
> then go through the respective maintainer trees.

I've been over the core richacl and nfsd parts very carefully, and they
definitely look ready to me.

> Changes since the last posting (https://lwn.net/Articles/656704/):
>
> * The MAY_DELETE_SELF permission now also overrides the sticky
>    directory checks.
>
> * Fix the permission check algorithm to apply the owner mask instead
>   of the group mask to user entries matching the current owner. That way,
>   the owner will retain the permissions in those entries when creating
>   objects with create mode 0700 and similar. (A chmod to mode 0700 already
>   creates an owner@:rwpx::allow ace, which was hiding this bug.)
>
> * Fix richacl_apply_masks to properly insert deny aces when raising the
>   permissions of the other class. The bug could be triggered by
>   chmod'ing a group@:r::allow acl to mode 0077, for example.
>
> * Various cleanups and improvements to comments.
>
>
> The complete patch queue is available here:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/agruen/linux-richacl.git \
>           richacl-2015-09-28
>
>
> The richacl user-space utilitites and test suite are available here:
>
>   https://github.com/andreas-gruenbacher/richacl/
>
>
> Open issues in nfs:
>
> * When a user or group name cannot be mapped, nfs's idmapper always maps it
>   to nobody. That's good enough for mapping the file owner and owning
>   group, but not for identifiers in acls. For now, to get the nfs richacl
>   support somewhat working, I'm explicitly checking if mapping has resulted
>   in uid/gid 99 in the kernel.
>
> * When the nfs server replies with NFS4ERR_BADNAME for any user or group
>   name lookup, the client will stop sending numeric uids and gids to the
>   server even when the lookup wasn't numeric.  From then on, the client
>   will translate uids and gids that have no mapping to the string "nobody",
>   and the server will reject them.  This problem is not specific to acls.

Do you have fixes in mind for these two issues?

--b.


>
> Thanks,
> Andreas
>
> Andreas Gruenbacher (39):
>   vfs: Add IS_ACL() and IS_RICHACL() tests
>   vfs: Add MAY_CREATE_FILE and MAY_CREATE_DIR permission flags
>   vfs: Add MAY_DELETE_SELF and MAY_DELETE_CHILD permission flags
>   vfs: Make the inode passed to inode_change_ok non-const
>   vfs: Add permission flags for setting file attributes
>   richacl: In-memory representation and helper functions
>   richacl: Permission mapping functions
>   richacl: Compute maximum file masks from an acl
>   richacl: Update the file masks in chmod()
>   richacl: Permission check algorithm
>   vfs: Cache base_acl objects in inodes
>   vfs: Cache richacl in struct inode
>   richacl: Check if an acl is equivalent to a file mode
>   richacl: Create-time inheritance
>   richacl: Automatic Inheritance
>   richacl: xattr mapping functions
>   vfs: Add richacl permission checking
>   richacl: acl editing helper functions
>   richacl: Move everyone@ aces down the acl
>   richacl: Propagate everyone@ permissions to other aces
>   richacl: Set the owner permissions to the owner mask
>   richacl: Set the other permissions to the other mask
>   richacl: Isolate the owner and group classes
>   richacl: Apply the file masks to a richacl
>   richacl: Create richacl from mode values
>   nfsd: Keep list of acls to dispose of in compoundargs
>   nfsd: Use richacls as internal acl representation
>   nfsd: Add richacl support
>   nfsd: Add support for the v4.1 dacl attribute
>   nfsd: Add support for the MAY_CREATE_{FILE,DIR} permissions
>   richacl: Add support for unmapped identifiers
>   ext4: Don't allow unmapped identifiers in richacls
>   sunrpc: Allow to demand-allocate pages to encode into
>   sunrpc: Add xdr_init_encode_pages
>   nfs: Fix GETATTR bitmap verification
>   nfs: Remove unused xdr page offsets in getacl/setacl arguments
>   nfs: Add richacl support
>   nfs: Add support for the v4.1 dacl attribute
>   richacl: uapi header split
>
> Aneesh Kumar K.V (2):
>   ext4: Add richacl support
>   ext4: Add richacl feature flag
>
>  drivers/staging/lustre/lustre/llite/llite_lib.c |   2 +-
>  fs/Kconfig                                      |   9 +
>  fs/Makefile                                     |   3 +
>  fs/attr.c                                       |  81 ++-
>  fs/ext4/Kconfig                                 |  15 +
>  fs/ext4/Makefile                                |   1 +
>  fs/ext4/acl.c                                   |   6 +-
>  fs/ext4/acl.h                                   |  12 +-
>  fs/ext4/ext4.h                                  |   6 +-
>  fs/ext4/file.c                                  |   6 +-
>  fs/ext4/ialloc.c                                |   7 +-
>  fs/ext4/inode.c                                 |  10 +-
>  fs/ext4/namei.c                                 |  11 +-
>  fs/ext4/richacl.c                               | 218 ++++++
>  fs/ext4/richacl.h                               |  47 ++
>  fs/ext4/super.c                                 |  42 +-
>  fs/ext4/xattr.c                                 |   6 +
>  fs/ext4/xattr.h                                 |   1 +
>  fs/f2fs/acl.c                                   |   4 +-
>  fs/inode.c                                      |  15 +-
>  fs/jffs2/acl.c                                  |   6 +-
>  fs/namei.c                                      | 111 ++-
>  fs/nfs/inode.c                                  |   3 -
>  fs/nfs/nfs4proc.c                               | 701 +++++++++++++-----
>  fs/nfs/nfs4xdr.c                                | 257 ++++++-
>  fs/nfs/super.c                                  |   4 +-
>  fs/nfs_common/Makefile                          |   1 +
>  fs/nfs_common/nfs4acl.c                         |  44 ++
>  fs/nfsd/Kconfig                                 |   1 +
>  fs/nfsd/acl.h                                   |  23 +-
>  fs/nfsd/nfs4acl.c                               | 482 +++++++------
>  fs/nfsd/nfs4proc.c                              |  25 +-
>  fs/nfsd/nfs4xdr.c                               | 268 ++++---
>  fs/nfsd/nfsd.h                                  |   6 +-
>  fs/nfsd/nfsfh.c                                 |   8 +-
>  fs/nfsd/vfs.c                                   |  28 +-
>  fs/nfsd/vfs.h                                   |  17 +-
>  fs/nfsd/xdr4.h                                  |  12 +-
>  fs/posix_acl.c                                  |  26 +-
>  fs/richacl_base.c                               | 682 ++++++++++++++++++
>  fs/richacl_compat.c                             | 915 ++++++++++++++++++++++++
>  fs/richacl_inode.c                              | 297 ++++++++
>  fs/richacl_xattr.c                              | 267 +++++++
>  fs/xattr.c                                      |  34 +-
>  include/linux/fs.h                              |  50 +-
>  include/linux/nfs4.h                            |  24 +-
>  include/linux/nfs4acl.h                         |   7 +
>  include/linux/nfs_fs.h                          |   1 -
>  include/linux/nfs_fs_sb.h                       |   2 +
>  include/linux/nfs_xdr.h                         |  13 +-
>  include/linux/posix_acl.h                       |  12 +-
>  include/linux/richacl.h                         | 275 +++++++
>  include/linux/richacl_compat.h                  |  40 ++
>  include/linux/richacl_xattr.h                   |  47 ++
>  include/linux/sunrpc/xdr.h                      |   2 +
>  include/uapi/linux/Kbuild                       |   2 +
>  include/uapi/linux/fs.h                         |   3 +-
>  include/uapi/linux/nfs4.h                       |   3 +-
>  include/uapi/linux/richacl.h                    | 111 +++
>  include/uapi/linux/richacl_xattr.h              |  43 ++
>  include/uapi/linux/xattr.h                      |   2 +
>  net/sunrpc/xdr.c                                |  34 +
>  62 files changed, 4659 insertions(+), 732 deletions(-)
>  create mode 100644 fs/ext4/richacl.c
>  create mode 100644 fs/ext4/richacl.h
>  create mode 100644 fs/nfs_common/nfs4acl.c
>  create mode 100644 fs/richacl_base.c
>  create mode 100644 fs/richacl_compat.c
>  create mode 100644 fs/richacl_inode.c
>  create mode 100644 fs/richacl_xattr.c
>  create mode 100644 include/linux/nfs4acl.h
>  create mode 100644 include/linux/richacl.h
>  create mode 100644 include/linux/richacl_compat.h
>  create mode 100644 include/linux/richacl_xattr.h
>  create mode 100644 include/uapi/linux/richacl.h
>  create mode 100644 include/uapi/linux/richacl_xattr.h
>
> --
> 2.4.3
>
>
> Andreas Gruenbacher (39):
>   vfs: Add IS_ACL() and IS_RICHACL() tests
>   vfs: Add MAY_CREATE_FILE and MAY_CREATE_DIR permission flags
>   vfs: Add MAY_DELETE_SELF and MAY_DELETE_CHILD permission flags
>   vfs: Make the inode passed to inode_change_ok non-const
>   vfs: Add permission flags for setting file attributes
>   richacl: In-memory representation and helper functions
>   richacl: Permission mapping functions
>   richacl: Compute maximum file masks from an acl
>   richacl: Update the file masks in chmod()
>   richacl: Permission check algorithm
>   vfs: Cache base_acl objects in inodes
>   vfs: Cache richacl in struct inode
>   richacl: Check if an acl is equivalent to a file mode
>   richacl: Create-time inheritance
>   richacl: Automatic Inheritance
>   richacl: xattr mapping functions
>   vfs: Add richacl permission checking
>   richacl: acl editing helper functions
>   richacl: Move everyone@ aces down the acl
>   richacl: Propagate everyone@ permissions to other aces
>   richacl: Set the owner permissions to the owner mask
>   richacl: Set the other permissions to the other mask
>   richacl: Isolate the owner and group classes
>   richacl: Apply the file masks to a richacl
>   richacl: Create richacl from mode values
>   nfsd: Keep list of acls to dispose of in compoundargs
>   nfsd: Use richacls as internal acl representation
>   nfsd: Add richacl support
>   nfsd: Add support for the v4.1 dacl attribute
>   nfsd: Add support for the MAY_CREATE_{FILE,DIR} permissions
>   richacl: Add support for unmapped identifiers
>   ext4: Don't allow unmapped identifiers in richacls
>   sunrpc: Allow to demand-allocate pages to encode into
>   sunrpc: Add xdr_init_encode_pages
>   nfs: Fix GETATTR bitmap verification
>   nfs: Remove unused xdr page offsets in getacl/setacl arguments
>   nfs: Add richacl support
>   nfs: Add support for the v4.1 dacl attribute
>   richacl: uapi header split
>
> Aneesh Kumar K.V (2):
>   ext4: Add richacl support
>   ext4: Add richacl feature flag
>
>  drivers/staging/lustre/lustre/llite/llite_lib.c |   2 +-
>  fs/Kconfig                                      |   9 +
>  fs/Makefile                                     |   3 +
>  fs/attr.c                                       |  81 ++-
>  fs/ext4/Kconfig                                 |  15 +
>  fs/ext4/Makefile                                |   1 +
>  fs/ext4/acl.c                                   |   6 +-
>  fs/ext4/acl.h                                   |  12 +-
>  fs/ext4/ext4.h                                  |   6 +-
>  fs/ext4/file.c                                  |   6 +-
>  fs/ext4/ialloc.c                                |   7 +-
>  fs/ext4/inode.c                                 |  10 +-
>  fs/ext4/namei.c                                 |  11 +-
>  fs/ext4/richacl.c                               | 218 ++++++
>  fs/ext4/richacl.h                               |  47 ++
>  fs/ext4/super.c                                 |  42 +-
>  fs/ext4/xattr.c                                 |   6 +
>  fs/ext4/xattr.h                                 |   1 +
>  fs/f2fs/acl.c                                   |   4 +-
>  fs/inode.c                                      |  15 +-
>  fs/jffs2/acl.c                                  |   6 +-
>  fs/namei.c                                      | 111 ++-
>  fs/nfs/inode.c                                  |   3 -
>  fs/nfs/nfs4proc.c                               | 701 +++++++++++++-----
>  fs/nfs/nfs4xdr.c                                | 257 ++++++-
>  fs/nfs/super.c                                  |   4 +-
>  fs/nfs_common/Makefile                          |   1 +
>  fs/nfs_common/nfs4acl.c                         |  44 ++
>  fs/nfsd/Kconfig                                 |   1 +
>  fs/nfsd/acl.h                                   |  23 +-
>  fs/nfsd/nfs4acl.c                               | 482 +++++++------
>  fs/nfsd/nfs4proc.c                              |  25 +-
>  fs/nfsd/nfs4xdr.c                               | 268 ++++---
>  fs/nfsd/nfsd.h                                  |   6 +-
>  fs/nfsd/nfsfh.c                                 |   8 +-
>  fs/nfsd/vfs.c                                   |  28 +-
>  fs/nfsd/vfs.h                                   |  17 +-
>  fs/nfsd/xdr4.h                                  |  12 +-
>  fs/posix_acl.c                                  |  26 +-
>  fs/richacl_base.c                               | 682 ++++++++++++++++++
>  fs/richacl_compat.c                             | 915 ++++++++++++++++++++++++
>  fs/richacl_inode.c                              | 297 ++++++++
>  fs/richacl_xattr.c                              | 267 +++++++
>  fs/xattr.c                                      |  34 +-
>  include/linux/fs.h                              |  50 +-
>  include/linux/nfs4.h                            |  24 +-
>  include/linux/nfs4acl.h                         |   7 +
>  include/linux/nfs_fs.h                          |   1 -
>  include/linux/nfs_fs_sb.h                       |   2 +
>  include/linux/nfs_xdr.h                         |  13 +-
>  include/linux/posix_acl.h                       |  12 +-
>  include/linux/richacl.h                         | 275 +++++++
>  include/linux/richacl_compat.h                  |  40 ++
>  include/linux/richacl_xattr.h                   |  47 ++
>  include/linux/sunrpc/xdr.h                      |   2 +
>  include/uapi/linux/Kbuild                       |   2 +
>  include/uapi/linux/fs.h                         |   3 +-
>  include/uapi/linux/nfs4.h                       |   3 +-
>  include/uapi/linux/richacl.h                    | 111 +++
>  include/uapi/linux/richacl_xattr.h              |  43 ++
>  include/uapi/linux/xattr.h                      |   2 +
>  net/sunrpc/xdr.c                                |  34 +
>  62 files changed, 4659 insertions(+), 732 deletions(-)
>  create mode 100644 fs/ext4/richacl.c
>  create mode 100644 fs/ext4/richacl.h
>  create mode 100644 fs/nfs_common/nfs4acl.c
>  create mode 100644 fs/richacl_base.c
>  create mode 100644 fs/richacl_compat.c
>  create mode 100644 fs/richacl_inode.c
>  create mode 100644 fs/richacl_xattr.c
>  create mode 100644 include/linux/nfs4acl.h
>  create mode 100644 include/linux/richacl.h
>  create mode 100644 include/linux/richacl_compat.h
>  create mode 100644 include/linux/richacl_xattr.h
>  create mode 100644 include/uapi/linux/richacl.h
>  create mode 100644 include/uapi/linux/richacl_xattr.h
>
> --
> 2.4.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v8 10/41] richacl: Permission check algorithm

Andreas Grünbacher
In reply to this post by J. Bruce Fields
2015-09-28 18:29 GMT+02:00 J. Bruce Fields <[hidden email]>:
> On Mon, Sep 28, 2015 at 06:25:23PM +0200, Andreas Grünbacher wrote:
>> 2015-09-28 18:08 GMT+02:00 J. Bruce Fields <[hidden email]>:
>> > The above also skips the following group_mask application on any unix
>> > group.
>>
>> Really? How does it do that?
>
> Sorry, I meant "unix user", not "unix group"!

Indeed, that's a bit tricky. Probably worth changing if just for clarity:

-                       goto entry_matches_owner;
+                       if (uid_eq(current_fsuid(), inode->i_uid))
+                               goto entry_matches_owner;

Thanks,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v8 00/41] Richacls

Andreas Grünbacher
In reply to this post by J. Bruce Fields
2015-09-28 18:35 GMT+02:00 J. Bruce Fields <[hidden email]>:
> On Mon, Sep 28, 2015 at 12:08:51AM +0200, Andreas Gruenbacher wrote:
>> here's another update of the richacl patch queue.  At this stage, I would
>> like to ask for final feedback so that the core and ext4 code (patches
>> 1-19) can be merged in the 4.4 merge window.  The nfsd and nfs code should
>> then go through the respective maintainer trees.
>
> I've been over the core richacl and nfsd parts very carefully, and they
> definitely look ready to me.

Thanks a lot for all that work, by the way.

>> Changes since the last posting (https://lwn.net/Articles/656704/):
>>
>> * The MAY_DELETE_SELF permission now also overrides the sticky
>>    directory checks.
>>
>> * Fix the permission check algorithm to apply the owner mask instead
>>   of the group mask to user entries matching the current owner. That way,
>>   the owner will retain the permissions in those entries when creating
>>   objects with create mode 0700 and similar. (A chmod to mode 0700 already
>>   creates an owner@:rwpx::allow ace, which was hiding this bug.)
>>
>> * Fix richacl_apply_masks to properly insert deny aces when raising the
>>   permissions of the other class. The bug could be triggered by
>>   chmod'ing a group@:r::allow acl to mode 0077, for example.
>>
>> * Various cleanups and improvements to comments.
>>
>>
>> The complete patch queue is available here:
>>
>>   git://git.kernel.org/pub/scm/linux/kernel/git/agruen/linux-richacl.git \
>>           richacl-2015-09-28
>>
>>
>> The richacl user-space utilitites and test suite are available here:
>>
>>   https://github.com/andreas-gruenbacher/richacl/
>>
>>
>> Open issues in nfs:
>>
>> * When a user or group name cannot be mapped, nfs's idmapper always maps it
>>   to nobody. That's good enough for mapping the file owner and owning
>>   group, but not for identifiers in acls. For now, to get the nfs richacl
>>   support somewhat working, I'm explicitly checking if mapping has resulted
>>   in uid/gid 99 in the kernel.
>>
>> * When the nfs server replies with NFS4ERR_BADNAME for any user or group
>>   name lookup, the client will stop sending numeric uids and gids to the
>>   server even when the lookup wasn't numeric.  From then on, the client
>>   will translate uids and gids that have no mapping to the string "nobody",
>>   and the server will reject them.  This problem is not specific to acls.
>
> Do you have fixes in mind for these two issues?

I'm not sure how to best fix the idmapper problem, with backwards
compatibility and all. The second problem shouldn't be too hard to
fix.

Thanks,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v8 00/41] Richacls

J. Bruce Fields
On Mon, Sep 28, 2015 at 07:10:06PM +0200, Andreas Grünbacher wrote:

> 2015-09-28 18:35 GMT+02:00 J. Bruce Fields <[hidden email]>:
> > On Mon, Sep 28, 2015 at 12:08:51AM +0200, Andreas Gruenbacher wrote:
> >> Open issues in nfs:
> >>
> >> * When a user or group name cannot be mapped, nfs's idmapper always maps it
> >>   to nobody. That's good enough for mapping the file owner and owning
> >>   group, but not for identifiers in acls. For now, to get the nfs richacl
> >>   support somewhat working, I'm explicitly checking if mapping has resulted
> >>   in uid/gid 99 in the kernel.
> >>
> >> * When the nfs server replies with NFS4ERR_BADNAME for any user or group
> >>   name lookup, the client will stop sending numeric uids and gids to the
> >>   server even when the lookup wasn't numeric.  From then on, the client
> >>   will translate uids and gids that have no mapping to the string "nobody",
> >>   and the server will reject them.  This problem is not specific to acls.
> >
> > Do you have fixes in mind for these two issues?
>
> I'm not sure how to best fix the idmapper problem, with backwards
> compatibility and all.

I haven't looked at the current nfsidmap interface....  So it's
completely lacking any way to communicate failure?

> The second problem shouldn't be too hard to fix.

Is it enough to turn off the failover in the case there's no possibility
it could have been caused by a numeric id?

If any user can set ACLs with arbitrary strings as names, then we'd be
giving any user unprivileged user the ability to turn off numeric
idmapping, so I think we need to fix that.

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v8 09/41] richacl: Update the file masks in chmod()

Andreas Gruenbacher-6
In reply to this post by J. Bruce Fields
2015-09-28 17:28 GMT+02:00 J. Bruce Fields <[hidden email]>:
> On Mon, Sep 28, 2015 at 12:09:00AM +0200, Andreas Gruenbacher wrote:
>> +         (acl->a_flags & (RICHACL_WRITE_THROUGH | RICHACL_MASKED)))
>
> I think you meant to require both of those bits to be set.

Yes, indeed.

Thanks,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v8 00/41] Richacls

Andreas Grünbacher
In reply to this post by J. Bruce Fields
2015-09-28 19:46 GMT+02:00 J. Bruce Fields <[hidden email]>:

> On Mon, Sep 28, 2015 at 07:10:06PM +0200, Andreas Grünbacher wrote:
>> 2015-09-28 18:35 GMT+02:00 J. Bruce Fields <[hidden email]>:
>> > On Mon, Sep 28, 2015 at 12:08:51AM +0200, Andreas Gruenbacher wrote:
>> >> Open issues in nfs:
>> >>
>> >> * When a user or group name cannot be mapped, nfs's idmapper always maps it
>> >>   to nobody. That's good enough for mapping the file owner and owning
>> >>   group, but not for identifiers in acls. For now, to get the nfs richacl
>> >>   support somewhat working, I'm explicitly checking if mapping has resulted
>> >>   in uid/gid 99 in the kernel.
>> >>
>> >> * When the nfs server replies with NFS4ERR_BADNAME for any user or group
>> >>   name lookup, the client will stop sending numeric uids and gids to the
>> >>   server even when the lookup wasn't numeric.  From then on, the client
>> >>   will translate uids and gids that have no mapping to the string "nobody",
>> >>   and the server will reject them.  This problem is not specific to acls.
>> >
>> > Do you have fixes in mind for these two issues?
>>
>> I'm not sure how to best fix the idmapper problem, with backwards
>> compatibility and all.
>
> I haven't looked at the current nfsidmap interface....  So it's
> completely lacking any way to communicate failure?

Yes, when a user doesn't exist, idmapper maps that to the nobody
uid/gid. That's the failure mode of stat. In the acl case, we do want
to map user and group names to their respective ids where possible (so
that the acl makes sense in the local system context), but we do want
to preserve the original user and group names when there is no such
mapping instead of mapping to the nobody uid/gid.

>> The second problem shouldn't be too hard to fix.
>
> Is it enough to turn off the failover in the case there's no possibility
> it could have been caused by a numeric id?

Yes, I believe that would be enough.

> If any user can set ACLs with arbitrary strings as names, then we'd be
> giving any user unprivileged user the ability to turn off numeric
> idmapping, so I think we need to fix that.

The bug can be triggered by unprivileged users with nfs4_setfacl.

Thanks,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v8 00/41] Richacls

Christoph Hellwig
In reply to this post by Andreas Gruenbacher-6
On Mon, Sep 28, 2015 at 12:08:51AM +0200, Andreas Gruenbacher wrote:
> Hello,
>
> here's another update of the richacl patch queue.  At this stage, I would
> like to ask for final feedback so that the core and ext4 code (patches
> 1-19) can be merged in the 4.4 merge window.  The nfsd and nfs code should
> then go through the respective maintainer trees.

Now way in this form even if everyone agrees we should have these
bastard ACLs.  I certainly disagree.

Ayway, back to the VFS <-> FS interface.  You still require tons of
boilderplate code in the filesystem which isn't required and we got rid
of for Posix ACLs.  The filesystem should not look at the userspace
xattr format, please follow a model similar to ->get_acl and ->set_acl
for Posix ACLs.  After that the wire up should be so trivial that you
can wire up btrfs, xfs and f2fs as well, which is important to make the
feature mergeable.

And honestly I tink adding even more overload to xattrs is a really bad
idea and after 10 years of experience with that junk we really need to
learn and make new overloads proper system calls.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v8 00/41] Richacls

Andreas Gruenbacher-6
On Sun, Oct 4, 2015 at 8:23 AM, Christoph Hellwig <[hidden email]> wrote:

> On Mon, Sep 28, 2015 at 12:08:51AM +0200, Andreas Gruenbacher wrote:
>> Hello,
>>
>> here's another update of the richacl patch queue.  At this stage, I would
>> like to ask for final feedback so that the core and ext4 code (patches
>> 1-19) can be merged in the 4.4 merge window.  The nfsd and nfs code should
>> then go through the respective maintainer trees.
>
> Now way in this form even if everyone agrees we should have these
> bastard ACLs.  I certainly disagree.

Well, thanks for having a look at the patches.

> Ayway, back to the VFS <-> FS interface.  You still require tons of
> boilderplate code in the filesystem which isn't required and we got rid
> of for Posix ACLs.  The filesystem should not look at the userspace
> xattr format, please follow a model similar to ->get_acl and ->set_acl
> for Posix ACLs.

I will repost a version that has this cleaned up.

> After that the wire up should be so trivial that you can wire up btrfs,
> xfs and f2fs as well, which is important to make the feature mergeable.

Why would the patch queue become more mergeable by having support for
more filesystems in it? The filesystem specific code really isn't all
that interesting.

> And honestly I tink adding even more overload to xattrs is a really bad
> idea and after 10 years of experience with that junk we really need to
> learn and make new overloads proper system calls.

Thanks,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v8 00/41] Richacls

Austin S. Hemmelgarn
On 2015-10-05 14:45, Andreas Gruenbacher wrote:

> On Sun, Oct 4, 2015 at 8:23 AM, Christoph Hellwig <[hidden email]> wrote:
>> On Mon, Sep 28, 2015 at 12:08:51AM +0200, Andreas Gruenbacher wrote:
>>> Hello,
>>>
>>> here's another update of the richacl patch queue.  At this stage, I would
>>> like to ask for final feedback so that the core and ext4 code (patches
>>> 1-19) can be merged in the 4.4 merge window.  The nfsd and nfs code should
>>> then go through the respective maintainer trees.
>>
>> Now way in this form even if everyone agrees we should have these
>> bastard ACLs.  I certainly disagree.
>
> Well, thanks for having a look at the patches.
>
>> Ayway, back to the VFS <-> FS interface.  You still require tons of
>> boilderplate code in the filesystem which isn't required and we got rid
>> of for Posix ACLs.  The filesystem should not look at the userspace
>> xattr format, please follow a model similar to ->get_acl and ->set_acl
>> for Posix ACLs.
>
> I will repost a version that has this cleaned up.
>
>> After that the wire up should be so trivial that you can wire up btrfs,
>> xfs and f2fs as well, which is important to make the feature mergeable.
>
> Why would the patch queue become more mergeable by having support for
> more filesystems in it? The filesystem specific code really isn't all
> that interesting.
I think the point is that a new VFS feature that is easy to integrate in
multiple filesystems should have support for those filesystems.  A
decade ago, just having ext* support would probably have been fine, but
these days, XFS, BTRFS, and F2FS are used just as much (if not more) on
production systems as ext4, and having support for them right from the
start would significantly help with adoption of richacls.

>
>> And honestly I tink adding even more overload to xattrs is a really bad
>> idea and after 10 years of experience with that junk we really need to
>> learn and make new overloads proper system calls.
>
> Thanks,
> Andreas
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [hidden email]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>


smime.p7s (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v8 00/41] Richacls

Dave Chinner
In reply to this post by Andreas Gruenbacher-6
On Mon, Oct 05, 2015 at 08:45:40PM +0200, Andreas Gruenbacher wrote:

> On Sun, Oct 4, 2015 at 8:23 AM, Christoph Hellwig <[hidden email]> wrote:
> > On Mon, Sep 28, 2015 at 12:08:51AM +0200, Andreas Gruenbacher wrote:
> >> Hello,
> >>
> >> here's another update of the richacl patch queue.  At this stage, I would
> >> like to ask for final feedback so that the core and ext4 code (patches
> >> 1-19) can be merged in the 4.4 merge window.  The nfsd and nfs code should
> >> then go through the respective maintainer trees.
> >
> > Now way in this form even if everyone agrees we should have these
> > bastard ACLs.  I certainly disagree.
>
> Well, thanks for having a look at the patches.
>
> > Ayway, back to the VFS <-> FS interface.  You still require tons of
> > boilderplate code in the filesystem which isn't required and we got rid
> > of for Posix ACLs.  The filesystem should not look at the userspace
> > xattr format, please follow a model similar to ->get_acl and ->set_acl
> > for Posix ACLs.
>
> I will repost a version that has this cleaned up.
>
> > After that the wire up should be so trivial that you can wire up btrfs,
> > xfs and f2fs as well, which is important to make the feature mergeable.
>
> Why would the patch queue become more mergeable by having support for
> more filesystems in it? The filesystem specific code really isn't all
> that interesting.

The hardest part for the filesystem support is the on-disk feature
flag that needs to be set. The kernel part of that is easy, but it's
an on-disk format change and so there's also all the userspace side
for mkfs, fsck, debug tools, etc, that also need to be able to parse
and understand it. So while the xattr code can be made much more
generic, there's a bunch of filesystem specific code that needs to
go into multiple different repositories and userspace packages for
this.

Andreas, I also can't remember if any xfstests have been written for
these ACLs? That would certainly help make sure all these
filesystems have equivalent behaviour...

Cheers,

Dave.
--
Dave Chinner
[hidden email]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v8 00/41] Richacls

Andreas Gruenbacher-6
On Mon, Oct 5, 2015 at 11:17 PM, Dave Chinner <[hidden email]> wrote:

> On Mon, Oct 05, 2015 at 08:45:40PM +0200, Andreas Gruenbacher wrote:
>> On Sun, Oct 4, 2015 at 8:23 AM, Christoph Hellwig <[hidden email]> wrote:
>> > After that the wire up should be so trivial that you can wire up btrfs,
>> > xfs and f2fs as well, which is important to make the feature mergeable.
>>
>> Why would the patch queue become more mergeable by having support for
>> more filesystems in it? The filesystem specific code really isn't all
>> that interesting.
>
> The hardest part for the filesystem support is the on-disk feature
> flag that needs to be set. The kernel part of that is easy, but it's
> an on-disk format change and so there's also all the userspace side
> for mkfs, fsck, debug tools, etc, that also need to be able to parse
> and understand it. So while the xattr code can be made much more
> generic, there's a bunch of filesystem specific code that needs to
> go into multiple different repositories and userspace packages for
> this.

Yes.

> Andreas, I also can't remember if any xfstests have been written for
> these ACLs? That would certainly help make sure all these
> filesystems have equivalent behaviour...

There's a reasonable amount of tests in the richacl user-space package
which are shell based, with a few small C helpers. We could move those
into xfstests eventually; now seems a bit early to me.

Thanks,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v8 00/41] Richacls

Dave Chinner
On Tue, Oct 06, 2015 at 12:01:19AM +0200, Andreas Gruenbacher wrote:

> On Mon, Oct 5, 2015 at 11:17 PM, Dave Chinner <[hidden email]> wrote:
> > On Mon, Oct 05, 2015 at 08:45:40PM +0200, Andreas Gruenbacher wrote:
> >> On Sun, Oct 4, 2015 at 8:23 AM, Christoph Hellwig <[hidden email]> wrote:
> >> > After that the wire up should be so trivial that you can wire up btrfs,
> >> > xfs and f2fs as well, which is important to make the feature mergeable.
> >>
> >> Why would the patch queue become more mergeable by having support for
> >> more filesystems in it? The filesystem specific code really isn't all
> >> that interesting.
> >
> > The hardest part for the filesystem support is the on-disk feature
> > flag that needs to be set. The kernel part of that is easy, but it's
> > an on-disk format change and so there's also all the userspace side
> > for mkfs, fsck, debug tools, etc, that also need to be able to parse
> > and understand it. So while the xattr code can be made much more
> > generic, there's a bunch of filesystem specific code that needs to
> > go into multiple different repositories and userspace packages for
> > this.
>
> Yes.
>
> > Andreas, I also can't remember if any xfstests have been written for
> > these ACLs? That would certainly help make sure all these
> > filesystems have equivalent behaviour...
>
> There's a reasonable amount of tests in the richacl user-space package
> which are shell based, with a few small C helpers. We could move those
> into xfstests eventually; now seems a bit early to me.

Well, all the fs developers that will do the userspace work are
already running xfstests. If you want us to be able to test the
richACL code as we add all the fs specific flags to the userspace
code, then we need the tests in xfstests at the same time the
infrastructure appears in the kernel...

Cheers,

Dave.
--
Dave Chinner
[hidden email]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v8 00/41] Richacls

James Morris-2
In reply to this post by Christoph Hellwig
On Sat, 3 Oct 2015, Christoph Hellwig wrote:

> On Mon, Sep 28, 2015 at 12:08:51AM +0200, Andreas Gruenbacher wrote:
> > Hello,
> >
> > here's another update of the richacl patch queue.  At this stage, I would
> > like to ask for final feedback so that the core and ext4 code (patches
> > 1-19) can be merged in the 4.4 merge window.  The nfsd and nfs code should
> > then go through the respective maintainer trees.
>
> Now way in this form even if everyone agrees we should have these
> bastard ACLs.  I certainly disagree.
>

Where is the rationale for them?

This url doesn't work: http://acl.bestbits.at/richacl/





--
James Morris
<[hidden email]>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v8 00/41] Richacls

Christoph Hellwig
In reply to this post by Austin S. Hemmelgarn
On Mon, Oct 05, 2015 at 02:58:36PM -0400, Austin S Hemmelgarn wrote:
> I think the point is that a new VFS feature that is easy to integrate in
> multiple filesystems should have support for those filesystems.  A decade
> ago, just having ext* support would probably have been fine, but these days,
> XFS, BTRFS, and F2FS are used just as much (if not more) on production
> systems as ext4, and having support for them right from the start would
> significantly help with adoption of richacls.

That's one reason.  The other is that actually wiring it up for more
than a single consumer shows its actually reasonable generic.  I don't
want to end up with a situration like Posix ACLs again where different
file systems using different on disk formats again.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
1234