From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f196.google.com ([209.85.192.196]:36481 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726960AbeGNTbG (ORCPT ); Sat, 14 Jul 2018 15:31:06 -0400 Received: by mail-pf0-f196.google.com with SMTP id d14-v6so595044pfo.3 for ; Sat, 14 Jul 2018 12:11:05 -0700 (PDT) Message-ID: <1531595463.5345.1.camel@dubeyko.com> Subject: Re: [PATCH] hfsplus: drop ACL support From: Viacheslav Dubeyko To: "Ernesto A." =?ISO-8859-1?Q?Fern=E1ndez?= , linux-fsdevel@vger.kernel.org Cc: Andrew Morton , Christoph Hellwig , Jan Kara Date: Sat, 14 Jul 2018 12:11:03 -0700 In-Reply-To: <20180714190608.wtnmmtjqeyladkut@eaf> References: <20180714190608.wtnmmtjqeyladkut@eaf> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Sat, 2018-07-14 at 16:06 -0300, Ernesto A. Fernández wrote: > The HFS+ Access Control Lists have not worked at all for the past > five > years, and nobody seems to have noticed. Besides, POSIX draft ACLs > are > not compatible with MacOS. Drop the feature entirely. > Bugs need to be fixed but not to drop. Otherwise, it needs to drop the whole HFS+ support from the kernel. Thanks, Vyacheslav Dubeyko. > Signed-off-by: Ernesto A. Fernández > Acked-by: Christoph Hellwig > --- >  fs/hfsplus/Kconfig          |  15 ----- >  fs/hfsplus/Makefile         |   2 - >  fs/hfsplus/acl.h            |  28 --------- >  fs/hfsplus/dir.c            |   9 +-- >  fs/hfsplus/hfsplus_fs.h     |   1 - >  fs/hfsplus/inode.c          |  11 ---- >  fs/hfsplus/posix_acl.c      | 144 ---------------------------------- > ---------- >  fs/hfsplus/super.c          |   4 +- >  fs/hfsplus/xattr.c          |   6 -- >  fs/hfsplus/xattr.h          |   3 - >  fs/hfsplus/xattr_security.c |  13 ---- >  11 files changed, 4 insertions(+), 232 deletions(-) >  delete mode 100644 fs/hfsplus/acl.h >  delete mode 100644 fs/hfsplus/posix_acl.c > > diff --git a/fs/hfsplus/Kconfig b/fs/hfsplus/Kconfig > index 7cc8b4acf66a..a63371815aab 100644 > --- a/fs/hfsplus/Kconfig > +++ b/fs/hfsplus/Kconfig > @@ -11,18 +11,3 @@ config HFSPLUS_FS >     MacOS 8. It includes all Mac specific filesystem data such > as >     data forks and creator codes, but it also has several UNIX >     style features such as file ownership and permissions. > - > -config HFSPLUS_FS_POSIX_ACL > - bool "HFS+ POSIX Access Control Lists" > - depends on HFSPLUS_FS > - select FS_POSIX_ACL > - help > -   POSIX Access Control Lists (ACLs) support permissions for > users and > -   groups beyond the owner/group/world scheme. > - > -   It needs to understand that POSIX ACLs are treated only > under > -   Linux. POSIX ACLs doesn't mean something under Mac OS X. > -   Mac OS X beginning with version 10.4 ("Tiger") support > NFSv4 ACLs, > -   which are part of the NFSv4 standard. > - > -   If you don't know what Access Control Lists are, say N > diff --git a/fs/hfsplus/Makefile b/fs/hfsplus/Makefile > index f6a56542f8d7..9ed20e64b983 100644 > --- a/fs/hfsplus/Makefile > +++ b/fs/hfsplus/Makefile > @@ -8,5 +8,3 @@ obj-$(CONFIG_HFSPLUS_FS) += hfsplus.o >  hfsplus-objs := super.o options.o inode.o ioctl.o extents.o > catalog.o dir.o btree.o \ >   bnode.o brec.o bfind.o tables.o unicode.o wrapper.o > bitmap.o part_tbl.o \ >   attributes.o xattr.o xattr_user.o xattr_security.o > xattr_trusted.o > - > -hfsplus-$(CONFIG_HFSPLUS_FS_POSIX_ACL) += posix_acl.o > diff --git a/fs/hfsplus/acl.h b/fs/hfsplus/acl.h > deleted file mode 100644 > index 488c2b75cf41..000000000000 > --- a/fs/hfsplus/acl.h > +++ /dev/null > @@ -1,28 +0,0 @@ > -/* SPDX-License-Identifier: GPL-2.0 */ > -/* > - * linux/fs/hfsplus/acl.h > - * > - * Vyacheslav Dubeyko > - * > - * Handler for Posix Access Control Lists (ACLs) support. > - */ > - > -#include > - > -#ifdef CONFIG_HFSPLUS_FS_POSIX_ACL > - > -/* posix_acl.c */ > -struct posix_acl *hfsplus_get_posix_acl(struct inode *inode, int > type); > -int hfsplus_set_posix_acl(struct inode *inode, struct posix_acl > *acl, > - int type); > -extern int hfsplus_init_posix_acl(struct inode *, struct inode *); > - > -#else  /* CONFIG_HFSPLUS_FS_POSIX_ACL */ > -#define hfsplus_get_posix_acl NULL > -#define hfsplus_set_posix_acl NULL > - > -static inline int hfsplus_init_posix_acl(struct inode *inode, struct > inode *dir) > -{ > - return 0; > -} > -#endif  /* CONFIG_HFSPLUS_FS_POSIX_ACL */ > diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c > index b5254378f011..c5a70f83dbe7 100644 > --- a/fs/hfsplus/dir.c > +++ b/fs/hfsplus/dir.c > @@ -18,7 +18,6 @@ >  #include "hfsplus_fs.h" >  #include "hfsplus_raw.h" >  #include "xattr.h" > -#include "acl.h" >   >  static inline void hfsplus_instantiate(struct dentry *dentry, >          struct inode *inode, u32 > cnid) > @@ -455,7 +454,7 @@ static int hfsplus_symlink(struct inode *dir, > struct dentry *dentry, >   if (res) >   goto out_err; >   > - res = hfsplus_init_inode_security(inode, dir, &dentry- > >d_name); > + res = hfsplus_init_security(inode, dir, &dentry->d_name); >   if (res == -EOPNOTSUPP) >   res = 0; /* Operation is not supported. */ >   else if (res) { > @@ -496,7 +495,7 @@ static int hfsplus_mknod(struct inode *dir, > struct dentry *dentry, >   if (res) >   goto failed_mknod; >   > - res = hfsplus_init_inode_security(inode, dir, &dentry- > >d_name); > + res = hfsplus_init_security(inode, dir, &dentry->d_name); >   if (res == -EOPNOTSUPP) >   res = 0; /* Operation is not supported. */ >   else if (res) { > @@ -567,10 +566,6 @@ const struct inode_operations > hfsplus_dir_inode_operations = { >   .mknod = hfsplus_mknod, >   .rename = hfsplus_rename, >   .listxattr = hfsplus_listxattr, > -#ifdef CONFIG_HFSPLUS_FS_POSIX_ACL > - .get_acl = hfsplus_get_posix_acl, > - .set_acl = hfsplus_set_posix_acl, > -#endif >  }; >   >  const struct file_operations hfsplus_dir_operations = { > diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h > index d9255abafb81..8e039435958a 100644 > --- a/fs/hfsplus/hfsplus_fs.h > +++ b/fs/hfsplus/hfsplus_fs.h > @@ -31,7 +31,6 @@ >  #define DBG_EXTENT 0x00000020 >  #define DBG_BITMAP 0x00000040 >  #define DBG_ATTR_MOD 0x00000080 > -#define DBG_ACL_MOD 0x00000100 >   >  #if 0 >  #define DBG_MASK (DBG_EXTENT|DBG_INODE|DBG_BNODE_MOD) > diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c > index c824f702feec..8e9427a42b81 100644 > --- a/fs/hfsplus/inode.c > +++ b/fs/hfsplus/inode.c > @@ -21,7 +21,6 @@ >  #include "hfsplus_fs.h" >  #include "hfsplus_raw.h" >  #include "xattr.h" > -#include "acl.h" >   >  static int hfsplus_readpage(struct file *file, struct page *page) >  { > @@ -267,12 +266,6 @@ static int hfsplus_setattr(struct dentry > *dentry, struct iattr *attr) >   setattr_copy(inode, attr); >   mark_inode_dirty(inode); >   > - if (attr->ia_valid & ATTR_MODE) { > - error = posix_acl_chmod(inode, inode->i_mode); > - if (unlikely(error)) > - return error; > - } > - >   return 0; >  } >   > @@ -336,10 +329,6 @@ int hfsplus_file_fsync(struct file *file, loff_t > start, loff_t end, >  static const struct inode_operations hfsplus_file_inode_operations = > { >   .setattr = hfsplus_setattr, >   .listxattr = hfsplus_listxattr, > -#ifdef CONFIG_HFSPLUS_FS_POSIX_ACL > - .get_acl = hfsplus_get_posix_acl, > - .set_acl = hfsplus_set_posix_acl, > -#endif >  }; >   >  static const struct file_operations hfsplus_file_operations = { > diff --git a/fs/hfsplus/posix_acl.c b/fs/hfsplus/posix_acl.c > deleted file mode 100644 > index 066114dcc3a2..000000000000 > --- a/fs/hfsplus/posix_acl.c > +++ /dev/null > @@ -1,144 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0 > -/* > - * linux/fs/hfsplus/posix_acl.c > - * > - * Vyacheslav Dubeyko > - * > - * Handler for Posix Access Control Lists (ACLs) support. > - */ > - > -#include "hfsplus_fs.h" > -#include "xattr.h" > -#include "acl.h" > - > -struct posix_acl *hfsplus_get_posix_acl(struct inode *inode, int > type) > -{ > - struct posix_acl *acl; > - char *xattr_name; > - char *value = NULL; > - ssize_t size; > - > - hfs_dbg(ACL_MOD, "[%s]: ino %lu\n", __func__, inode->i_ino); > - > - switch (type) { > - case ACL_TYPE_ACCESS: > - xattr_name = XATTR_NAME_POSIX_ACL_ACCESS; > - break; > - case ACL_TYPE_DEFAULT: > - xattr_name = XATTR_NAME_POSIX_ACL_DEFAULT; > - break; > - default: > - return ERR_PTR(-EINVAL); > - } > - > - size = __hfsplus_getxattr(inode, xattr_name, NULL, 0); > - > - if (size > 0) { > - value = (char *)hfsplus_alloc_attr_entry(); > - if (unlikely(!value)) > - return ERR_PTR(-ENOMEM); > - size = __hfsplus_getxattr(inode, xattr_name, value, > size); > - } > - > - if (size > 0) > - acl = posix_acl_from_xattr(&init_user_ns, value, > size); > - else if (size == -ENODATA) > - acl = NULL; > - else > - acl = ERR_PTR(size); > - > - hfsplus_destroy_attr_entry((hfsplus_attr_entry *)value); > - > - return acl; > -} > - > -static int __hfsplus_set_posix_acl(struct inode *inode, struct > posix_acl *acl, > -    int type) > -{ > - int err; > - char *xattr_name; > - size_t size = 0; > - char *value = NULL; > - > - hfs_dbg(ACL_MOD, "[%s]: ino %lu\n", __func__, inode->i_ino); > - > - switch (type) { > - case ACL_TYPE_ACCESS: > - xattr_name = XATTR_NAME_POSIX_ACL_ACCESS; > - break; > - > - case ACL_TYPE_DEFAULT: > - xattr_name = XATTR_NAME_POSIX_ACL_DEFAULT; > - if (!S_ISDIR(inode->i_mode)) > - return acl ? -EACCES : 0; > - break; > - > - default: > - return -EINVAL; > - } > - > - if (acl) { > - size = posix_acl_xattr_size(acl->a_count); > - if (unlikely(size > HFSPLUS_MAX_INLINE_DATA_SIZE)) > - return -ENOMEM; > - value = (char *)hfsplus_alloc_attr_entry(); > - if (unlikely(!value)) > - return -ENOMEM; > - err = posix_acl_to_xattr(&init_user_ns, acl, value, > size); > - if (unlikely(err < 0)) > - goto end_set_acl; > - } > - > - err = __hfsplus_setxattr(inode, xattr_name, value, size, 0); > - > -end_set_acl: > - hfsplus_destroy_attr_entry((hfsplus_attr_entry *)value); > - > - if (!err) > - set_cached_acl(inode, type, acl); > - > - return err; > -} > - > -int hfsplus_set_posix_acl(struct inode *inode, struct posix_acl > *acl, int type) > -{ > - int err; > - > - if (type == ACL_TYPE_ACCESS && acl) { > - err = posix_acl_update_mode(inode, &inode->i_mode, > &acl); > - if (err) > - return err; > - } > - return __hfsplus_set_posix_acl(inode, acl, type); > -} > - > -int hfsplus_init_posix_acl(struct inode *inode, struct inode *dir) > -{ > - int err = 0; > - struct posix_acl *default_acl, *acl; > - > - hfs_dbg(ACL_MOD, > - "[%s]: ino %lu, dir->ino %lu\n", > - __func__, inode->i_ino, dir->i_ino); > - > - if (S_ISLNK(inode->i_mode)) > - return 0; > - > - err = posix_acl_create(dir, &inode->i_mode, &default_acl, > &acl); > - if (err) > - return err; > - > - if (default_acl) { > - err = __hfsplus_set_posix_acl(inode, default_acl, > -       ACL_TYPE_DEFAULT); > - posix_acl_release(default_acl); > - } > - > - if (acl) { > - if (!err) > - err = __hfsplus_set_posix_acl(inode, acl, > -       ACL_TYPE_ACCES > S); > - posix_acl_release(acl); > - } > - return err; > -} > diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c > index a6c0f54c48c3..0e88d1a270f0 100644 > --- a/fs/hfsplus/super.c > +++ b/fs/hfsplus/super.c > @@ -562,8 +562,8 @@ static int hfsplus_fill_super(struct super_block > *sb, void *data, int silent) >   goto out_put_hidden_dir; >   } >   > - err = hfsplus_init_inode_security(sbi- > >hidden_dir, > - root > , &str); > + err = hfsplus_init_security(sbi->hidden_dir, > + root, &str); >   if (err == -EOPNOTSUPP) >   err = 0; /* Operation is not > supported. */ >   else if (err) { > diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c > index e538b758c448..d5403b4004c9 100644 > --- a/fs/hfsplus/xattr.c > +++ b/fs/hfsplus/xattr.c > @@ -8,10 +8,8 @@ >   */ >   >  #include "hfsplus_fs.h" > -#include >  #include >  #include "xattr.h" > -#include "acl.h" >   >  static int hfsplus_removexattr(struct inode *inode, const char > *name); >   > @@ -19,10 +17,6 @@ const struct xattr_handler > *hfsplus_xattr_handlers[] = { >   &hfsplus_xattr_osx_handler, >   &hfsplus_xattr_user_handler, >   &hfsplus_xattr_trusted_handler, > -#ifdef CONFIG_HFSPLUS_FS_POSIX_ACL > - &posix_acl_access_xattr_handler, > - &posix_acl_default_xattr_handler, > -#endif >   &hfsplus_xattr_security_handler, >   NULL >  }; > diff --git a/fs/hfsplus/xattr.h b/fs/hfsplus/xattr.h > index a4e611d69710..d14e362b3eba 100644 > --- a/fs/hfsplus/xattr.h > +++ b/fs/hfsplus/xattr.h > @@ -38,7 +38,4 @@ ssize_t hfsplus_listxattr(struct dentry *dentry, > char *buffer, size_t size); >  int hfsplus_init_security(struct inode *inode, struct inode *dir, >   const struct qstr *qstr); >   > -int hfsplus_init_inode_security(struct inode *inode, struct inode > *dir, > - const struct qstr *qstr); > - >  #endif > diff --git a/fs/hfsplus/xattr_security.c > b/fs/hfsplus/xattr_security.c > index f5550b006e88..cfbe6a3bfb1e 100644 > --- a/fs/hfsplus/xattr_security.c > +++ b/fs/hfsplus/xattr_security.c > @@ -12,7 +12,6 @@ >   >  #include "hfsplus_fs.h" >  #include "xattr.h" > -#include "acl.h" >   >  static int hfsplus_security_getxattr(const struct xattr_handler > *handler, >        struct dentry *unused, struct > inode *inode, > @@ -72,18 +71,6 @@ int hfsplus_init_security(struct inode *inode, > struct inode *dir, >   &hfsplus_initxattrs, NULL); >  } >   > -int hfsplus_init_inode_security(struct inode *inode, > - struct inode *dir, > - const struct qstr > *qstr) > -{ > - int err; > - > - err = hfsplus_init_posix_acl(inode, dir); > - if (!err) > - err = hfsplus_init_security(inode, dir, qstr); > - return err; > -} > - >  const struct xattr_handler hfsplus_xattr_security_handler = { >   .prefix = XATTR_SECURITY_PREFIX, >   .get = hfsplus_security_getxattr,