linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hfsplus: drop ACL support
@ 2018-07-14 19:06 Ernesto A. Fernández
  2018-07-14 19:11 ` Viacheslav Dubeyko
  2018-07-14 19:29 ` Viacheslav Dubeyko
  0 siblings, 2 replies; 11+ messages in thread
From: Ernesto A. Fernández @ 2018-07-14 19:06 UTC (permalink / raw)
  To: linux-fsdevel; +Cc: Andrew Morton, Christoph Hellwig, Jan Kara

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.

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_ACCESS);
-		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,
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] hfsplus: drop ACL support
  2018-07-14 19:06 [PATCH] hfsplus: drop ACL support Ernesto A. Fernández
@ 2018-07-14 19:11 ` Viacheslav Dubeyko
  2018-07-17  0:07   ` Andrew Morton
  2018-07-14 19:29 ` Viacheslav Dubeyko
  1 sibling, 1 reply; 11+ messages in thread
From: Viacheslav Dubeyko @ 2018-07-14 19:11 UTC (permalink / raw)
  To: Ernesto A. Fernández, linux-fsdevel
  Cc: Andrew Morton, Christoph Hellwig, Jan Kara

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,

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hfsplus: drop ACL support
  2018-07-14 19:06 [PATCH] hfsplus: drop ACL support Ernesto A. Fernández
  2018-07-14 19:11 ` Viacheslav Dubeyko
@ 2018-07-14 19:29 ` Viacheslav Dubeyko
  1 sibling, 0 replies; 11+ messages in thread
From: Viacheslav Dubeyko @ 2018-07-14 19:29 UTC (permalink / raw)
  To: Ernesto A. Fernández, linux-fsdevel
  Cc: Andrew Morton, Christoph Hellwig, Jan Kara

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.
> 

By the way, we have richacl in the kernel now. You can implement
richacl support in HFS+. It will be the more natural way to support
NFS-like ACLs.

Thanks,
Vyacheslav Dubeyko.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hfsplus: drop ACL support
  2018-07-14 19:11 ` Viacheslav Dubeyko
@ 2018-07-17  0:07   ` Andrew Morton
  2018-07-17  0:21     ` Ernesto A. Fernández
  2018-07-17 13:18     ` Christoph Hellwig
  0 siblings, 2 replies; 11+ messages in thread
From: Andrew Morton @ 2018-07-17  0:07 UTC (permalink / raw)
  To: Viacheslav Dubeyko
  Cc: Ernesto A. Fernández, linux-fsdevel, Christoph Hellwig, Jan Kara

On Sat, 14 Jul 2018 12:11:03 -0700 Viacheslav Dubeyko <slava@dubeyko.com> wrote:

> 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.

Yes, we could make it depend on BROKEN and hope that someone comes
along and fixes it - there's little cost in carrying such a thing.

Or maybe it can never work for some reason and should indeed be
dropped, I don't know.  I was interested in understanding the reasoning
behind hch's ack, but I can't find it on linux-fs-devel.  Help?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hfsplus: drop ACL support
  2018-07-17  0:07   ` Andrew Morton
@ 2018-07-17  0:21     ` Ernesto A. Fernández
  2018-07-27 17:48       ` Ernesto A. Fernández
  2018-07-17 13:18     ` Christoph Hellwig
  1 sibling, 1 reply; 11+ messages in thread
From: Ernesto A. Fernández @ 2018-07-17  0:21 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Viacheslav Dubeyko, linux-fsdevel, Christoph Hellwig, Jan Kara

On Mon, Jul 16, 2018 at 05:07:48PM -0700, Andrew Morton wrote:
> On Sat, 14 Jul 2018 12:11:03 -0700 Viacheslav Dubeyko <slava@dubeyko.com> wrote:
> 
> > 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.
> 
> Yes, we could make it depend on BROKEN and hope that someone comes
> along and fixes it - there's little cost in carrying such a thing.
> 
> Or maybe it can never work for some reason and should indeed be
> dropped, I don't know.  I was interested in understanding the reasoning
> behind hch's ack, but I can't find it on linux-fs-devel.  Help?
> 

Sure, it's an old discussion:

https://marc.info/?l=linux-fsdevel&m=150813973409081&w=2

It's actually pretty easy to get it to work and I first sent patches to
do just that, but I was talked out of it.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hfsplus: drop ACL support
  2018-07-17  0:07   ` Andrew Morton
  2018-07-17  0:21     ` Ernesto A. Fernández
@ 2018-07-17 13:18     ` Christoph Hellwig
  2018-07-17 18:29       ` Viacheslav Dubeyko
  1 sibling, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2018-07-17 13:18 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Viacheslav Dubeyko, Ernesto A. Fernández, linux-fsdevel,
	Christoph Hellwig, Jan Kara

On Mon, Jul 16, 2018 at 05:07:48PM -0700, Andrew Morton wrote:
> On Sat, 14 Jul 2018 12:11:03 -0700 Viacheslav Dubeyko <slava@dubeyko.com> wrote:
> 
> > 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.
> 
> Yes, we could make it depend on BROKEN and hope that someone comes
> along and fixes it - there's little cost in carrying such a thing.
> 
> Or maybe it can never work for some reason and should indeed be
> dropped, I don't know.  I was interested in understanding the reasoning
> behind hch's ack, but I can't find it on linux-fs-devel.  Help?

The issue is that hfsplus supports it's own NFSv4-like ACLs, which
are nothing like the Posix ACLs supported by the Linux driver.  Or
in other words: they were always a Linux-only incompatible extension
that should never have been merged.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hfsplus: drop ACL support
  2018-07-17 13:18     ` Christoph Hellwig
