[PATCH] fs: don't call posix_acl_listxattr in generic_listxattr

[PATCH] fs: don't call posix_acl_listxattr in generic_listxattr

[Jeff Layton]

Commit f2620f166e2a caused the kernel to start emitting POSIX ACL xattrs
for NFSv4 inodes, which it doesn't support. The only other user of
generic_listxattr is HFS (classic) and it doesn't support POSIX ACLs
either.

Fixes: f2620f166e2a xattr: simplify listxattr helpers
Reported-by: Ondrej Valousek <ondrej.valousek.xm@renesas.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/xattr.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index fcf67d80d7f9..e7bbb7f57557 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -985,9 +985,16 @@ int xattr_list_one(char **buffer, ssize_t *remaining_size, const char *name)
 	return 0;
 }
 
-/*
+/**
+ * generic_listxattr - run through a dentry's xattr list() operations
+ * @dentry: dentry to list the xattrs
+ * @buffer: result buffer
+ * @buffer_size: size of @buffer
+ *
  * Combine the results of the list() operation from every xattr_handler in the
- * list.
+ * xattr_handler stack.
+ *
+ * Note that this will not include the entries for POSIX ACLs.
  */
 ssize_t
 generic_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
@@ -996,10 +1003,6 @@ generic_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
 	ssize_t remaining_size = buffer_size;
 	int err = 0;
 
