[PATCH 1/2] hfsplus: preserve i_mode if __hfsplus_set_posix_acl() fails

[PATCH 1/2] hfsplus: preserve i_mode if __hfsplus_set_posix_acl() fails

[Ernesto A. Fernández]

When changing a file's acl mask, hfsplus_set_posix_acl() will first set
the group bits of i_mode to the value of the mask, and only then set the
actual extended attribute representing the new acl.

If the second part fails (due to lack of space, for example) and the
file had no acl attribute to begin with, the system will from now on
assume that the mask permission bits are actual group permission bits,
potentially granting access to the wrong users.

Prevent this by only changing the inode mode after the acl has been set.

Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
---
The same issue affects several filesystems; some of them have already
applied patches, see for example:

  fe26569 ext2: preserve i_mode if ext2_set_acl() fails

In order to test this I had to add a mount option to enable acls. That
patch is sent next.

 fs/hfsplus/posix_acl.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/hfsplus/posix_acl.c b/fs/hfsplus/posix_acl.c
index 6bb5d7c..24a1cdf 100644
--- a/fs/hfsplus/posix_acl.c
+++ b/fs/hfsplus/posix_acl.c
@@ -102,13 +102,19 @@ static int __hfsplus_set_posix_acl(struct inode *inode, struct posix_acl *acl,
 int hfsplus_set_posix_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
 	int err;
+	int update_mode = 0;
+	umode_t mode = inode->i_mode;
 
 	if (type == ACL_TYPE_ACCESS && acl) {
-		err = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+		err = posix_acl_update_mode(inode, &mode, &acl);
 		if (err)
 			return err;
+		update_mode = 1;
 	}
-	return __hfsplus_set_posix_acl(inode, acl, type);
+	err = __hfsplus_set_posix_acl(inode, acl, type);
+	if (!err && update_mode)
+		inode->i_mode = mode;
+	return err;
 }
 
 int hfsplus_init_posix_acl(struct inode *inode, struct inode *dir)
-- 
2.1.4

[PATCH 2/2] hfsplus: add mount option to enable ACL support

[Ernesto A. Fernández]

The hfsplus module already has the code to support access control
lists, but there is no corresponding mount option. Add it and keep it
disabled by default.

Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
---
 Documentation/filesystems/hfsplus.txt |  4 ++++
 fs/hfsplus/hfsplus_fs.h               |  1 +
 fs/hfsplus/options.c                  | 15 ++++++++++++++-
 fs/hfsplus/super.c                    |  1 +
 4 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/Documentation/filesystems/hfsplus.txt b/Documentation/filesystems/hfsplus.txt
index 59f7569..83670ac 100644
--- a/Documentation/filesystems/hfsplus.txt
+++ b/Documentation/filesystems/hfsplus.txt
@@ -43,6 +43,10 @@ When mounting an HFSPlus filesystem, the following options are accepted:
   nodecompose
 	Do not decompose file name characters.
 
+  acl
+	Enable POSIX Access Control Lists support, disabled by default.
+	Requires CONFIG_HFSPLUS_FS_POSIX_ACL set in the kernel configuration.
+
   force
 	Used to force write access to volumes that are marked as journalled
 	or locked.  Use at your own risk.
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index a3f03b2..8bc78c1 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -153,6 +153,7 @@ struct hfsplus_sb_info {
 	struct inode *alloc_file;
 	struct inode *hidden_dir;
 	struct nls_table *nls;
+	struct super_block *sb;
 
 	/* Runtime variables */
 	u32 blockoffset;
diff --git a/fs/hfsplus/options.c b/fs/hfsplus/options.c
index bb806e5..6f8b5a9 100644
--- a/fs/hfsplus/options.c
+++ b/fs/hfsplus/options.c
@@ -24,7 +24,7 @@ enum {
 	opt_part, opt_session, opt_nls,
 	opt_nodecompose, opt_decompose,
 	opt_barrier, opt_nobarrier,
-	opt_force, opt_err
+	opt_force, opt_acl, opt_err
 };
 
 static const match_table_t tokens = {
@@ -41,6 +41,7 @@ static const match_table_t tokens = {
 	{ opt_barrier, "barrier" },
 	{ opt_nobarrier, "nobarrier" },
 	{ opt_force, "force" },
+	{ opt_acl, "acl" },
 	{ opt_err, NULL }
 };
 
@@ -195,6 +196,14 @@ int hfsplus_parse_options(char *input, struct hfsplus_sb_info *sbi)
 		case opt_force:
 			set_bit(HFSPLUS_SB_FORCE, &sbi->flags);
 			break;
+		case opt_acl:
+#ifdef CONFIG_HFSPLUS_FS_POSIX_ACL
+			sbi->sb->s_flags |= MS_POSIXACL;
+			break;
+#else
+			pr_err("support for ACL not compiled in!");
+			return 0;
+#endif
 		default:
 			return 0;
 		}
@@ -234,5 +243,9 @@ int hfsplus_show_options(struct seq_file *seq, struct dentry *root)
 		seq_puts(seq, ",nodecompose");
 	if (test_bit(HFSPLUS_SB_NOBARRIER, &sbi->flags))
 		seq_puts(seq, ",nobarrier");
+#ifdef CONFIG_HFSPLUS_FS_POSIX_ACL
+	if (sbi->sb->s_flags & MS_POSIXACL)
+		seq_puts(seq, ",acl");
+#endif
 	return 0;
 }
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 67aedf4..258fb86 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -389,6 +389,7 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 		goto out;
 
 	sb->s_fs_info = sbi;