@ 2018-07-17 18:29       ` Viacheslav Dubeyko
  2018-07-17 18:34         ` Eric Biggers
  0 siblings, 1 reply; 11+ messages in thread
From: Viacheslav Dubeyko @ 2018-07-17 18:29 UTC (permalink / raw)
  To: Christoph Hellwig, Andrew Morton
  Cc: Ernesto A. Fernández, linux-fsdevel, Jan Kara

On Tue, 2018-07-17 at 15:18 +0200, Christoph Hellwig wrote:
> On Mon, Jul 16, 2018 at 05:07:48PM -0700, Andrew Morton wrote:
> > 
> > On Sat, 14 Jul 2018 12:11:03 -0700 Viacheslav Dubeyko <slava@dubeyk
> > o.com> wrote:
> > 
> > > 
> > > 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.
> > Yes, we could make it depend on BROKEN and hope that someone comes
> > along and fixes it - there's little cost in carrying such a thing.
> > 
> > Or maybe it can never work for some reason and should indeed be
> > dropped, I don't know.  I was interested in understanding the
> > reasoning
> > behind hch's ack, but I can't find it on linux-fs-devel.  Help?
> The issue is that hfsplus supports it's own NFSv4-like ACLs, which
> are nothing like the Posix ACLs supported by the Linux driver.  Or
> in other words: they were always a Linux-only incompatible extension
> that should never have been merged.


The richacls subsystem was not in the kernel at the time of ACL support
implementation. So, the idea of Linux-like ACLs in HFS+ was not to be
compatible with MAC OS X. The goal was to provide the opportunity to
use the ACLs only in Linux if anybody likes this. The MAC OS X will
ignore such xattrs.

As far as I can see, the ACL subsystem in HFS+ stops to work after
Christoph's reworking the ACL subsystem in the Linux. So, we have
richacls subsystem in the Linux kernel now. I believe that it makes
sense to implement the richacls support in HFS+ at first. Then we can
discuss to drop or not to drop the current ACL implementation in HFS+. 

Also, there is CONFIG_HFSPLUS_FS_POSIX_ACL option that provides
opportunity to exclude the ACL support in HFS+ from the compilation.

Thanks,
Vyacheslav Dubeyko.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hfsplus: drop ACL support
  2018-07-17 18:29       ` Viacheslav Dubeyko
@ 2018-07-17 18:34         ` Eric Biggers
  0 siblings, 0 replies; 11+ messages in thread
From: Eric Biggers @ 2018-07-17 18:34 UTC (permalink / raw)
  To: Viacheslav Dubeyko
  Cc: Christoph Hellwig, Andrew Morton, Ernesto A. Fernández,
	linux-fsdevel, Jan Kara

On Tue, Jul 17, 2018 at 11:29:17AM -0700, Viacheslav Dubeyko wrote:
> On Tue, 2018-07-17 at 15:18 +0200, Christoph Hellwig wrote:
> > On Mon, Jul 16, 2018 at 05:07:48PM -0700, Andrew Morton wrote:
> > > 
> > > On Sat, 14 Jul 2018 12:11:03 -0700 Viacheslav Dubeyko <slava@dubeyk
> > > o.com> wrote:
> > > 
> > > > 
> > > > 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.
> > > Yes, we could make it depend on BROKEN and hope that someone comes
> > > along and fixes it - there's little cost in carrying such a thing.
> > > 
> > > Or maybe it can never work for some reason and should indeed be
> > > dropped, I don't know.��I was interested in understanding the
> > > reasoning
> > > behind hch's ack, but I can't find it on linux-fs-devel.��Help?
> > The issue is that hfsplus supports it's own NFSv4-like ACLs, which
> > are nothing like the Posix ACLs supported by the Linux driver.��Or
> > in other words: they were always a Linux-only incompatible extension
> > that should never have been merged.
> 
> 
> The richacls subsystem was not in the kernel at the time of ACL support
> implementation. So, the idea of Linux-like ACLs in HFS+ was not to be
> compatible with MAC OS X. The goal was to provide the opportunity to
> use the ACLs only in Linux if anybody likes this. The MAC OS X will
> ignore such xattrs.
> 
> As far as I can see, the ACL subsystem in HFS+ stops to work after
> Christoph's reworking the ACL subsystem in the Linux. So, we have
> richacls subsystem in the Linux kernel now. I believe that it makes
> sense to implement the richacls support in HFS+ at first. Then we can
> discuss to drop or not to drop the current ACL implementation in HFS+.�

Richacls are *not* in the kernel yet.

- Eric

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hfsplus: drop ACL support
  2018-07-17  0:21     ` Ernesto A. Fernández
@ 2018-07-27 17:48       ` Ernesto A. Fernández
  0 siblings, 0 replies; 11+ messages in thread
From: Ernesto A. Fernández @ 2018-07-27 17:48 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Viacheslav Dubeyko, linux-fsdevel, Christoph Hellwig, Jan Kara

On Mon, Jul 16, 2018 at 09:21:43PM -0300, Ernesto A. Fernández wrote:
> On Mon, Jul 16, 2018 at 05:07:48PM -0700, Andrew Morton wrote:
> > On Sat, 14 Jul 2018 12:11:03 -0700 Viacheslav Dubeyko <slava@dubeyko.com> wrote:
> > 
> > > 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.
> > 
> > Yes, we could make it depend on BROKEN and hope that someone comes
> > along and fixes it - there's little cost in carrying such a thing.
> > 
> > Or maybe it can never work for some reason and should indeed be
> > dropped, I don't know.  I was interested in understanding the reasoning
> > behind hch's ack, but I can't find it on linux-fs-devel.  Help?
> > 
> 
> Sure, it's an old discussion:
> 
> https://marc.info/?l=linux-fsdevel&m=150813973409081&w=2
> 
> It's actually pretty easy to get it to work and I first sent patches to
> do just that, but I was talked out of it.

So what do you say? Will you pick this up?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hfsplus: drop ACL support
  2017-12-03  1:41 Ernesto A. Fernández
@ 2017-12-03 20:59 ` Viacheslav Dubeyko
  0 siblings, 0 replies; 11+ messages in thread
From: Viacheslav Dubeyko @ 2017-12-03 20:59 UTC (permalink / raw)
  To: Ernesto A. Fernández, Alexander Viro; +Cc: linux-fsdevel

On Sat, 2017-12-02 at 22:41 -0300, Ernesto A. Fernández wrote:
> The HFS+ Access Control Lists have not been working at all for the
> past
> four years, and nobody seems to have noticed. Besides, they are not
> compatible with MacOS. Drop the feature entirely.
> 

If we have some bugs then it makes sense to fix the bugs. The simple
deletion is the wrong way. Moreover, it needs to modify this support
for richacls level because HFS+ ACLs model is based on NFSv4 ACLs.

Thanks,
Vyacheslav Dubeyko.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] hfsplus: drop ACL support
@ 2017-12-03  1:41 Ernesto A. Fernández
  2017-12-03 20:59 ` Viacheslav Dubeyko
  0 siblings, 1 reply; 11+ messages in thread
From: Ernesto A. Fernández @ 2017-12-03  1:41 UTC (permalink / raw)
  To: Alexander Viro; +Cc: linux-fsdevel, Ernesto A. Fernández

The HFS+ Access Control Lists have not been working at all for the past
four years, and nobody seems to have noticed. Besides, they are not
compatible with MacOS. Drop the feature entirely.

Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
Acked-by: Christoph Hellwig <hch@lst.de>
---
 fs/hfsplus/Kconfig          |  18 ------
 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(+), 235 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 24bc20f..a633718 100644
--- a/fs/hfsplus/Kconfig
+++ b/fs/hfsplus/Kconfig
@@ -11,21 +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.
-
-	  To learn more about Access Control Lists, visit the POSIX ACLs for
-	  Linux website <http://acl.bestbits.at/>.
-
-	  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 f6a5654..9ed20e6 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 488c2b7..0000000
--- 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 e8120a2..92a28da 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)
@@ -456,7 +455,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) {
@@ -497,7 +496,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) {
@@ -568,10 +567,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 a015044..36ca6e17 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 190c60e..72801ca 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 066114d..0000000
--- 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_ACCESS);
-		posix_acl_release(acl);
-	}
-	return err;
-}
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 1d458b7..d5e6a71 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 e538b75..d5403b4 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 a4e611d..d14e362 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 f5550b0..cfbe6a3 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,
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-07-27 19:11 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-14 19:06 [PATCH] hfsplus: drop ACL support Ernesto A. Fernández
2018-07-14 19:11 ` Viacheslav Dubeyko
2018-07-17  0:07   ` Andrew Morton
2018-07-17  0:21     ` Ernesto A. Fernández
2018-07-27 17:48       ` Ernesto A. Fernández
2018-07-17 13:18     ` Christoph Hellwig
2018-07-17 18:29       ` Viacheslav Dubeyko
2018-07-17 18:34         ` Eric Biggers
2018-07-14 19:29 ` Viacheslav Dubeyko
  -- strict thread matches above, loose matches on Subject: below --
2017-12-03  1:41 Ernesto A. Fernández
2017-12-03 20:59 ` Viacheslav Dubeyko

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