Re: Regression with getcifsacl(1) in v6.14-rc1

[Paulo Alcantara <pc@manguebit.com>]

Pali Rohár <pali@kernel.org> writes:

> On Wednesday 12 February 2025 19:19:00 Paulo Alcantara wrote:
>> Pali Rohár <pali@kernel.org> writes:
>> 
>> > On Wednesday 12 February 2025 17:49:31 Paulo Alcantara wrote:
>> >> Steve,
>> >> 
>> >> The commit 438e2116d7bd ("cifs: Change translation of
>> >> STATUS_PRIVILEGE_NOT_HELD to -EPERM") regressed getcifsacl(1) because it
>> >> expects -EIO to be returned from getxattr(2) when the client can't read
>> >> system.cifs_ntsd_full attribute and then fall back to system.cifs_acl
>> >> attribute.  Either -EIO or -EPERM is wrong for getxattr(2), but that's a
>> >> different problem, though.
>> >> 
>> >> Reproduced against samba-4.22 server.
>> >
>> > That is bad. I can prepare a fix for cifs.ko getxattr syscall to
>> > translate -EPERM to -EIO. This will ensure that getcifsacl will work as
>> > before as it would still see -EIO error.
>> 
>> Sounds good.
>> 
>> > But as discussed before, we need to distinguish between
>> > privilege/permission error and other generic errors (access/io).
>> > So I think that we need 438e2116d7bd commit.
>> 
>> OK.
>> 
>> > Based on linux-fsdevel discussion it is a good idea to distinguish
>> > between errors by mapping status codes to appropriate posix errno, and
>> > then updating linux syscall manpages.
>> 
>> Either way, we shouldn't be leaking -EIO or -EPERM to userland from
>> getxattr(2).  By looking at the man pages, -ENODATA seems to be the
>> appropriate error to return instead.
>
> It looks like there are missing error codes for getxattr. Because any
> path based syscall can return -EACCES if trying to open path to which
> calling process does not have access.
>
> And EACCES is not mentioned nor documented in getxattr(2). Same applies
> for listxattr(2). Now I have tried listxattr() and it really returns
> EACCES for /root/file called by nobody.

Both man pages have this:

        > In addition, the errors documented in stat(2) can also occur.

and stat(2) actually documents EACCES.

> -EIO is generic I/O error. And I think that this error code could be
> returned by any I/O syscall when unknown I/O error occurs.

Makes sense.

> Returning -ENODATA for generic or unknown I/O error is a bad idea
> because ENODATA (= ENOATTR) has already specific meaning when attribute
> does not exists at all (or process does not have access to it).

You are right.

> For me it makes sense to return -EIO and -EPERM by those syscalls. But
> for getxattr() we cannot do it due that backward compatibility needed by
> getcifsacl application.

-EACCES seems the correct one.  But yeah, we can't do it due to
 getcifsacl(1) relying on -EIO.