+	sbi->sb = sb;
 	mutex_init(&sbi->alloc_mutex);
 	mutex_init(&sbi->vh_mutex);
 	spin_lock_init(&sbi->work_lock);
-- 
2.1.4

Re: [PATCH 1/2] hfsplus: preserve i_mode if __hfsplus_set_posix_acl() fails

[Jan Kara]

On Thu 27-07-17 00:10:20, Ernesto A. Fern�ndez wrote:
> When changing a file's acl mask, hfsplus_set_posix_acl() will first set
> the group bits of i_mode to the value of the mask, and only then set the
> actual extended attribute representing the new acl.
> 
> If the second part fails (due to lack of space, for example) and the
> file had no acl attribute to begin with, the system will from now on
> assume that the mask permission bits are actual group permission bits,
> potentially granting access to the wrong users.
> 
> Prevent this by only changing the inode mode after the acl has been set.
> 
> Signed-off-by: Ernesto A. Fern�ndez <ernesto.mnd.fernandez@gmail.com>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
> The same issue affects several filesystems; some of them have already
> applied patches, see for example:
> 
>   fe26569 ext2: preserve i_mode if ext2_set_acl() fails
> 
> In order to test this I had to add a mount option to enable acls. That
> patch is sent next.
> 
>  fs/hfsplus/posix_acl.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/hfsplus/posix_acl.c b/fs/hfsplus/posix_acl.c
> index 6bb5d7c..24a1cdf 100644
> --- a/fs/hfsplus/posix_acl.c
> +++ b/fs/hfsplus/posix_acl.c
> @@ -102,13 +102,19 @@ static int __hfsplus_set_posix_acl(struct inode *inode, struct posix_acl *acl,
>  int hfsplus_set_posix_acl(struct inode *inode, struct posix_acl *acl, int type)
>  {
>  	int err;
> +	int update_mode = 0;
> +	umode_t mode = inode->i_mode;
>  
>  	if (type == ACL_TYPE_ACCESS && acl) {
> -		err = posix_acl_update_mode(inode, &inode->i_mode, &acl);
> +		err = posix_acl_update_mode(inode, &mode, &acl);
>  		if (err)
>  			return err;
> +		update_mode = 1;
>  	}
> -	return __hfsplus_set_posix_acl(inode, acl, type);
> +	err = __hfsplus_set_posix_acl(inode, acl, type);
> +	if (!err && update_mode)
> +		inode->i_mode = mode;
> +	return err;
>  }
>  
>  int hfsplus_init_posix_acl(struct inode *inode, struct inode *dir)
> -- 
> 2.1.4
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/2] hfsplus: add mount option to enable ACL support

[Jan Kara]

On Thu 27-07-17 00:13:54, Ernesto A. Fern�ndez wrote:
> The hfsplus module already has the code to support access control
> lists, but there is no corresponding mount option. Add it and keep it
> disabled by default.

Hum, this seems to be a regression caused by Christoph's ACL rework in
2013. Since then HFS+ acls didn't work. So much for how much they are
used... Maybe time to drop the feature when nobody noticed it does not work
for 4 years?

