Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1265569 > unrolled thread

[PATCH v15 00/22] Richacls (Core and Ext4)

Started byAndreas Gruenbacher <agruenba@redhat.com>
First post2015-11-09 12:10 +0100
Last post2015-11-11 15:00 +0100
Articles 9 on this page of 29 — 4 participants

Back to article view | Back to linux.kernel


Contents

  [PATCH v15 00/22] Richacls (Core and Ext4) Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:10 +0100
    [PATCH v15 02/22] vfs: Add MAY_CREATE_FILE and MAY_CREATE_DIR permission flags Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:10 +0100
    [PATCH v15 21/22] ext4: Add richacl support Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:20 +0100
    [PATCH v15 06/22] richacl: In-memory representation and helper functions Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:20 +0100
    [PATCH v15 20/22] vfs: Add richacl permission checking Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:20 +0100
    [PATCH v15 09/22] richacl: Permission check algorithm Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:20 +0100
    [PATCH v15 14/22] richacl: Update the file masks in chmod() Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:20 +0100
    [PATCH v15 19/22] richacl: Add richacl xattr handler Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:20 +0100
      Re: [PATCH v15 19/22] richacl: Add richacl xattr handler Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-11 02:20 +0100
    [PATCH v15 18/22] richacl: xattr mapping functions Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:20 +0100
    [PATCH v15 22/22] ext4: Add richacl feature flag Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:20 +0100
    [PATCH v15 08/22] richacl: Compute maximum file masks from an acl Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:20 +0100
    [PATCH v15 13/22] vfs: Cache richacl in struct inode Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:20 +0100
    [PATCH v15 17/22] richacl: Automatic Inheritance Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:20 +0100
    [PATCH v15 12/22] vfs: Add get_richacl and set_richacl inode operations Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:20 +0100
    [PATCH v15 15/22] richacl: Check if an acl is equivalent to a file mode Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:20 +0100
    [PATCH v15 16/22] richacl: Create-time inheritance Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:20 +0100
    [PATCH v15 10/22] posix_acl: Unexport acl_by_type and make it static Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:20 +0100
    [PATCH v15 11/22] vfs: Cache base_acl objects in inodes Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:30 +0100
    [PATCH v15 07/22] richacl: Permission mapping functions Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:30 +0100
    [PATCH v15 01/22] vfs: Add IS_ACL() and IS_RICHACL() tests Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-09 12:30 +0100
    Re: [PATCH v15 00/22] Richacls (Core and Ext4) Christoph Hellwig <hch@infradead.org> - 2015-11-10 12:30 +0100
      Re: [PATCH v15 00/22] Richacls (Core and Ext4) Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-10 13:50 +0100
        Re: [PATCH v15 00/22] Richacls (Core and Ext4) Steve French <smfrench@gmail.com> - 2015-11-10 17:50 +0100
          Re: [PATCH v15 00/22] Richacls (Core and Ext4) "J. Bruce Fields" <bfields@fieldses.org> - 2015-11-10 18:10 +0100
            Re: [PATCH v15 00/22] Richacls (Core and Ext4) Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-10 19:10 +0100
              Re: [PATCH v15 00/22] Richacls (Core and Ext4) "J. Bruce Fields" <bfields@fieldses.org> - 2015-11-10 20:20 +0100
        Re: [PATCH v15 00/22] Richacls (Core and Ext4) Christoph Hellwig <hch@infradead.org> - 2015-11-11 09:00 +0100
          Re: [PATCH v15 00/22] Richacls (Core and Ext4) Andreas Gruenbacher <agruenba@redhat.com> - 2015-11-11 15:00 +0100

Page 2 of 2 — ← Prev page 1 [2]


#1265594 — [PATCH v15 01/22] vfs: Add IS_ACL() and IS_RICHACL() tests

