Re: [PATCH] hfsplus: drop ACL support

[Viacheslav Dubeyko <slava@dubeyko.com>]

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 <ernesto.mnd.fernandez@gmail.com>
> Acked-by: Christoph Hellwig <hch@lst.de>
> ---
>  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 <slava@dubeyko.com>
> - *
> - * Handler for Posix Access Control Lists (ACLs) support.
> - */
> -
> -#include <linux/posix_acl_xattr.h>
> -
> -#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 <slava@dubeyko.com>
> - *
> - * 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 <linux/posix_acl_xattr.h>
>  #include <linux/nls.h>
>  #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,