If people think the functionality should be kept, the patch makes sense so
you can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> Signed-off-by: Ernesto A. Fern�ndez <ernesto.mnd.fernandez@gmail.com>
> ---
>  Documentation/filesystems/hfsplus.txt |  4 ++++
>  fs/hfsplus/hfsplus_fs.h               |  1 +
>  fs/hfsplus/options.c                  | 15 ++++++++++++++-
>  fs/hfsplus/super.c                    |  1 +
>  4 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/filesystems/hfsplus.txt b/Documentation/filesystems/hfsplus.txt
> index 59f7569..83670ac 100644
> --- a/Documentation/filesystems/hfsplus.txt
> +++ b/Documentation/filesystems/hfsplus.txt
> @@ -43,6 +43,10 @@ When mounting an HFSPlus filesystem, the following options are accepted:
>    nodecompose
>  	Do not decompose file name characters.
>  
> +  acl
> +	Enable POSIX Access Control Lists support, disabled by default.
> +	Requires CONFIG_HFSPLUS_FS_POSIX_ACL set in the kernel configuration.
> +
>    force
>  	Used to force write access to volumes that are marked as journalled
>  	or locked.  Use at your own risk.
> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> index a3f03b2..8bc78c1 100644
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -153,6 +153,7 @@ struct hfsplus_sb_info {
>  	struct inode *alloc_file;
>  	struct inode *hidden_dir;
>  	struct nls_table *nls;
> +	struct super_block *sb;
>  
>  	/* Runtime variables */
>  	u32 blockoffset;
> diff --git a/fs/hfsplus/options.c b/fs/hfsplus/options.c
> index bb806e5..6f8b5a9 100644
> --- a/fs/hfsplus/options.c
> +++ b/fs/hfsplus/options.c
> @@ -24,7 +24,7 @@ enum {
>  	opt_part, opt_session, opt_nls,
>  	opt_nodecompose, opt_decompose,
>  	opt_barrier, opt_nobarrier,
> -	opt_force, opt_err
> +	opt_force, opt_acl, opt_err
>  };
>  
>  static const match_table_t tokens = {
> @@ -41,6 +41,7 @@ static const match_table_t tokens = {
>  	{ opt_barrier, "barrier" },
>  	{ opt_nobarrier, "nobarrier" },
>  	{ opt_force, "force" },
> +	{ opt_acl, "acl" },
>  	{ opt_err, NULL }
>  };
>  
> @@ -195,6 +196,14 @@ int hfsplus_parse_options(char *input, struct hfsplus_sb_info *sbi)
>  		case opt_force:
>  			set_bit(HFSPLUS_SB_FORCE, &sbi->flags);
>  			break;
> +		case opt_acl:
> +#ifdef CONFIG_HFSPLUS_FS_POSIX_ACL
> +			sbi->sb->s_flags |= MS_POSIXACL;
> +			break;
> +#else
> +			pr_err("support for ACL not compiled in!");
> +			return 0;
> +#endif
>  		default:
>  			return 0;
>  		}
> @@ -234,5 +243,9 @@ int hfsplus_show_options(struct seq_file *seq, struct dentry *root)
>  		seq_puts(seq, ",nodecompose");
>  	if (test_bit(HFSPLUS_SB_NOBARRIER, &sbi->flags))
>  		seq_puts(seq, ",nobarrier");
> +#ifdef CONFIG_HFSPLUS_FS_POSIX_ACL
> +	if (sbi->sb->s_flags & MS_POSIXACL)
> +		seq_puts(seq, ",acl");
> +#endif
>  	return 0;
>  }
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index 67aedf4..258fb86 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -389,6 +389,7 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
>  		goto out;
>  
>  	sb->s_fs_info = sbi;
> +	sbi->sb = sb;
>  	mutex_init(&sbi->alloc_mutex);
>  	mutex_init(&sbi->vh_mutex);
>  	spin_lock_init(&sbi->work_lock);
> -- 
> 2.1.4
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/2] hfsplus: add mount option to enable ACL support

[Christoph Hellwig]

On Thu, Jul 27, 2017 at 11:41:47AM +0200, Jan Kara wrote:
> On Thu 27-07-17 00:13:54, Ernesto A. Fern�ndez wrote:
> > The hfsplus module already has the code to support access control
> > lists, but there is no corresponding mount option. Add it and keep it
> > disabled by default.
> 
> Hum, this seems to be a regression caused by Christoph's ACL rework in
> 2013.

