| Message ID | 20220922151728.1557914-1-brauner@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | acl: add vfs posix acl api | expand |
On 9/22/2022 8:16 AM, Christian Brauner wrote:
> From: "Christian Brauner (Microsoft)" <brauner@kernel.org>
Could we please see the entire patch set on the LSM list?
( linux-security-module@vger.kernel.org )
It's really tough to judge the importance of adding a new
LSM hook without seeing both how it is called and how the
security modules are expected to fulfill it. In particular,
it is important to see how a posix acl is different from
any other xattr.
On Thu, Sep 22, 2022 at 12:27 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 9/22/2022 8:16 AM, Christian Brauner wrote: > > From: "Christian Brauner (Microsoft)" <brauner@kernel.org> > > Could we please see the entire patch set on the LSM list? > ( linux-security-module@vger.kernel.org ) > It's really tough to judge the importance of adding a new > LSM hook without seeing both how it is called and how the > security modules are expected to fulfill it. In particular, > it is important to see how a posix acl is different from > any other xattr. Yes, exactly. I understand the desire to avoid dumping a large~ish patchset on a lot of lists, but it's really hard to adequately review something when you only see a small fraction of the overall change.
On Thu, Sep 22, 2022 at 9:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > Could we please see the entire patch set on the LSM list? While I don't think that's necessarily wrong, I would like to point out that the gitweb interface actually does make it fairly easy to just see the whole patch-set. IOW, that https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework that Christian pointed to is not a horrible way to see it all. Go to the top-most commit, and it's easy to follow the parent links. It's a bit more work to see them in another order, but I find the easiest way is actually to just follow the parent links to get the overview of what is going on (reading just the commit messages), and then after that you "reverse course" and use the browser back button to just go the other way while looking at the details of the patches. And I suspect a lot of people are happier *without* large patch-sets being posted to the mailing lists when most patches aren't necessarily at all relevant to that mailing list except as context. Linus
On 9/22/2022 10:57 AM, Linus Torvalds wrote: > On Thu, Sep 22, 2022 at 9:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote: >> Could we please see the entire patch set on the LSM list? > While I don't think that's necessarily wrong, I would like to point > out that the gitweb interface actually does make it fairly easy to > just see the whole patch-set. > > IOW, that > > https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework > > that Christian pointed to is not a horrible way to see it all. Go to > the top-most commit, and it's easy to follow the parent links. I understand that the web interface is fine for browsing the changes. It isn't helpful for making comments on the changes. The discussion on specific patches (e.g. selinux) may have impact on other parts of the system (e.g. integrity) or be relevant elsewhere (e.g. smack). It can be a real problem if the higher level mailing list (the LSM list in this case) isn't included. > > It's a bit more work to see them in another order, but I find the > easiest way is actually to just follow the parent links to get the > overview of what is going on (reading just the commit messages), and > then after that you "reverse course" and use the browser back button > to just go the other way while looking at the details of the patches. > > And I suspect a lot of people are happier *without* large patch-sets > being posted to the mailing lists when most patches aren't necessarily > at all relevant to that mailing list except as context. I can certainly understand that. I don't think that the filesystem specific bits are going to be especially interesting to me, but if they are I do want to be able to comment on them. > > Linus
On Thu, Sep 22, 2022 at 2:54 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 9/22/2022 10:57 AM, Linus Torvalds wrote: > > On Thu, Sep 22, 2022 at 9:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > >> Could we please see the entire patch set on the LSM list? > > While I don't think that's necessarily wrong, I would like to point > > out that the gitweb interface actually does make it fairly easy to > > just see the whole patch-set. > > > > IOW, that > > > > https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework > > > > that Christian pointed to is not a horrible way to see it all. Go to > > the top-most commit, and it's easy to follow the parent links. > > I understand that the web interface is fine for browsing the changes. > It isn't helpful for making comments on the changes. The discussion > on specific patches (e.g. selinux) may have impact on other parts of > the system (e.g. integrity) or be relevant elsewhere (e.g. smack). It > can be a real problem if the higher level mailing list (the LSM list > in this case) isn't included. This is probably one of those few cases where Casey and I are in perfect agreement. I'd much rather see the patches hit my inbox than have to go hunting for them and then awkwardly replying to them (and yes, I know there are ways to do that, I just personally find it annoying). I figure we are all deluged with email on a daily basis and have developed mechanisms to deal with that in a sane way, what is 29 more patches on the pile?
On Thu, Sep 22, 2022 at 03:07:44PM -0400, Paul Moore wrote: > On Thu, Sep 22, 2022 at 2:54 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 9/22/2022 10:57 AM, Linus Torvalds wrote: > > > On Thu, Sep 22, 2022 at 9:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > >> Could we please see the entire patch set on the LSM list? > > > While I don't think that's necessarily wrong, I would like to point > > > out that the gitweb interface actually does make it fairly easy to > > > just see the whole patch-set. > > > > > > IOW, that > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework > > > > > > that Christian pointed to is not a horrible way to see it all. Go to > > > the top-most commit, and it's easy to follow the parent links. > > > > I understand that the web interface is fine for browsing the changes. > > It isn't helpful for making comments on the changes. The discussion > > on specific patches (e.g. selinux) may have impact on other parts of > > the system (e.g. integrity) or be relevant elsewhere (e.g. smack). It > > can be a real problem if the higher level mailing list (the LSM list > > in this case) isn't included. > > This is probably one of those few cases where Casey and I are in > perfect agreement. I'd much rather see the patches hit my inbox than > have to go hunting for them and then awkwardly replying to them (and > yes, I know there are ways to do that, I just personally find it > annoying). I figure we are all deluged with email on a daily basis > and have developed mechanisms to deal with that in a sane way, what is > 29 more patches on the pile? Even better than the web interface, is find the message-id in any of the emails you did get, and run b4 mbox 20220922151728.1557914-1-brauner@kernel.org In general I'd agree with sending the whole set to the lsm list, but then one needs to start knowing which lists do and don't want the whole set... b4 mbox and lei are now how I read all kernel related lists. -serge
On Thu, Sep 22, 2022 at 5:57 PM Serge E. Hallyn <serge@hallyn.com> wrote: > On Thu, Sep 22, 2022 at 03:07:44PM -0400, Paul Moore wrote: > > On Thu, Sep 22, 2022 at 2:54 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > On 9/22/2022 10:57 AM, Linus Torvalds wrote: > > > > On Thu, Sep 22, 2022 at 9:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > >> Could we please see the entire patch set on the LSM list? > > > > While I don't think that's necessarily wrong, I would like to point > > > > out that the gitweb interface actually does make it fairly easy to > > > > just see the whole patch-set. > > > > > > > > IOW, that > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework > > > > > > > > that Christian pointed to is not a horrible way to see it all. Go to > > > > the top-most commit, and it's easy to follow the parent links. > > > > > > I understand that the web interface is fine for browsing the changes. > > > It isn't helpful for making comments on the changes. The discussion > > > on specific patches (e.g. selinux) may have impact on other parts of > > > the system (e.g. integrity) or be relevant elsewhere (e.g. smack). It > > > can be a real problem if the higher level mailing list (the LSM list > > > in this case) isn't included. > > > > This is probably one of those few cases where Casey and I are in > > perfect agreement. I'd much rather see the patches hit my inbox than > > have to go hunting for them and then awkwardly replying to them (and > > yes, I know there are ways to do that, I just personally find it > > annoying). I figure we are all deluged with email on a daily basis > > and have developed mechanisms to deal with that in a sane way, what is > > 29 more patches on the pile? > > Even better than the web interface, is find the message-id in any of the > emails you did get, and run > > b4 mbox 20220922151728.1557914-1-brauner@kernel.org > > In general I'd agree with sending the whole set to the lsm list, but > then one needs to start knowing which lists do and don't want the whole > set... b4 mbox and lei are now how I read all kernel related lists. In my opinion, sending the entire patchset to the relevant lists should be the default for all the reasons mentioned above. All the other methods are fine, and I don't want to stop anyone from using their favorite tool, but *requiring* the use of a separate tool to properly review and comment on patches gets us away from the email-is-universal argument. Yes, all the other tools mentioned are still based in a world of email, but if you are not emailing the relevant stakeholders directly (or indirectly via a list), you are placing another hurdle in front of the reviewers by requiring them to leave their email client based workflow and jump over to lore, b4, etc. to review the patchset. The lore.kernel.org instance is wonderful, full stop, and the b4 tool is equally wonderful, full stop, but they are tools intended to assist and optimize; they should not replace the practice of sending patches, with the full context, to the relevant parties.
On Thu, Sep 22, 2022 at 06:13:44PM -0400, Paul Moore wrote: > In my opinion, sending the entire patchset to the relevant lists > should be the default for all the reasons mentioned above. Agreed. I'm perfectly fine when people minimize the CCs to actual people (but then for the entire patch set), but having only the partial series in an mbox just makes it useful. Either the list or person on everything or nothing. I can't actually do anything with a partial CC except for either ignoring it or shoting at you that I need the entire series to do something useful with it. (although in this case I did get all of it anyway).
On Thu, Sep 22, 2022 at 10:57:38AM -0700, Linus Torvalds wrote: > On Thu, Sep 22, 2022 at 9:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > > Could we please see the entire patch set on the LSM list? > > While I don't think that's necessarily wrong, I would like to point > out that the gitweb interface actually does make it fairly easy to > just see the whole patch-set. > > IOW, that > > https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework > > that Christian pointed to is not a horrible way to see it all. Go to > the top-most commit, and it's easy to follow the parent links. > > It's a bit more work to see them in another order, but I find the > easiest way is actually to just follow the parent links to get the > overview of what is going on (reading just the commit messages), and > then after that you "reverse course" and use the browser back button > to just go the other way while looking at the details of the patches. > > And I suspect a lot of people are happier *without* large patch-sets > being posted to the mailing lists when most patches aren't necessarily > at all relevant to that mailing list except as context. The problem is also that it's impossible to please both parties here. A good portion of people doesn't like being flooded with patches they don't really care about and the other portion gets worked up when they only see a single patch. So honestly I just always make a judgement call based on the series. But b4 makes it so so easy to just retrieve the whole series. So even if I only receive a single patch and am curious then I just use b4. I've even got it integrated into mutt directly: # Pipe message to b4 to download patches and threads macro index,pager A "<pipe-message>b4 am --apply-cover-trailers --sloppy-trailers --add-my-sob --guess-base --check-newer-revisions --no-cache --quilt-ready <enter>" macro index,pager M "<pipe-message>b4 mbox <enter>"
On Thu, Sep 22, 2022 at 06:13:44PM -0400, Paul Moore wrote: > On Thu, Sep 22, 2022 at 5:57 PM Serge E. Hallyn <serge@hallyn.com> wrote: > > On Thu, Sep 22, 2022 at 03:07:44PM -0400, Paul Moore wrote: > > > On Thu, Sep 22, 2022 at 2:54 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > > On 9/22/2022 10:57 AM, Linus Torvalds wrote: > > > > > On Thu, Sep 22, 2022 at 9:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > > >> Could we please see the entire patch set on the LSM list? > > > > > While I don't think that's necessarily wrong, I would like to point > > > > > out that the gitweb interface actually does make it fairly easy to > > > > > just see the whole patch-set. > > > > > > > > > > IOW, that > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework > > > > > > > > > > that Christian pointed to is not a horrible way to see it all. Go to > > > > > the top-most commit, and it's easy to follow the parent links. > > > > > > > > I understand that the web interface is fine for browsing the changes. > > > > It isn't helpful for making comments on the changes. The discussion > > > > on specific patches (e.g. selinux) may have impact on other parts of > > > > the system (e.g. integrity) or be relevant elsewhere (e.g. smack). It > > > > can be a real problem if the higher level mailing list (the LSM list > > > > in this case) isn't included. > > > > > > This is probably one of those few cases where Casey and I are in > > > perfect agreement. I'd much rather see the patches hit my inbox than > > > have to go hunting for them and then awkwardly replying to them (and > > > yes, I know there are ways to do that, I just personally find it > > > annoying). I figure we are all deluged with email on a daily basis > > > and have developed mechanisms to deal with that in a sane way, what is > > > 29 more patches on the pile? > > > > Even better than the web interface, is find the message-id in any of the > > emails you did get, and run > > > > b4 mbox 20220922151728.1557914-1-brauner@kernel.org > > > > In general I'd agree with sending the whole set to the lsm list, but > > then one needs to start knowing which lists do and don't want the whole > > set... b4 mbox and lei are now how I read all kernel related lists. > > In my opinion, sending the entire patchset to the relevant lists > should be the default for all the reasons mentioned above. All the > other methods are fine, and I don't want to stop anyone from using > their favorite tool, but *requiring* the use of a separate tool to > properly review and comment on patches gets us away from the > email-is-universal argument. Yes, all the other tools mentioned are > still based in a world of email, but if you are not emailing the > relevant stakeholders directly (or indirectly via a list), you are > placing another hurdle in front of the reviewers by requiring them to > leave their email client based workflow and jump over to lore, b4, > etc. to review the patchset. > > The lore.kernel.org instance is wonderful, full stop, and the b4 tool > is equally wonderful, full stop, but they are tools intended to assist > and optimize; they should not replace the practice of sending patches, > with the full context, to the relevant parties. I'm happy to send all of v2 to the security mailing list. But for v1 could you compromise and just use b4? b4 mbox 20220922151728.1557914-1-brauner@kernel.org This would mean you could provide reviews for v1 and we don't need to fragment the v1 discussion because of a resend to include a mailing list.
On Fri, Sep 23, 2022 at 4:46 AM Christian Brauner <brauner@kernel.org> wrote: > On Thu, Sep 22, 2022 at 10:57:38AM -0700, Linus Torvalds wrote: > > On Thu, Sep 22, 2022 at 9:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > > > > Could we please see the entire patch set on the LSM list? > > > > While I don't think that's necessarily wrong, I would like to point > > out that the gitweb interface actually does make it fairly easy to > > just see the whole patch-set. > > > > IOW, that > > > > https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework > > > > that Christian pointed to is not a horrible way to see it all. Go to > > the top-most commit, and it's easy to follow the parent links. > > > > It's a bit more work to see them in another order, but I find the > > easiest way is actually to just follow the parent links to get the > > overview of what is going on (reading just the commit messages), and > > then after that you "reverse course" and use the browser back button > > to just go the other way while looking at the details of the patches. > > > > And I suspect a lot of people are happier *without* large patch-sets > > being posted to the mailing lists when most patches aren't necessarily > > at all relevant to that mailing list except as context. > > The problem is also that it's impossible to please both parties here. Oh the trials and tribulations of Linux Kernel development! ;) I'm joking, but I do understand the difficulty of pleasing a large group of people with very different desires. > A good portion of people doesn't like being flooded with patches they > don't really care about and the other portion gets worked up when they > only see a single patch. You are obviously never going to be able to make everyone happy, and everyone with a solution to share obviously has some bias (I'm definitely including myself in this statement), but I tend to fall back on the idea that upstream kernel development has always required those involved to deal with a large amount of email, so sending a full patchset is not new. > So honestly I just always make a judgement call based on the series. But > b4 makes it so so easy to just retrieve the whole series. So even if I > only receive a single patch and am curious then I just use b4. As I mentioned previously in this thread, the issue is more on the reply side. Reading from lore or b4 isn't terrible for me, but replying is a pain for me and my mail setup. > I've even got it integrated into mutt directly: I'm glad it works for you. Although I would like to take this opportunity to remind anyone still following this tangent that not everyone uses mutt, some of us* really dislike it, but due to the magic of email we are still able to participate with other mail clients, services, and tools. * I'm using "us" somewhat liberally here, I have no data to back up my claims. However, I'm fully prepared to accept the idea that I'm the only person out of the thousands of kernel devs who dislikes mutt. Bring it on haters, just know that you're all wrong ;)
On 9/23/2022 1:52 AM, Christian Brauner wrote: > On Thu, Sep 22, 2022 at 06:13:44PM -0400, Paul Moore wrote: >> On Thu, Sep 22, 2022 at 5:57 PM Serge E. Hallyn <serge@hallyn.com> wrote: >>> On Thu, Sep 22, 2022 at 03:07:44PM -0400, Paul Moore wrote: >>>> On Thu, Sep 22, 2022 at 2:54 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >>>>> On 9/22/2022 10:57 AM, Linus Torvalds wrote: >>>>>> On Thu, Sep 22, 2022 at 9:27 AM Casey Schaufler <casey@schaufler-ca.com> wrote: >>>>>>> Could we please see the entire patch set on the LSM list? >>>>>> While I don't think that's necessarily wrong, I would like to point >>>>>> out that the gitweb interface actually does make it fairly easy to >>>>>> just see the whole patch-set. >>>>>> >>>>>> IOW, that >>>>>> >>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework >>>>>> >>>>>> that Christian pointed to is not a horrible way to see it all. Go to >>>>>> the top-most commit, and it's easy to follow the parent links. >>>>> I understand that the web interface is fine for browsing the changes. >>>>> It isn't helpful for making comments on the changes. The discussion >>>>> on specific patches (e.g. selinux) may have impact on other parts of >>>>> the system (e.g. integrity) or be relevant elsewhere (e.g. smack). It >>>>> can be a real problem if the higher level mailing list (the LSM list >>>>> in this case) isn't included. >>>> This is probably one of those few cases where Casey and I are in >>>> perfect agreement. I'd much rather see the patches hit my inbox than >>>> have to go hunting for them and then awkwardly replying to them (and >>>> yes, I know there are ways to do that, I just personally find it >>>> annoying). I figure we are all deluged with email on a daily basis >>>> and have developed mechanisms to deal with that in a sane way, what is >>>> 29 more patches on the pile? >>> Even better than the web interface, is find the message-id in any of the >>> emails you did get, and run >>> >>> b4 mbox 20220922151728.1557914-1-brauner@kernel.org >>> >>> In general I'd agree with sending the whole set to the lsm list, but >>> then one needs to start knowing which lists do and don't want the whole >>> set... b4 mbox and lei are now how I read all kernel related lists. Because of commonalities and interactions among the various security modules, along with the ongoing efforts to enhance the infrastructure and the close ties with the vfs and audit system, it's rare that the LSM crowd isn't going to want to see the whole of a change. >> In my opinion, sending the entire patchset to the relevant lists >> should be the default for all the reasons mentioned above. All the >> other methods are fine, and I don't want to stop anyone from using >> their favorite tool, but *requiring* the use of a separate tool to >> properly review and comment on patches gets us away from the >> email-is-universal argument. Yes, all the other tools mentioned are >> still based in a world of email, but if you are not emailing the >> relevant stakeholders directly (or indirectly via a list), you are >> placing another hurdle in front of the reviewers by requiring them to >> leave their email client based workflow and jump over to lore, b4, >> etc. to review the patchset. >> >> The lore.kernel.org instance is wonderful, full stop, and the b4 tool >> is equally wonderful, full stop, but they are tools intended to assist >> and optimize; they should not replace the practice of sending patches, >> with the full context, to the relevant parties. > I'm happy to send all of v2 to the security mailing list. Thank you. > But for v1 could you compromise and just use b4? I cringe whenever someone says "just". I'm sure b4 is a fine tool. I'm told mutt is useful. Gitweb is kewl. But adopting a new and exciting development methodology every few years since about 1978 has given me a real appreciation for the raw email approach. I'll wait for v2. > > b4 mbox 20220922151728.1557914-1-brauner@kernel.org > > This would mean you could provide reviews for v1 and we don't need to > fragment the v1 discussion because of a resend to include a mailing list. Right, but I would need to learn yet another development tool set. I fully expect you'd have v2 ready before I could be sufficiently proficient with b4+mutt to contribute.
From: "Christian Brauner (Microsoft)" <brauner@kernel.org> Hey everyone, As we discussed and seen multiple times the current state of how posix acls are handled isn't nice and comes with a lot of problems. For a long and detailed explanation for just some of the issues [1] provides a good summary. The current way of handling posix acls via the generic xattr api is error prone, hard to maintain, and type unsafe for the vfs until we call into the filesystem's dedicated get and set inode operations. It is already the case that posix acls are special-cased to death all the way through the vfs. There are an uncounted number of hacks that operate on the uapi posix acl struct instead of the dedicated vfs struct posix_acl. And the vfs must be involved in order to interpret and fixup posix acls before storing them to the backing store, caching them, reporting them to userspace, or for permission checking. Currently a range of hacks and duct tape exist to make this work. As with most things this is really no ones fault it's just something that happened over time. But the code is hard to understand and difficult to maintain and one is constantly at risk of introducing bugs and regressions when having to touch it. Instead of continuing to hack posix acls through the xattr handlers this series builds a dedicated posix acl api solely around the get and set inode operations. Going forward, the vfs_get_acl(), vfs_remove_acl(), and vfs_set_acl() helpers must be used in order to interact with posix acls. They operate directly on the vfs internal struct posix_acl instead of abusing the uapi posix acl struct as we currently do. In the end this removes all of the hackiness, makes the codepaths easier to maintain, and gets us type safety. This series passes the LTP and xfstests suites without any regressions. For xfstests the following combinations were tested: * xfs * ext4 * btrfs * overlayfs * overlayfs on top of idmapped mounts For people wanting to run their own xfstests I'd recommend to shorten their test runs via: ./check -g acl,attr,cap,idmapped,io_uring,perms,subvol,unlink I would appreciate if the 9p and cifs folks could run any posix acl related tests as I have no setup to really do this without causing me a lot of pain. Very likely there's a lot more simplifications for posix acls that we can make in the future if the basic api has made it. A few implementation details: * The series makes sure to retain exactly the same security and integrity module permission checks. See [2] for annotated callchains. Especially for the integrity modules this api is a win because right now they convert the uapi posix acl struct passed to them via a void pointer into the vfs struct posix_acl format to perform permission checking on the mode. There's a new dedicated security hook for setting posix acls which passes the vfs struct posix_acl not a void pointer. Basing checking on the posix acl stored in the uapi format is really unreliable. The vfs currently hacks around directly in the uapi struct storing values that frankly the security and integrity modules can't correctly interpret as evidenced by bugs we reported and fixed in this area. It's not necessarily even their fault it's just that the format we provide to them is sub optimal. * Some filesystems like 9p and cifs need access to the dentry in order to get and set posix acls which is why they either only partially or not even at all implement get and set inode operations. For example, cifs allows setxattr() and getxattr() operations but doesn't allow permission checking based on posix acls because it can't implement a get acl inode operation. Thus, this patch series updates the set acl inode operation to take a dentry instead of an inode argument. However, for the get acl inode operation we can't do this as the old get acl method is called in e.g., generic_permission() and inode_permission(). These helpers in turn are called in various filesystem's permission inode operation. So passing a dentry argument to the old get acl inode operation would amount to passing a dentry to the permission inode operation which we shouldn't and probably can't do. So instead of extending the existing inode operation Christoph suggested to add a new one. He also requested to ensure that the get and set acl inode operation taking a dentry are consistently named. So for this version the old get acl operation is renamed to ->get_inode_acl() and a new ->get_acl() inode operation taking a dentry is added. With this we can give both 9p and cifs get and set acl inode operations and in turn remove their complex custom posix xattr handlers. * I've done a full audit of every codepaths using variant of the current generic xattr api to get and set posix acls and surprisingly it isn't that many places. There's of course always a chance that I might have missed some and I'm sure we'll find them soon enough. The crucial codepaths to be converted are obviously stacking filesystems such as ecryptfs and overlayfs. For a list of all callers currently using generic xattr api helpers see [2] including comments whether they support posix acls or not. * The old vfs generic posix acl infrastructure doesn't obey the create and replace semantics promised on the setxattr(2) manpage. This patch series doesn't address this. It really is something we should revisit later though. The patch series is roughly organized as follows: // intended to be a non-functional change 1. Change existing set acl inode operation to take a dentry argument. // intended to be a non-functional change 2. Rename existing get acl method. // intended to be a non-functional change 3. Implement get and set acl inode operations for filesystems that couldn't implement one before because of the missing dentry. That's mostly 9p and cifs. // intended to be a non-functional change 4. Build posix acl api, i.e., add vfs_get_acl(), vfs_remove_acl(), and vfs_set_acl() including security and integrity hooks. // intended to be a non-functional change 5. Implement get and set acl inode operations for stacking filesystems. // semantical change 6. Switch posix acl handling in stacking filesystems to new posix acl api now that all filesystems it can stack upon support it. // semantical change 7. Switch vfs to new posix acl api 8. Remove all now unused helpers The series can be pulled from: https://gitlab.com/brauner/linux/-/commits/fs.acl.rework https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git/log/?h=fs.acl.rework The series contains a few preliminary patches which are scheduled for the next merge window. It was just easier to base the series on top of them. But if you pull this branch you'll get them included. I've been working on this for a while and before going any further it'd be nice to get some reviews. I think that it should be fine to have get and set acl inode operations that operate on the dentry at least nothing stuck out immediately that would prevent this. But obviously having other people point out issues with that would be helpful. Thanks to Seth for a lot of good discussion around this and encouragement and input from Christoph. [1]: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [2]: https://gist.github.com/brauner/12c795b93a05dc3b3056b1982549a633 Thanks! Christian Christian Brauner (29): fs: pass dentry to set acl method fs: rename current get acl method fs: add new get acl method cifs: implement get acl method cifs: implement set acl method 9p: implement get acl method 9p: implement set acl method acl: add vfs_set_acl() security: add set acl hook selinux: implement set acl hook smack: implement set acl hook evm: implement set acl hook acl: use set acl hook evm: add post set acl hook acl: add vfs_get_acl() acl: add vfs_remove_acl() evm: simplify evm_xattr_acl_change() ksmbd: use vfs_remove_acl() ecryptfs: implement get acl method ecryptfs: implement set acl method ovl: implement get acl method ovl: implement set acl method ovl: use posix acl api xattr: use posix acl api ecryptfs: use stub posix acl handlers ovl: use stub posix acl handlers cifs: use stub posix acl handlers 9p: use stub posix acl handlers acl: remove a slew of now unused helpers Documentation/filesystems/locking.rst | 4 +- Documentation/filesystems/porting.rst | 4 +- Documentation/filesystems/vfs.rst | 3 +- fs/9p/acl.c | 307 ++++++------ fs/9p/acl.h | 19 +- fs/9p/vfs_inode_dotl.c | 4 + fs/9p/xattr.c | 7 +- fs/9p/xattr.h | 2 - fs/bad_inode.c | 4 +- fs/btrfs/acl.c | 3 +- fs/btrfs/ctree.h | 2 +- fs/btrfs/inode.c | 8 +- fs/ceph/acl.c | 3 +- fs/ceph/dir.c | 2 +- fs/ceph/inode.c | 4 +- fs/ceph/super.h | 2 +- fs/cifs/cifsacl.c | 137 +++++ fs/cifs/cifsfs.c | 4 + fs/cifs/cifsproto.h | 20 +- fs/cifs/cifssmb.c | 236 +++++---- fs/cifs/xattr.c | 68 +-- fs/ecryptfs/inode.c | 32 ++ fs/erofs/inode.c | 6 +- fs/erofs/namei.c | 2 +- fs/ext2/acl.c | 3 +- fs/ext2/acl.h | 2 +- fs/ext2/file.c | 2 +- fs/ext2/inode.c | 2 +- fs/ext2/namei.c | 4 +- fs/ext4/acl.c | 3 +- fs/ext4/acl.h | 2 +- fs/ext4/file.c | 2 +- fs/ext4/inode.c | 2 +- fs/ext4/namei.c | 4 +- fs/f2fs/acl.c | 4 +- fs/f2fs/acl.h | 2 +- fs/f2fs/file.c | 4 +- fs/f2fs/namei.c | 4 +- fs/fuse/acl.c | 3 +- fs/fuse/dir.c | 4 +- fs/fuse/fuse_i.h | 2 +- fs/gfs2/acl.c | 3 +- fs/gfs2/acl.h | 2 +- fs/gfs2/inode.c | 6 +- fs/internal.h | 1 + fs/jffs2/acl.c | 3 +- fs/jffs2/acl.h | 2 +- fs/jffs2/dir.c | 2 +- fs/jffs2/file.c | 2 +- fs/jffs2/fs.c | 2 +- fs/jfs/acl.c | 3 +- fs/jfs/file.c | 4 +- fs/jfs/jfs_acl.h | 2 +- fs/jfs/namei.c | 2 +- fs/ksmbd/smb2pdu.c | 4 +- fs/ksmbd/smbacl.c | 4 +- fs/ksmbd/vfs.c | 17 +- fs/ksmbd/vfs.h | 4 +- fs/namei.c | 2 +- fs/nfs/nfs3_fs.h | 2 +- fs/nfs/nfs3acl.c | 3 +- fs/nfs/nfs3proc.c | 4 +- fs/nfsd/nfs2acl.c | 4 +- fs/nfsd/nfs3acl.c | 4 +- fs/nfsd/vfs.c | 4 +- fs/ntfs3/file.c | 4 +- fs/ntfs3/namei.c | 4 +- fs/ntfs3/ntfs_fs.h | 4 +- fs/ntfs3/xattr.c | 9 +- fs/ocfs2/acl.c | 3 +- fs/ocfs2/acl.h | 2 +- fs/ocfs2/file.c | 4 +- fs/ocfs2/namei.c | 2 +- fs/orangefs/acl.c | 47 +- fs/orangefs/inode.c | 47 +- fs/orangefs/namei.c | 2 +- fs/orangefs/orangefs-kernel.h | 9 +- fs/orangefs/orangefs-utils.c | 12 +- fs/overlayfs/copy_up.c | 9 + fs/overlayfs/dir.c | 22 +- fs/overlayfs/inode.c | 140 +++++- fs/overlayfs/overlayfs.h | 36 +- fs/overlayfs/super.c | 107 +--- fs/overlayfs/util.c | 42 ++ fs/posix_acl.c | 688 +++++++++++++------------- fs/reiserfs/acl.h | 6 +- fs/reiserfs/file.c | 2 +- fs/reiserfs/inode.c | 2 +- fs/reiserfs/namei.c | 4 +- fs/reiserfs/xattr_acl.c | 9 +- fs/xattr.c | 78 ++- fs/xfs/xfs_acl.c | 3 +- fs/xfs/xfs_acl.h | 2 +- fs/xfs/xfs_iops.c | 16 +- include/linux/evm.h | 22 + include/linux/fs.h | 10 +- include/linux/lsm_hook_defs.h | 2 + include/linux/lsm_hooks.h | 4 + include/linux/posix_acl.h | 35 +- include/linux/posix_acl_xattr.h | 43 +- include/linux/security.h | 11 + include/linux/xattr.h | 8 + io_uring/xattr.c | 2 + mm/shmem.c | 2 +- security/integrity/evm/evm_main.c | 128 +++-- security/security.c | 16 + security/selinux/hooks.c | 8 + security/smack/smack_lsm.c | 24 + 108 files changed, 1586 insertions(+), 1062 deletions(-) base-commit: 38e316398e4e6338b80223fb5f74415c0513718f