diff mbox

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

Message ID 20171014024318.GA8995@debian.home (mailing list archive)
State New, archived
Headers show

Commit Message

Ernesto A. Fernández Oct. 14, 2017, 2:43 a.m. UTC
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

Comments

Christoph Hellwig Oct. 16, 2017, 7:42 a.m. UTC | #1
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>
Viacheslav Dubeyko Oct. 16, 2017, 3:50 p.m. UTC | #2
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.
Jan Kara Oct. 17, 2017, 7:53 a.m. UTC | #3
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
Christoph Hellwig Oct. 17, 2017, 8:03 a.m. UTC | #4
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
Viacheslav Dubeyko Oct. 17, 2017, 3:20 p.m. UTC | #5
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
diff mbox

Patch

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,