Ooops.

> Since then HFS+ acls didn't work. So much for how much they are
> used... Maybe time to drop the feature when nobody noticed it does not work
> for 4 years?

Especially given that they aren't actually compatible with MacOS
I'm all in favor of dropping the support.

Re: [PATCH 2/2] hfsplus: add mount option to enable ACL support

[Ernesto A. Fernández]

On Thu, Jul 27, 2017 at 11:41:47AM +0200, Jan Kara wrote:
> On Thu 27-07-17 00:13:54, Ernesto A. Fernández wrote:
> > The hfsplus module already has the code to support access control
> > lists, but there is no corresponding mount option. Add it and keep it
> > disabled by default.
> 
> Hum, this seems to be a regression caused by Christoph's ACL rework in
> 2013. Since then HFS+ acls didn't work. So much for how much they are
> used... Maybe time to drop the feature when nobody noticed it does not work
> for 4 years?

I'm not sure the lack of use is a problem of ACLs in particular. I quickly
ran into trouble with extended attributes as well, so my guess is that all
of the module gets very limited use. I'll try and send some patches as
soon as I can.

Re: [PATCH 2/2] hfsplus: add mount option to enable ACL support

[Ernesto A. Fernández]

On Thu, Jul 27, 2017 at 02:50:53AM -0700, Christoph Hellwig wrote:
> On Thu, Jul 27, 2017 at 11:41:47AM +0200, Jan Kara wrote:
> > On Thu 27-07-17 00:13:54, Ernesto A. Fernández wrote:
> > > The hfsplus module already has the code to support access control
> > > lists, but there is no corresponding mount option. Add it and keep it
> > > disabled by default.
> > 
> > Hum, this seems to be a regression caused by Christoph's ACL rework in
> > 2013.
> 
> Ooops.
> 
> > Since then HFS+ acls didn't work. So much for how much they are
> > used... Maybe time to drop the feature when nobody noticed it does not work
> > for 4 years?
> 
> Especially given that they aren't actually compatible with MacOS
> I'm all in favor of dropping the support.

Since there was no more discussion I suppose I should go ahead and
do it. I'm using your rationale in the commit message, let me know if
either of you wants to be tagged as "Suggested-by", or in any other
way. Thank you.

-- >8 --
Subject: [PATCH] hfsplus: drop ACL support

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>
---
 fs/hfsplus/Kconfig          |  18 ------
 fs/hfsplus/Makefile         |   2 -
 fs/hfsplus/acl.h            |  27 ---------
 fs/hfsplus/dir.c            |   9 +--
 fs/hfsplus/hfsplus_fs.h     |   1 -
 fs/hfsplus/inode.c          |  11 ----
 fs/hfsplus/posix_acl.c      | 143 --------------------------------------------
 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(+), 233 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 683fca2..09d278b 100644
--- a/fs/hfsplus/Makefile
+++ b/fs/hfsplus/Makefile
@@ -7,5 +7,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 95c8ed9..0000000
--- a/fs/hfsplus/acl.h
+++ /dev/null
@@ -1,27 +0,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 31d5e3f..06d921c 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -17,7 +17,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 a3f03b2..c789b42 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -30,7 +30,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 4f26b68..b611637 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -20,7 +20,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)
 {
@@ -266,12 +265,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;
 }
 
@@ -335,10 +328,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 6bb5d7c..0000000
--- a/fs/hfsplus/posix_acl.c
+++ /dev/null
@@ -1,143 +0,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 e5bb2de..6ef5199 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 d37bb88..cdce37d 100644
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -7,10 +7,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);
 
@@ -18,10 +16,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 68f6b53..6fdd677 100644
--- a/fs/hfsplus/xattr.h
+++ b/fs/hfsplus/xattr.h
@@ -37,7 +37,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 37b3efa..1330ab8 100644
--- a/fs/hfsplus/xattr_security.c
+++ b/fs/hfsplus/xattr_security.c
@@ -11,7 +11,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,
@@ -71,18 +70,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

Re: [PATCH 2/2] hfsplus: add mount option to enable ACL support

[Christoph Hellwig]