-	err = posix_acl_listxattr(d_inode(dentry), &buffer, &remaining_size);
-	if (err)
-		return err;
-
 	for_each_xattr_handler(handlers, handler) {
 		if (!handler->name || (handler->list && !handler->list(dentry)))
 			continue;
-- 
2.40.1

Re: [PATCH] fs: don't call posix_acl_listxattr in generic_listxattr

[Christian Brauner]

On Tue, 16 May 2023 08:46:54 -0400, Jeff Layton wrote:
> Commit f2620f166e2a caused the kernel to start emitting POSIX ACL xattrs
> for NFSv4 inodes, which it doesn't support. The only other user of
> generic_listxattr is HFS (classic) and it doesn't support POSIX ACLs
> either.
> 
> 

Applied to the vfs.misc.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.misc.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc.fixes

[1/1] fs: don't call posix_acl_listxattr in generic_listxattr
      https://git.kernel.org/vfs/vfs/c/f3689fa785f0

Re: A pass-through support for NFSv4 style ACL

[Jeff Layton]

On Tue, 2023-05-16 at 20:50 +0000, Ondrej Valousek wrote:
> 
> Hi Christian,
> 
> Would it be possible to patch kernel the way it accepts native (i.e no
> conversion to Posix ACL) NFSv4 style ACLs for filesystems that can
> support them?
> I.E. OpenZFS, NTFS, could be also interesting for Microsofts WSL2  or
> Samba right?
> 
> I mean, I am not trying to push richacl again knowing they have been
> rejected, but just NFS4 style Acls as they are so similar to Windows
> ACLs.
> 

Erm, except you kind of are if you want to do this. I don't see how this
idea works unless you resurrect RichACLs or something like them.

> The idea here would be that we could
> - mount NTFS/ZFS filesystem and inspect ACLs using existing tools
> (nfs4_getacl)
> - share with NFSv4 in a pass through mode
> - in Windows WSL2 we could inspect local filesystem ACLs using the
> same tools
> 
> Does it make any sense or it would require lot of changes to VFS
> subsystem or its a nonsense altogether?
> 

Eventually you have to actually enforce the ACL. Do NTFS/ZFS already
have code to do this? If not then someone would need to write it.

Also windows and nfs acls do have some differences, so you'll need a
translation layer too.
-- 
Jeff Layton <jlayton@kernel.org>

Re: A pass-through support for NFSv4 style ACL

[Christian Brauner]

On Tue, May 16, 2023 at 05:22:30PM -0400, Jeff Layton wrote:
> On Tue, 2023-05-16 at 20:50 +0000, Ondrej Valousek wrote:
> > 
> > Hi Christian,
> > 
> > Would it be possible to patch kernel the way it accepts native (i.e no
> > conversion to Posix ACL) NFSv4 style ACLs for filesystems that can
> > support them?
> > I.E. OpenZFS, NTFS, could be also interesting for Microsofts WSL2  or
> > Samba right?
> > 
> > I mean, I am not trying to push richacl again knowing they have been
> > rejected, but just NFS4 style Acls as they are so similar to Windows
> > ACLs.
> > 
> 
> Erm, except you kind of are if you want to do this. I don't see how this
> idea works unless you resurrect RichACLs or something like them.

I have no idea about the original flame war that ended RichACLs in
additition to having no clear clue what RichACLs are supposed to
achieve. My current knowledge extends to "Christoph didn't like them".

> 
> > The idea here would be that we could
> > - mount NTFS/ZFS filesystem and inspect ACLs using existing tools
> > (nfs4_getacl)
> > - share with NFSv4 in a pass through mode
> > - in Windows WSL2 we could inspect local filesystem ACLs using the
> > same tools
> > 
> > Does it make any sense or it would require lot of changes to VFS
> > subsystem or its a nonsense altogether?

Yes, very likely.

We'd either have to change the current inode operations for getting and
setting acls to take a new struct acl that can contain either posix acls
or rich acls or add new ones just for these new fangled ones.

Choosing the first - more sensible - of these two options will mean
updating each filesystem's acl inode operations. Might turn out to not
be invasive code as it might boil down to struct posix_acl *acl =
acl->posix at the beginning of each method but still.

Then we'd probably also need to:

* handle permission checking (see Jeff's comment below)
* change/update the ACL caching layer
* if the past hast taught me anything then overlayfs would probably need
  some additional logic as well

> > 
> 
> Eventually you have to actually enforce the ACL. Do NTFS/ZFS already
> have code to do this? If not then someone would need to write it.
> 
> Also windows and nfs acls do have some differences, so you'll need a
> translation layer too.

Jeff, I know you have some knowledge in this area you probably are
better equipped to judge the sanity and feasibility of this.

Re: A pass-through support for NFSv4 style ACL

[Christoph Hellwig]

On Wed, May 17, 2023 at 09:42:59AM +0200, Christian Brauner wrote:
> I have no idea about the original flame war that ended RichACLs in
> additition to having no clear clue what RichACLs are supposed to
> achieve. My current knowledge extends to "Christoph didn't like them".

Christoph certainly doesn't like Rich ACLs, as do many other people.

But the deal block was that the patchset:

 - totally duplicated the VFS level ACL handling instead of having
   a common object for Posix and the new ACLs
 - did add even more mess to the already horrible xattr interface
   instead of adding syscalls.

Re: A pass-through support for NFSv4 style ACL

[Christian Brauner]

On Wed, May 17, 2023 at 12:45:13AM -0700, Christoph Hellwig wrote:
> On Wed, May 17, 2023 at 09:42:59AM +0200, Christian Brauner wrote:
> > I have no idea about the original flame war that ended RichACLs in
> > additition to having no clear clue what RichACLs are supposed to
> > achieve. My current knowledge extends to "Christoph didn't like them".
> 
> Christoph certainly doesn't like Rich ACLs, as do many other people.
> 
> But the deal block was that the patchset:
> 
>  - totally duplicated the VFS level ACL handling instead of having
>    a common object for Posix and the new ACLs

Which seems like a pretty obvious choice... That was the first thing I
thought of doing (see earlier mail).

>  - did add even more mess to the already horrible xattr interface
>    instead of adding syscalls.

Plus that was before I moved POSIX ACLs out of the xattr handlers so
they would've had to get that work done first for this to not end up a
horrible horrible mess...

RE: A pass-through support for NFSv4 style ACL

[Ondrej Valousek]

Ok thanks guys for input.

I do not like richacl, too - for reasons mentioned here:
http://michael.orlitzky.com/articles/there_was_an_attempt_to_save_linux_filesystem_acls.xhtml
The "mask" hack in Posix ACLs and RichACLs is something that nobody really understands.
 I would (and not only me) like to have a clean NFSv4 acls implemented - just like Netapp, OmniOS, Solaris or FreeBSD.
The similarity with Windows ACLs is just another benefit. Yes there are small differences (mainly the OWNER@ and GROUP@ special principals there but I am sure it would be possible to resolve)

As per:
https://github.com/openzfs/zfs/issues/4966
I got impression the code on the ZFS is pretty much ready (including the XDR translation), but not sure if it's actually enforcing acls, probably not as it's task for VFS right?

Anyhow, pity that it's complicated to implement, it would be nice to have a functioning NFSv4 server with ACLs running on Linux.

Ondrej
 

-----Original Message-----
From: Christian Brauner <brauner@kernel.org> 
Sent: Mittwoch, 17. Mai 2023 09:43
To: Jeff Layton <jlayton@kernel.org>
Cc: Ondrej Valousek <ondrej.valousek.xm@renesas.com>; trondmy@hammerspace.com; linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: A pass-through support for NFSv4 style ACL

On Tue, May 16, 2023 at 05:22:30PM -0400, Jeff Layton wrote:
> On Tue, 2023-05-16 at 20:50 +0000, Ondrej Valousek wrote:
> > 
> > Hi Christian,
> > 
> > Would it be possible to patch kernel the way it accepts native (i.e 
> > no conversion to Posix ACL) NFSv4 style ACLs for filesystems that 
> > can support them?
> > I.E. OpenZFS, NTFS, could be also interesting for Microsofts WSL2  
> > or Samba right?
> > 
> > I mean, I am not trying to push richacl again knowing they have been 
> > rejected, but just NFS4 style Acls as they are so similar to Windows 
> > ACLs.
> > 
> 
> Erm, except you kind of are if you want to do this. I don't see how 
> this idea works unless you resurrect RichACLs or something like them.

I have no idea about the original flame war that ended RichACLs in additition to having no clear clue what RichACLs are supposed to achieve. My current knowledge extends to "Christoph didn't like them".

> 
> > The idea here would be that we could
> > - mount NTFS/ZFS filesystem and inspect ACLs using existing tools
> > (nfs4_getacl)
> > - share with NFSv4 in a pass through mode
> > - in Windows WSL2 we could inspect local filesystem ACLs using the 
> > same tools
> > 
> > Does it make any sense or it would require lot of changes to VFS 
> > subsystem or its a nonsense altogether?

Yes, very likely.

We'd either have to change the current inode operations for getting and setting acls to take a new struct acl that can contain either posix acls or rich acls or add new ones just for these new fangled ones.

Choosing the first - more sensible - of these two options will mean updating each filesystem's acl inode operations. Might turn out to not be invasive code as it might boil down to struct posix_acl *acl =
acl->posix at the beginning of each method but still.

Then we'd probably also need to:

* handle permission checking (see Jeff's comment below)
* change/update the ACL caching layer
* if the past hast taught me anything then overlayfs would probably need
  some additional logic as well

> > 
> 
> Eventually you have to actually enforce the ACL. Do NTFS/ZFS already 
> have code to do this? If not then someone would need to write it.
> 
> Also windows and nfs acls do have some differences, so you'll need a 
> translation layer too.

Jeff, I know you have some knowledge in this area you probably are better equipped to judge the sanity and feasibility of this.

Re: A pass-through support for NFSv4 style ACL

[Jeff Layton]

On Wed, 2023-05-17 at 09:42 +0200, Christian Brauner wrote:
> On Tue, May 16, 2023 at 05:22:30PM -0400, Jeff Layton wrote:
> > On Tue, 2023-05-16 at 20:50 +0000, Ondrej Valousek wrote:
> > > 
> > > Hi Christian,
> > > 
> > > Would it be possible to patch kernel the way it accepts native (i.e no
> > > conversion to Posix ACL) NFSv4 style ACLs for filesystems that can
> > > support them?
> > > I.E. OpenZFS, NTFS, could be also interesting for Microsofts WSL2  or
> > > Samba right?
> > > 
> > > I mean, I am not trying to push richacl again knowing they have been
> > > rejected, but just NFS4 style Acls as they are so similar to Windows
> > > ACLs.
> > > 
> > 
> > Erm, except you kind of are if you want to do this. I don't see how this
> > idea works unless you resurrect RichACLs or something like them.
> 
> I have no idea about the original flame war that ended RichACLs in
> additition to having no clear clue what RichACLs are supposed to
> achieve. My current knowledge extends to "Christoph didn't like them".
> 
> > 
> > > The idea here would be that we could
> > > - mount NTFS/ZFS filesystem and inspect ACLs using existing tools
> > > (nfs4_getacl)
> > > - share with NFSv4 in a pass through mode
> > > - in Windows WSL2 we could inspect local filesystem ACLs using the
> > > same tools
> > > 
> > > Does it make any sense or it would require lot of changes to VFS
> > > subsystem or its a nonsense altogether?
> 
> Yes, very likely.
> 
> We'd either have to change the current inode operations for getting and
> setting acls to take a new struct acl that can contain either posix acls
> or rich acls or add new ones just for these new fangled ones.
> 
> Choosing the first - more sensible - of these two options will mean
> updating each filesystem's acl inode operations. Might turn out to not
> be invasive code as it might boil down to struct posix_acl *acl =
> acl->posix at the beginning of each method but still.
> 
> Then we'd probably also need to:
> 
> * handle permission checking (see Jeff's comment below)
> * change/update the ACL caching layer
> * if the past hast taught me anything then overlayfs would probably need
>   some additional logic as well
> 

Yeah, it's a significant project.

> > > 
> > 
> > Eventually you have to actually enforce the ACL. Do NTFS/ZFS already
> > have code to do this? If not then someone would need to write it.
> > 
> > Also windows and nfs acls do have some differences, so you'll need a
> > translation layer too.
> 
> Jeff, I know you have some knowledge in this area you probably are
> better equipped to judge the sanity and feasibility of this.

I know a bit, but Andreas (cc'ed) is the undisputed expert. If you're
looking to resurrect this effort, then you should definitely loop him
in.

-- 
Jeff Layton <jlayton@kernel.org>

Re: A pass-through support for NFSv4 style ACL

[Theodore Ts'o]

On Wed, May 17, 2023 at 09:42:59AM +0200, Christian Brauner wrote:
> 
> I have no idea about the original flame war that ended RichACLs in
> additition to having no clear clue what RichACLs are supposed to
> achieve. My current knowledge extends to "Christoph didn't like them".

As to what RichACL's are supposed to achieve....

Windows/NFSv4 -style ACL's are very different from POSIX semantics, in
a gazillion ways.  For example, if you set a top-level acl, it will
automatically affect all of the ACL's in the subhierarcy.  This is
trivially easy in Windows given that apparently ACL's are evaluated by
path every time you try to operate on a file (or at least, that's how
it works effectively; having not taken a look at Windows source code,
I can't vouch for how it is actually implemented.)  This is, of
course, a performance disaster and doesn't work all that well for
Linux where we can do things like like fchdir() and use O_PATH file
descriptors and *at() system calls.  Moreover, Windows doesn't have
things like the mode parameter to open(2) and mkdir(2) system calls.

As a result, RichACL's are quite a bit more complicated than Posix
ACL's or the Windows-style ACL's from which they were derived because
they have to compromise between the Windows authorization model and
the Posix/Linux authorization model while being expressive enough
to mostly emulate Windows-style ACL's.  For example, instead of
implementing Windows-style "automatic inheritance", setrichacl(1) will
do the moral equivalent of chmod -R, and then add a lot of hair in the
form of "file_inherit, dir_inherit, no_propagate, and inherit_only"
flags to each ACL entry, which are all there to try to mostly (but not
completely) handle make Windows-style and Linux/POSIX acl's work
within the same file system.  There's a lot more detail of the hair
documented here[1].

[1] https://www.systutorials.com/docs/linux/man/7-richacl/

I'll note most of this complexity is only necessary if you want to
have local file access to the file system work with similar semantics
as what would get exported via NFSv4.  If you didn't, you could just
store the Windows-style ACL in an xattr and just let it be set via the
remote file system, and return it when the remote file system queries
it.  The problem comes when you want to have "RichACLs" actually
influence the local Linux permissions check.

Personally, I think this is rarely needed, since (a) most people
implementing a Windows-style filer for Windows client are doing so on
a dedicated file server, and local access to the exported files are
rarely needed.  Secondly, (b) Windows-style access need to be
expressed in terms of Windows-style Security ID's for large complex
enterprise deployment where you have nested Windows-style domains, and
how you map a 128+ bit Windows SID to a local Unix UID is a previously
unsolved problem.

So it's a pretty rare niche use case, and *I've* never thought that
this is all that important.  On the other hand, for those people who
believe that "This is the year of the Linux Desktop", and who might
want to export their local Linux directories on their Desktop to a set
of peer Windows clients, and who are using complex Windows-style ACL's
(but not *too* complex such that they have nested domain identifiers),
then RichACL's might be just the ticket.  Furthermore, RichACL's are
apparently an out-of-tree patch maintained by some distros in their
distro kernel, so maybe they know something I don't.

Add to that the issues of the implementation level concerns which
Cristoph has already described, and it's really not all that
surprising that the progress on the patchset kind of stalled....

	   	    	     	    - Ted

Re: A pass-through support for NFSv4 style ACL

[Christian Brauner]

On Wed, May 17, 2023 at 08:39:14AM -0400, Theodore Ts'o wrote:
> On Wed, May 17, 2023 at 09:42:59AM +0200, Christian Brauner wrote:
> > 
> > I have no idea about the original flame war that ended RichACLs in
> > additition to having no clear clue what RichACLs are supposed to
> > achieve. My current knowledge extends to "Christoph didn't like them".
> 
> As to what RichACL's are supposed to achieve....

Interesting, thanks for all the details!

> 
> Windows/NFSv4 -style ACL's are very different from POSIX semantics, in
> a gazillion ways.  For example, if you set a top-level acl, it will
> automatically affect all of the ACL's in the subhierarcy.  This is
> trivially easy in Windows given that apparently ACL's are evaluated by
> path every time you try to operate on a file (or at least, that's how
> it works effectively; having not taken a look at Windows source code,
> I can't vouch for how it is actually implemented.)  This is, of
> course, a performance disaster and doesn't work all that well for
> Linux where we can do things like like fchdir() and use O_PATH file
> descriptors and *at() system calls.  Moreover, Windows doesn't have
> things like the mode parameter to open(2) and mkdir(2) system calls.
> 
> As a result, RichACL's are quite a bit more complicated than Posix
> ACL's or the Windows-style ACL's from which they were derived because
> they have to compromise between the Windows authorization model and
> the Posix/Linux authorization model while being expressive enough
> to mostly emulate Windows-style ACL's.  For example, instead of
> implementing Windows-style "automatic inheritance", setrichacl(1) will
> do the moral equivalent of chmod -R, and then add a lot of hair in the
> form of "file_inherit, dir_inherit, no_propagate, and inherit_only"
> flags to each ACL entry, which are all there to try to mostly (but not
> completely) handle make Windows-style and Linux/POSIX acl's work
> within the same file system.  There's a lot more detail of the hair
> documented here[1].
> 
> [1] https://www.systutorials.com/docs/linux/man/7-richacl/
> 
> I'll note most of this complexity is only necessary if you want to
> have local file access to the file system work with similar semantics
> as what would get exported via NFSv4.  If you didn't, you could just
> store the Windows-style ACL in an xattr and just let it be set via the
> remote file system, and return it when the remote file system queries
> it.  The problem comes when you want to have "RichACLs" actually
> influence the local Linux permissions check.

Yeah, I'm already scared enough.

RE: A pass-through support for NFSv4 style ACL

[Ondrej Valousek]

> 
> I'll note most of this complexity is only necessary if you want to 
> have local file access to the file system work with similar semantics 
> as what would get exported via NFSv4.  If you didn't, you could just 
> store the Windows-style ACL in an xattr and just let it be set via the 
> remote file system, and return it when the remote file system queries 
> it.  The problem comes when you want to have "RichACLs" actually 
> influence the local Linux permissions check.

> Yeah, I'm already scared enough.

Well I do not think it's that difficult. As I said, just take a look how OmniOS does things, very nice - you can set up a VM with it in just a half an hour and you get a system with ZFS and native NFSv4 working.
True it's not Richacl, but just NFSv4 style acl - even better.

As for the implementation, lot of code could be presumably taken from Samba which is already doing Windows style-ACL to NFSv4 translation.

To me interesting bit was that the original path from Andreas was not accepted largely because it would add another piece of mess to the already messy code in the kernel, I did not know that.
I hoped that  now that Christian cleaned the code recently, it would perhaps allow us to reconsider things, but maybe I am too naive here 😊

Re: A pass-through support for NFSv4 style ACL

[Christian Brauner]

On Fri, May 19, 2023 at 11:38:30AM +0000, Ondrej Valousek wrote:
> > 
> > I'll note most of this complexity is only necessary if you want to 
> > have local file access to the file system work with similar semantics 
> > as what would get exported via NFSv4.  If you didn't, you could just 
> > store the Windows-style ACL in an xattr and just let it be set via the 
> > remote file system, and return it when the remote file system queries 
> > it.  The problem comes when you want to have "RichACLs" actually 
> > influence the local Linux permissions check.
> 
> > Yeah, I'm already scared enough.
> 
> Well I do not think it's that difficult. As I said, just take a look how OmniOS does things, very nice - you can set up a VM with it in just a half an hour and you get a system with ZFS and native NFSv4 working.
> True it's not Richacl, but just NFSv4 style acl - even better.
> 
> As for the implementation, lot of code could be presumably taken from Samba which is already doing Windows style-ACL to NFSv4 translation.
> 
> To me interesting bit was that the original path from Andreas was not accepted largely because it would add another piece of mess to the already messy code in the kernel, I did not know that.
> I hoped that  now that Christian cleaned the code recently, it would perhaps allow us to reconsider things, but maybe I am too naive here 😊

Noo one is going to stop you from writing the code and posting it on the
list. But I think none of us here will be very eager to implement it. If
it can be done cleanly without performance regressions or unwiedly
complications in the generic lookup and permission checking code and
both posix acls and these nfs4 style acls can be abstracted away nicely
in a single file, and have well-defined semantics and there's a clear
use-case that isn't just someone's hobby project then it might be
considered. But it might also mean you've spent significant effort just
to hear a no in the end.

RE: [PATCH] fs: don't call posix_acl_listxattr in generic_listxattr

[Ondrej Valousek]

Hi Jeff,

I can confirm that with rawhide kernel 6.5 the error is gone, i.e.

Listxattr() shows only "system.nfs4_acl" attribute on NFSv4 filesystem,

Problem is, that (on the same kernel) getxattr(name,XATTR_NAME_POSIX_ACL_ACCESS, 0,0)
Sets errno to ENODATA where "name" is file on NFSv4.

This is different behavior to the previous versions, i.e. on RHEL8 getxattr() sets errno to ENOTSUP in the same scenario - which is what I'd expect more.

Is the change of the getxattr() behavior expected or not?

Thanks,
Ondrej

-----Original Message-----
From: Jeff Layton <jlayton@kernel.org> 
Sent: Dienstag, 16. Mai 2023 14:47
To: Alexander Viro <viro@zeniv.linux.org.uk>; Christian Brauner <brauner@kernel.org>
Cc: trondmy@hammerspace.com; eggert@cs.ucla.edu; bruno@clisp.org; Ondrej Valousek <ondrej.valousek.xm@renesas.com>; linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: [PATCH] fs: don't call posix_acl_listxattr in generic_listxattr

Commit f2620f166e2a caused the kernel to start emitting POSIX ACL xattrs for NFSv4 inodes, which it doesn't support. The only other user of generic_listxattr is HFS (classic) and it doesn't support POSIX ACLs either.

Fixes: f2620f166e2a xattr: simplify listxattr helpers
Reported-by: Ondrej Valousek <ondrej.valousek.xm@renesas.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/xattr.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index fcf67d80d7f9..e7bbb7f57557 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -985,9 +985,16 @@ int xattr_list_one(char **buffer, ssize_t *remaining_size, const char *name)
 	return 0;
 }
 
-/*
+/**
+ * generic_listxattr - run through a dentry's xattr list() operations
+ * @dentry: dentry to list the xattrs
+ * @buffer: result buffer
+ * @buffer_size: size of @buffer
+ *
  * Combine the results of the list() operation from every xattr_handler in the
- * list.
+ * xattr_handler stack.
+ *
+ * Note that this will not include the entries for POSIX ACLs.
  */
 ssize_t
 generic_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size) @@ -996,10 +1003,6 @@ generic_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
 	ssize_t remaining_size = buffer_size;
 	int err = 0;
 
-	err = posix_acl_listxattr(d_inode(dentry), &buffer, &remaining_size);
-	if (err)
-		return err;
-
 	for_each_xattr_handler(handlers, handler) {
 		if (!handler->name || (handler->list && !handler->list(dentry)))
 			continue;
--
2.40.1

Re: [PATCH] fs: don't call posix_acl_listxattr in generic_listxattr

[Jeff Layton]

On Mon, 2023-09-04 at 20:36 +0000, Ondrej Valousek wrote:
> Hi Jeff,
> 
> I can confirm that with rawhide kernel 6.5 the error is gone, i.e.
> 
> Listxattr() shows only "system.nfs4_acl" attribute on NFSv4 filesystem,
> 
> Problem is, that (on the same kernel) getxattr(name,XATTR_NAME_POSIX_ACL_ACCESS, 0,0)
> Sets errno to ENODATA where "name" is file on NFSv4.
> 
> This is different behavior to the previous versions, i.e. on RHEL8 getxattr() sets errno to ENOTSUP in the same scenario - which is what I'd expect more.
> 
> Is the change of the getxattr() behavior expected or not?
> 
> Thanks,
> Ondrej
> 

I'd say not. I've been working through xfstests failures on NFS and
didn't realize that this was a regression, and posted this fstests patch
recently:

    https://lore.kernel.org/fstests/20230830-fixes-v4-1-88d7b8572aa3@kernel.org/

It would be nice if getxattr were also fixed here.

Cheers,
Jeff

> -----Original Message-----
> From: Jeff Layton <jlayton@kernel.org> 
> Sent: Dienstag, 16. Mai 2023 14:47
> To: Alexander Viro <viro@zeniv.linux.org.uk>; Christian Brauner <brauner@kernel.org>
> Cc: trondmy@hammerspace.com; eggert@cs.ucla.edu; bruno@clisp.org; Ondrej Valousek <ondrej.valousek.xm@renesas.com>; linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH] fs: don't call posix_acl_listxattr in generic_listxattr
> 
> Commit f2620f166e2a caused the kernel to start emitting POSIX ACL xattrs for NFSv4 inodes, which it doesn't support. The only other user of generic_listxattr is HFS (classic) and it doesn't support POSIX ACLs either.
> 
> Fixes: f2620f166e2a xattr: simplify listxattr helpers
> Reported-by: Ondrej Valousek <ondrej.valousek.xm@renesas.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/xattr.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xattr.c b/fs/xattr.c
> index fcf67d80d7f9..e7bbb7f57557 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -985,9 +985,16 @@ int xattr_list_one(char **buffer, ssize_t *remaining_size, const char *name)
>  	return 0;
>  }
>  
> -/*
> +/**
> + * generic_listxattr - run through a dentry's xattr list() operations
> + * @dentry: dentry to list the xattrs
> + * @buffer: result buffer
> + * @buffer_size: size of @buffer
> + *
>   * Combine the results of the list() operation from every xattr_handler in the
> - * list.
> + * xattr_handler stack.
> + *
> + * Note that this will not include the entries for POSIX ACLs.
>   */
>  ssize_t
>  generic_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size) @@ -996,10 +1003,6 @@ generic_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
>  	ssize_t remaining_size = buffer_size;
>  	int err = 0;
>  
> -	err = posix_acl_listxattr(d_inode(dentry), &buffer, &remaining_size);
> -	if (err)
> -		return err;
> -
>  	for_each_xattr_handler(handlers, handler) {
>  		if (!handler->name || (handler->list && !handler->list(dentry)))
>  			continue;
> --
> 2.40.1
> 

-- 
Jeff Layton <jlayton@kernel.org>

RE: [PATCH] fs: don't call posix_acl_listxattr in generic_listxattr

[Ondrej Valousek]

Ok, filed BZ for it, will see:
https://bugzilla.redhat.com/show_bug.cgi?id=2237410

-----Original Message-----
From: Jeff Layton <jlayton@kernel.org>
Sent: Dienstag, 5. September 2023 12:51
To: Ondrej Valousek <ondrej.valousek.xm@renesas.com>; Alexander Viro <viro@zeniv.linux.org.uk>; Christian Brauner <brauner@kernel.org>
Cc: trondmy@hammerspace.com; eggert@cs.ucla.edu; bruno@clisp.org; linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] fs: don't call posix_acl_listxattr in generic_listxattr

On Mon, 2023-09-04 at 20:36 +0000, Ondrej Valousek wrote:
> Hi Jeff,
>
> I can confirm that with rawhide kernel 6.5 the error is gone, i.e.
>
> Listxattr() shows only "system.nfs4_acl" attribute on NFSv4
> filesystem,
>
> Problem is, that (on the same kernel)
> getxattr(name,XATTR_NAME_POSIX_ACL_ACCESS, 0,0) Sets errno to ENODATA where "name" is file on NFSv4.
>
> This is different behavior to the previous versions, i.e. on RHEL8 getxattr() sets errno to ENOTSUP in the same scenario - which is what I'd expect more.
>
> Is the change of the getxattr() behavior expected or not?
>
> Thanks,
> Ondrej
>

I'd say not. I've been working through xfstests failures on NFS and didn't realize that this was a regression, and posted this fstests patch
recently:

    https://lore.kernel.org/fstests/20230830-fixes-v4-1-88d7b8572aa3@kernel.org/

It would be nice if getxattr were also fixed here.

Cheers,
Jeff

> -----Original Message-----
> From: Jeff Layton <jlayton@kernel.org>
> Sent: Dienstag, 16. Mai 2023 14:47
> To: Alexander Viro <viro@zeniv.linux.org.uk>; Christian Brauner
> <brauner@kernel.org>
> Cc: trondmy@hammerspace.com; eggert@cs.ucla.edu; bruno@clisp.org;
> Ondrej Valousek <ondrej.valousek.xm@renesas.com>;
> linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH] fs: don't call posix_acl_listxattr in
> generic_listxattr
>
> Commit f2620f166e2a caused the kernel to start emitting POSIX ACL xattrs for NFSv4 inodes, which it doesn't support. The only other user of generic_listxattr is HFS (classic) and it doesn't support POSIX ACLs either.
>
> Fixes: f2620f166e2a xattr: simplify listxattr helpers
> Reported-by: Ondrej Valousek <ondrej.valousek.xm@renesas.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/xattr.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> index fcf67d80d7f9..e7bbb7f57557 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -985,9 +985,16 @@ int xattr_list_one(char **buffer, ssize_t *remaining_size, const char *name)
>       return 0;
>  }
>
> -/*
> +/**
> + * generic_listxattr - run through a dentry's xattr list() operations
> + * @dentry: dentry to list the xattrs
> + * @buffer: result buffer
> + * @buffer_size: size of @buffer
> + *
>   * Combine the results of the list() operation from every
> xattr_handler in the
> - * list.
> + * xattr_handler stack.
> + *
> + * Note that this will not include the entries for POSIX ACLs.
>   */
>  ssize_t
>  generic_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size) @@ -996,10 +1003,6 @@ generic_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
>       ssize_t remaining_size = buffer_size;
>       int err = 0;
>
> -     err = posix_acl_listxattr(d_inode(dentry), &buffer, &remaining_size);
> -     if (err)
> -             return err;
> -
>       for_each_xattr_handler(handlers, handler) {
>               if (!handler->name || (handler->list && !handler->list(dentry)))
>                       continue;
> --
> 2.40.1
>

--
Jeff Layton <jlayton@kernel.org>