FromAndreas Gruenbacher <agruenba@redhat.com>
Date2015-11-09 12:30 +0100
Subject[PATCH v15 01/22] vfs: Add IS_ACL() and IS_RICHACL() tests
Message-ID<qsSO6-5KO-13@gated-at.bofh.it>
In reply to#1265569
The vfs does not apply the umask for file systems that support acls. The
test used for this used to be called IS_POSIXACL(). Switch to a new
IS_ACL() test to check for either posix acls or richacls instead. Add a new
MS_RICHACL flag and IS_RICHACL() test for richacls alone. The IS_POSIXACL()
test is still needed by nfsd.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
Reviewed-by: J. Bruce Fields <bfields@redhat.com>
Reviewed-by: Andreas Dilger <adilger@dilger.ca>
---
 fs/Kconfig              |  3 +++
 fs/namei.c              |  8 ++++----
 include/linux/fs.h      | 12 ++++++++++++
 include/uapi/linux/fs.h |  3 ++-
 4 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/fs/Kconfig b/fs/Kconfig
index da3f32f..bff2879 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -56,6 +56,9 @@ endif # BLOCK
 config FS_POSIX_ACL
 	def_bool n
 
+config FS_RICHACL
+	def_bool n
+
 config EXPORTFS
 	tristate
 
diff --git a/fs/namei.c b/fs/namei.c
index 33e9495..224ecf1 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2798,7 +2798,7 @@ static int atomic_open(struct nameidata *nd, struct dentry *dentry,
 	}
 
 	mode = op->mode;
-	if ((open_flag & O_CREAT) && !IS_POSIXACL(dir))
+	if ((open_flag & O_CREAT) && !IS_ACL(dir))
 		mode &= ~current_umask();
 
 	excl = (open_flag & (O_EXCL | O_CREAT)) == (O_EXCL | O_CREAT);
