* [PATCH 1/2] hfsplus: preserve i_mode if __hfsplus_set_posix_acl() fails @ 2017-07-27 3:10 Ernesto A. Fernández 2017-07-27 3:13 ` [PATCH 2/2] hfsplus: add mount option to enable ACL support Ernesto A. Fernández 2017-07-27 9:18 ` [PATCH 1/2] hfsplus: preserve i_mode if __hfsplus_set_posix_acl() fails Jan Kara 0 siblings, 2 replies; 12+ messages in thread From: Ernesto A. Fernández @ 2017-07-27 3:10 UTC (permalink / raw) To: linux-fsdevel Cc: Jan Kara, Andreas Gruenbacher, Alexander Viro, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] hfsplus: add mount option to enable ACL support 2017-07-27 3:10 [PATCH 1/2] hfsplus: preserve i_mode if __hfsplus_set_posix_acl() fails Ernesto A. Fernández @ 2017-07-27 3:13 ` Ernesto A. Fernández 2017-07-27 9:41 ` Jan Kara 2017-07-27 9:18 ` [PATCH 1/2] hfsplus: preserve i_mode if __hfsplus_set_posix_acl() fails Jan Kara 1 sibling, 1 reply; 12+ messages in thread From: Ernesto A. Fernández @ 2017-07-27 3:13 UTC (permalink / raw) To: linux-fsdevel Cc: Jan Kara, Andreas Gruenbacher, Alexander Viro, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] hfsplus: add mount option to enable ACL support 2017-07-27 3:13 ` [PATCH 2/2] hfsplus: add mount option to enable ACL support Ernesto A. Fernández @ 2017-07-27 9:41 ` Jan Kara 2017-07-27 9:50 ` Christoph Hellwig 2017-07-27 21:29 ` Ernesto A. Fernández 0 siblings, 2 replies; 12+ messages in thread From: Jan Kara @ 2017-07-27 9:41 UTC (permalink / raw) To: Ernesto A. Fernández Cc: linux-fsdevel, Jan Kara, Andreas Gruenbacher, Alexander Viro, Christoph Hellwig, Vyacheslav Dubeyko 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] hfsplus: add mount option to enable ACL support 2017-07-27 9:41 ` Jan Kara @ 2017-07-27 9:50 ` Christoph Hellwig 2017-10-14 2:43 ` Ernesto A. Fernández 2017-07-27 21:29 ` Ernesto A. Fernández 1 sibling, 1 reply; 12+ messages in thread From: Christoph Hellwig @ 2017-07-27 9:50 UTC (permalink / raw) To: Jan Kara Cc: Ernesto A. Fernández, linux-fsdevel, Andreas Gruenbacher, Alexander Viro, Christoph Hellwig, Vyacheslav Dubeyko 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] hfsplus: add mount option to enable ACL support 2017-07-27 9:50 ` Christoph Hellwig @ 2017-10-14 2:43 ` Ernesto A. Fernández 2017-10-16 7:42 ` Christoph Hellwig 0 siblings, 1 reply; 12+ messages in thread From: Ernesto A. Fernández @ 2017-10-14 2:43 UTC (permalink / raw) To: Christoph Hellwig, Jan Kara Cc: Ernesto A. Fernández, linux-fsdevel, Andreas Gruenbacher, Alexander Viro, Vyacheslav Dubeyko 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] hfsplus: add mount option to enable ACL support 2017-10-14 2:43 ` Ernesto A. Fernández @ 2017-10-16 7:42 ` Christoph Hellwig 2017-10-16 15:50 ` Viacheslav Dubeyko 0 siblings, 1 reply; 12+ messages in thread From: Christoph Hellwig @ 2017-10-16 7:42 UTC (permalink / raw) To: Ernesto A. Fernández Cc: Christoph Hellwig, Jan Kara, linux-fsdevel, Andreas Gruenbacher, Alexander Viro, Vyacheslav Dubeyko 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> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] hfsplus: add mount option to enable ACL support 2017-10-16 7:42 ` Christoph Hellwig @ 2017-10-16 15:50 ` Viacheslav Dubeyko 2017-10-17 7:53 ` Jan Kara 0 siblings, 1 reply; 12+ messages in thread From: Viacheslav Dubeyko @ 2017-10-16 15:50 UTC (permalink / raw) To: Christoph Hellwig, Ernesto A. Fernández Cc: Jan Kara, linux-fsdevel, Andreas Gruenbacher, Alexander Viro 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] hfsplus: add mount option to enable ACL support 2017-10-16 15:50 ` Viacheslav Dubeyko @ 2017-10-17 7:53 ` Jan Kara 2017-10-17 8:03 ` Christoph Hellwig 0 siblings, 1 reply; 12+ messages in thread From: Jan Kara @ 2017-10-17 7:53 UTC (permalink / raw) To: Viacheslav Dubeyko Cc: Christoph Hellwig, Ernesto A. Fernández, Jan Kara, linux-fsdevel, Andreas Gruenbacher, Alexander Viro 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] hfsplus: add mount option to enable ACL support 2017-10-17 7:53 ` Jan Kara @ 2017-10-17 8:03 ` Christoph Hellwig 2017-10-17 15:20 ` Viacheslav Dubeyko 0 siblings, 1 reply; 12+ messages in thread From: Christoph Hellwig @ 2017-10-17 8:03 UTC (permalink / raw) To: Jan Kara Cc: Viacheslav Dubeyko, Christoph Hellwig, Ernesto A. Fernández, linux-fsdevel, Andreas Gruenbacher, Alexander Viro 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] hfsplus: add mount option to enable ACL support 2017-10-17 8:03 ` Christoph Hellwig @ 2017-10-17 15:20 ` Viacheslav Dubeyko 0 siblings, 0 replies; 12+ messages in thread From: Viacheslav Dubeyko @ 2017-10-17 15:20 UTC (permalink / raw) To: Christoph Hellwig, Jan Kara Cc: Ernesto A. Fernández, linux-fsdevel, Andreas Gruenbacher, Alexander Viro 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] hfsplus: add mount option to enable ACL support 2017-07-27 9:41 ` Jan Kara 2017-07-27 9:50 ` Christoph Hellwig @ 2017-07-27 21:29 ` Ernesto A. Fernández 1 sibling, 0 replies; 12+ messages in thread From: Ernesto A. Fernández @ 2017-07-27 21:29 UTC (permalink / raw) To: Jan Kara Cc: linux-fsdevel, Jan Kara, Andreas Gruenbacher, Alexander Viro, Christoph Hellwig, Vyacheslav Dubeyko, 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] hfsplus: preserve i_mode if __hfsplus_set_posix_acl() fails 2017-07-27 3:10 [PATCH 1/2] hfsplus: preserve i_mode if __hfsplus_set_posix_acl() fails Ernesto A. Fernández 2017-07-27 3:13 ` [PATCH 2/2] hfsplus: add mount option to enable ACL support Ernesto A. Fernández @ 2017-07-27 9:18 ` Jan Kara 1 sibling, 0 replies; 12+ messages in thread From: Jan Kara @ 2017-07-27 9:18 UTC (permalink / raw) To: Ernesto A. Fernández Cc: linux-fsdevel, Jan Kara, Andreas Gruenbacher, Alexander Viro 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-10-17 15:20 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-07-27 3:10 [PATCH 1/2] hfsplus: preserve i_mode if __hfsplus_set_posix_acl() fails Ernesto A. Fernández 2017-07-27 3:13 ` [PATCH 2/2] hfsplus: add mount option to enable ACL support Ernesto A. Fernández 2017-07-27 9:41 ` Jan Kara 2017-07-27 9:50 ` Christoph Hellwig 2017-10-14 2:43 ` Ernesto A. Fernández 2017-10-16 7:42 ` Christoph Hellwig 2017-10-16 15:50 ` Viacheslav Dubeyko 2017-10-17 7:53 ` Jan Kara 2017-10-17 8:03 ` Christoph Hellwig 2017-10-17 15:20 ` Viacheslav Dubeyko 2017-07-27 21:29 ` Ernesto A. Fernández 2017-07-27 9:18 ` [PATCH 1/2] hfsplus: preserve i_mode if __hfsplus_set_posix_acl() fails Jan Kara
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).