On Fri, Oct 13, 2017 at 11:43:19PM -0300, Ernesto A. Fern�ndez wrote:
> Since there was no more discussion I suppose I should go ahead and
> do it. I'm using your rationale in the commit message, let me know if
> either of you wants to be tagged as "Suggested-by", or in any other
> way. Thank you.

I don't think I suggested it, but I'd be happy to ACK it:


Acked-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] hfsplus: add mount option to enable ACL support

[Viacheslav Dubeyko]

On Mon, 2017-10-16 at 00:42 -0700, Christoph Hellwig wrote:
> On Fri, Oct 13, 2017 at 11:43:19PM -0300, Ernesto A. Fernández wrote:
> > 
> > Since there was no more discussion I suppose I should go ahead and
> > do it. I'm using your rationale in the commit message, let me know
> > if
> > either of you wants to be tagged as "Suggested-by", or in any other
> > way. Thank you.
> I don't think I suggested it, but I'd be happy to ACK it:
> 

I believe that it's not right way of doing things. The exclsuion of
this section of code doesn't mean that HFS+ file system driver will not
have any bugs. If we follow this logic then it makes sense to exclude
the whole HFS+ support from the Linux kernel especcially if nobody
cares about HFS+ support anymore.

Thanks,
Vyacheslav Dubeyko.

Re: [PATCH 2/2] hfsplus: add mount option to enable ACL support

[Jan Kara]

On Mon 16-10-17 08:50:30, Viacheslav Dubeyko wrote:
> On Mon, 2017-10-16 at 00:42 -0700, Christoph Hellwig wrote:
> > On Fri, Oct 13, 2017 at 11:43:19PM -0300, Ernesto A. Fern�ndez wrote:
> > > 
> > > Since there was no more discussion I suppose I should go ahead and
> > > do it. I'm using your rationale in the commit message, let me know
> > > if
> > > either of you wants to be tagged as "Suggested-by", or in any other
> > > way. Thank you.
> > I don't think I suggested it, but I'd be happy to ACK it:
> > 
> 
> I believe that it's not right way of doing things. The exclsuion of
> this section of code doesn't mean that HFS+ file system driver will not
> have any bugs. If we follow this logic then it makes sense to exclude
> the whole HFS+ support from the Linux kernel especcially if nobody
> cares about HFS+ support anymore.

Well, if nobody really uses HFS+ support, it would be best to drop it, yes.
However we don't know whether somebody uses it or not and my guess would be
that actually someone occasionally uses HFS+ support to read MacOS files.

With xattr support the situation is different. It has been broken for four
years and nobody complained. That is a good indication nobody uses that
code and we can drop it. Furthermore, Christoph pointed out that original
HFS+ in MacOS never supported xattrs which makes it even less likely that
someone uses that functionality (as you cannot use it if you want your
filesystem to stay compatible with MacOS).

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/2] hfsplus: add mount option to enable ACL support

[Christoph Hellwig]

On Tue, Oct 17, 2017 at 09:53:46AM +0200, Jan Kara wrote:
> With xattr support the situation is different. It has been broken for four
> years and nobody complained. That is a good indication nobody uses that
> code and we can drop it. Furthermore, Christoph pointed out that original
> HFS+ in MacOS never supported xattrs which makes it even less likely that
> someone uses that functionality (as you cannot use it if you want your
> filesystem to stay compatible with MacOS).

s/xattrs/Posix ACLs/g

Re: [PATCH 2/2] hfsplus: add mount option to enable ACL support

[Viacheslav Dubeyko]

On Tue, 2017-10-17 at 01:03 -0700, Christoph Hellwig wrote:
> On Tue, Oct 17, 2017 at 09:53:46AM +0200, Jan Kara wrote:
> > 
> > With xattr support the situation is different. It has been broken
> > for four
> > years and nobody complained. That is a good indication nobody uses
> > that
> > code and we can drop it. Furthermore, Christoph pointed out that
> > original
> > HFS+ in MacOS never supported xattrs which makes it even less
> > likely that
> > someone uses that functionality (as you cannot use it if you want
> > your
> > filesystem to stay compatible with MacOS).
> s/xattrs/Posix ACLs/g

The extended attributes supported in Mac OS X many years already [1].
Of course, it looks like more than Richacls [2]. But no Richacls was
available during the implementation phase.

[1] https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man1/xattr.1.html
[2] https://en.wikipedia.org/wiki/Richacls