@@ -2982,7 +2982,7 @@ static int lookup_open(struct nameidata *nd, struct path *path,
 	/* Negative dentry, just create the file */
 	if (!dentry->d_inode && (op->open_flag & O_CREAT)) {
 		umode_t mode = op->mode;
-		if (!IS_POSIXACL(dir->d_inode))
+		if (!IS_ACL(dir->d_inode))
 			mode &= ~current_umask();
 		/*
 		 * This write is needed to ensure that a
@@ -3553,7 +3553,7 @@ retry:
 	if (IS_ERR(dentry))
 		return PTR_ERR(dentry);
 
-	if (!IS_POSIXACL(path.dentry->d_inode))
+	if (!IS_ACL(path.dentry->d_inode))
 		mode &= ~current_umask();
 	error = security_path_mknod(&path, dentry, mode, dev);
 	if (error)
@@ -3622,7 +3622,7 @@ retry:
 	if (IS_ERR(dentry))
 		return PTR_ERR(dentry);
 
-	if (!IS_POSIXACL(path.dentry->d_inode))
+	if (!IS_ACL(path.dentry->d_inode))
 		mode &= ~current_umask();
 	error = security_path_mkdir(&path, dentry, mode);
 	if (!error)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 72d8a84..4efa435 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1781,6 +1781,12 @@ struct super_operations {
 #define IS_IMMUTABLE(inode)	((inode)->i_flags & S_IMMUTABLE)
 #define IS_POSIXACL(inode)	__IS_FLG(inode, MS_POSIXACL)
 
+#ifdef CONFIG_FS_RICHACL
+#define IS_RICHACL(inode)	__IS_FLG(inode, MS_RICHACL)
+#else
+#define IS_RICHACL(inode)	0
+#endif
+
 #define IS_DEADDIR(inode)	((inode)->i_flags & S_DEAD)
 #define IS_NOCMTIME(inode)	((inode)->i_flags & S_NOCMTIME)
 #define IS_SWAPFILE(inode)	((inode)->i_flags & S_SWAPFILE)
@@ -1794,6 +1800,12 @@ struct super_operations {
 				 (inode)->i_rdev == WHITEOUT_DEV)
 
 /*
+ * IS_ACL() tells the VFS to not apply the umask
+ * and use check_acl for acl permission checks when defined.
+ */
+#define IS_ACL(inode)		__IS_FLG(inode, MS_POSIXACL | MS_RICHACL)
+
+/*
  * Inode state bits.  Protected by inode->i_lock
  *
  * Three bits determine the dirty state of the inode, I_DIRTY_SYNC,
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 9b964a5..6ac6bc9 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -81,7 +81,7 @@ struct inodes_stat_t {
 #define MS_VERBOSE	32768	/* War is peace. Verbosity is silence.
 				   MS_VERBOSE is deprecated. */
 #define MS_SILENT	32768
-#define MS_POSIXACL	(1<<16)	/* VFS does not apply the umask */
+#define MS_POSIXACL	(1<<16)	/* Supports POSIX ACLs */
 #define MS_UNBINDABLE	(1<<17)	/* change to unbindable */
 #define MS_PRIVATE	(1<<18)	/* change to private */
 #define MS_SLAVE	(1<<19)	/* change to slave */
@@ -91,6 +91,7 @@ struct inodes_stat_t {
 #define MS_I_VERSION	(1<<23) /* Update inode I_version field */
 #define MS_STRICTATIME	(1<<24) /* Always perform atime updates */
 #define MS_LAZYTIME	(1<<25) /* Update the on-disk [acm]times lazily */
+#define MS_RICHACL	(1<<26) /* Supports richacls */
 
 /* These sb flags are internal to the kernel */
 #define MS_NOSEC	(1<<28)
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1266413

FromChristoph Hellwig <hch@infradead.org>
Date2015-11-10 12:30 +0100
Message-ID<qtfhD-4pX-7@gated-at.bofh.it>
In reply to#1265569
On Mon, Nov 09, 2015 at 12:08:41PM +0100, Andreas Gruenbacher wrote:
> Here is another update to the richacl patch queue.  This posting contains
> the patches ready to be merged; the patches later in the queue still need
> some more review.

It still has the same crappy fs interfaces with lots of boilerplate
code and still abuses xattrs instead of a proper syscall interface.
That's far from being ready to merge.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1266469

FromAndreas Gruenbacher <agruenba@redhat.com>
Date2015-11-10 13:50 +0100
Message-ID<qtgx4-57n-9@gated-at.bofh.it>
In reply to#1266413
On Tue, Nov 10, 2015 at 12:29 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Nov 09, 2015 at 12:08:41PM +0100, Andreas Gruenbacher wrote:
>> Here is another update to the richacl patch queue.  This posting contains
>> the patches ready to be merged; the patches later in the queue still need
>> some more review.
>
> It still has the same crappy fs interfaces with lots of boilerplate
> code

Could you please be more specific so that I can trace this complaint
to some actual code?

> and still abuses xattrs instead of a proper syscall interface.
> That's far from being ready to merge.

The xattr syscall interface is what's used for very similar kinds of
things today; using it for richacls as well sure does not count as
abuse. Things could be improved in the xattr interface and in its
implementation, but we need more substantial reasons than that for
reimplementing the wheel once again.

[The recently discussed xattr fixes
(http://permalink.gmane.org/gmane.linux.file-systems/101502) and two
small POSIX ACL fixes
(http://permalink.gmane.org/gmane.linux.file-systems/101499) are
currently stuck waiting for Al, by the way.]

Thanks,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1266638

FromSteve French <smfrench@gmail.com>
Date2015-11-10 17:50 +0100
Message-ID<qtkhl-7vy-43@gated-at.bofh.it>
In reply to#1266469
On Tue, Nov 10, 2015 at 6:39 AM, Andreas Gruenbacher
<agruenba@redhat.com> wrote:
> On Tue, Nov 10, 2015 at 12:29 PM, Christoph Hellwig <hch@infradead.org> wrote:
>> On Mon, Nov 09, 2015 at 12:08:41PM +0100, Andreas Gruenbacher wrote:
>>> Here is another update to the richacl patch queue.  This posting contains
>>> the patches ready to be merged; the patches later in the queue still need
>>> some more review.
<snip>
>> and still abuses xattrs instead of a proper syscall interface.
>> That's far from being ready to merge.
>
> The xattr syscall interface is what's used for very similar kinds of
> things today; using it for richacls as well sure does not count as
> abuse. Things could be improved in the xattr interface and in its
> implementation, but we need more substantial reasons than that for
> reimplementing the wheel once again.

I don't have strong disagreement with using pseudo-xattrs to
store/retrieve ACLs (we already do this) but retrieving/setting an ACL
all at once can be awkward  when ACLs are quite large e.g. when it
encodes to over 1MB (not all administrators think about the size of
ACLs when they add hundreds of users or groups or apps to ACLs).

The bigger problem is that when ACLs are created -- after -- the file
is created there is a potential race (harder to deal with in cluster
and network file systems).   Ideally we should be able to optionally
pass all the security information needed to create a file in the
create call itself.  For apps which don't care they can continue to
use the old syscalls.

In the meantime, I don't mind the approach of staging this in via a
pseudo-xattr, Samba can deal with that (and it will make some of the
backup and data movement tools easier for the cifs.ko client which
currently rely on a cifs specific xattr).

In cifs.ko I still need to enable the SMB3 ACL helper functions
(currently only enabled for the older cifs dialect) since that will
make it easier, and figure out a way to allow helper tools to view
"claims based ACLs" (DAC), not just traditional
CIFS/NTFS/SMB3/RichACLs.
-- 
Thanks,

Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1266647

From"J. Bruce Fields" <bfields@fieldses.org>
Date2015-11-10 18:10 +0100
Message-ID<qtkAF-7SL-11@gated-at.bofh.it>
In reply to#1266638
On Tue, Nov 10, 2015 at 10:43:46AM -0600, Steve French wrote:
> On Tue, Nov 10, 2015 at 6:39 AM, Andreas Gruenbacher
> <agruenba@redhat.com> wrote:
> > On Tue, Nov 10, 2015 at 12:29 PM, Christoph Hellwig <hch@infradead.org> wrote:
> >> On Mon, Nov 09, 2015 at 12:08:41PM +0100, Andreas Gruenbacher wrote:
> >>> Here is another update to the richacl patch queue.  This posting contains
> >>> the patches ready to be merged; the patches later in the queue still need
> >>> some more review.
> <snip>
> >> and still abuses xattrs instead of a proper syscall interface.
> >> That's far from being ready to merge.
> >
> > The xattr syscall interface is what's used for very similar kinds of
> > things today; using it for richacls as well sure does not count as
> > abuse. Things could be improved in the xattr interface and in its
> > implementation, but we need more substantial reasons than that for
> > reimplementing the wheel once again.
> 
> I don't have strong disagreement with using pseudo-xattrs to
> store/retrieve ACLs (we already do this) but retrieving/setting an ACL
> all at once can be awkward  when ACLs are quite large e.g. when it
> encodes to over 1MB

At least in the NFS case, that's also a limitation of the protocol.  If
we really wanted to support massive ACLs then we'd need both syscall and
NFS interfaces to allow incrementally reading and writing ACLs, and I
don't even know what those would look like.

So this is a fine limitation as far as I'm concerned.

> (not all administrators think about the size of
> ACLs when they add hundreds of users or groups or apps to ACLs).
> 
> The bigger problem is that when ACLs are created -- after -- the file
> is created there is a potential race (harder to deal with in cluster
> and network file systems).   Ideally we should be able to optionally
> pass all the security information needed to create a file in the
> create call itself.  For apps which don't care they can continue to
> use the old syscalls.

That would be most of them, I'd think.

But I suppose windows apps (via Samba or Wine?) could use this.

Definitely a project for another day, in any case.

--b.

> In cifs.ko I still need to enable the SMB3 ACL helper functions
> (currently only enabled for the older cifs dialect) since that will
> make it easier, and figure out a way to allow helper tools to view
> "claims based ACLs" (DAC), not just traditional
> CIFS/NTFS/SMB3/RichACLs.
> -- 
> Thanks,
> 
> Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1266695

FromAndreas Gruenbacher <agruenba@redhat.com>
Date2015-11-10 19:10 +0100
Message-ID<qtlwL-8tw-33@gated-at.bofh.it>
In reply to#1266647
On Tue, Nov 10, 2015 at 6:07 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Tue, Nov 10, 2015 at 10:43:46AM -0600, Steve French wrote:
>> On Tue, Nov 10, 2015 at 6:39 AM, Andreas Gruenbacher
>> <agruenba@redhat.com> wrote:
>> > On Tue, Nov 10, 2015 at 12:29 PM, Christoph Hellwig <hch@infradead.org> wrote:
>> >> On Mon, Nov 09, 2015 at 12:08:41PM +0100, Andreas Gruenbacher wrote:
>> >>> Here is another update to the richacl patch queue.  This posting contains
>> >>> the patches ready to be merged; the patches later in the queue still need
>> >>> some more review.
>> <snip>
>> >> and still abuses xattrs instead of a proper syscall interface.
>> >> That's far from being ready to merge.
>> >
>> > The xattr syscall interface is what's used for very similar kinds of
>> > things today; using it for richacls as well sure does not count as
>> > abuse. Things could be improved in the xattr interface and in its
>> > implementation, but we need more substantial reasons than that for
>> > reimplementing the wheel once again.
>>
>> I don't have strong disagreement with using pseudo-xattrs to
>> store/retrieve ACLs (we already do this) but retrieving/setting an ACL
>> all at once can be awkward  when ACLs are quite large e.g. when it
>> encodes to over 1MB
>
> At least in the NFS case, that's also a limitation of the protocol.

I couldn't find a limit in the NFSv4 specification, but the client and
server implementations both define arbitrary ACL size limits. In
addition, the xattr syscalls allow attributes to be up to 64k long.

> If
> we really wanted to support massive ACLs then we'd need both syscall and
> NFS interfaces to allow incrementally reading and writing ACLs, and I
> don't even know what those would look like.
>
> So this is a fine limitation as far as I'm concerned.

The bigger problem would be incrementally setting ACLs. To prevent
processes from racing with each other, we would need a locking
mechanism. In addition, the memory overhead would be prohibitive and
access decisions would become extremely slow; we would have to come up
with mechanisms to avoid those problems.

Thanks,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1266732

From"J. Bruce Fields" <bfields@fieldses.org>
Date2015-11-10 20:20 +0100
Message-ID<qtmCt-IR-13@gated-at.bofh.it>
In reply to#1266695
On Tue, Nov 10, 2015 at 06:58:19PM +0100, Andreas Gruenbacher wrote:
> On Tue, Nov 10, 2015 at 6:07 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > On Tue, Nov 10, 2015 at 10:43:46AM -0600, Steve French wrote:
> >> I don't have strong disagreement with using pseudo-xattrs to
> >> store/retrieve ACLs (we already do this) but retrieving/setting an ACL
> >> all at once can be awkward  when ACLs are quite large e.g. when it
> >> encodes to over 1MB
> >
> > At least in the NFS case, that's also a limitation of the protocol.
> 
> I couldn't find a limit in the NFSv4 specification, but the client and
> server implementations both define arbitrary ACL size limits. In
> addition, the xattr syscalls allow attributes to be up to 64k long.

I don't recall 4.0 specifying any limit, 4.1 does include negotiation of
maximum rpc calls and replies, and that effectively limits ACL sizes
since they have to fit in a single rpc.

> The bigger problem would be incrementally setting ACLs. To prevent
> processes from racing with each other, we would need a locking
> mechanism. In addition, the memory overhead would be prohibitive and
> access decisions would become extremely slow; we would have to come up
> with mechanisms to avoid those problems.

Right.  Anyway, not worth the trouble, I think.

(Though what might be worth thinking about at some point is just making
sure we fail in helpful ways.)

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1267021

FromChristoph Hellwig <hch@infradead.org>
Date2015-11-11 09:00 +0100
Message-ID<qtytX-8jJ-9@gated-at.bofh.it>
In reply to#1266469
On Tue, Nov 10, 2015 at 01:39:52PM +0100, Andreas Gruenbacher wrote:
> > It still has the same crappy fs interfaces with lots of boilerplate
> > code
> 
> Could you please be more specific so that I can trace this complaint
> to some actual code?

if (IS_RICHACL())
	richacl_foo()
else
	posix_acl_foo()

for every call from the filesystem is the major one that came to mind.

> > and still abuses xattrs instead of a proper syscall interface.
> > That's far from being ready to merge.
> 
> The xattr syscall interface is what's used for very similar kinds of
> things today; using it for richacls as well sure does not count as
> abuse. Things could be improved in the xattr interface and in its
> implementation, but we need more substantial reasons than that for
> reimplementing the wheel once again.

And it's a horrible interface.  Look at all the pain for example in
XFS which has a different ACL format, or in fact everyone who just
uses a different xattr name or even none at all.  And all the mess
of people trying to shoe horn crazy interfaces into xattrs.

It was an experiment worth trying with Posix ACLs, but it failed, so
do not repeat it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]


#1267201

FromAndreas Gruenbacher <agruenba@redhat.com>
Date2015-11-11 15:00 +0100
Message-ID<qtE6m-3vE-13@gated-at.bofh.it>
In reply to#1267021
On Wed, Nov 11, 2015 at 8:57 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Nov 10, 2015 at 01:39:52PM +0100, Andreas Gruenbacher wrote:
>> > It still has the same crappy fs interfaces with lots of boilerplate
>> > code
>>
>> Could you please be more specific so that I can trace this complaint
>> to some actual code?
>
> if (IS_RICHACL())
>         richacl_foo()
> else
>         posix_acl_foo()
>
> for every call from the filesystem is the major one that came to mind.

There are two such places in ext4, ext4_new_acl and ext4_acl_chmod.  Those
could be replaced by function pointers as below, I'm not sure we seriously
want to consider that.

In xfs, we have xfs_acl_chmod which is similar.  xfs_generic_create doesn't
quite follow this pattern.

This seems to be about it though.

Thanks,
Andreas

---
 fs/ext4/ext4.h     | 10 ++++++++++
 fs/ext4/ialloc.c   | 11 +----------
 fs/ext4/inode.c    | 11 +----------
 fs/ext4/super.c    | 30 ++++++++++++++++++++++++++++++
 include/linux/fs.h |  1 +
 5 files changed, 43 insertions(+), 20 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index b97a3b1..5ff4556 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3049,6 +3049,16 @@ extern struct mutex ext4__aio_mutex[EXT4_WQ_HASH_SZ];
 extern int ext4_resize_begin(struct super_block *sb);
 extern void ext4_resize_end(struct super_block *sb);
 
+struct ext4_acl_ops {
+	int (*chmod)(struct inode *, umode_t);
+	int (*init_acl)(handle_t *, struct inode *, struct inode *);
+};
+
+static inline const struct ext4_acl_ops *ACL_OPS(struct inode *inode)
+{
+	return inode->i_sb->s_private;
+}
+
 #endif	/* __KERNEL__ */
 
 #endif	/* _EXT4_H */
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 9657b3a..e33646f 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -27,7 +27,6 @@
 #include "ext4_jbd2.h"
 #include "xattr.h"
 #include "acl.h"
-#include "richacl.h"
 
 #include <trace/events/ext4.h>
 
@@ -698,14 +697,6 @@ out:
 	return ret;
 }
 
-static inline int
-ext4_new_acl(handle_t *handle, struct inode *inode, struct inode *dir)
-{
-	if (IS_RICHACL(dir))
-		return ext4_init_richacl(handle, inode, dir);
-	return ext4_init_acl(handle, inode, dir);
-}
-
 /*
  * There are two policies for allocating an inode.  If the new inode is
  * a directory, then a forward search is made for a block group with both
@@ -1061,7 +1052,7 @@ got:
 	if (err)
 		goto fail_drop;
 
-	err = ext4_new_acl(handle, inode, dir);
+	err = ACL_OPS(inode)->init_acl(handle, inode, dir);
 	if (err)
 		goto fail_free_drop;
 
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 647f3c3..9f179ee 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -42,7 +42,6 @@
 #include "xattr.h"
 #include "acl.h"
 #include "truncate.h"
-#include "richacl.h"
 
 #include <trace/events/ext4.h>
 
@@ -4639,14 +4638,6 @@ static void ext4_wait_for_tail_page_commit(struct inode *inode)
 	}
 }
 
-static inline int
-ext4_acl_chmod(struct inode *inode, umode_t mode)
-{
-	if (IS_RICHACL(inode))
-		return richacl_chmod(inode, inode->i_mode);
-	return posix_acl_chmod(inode, inode->i_mode);
-}
-
 /*
  * ext4_setattr()
  *
@@ -4815,7 +4806,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 		ext4_orphan_del(NULL, inode);
 
 	if (!rc && (ia_valid & ATTR_MODE))
-		rc = ext4_acl_chmod(inode, inode->i_mode);
+		rc = ACL_OPS(inode)->chmod(inode, inode->i_mode);
 err_out:
 	ext4_std_error(inode->i_sb, error);
 	if (!error)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 7457ea8..879bc2c 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -49,6 +49,7 @@
 #include "ext4_jbd2.h"
 #include "xattr.h"
 #include "acl.h"
+#include "richacl.h"
 #include "mballoc.h"
 
 #define CREATE_TRACE_POINTS
@@ -1270,25 +1271,54 @@ static ext4_fsblk_t get_sb_block(void **data)
 	return sb_block;
 }
 
+static int no_chmod_acl(struct inode *inode, umode_t mode)
+{
+	return 0;
+}
+
+static int no_init_acl(handle_t *handle, struct inode *inode, struct inode *dir)
+{
+	return 0;
+}
+
+struct ext4_acl_ops no_acl_ops = {
+	.chmod = no_chmod_acl,
+	.init_acl = no_init_acl,
+};
+
+struct ext4_acl_ops ext4_posix_acl_ops = {
+	.chmod = posix_acl_chmod,
+	.init_acl = ext4_init_acl,
+};
+
+struct ext4_acl_ops ext4_richacl_ops = {
+	.chmod = richacl_chmod,
+	.init_acl = ext4_init_richacl,
+};
+
 static int enable_acl(struct super_block *sb)
 {
 	sb->s_flags &= ~(MS_POSIXACL | MS_RICHACL);
+	sb->s_private = &no_acl_ops;
 	if (test_opt(sb, ACL)) {
 		if (EXT4_HAS_INCOMPAT_FEATURE(sb,
 					      EXT4_FEATURE_INCOMPAT_RICHACL)) {
 #ifdef CONFIG_EXT4_FS_RICHACL
 			sb->s_flags |= MS_RICHACL;
+			sb->s_private = &ext4_richacl_ops;
 #else
 			return -EOPNOTSUPP;
 #endif
 		} else {
 #ifdef CONFIG_EXT4_FS_POSIX_ACL
 			sb->s_flags |= MS_POSIXACL;
+			sb->s_private = &ext4_posix_acl_ops;
 #else
 			return -EOPNOTSUPP;
 #endif
 		}
 	}
+
 	return 0;
 }
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 05fb943..5803bf6 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1324,6 +1324,7 @@ struct super_block {
 	void                    *s_security;
 #endif
 	const struct xattr_handler **s_xattr;
+	const void *s_private;
 
 	struct hlist_bl_head	s_anon;		/* anonymous dentries for (nfs) exporting */
 	struct list_head	s_mounts;	/* list of mounts; _not_ for fs use */
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]


Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel


csiph-web