ACL problem with NFSv4 and DELETE_ACCESS

Hi!

Attached find two patches that attempt to fix a bug we have
when "real" ACLs and not just posix mode bits are used. With
"real" I right now mean NFSv4, but others like for example
the AFS ACL module are also affected.

The problem is in can_delete_file_in_directory(). It right
now looks at the posix mode only if the owner of a directory
wants to delete a file within it. This is wrong in all the
more enhanced ACL schemes. It might be obvious, but it took
a while for me to understand how this should really work: We
are allowed to delete a file when either we have a direct
DELETE right on the object or if that is not there we have a
DELETE_CHILD right on the containing directory. The first
attached patch implements this.

The second patch is necessary because the default rwxr-xr-x
right on a normal file would map to the owner's DELETE bit
on a file that is about to be deleted. This is wrong, in the
non-acl case the right to delete a file is not determined by
the permissions on the file itself. The changed checks in
can_delete_file_in_directory take care of it by separately
looking at the directory permissions.

The downside of this patch is that we don't use the fast
path anymore in the non-acl case. I will measure next now
much we actually lose. And, I'm not sure if all mappings in
posix_acls.c are correct enough to actually make sure that
we get the checks right if we push it through
posix_get_nt_acl.

I'm not checking these patches in right away, because I need
to do more tests, but I'd like to hear some feedback
nevertheless, in particular from people who have worked with
Posix and NFSv4 ACLs in production.

Thanks,

Volker

PGP SIGNATURE
Version: GnuPG v1.4.5 (GNU/Linux)


2pjv6Rom6mZf7B+fMfpxiMI=
=n6DN
PGP SIGNATURE

Tue, Jun 17, 2008 at 08:45:52PM +0200, Nicolas Dorfsman wrote:
Your patch may fix some cases, but is it really sufficient ? Take a
look to the list I wrote in my bug report.

Thanks, I had forgotten that one.

Volker

PGP SIGNATURE
Version: GnuPG v1.4.5 (GNU/Linux)


vUd6kST/SqE5tJYB+U=
=nmTR
PGP SIGNATURE

Tue, Jun 17, 2008 at 04:41:48PM +0200, Volker Lendecke wrote:
Hi!

Attached find two patches that attempt to fix a bug we have
when "real" ACLs and not just posix mode bits are used. With
"real" I right now mean NFSv4, but others like for example
the AFS ACL module are also affected.

The problem is in can_delete_file_in_directory(). It right
now looks at the posix mode only if the owner of a directory
wants to delete a file within it. This is wrong in all the
more enhanced ACL schemes. It might be obvious, but it took
a while for me to understand how this should really work: We
are allowed to delete a file when either we have a direct
DELETE right on the object or if that is not there we have a
DELETE_CHILD right on the containing directory. The first
attached patch implements this.

The second patch is necessary because the default rwxr-xr-x
right on a normal file would map to the owner's DELETE bit
on a file that is about to be deleted. This is wrong, in the
non-acl case the right to delete a file is not determined by
the permissions on the file itself. The changed checks in
can_delete_file_in_directory take care of it by separately
looking at the directory permissions.

The downside of this patch is that we don't use the fast
path anymore in the non-acl case. I will measure next now
much we actually lose. And, I'm not sure if all mappings in
posix_acls.c are correct enough to actually make sure that
we get the checks right if we push it through
posix_get_nt_acl.

I'm not checking these patches in right away, because I need
to do more tests, but I'd like to hear some feedback
nevertheless, in particular from people who have worked with
Posix and NFSv4 ACLs in production.

As discussed, they look really good with the change from
UNIX_ACCESS_RWX to (UNIX_ACCESS_RWX & ~DELETE_ACCESS) and
the re-arrangement to check the directory acl first.

Cheers,

Jeremy.

Wed, Jun 18, 2008 at 04:37:26PM -0700, Jeremy Allison wrote:
As discussed, they look really good with the change from
UNIX_ACCESS_RWX to (UNIX_ACCESS_RWX & ~DELETE_ACCESS) and
the re-arrangement to check the directory acl first.

, pushed. Thanks for looking at it :-)

Volker

PGP SIGNATURE
Version: GnuPG v1.4.5 (GNU/Linux)


X5oXnwLLX+XGAI2A=
=Sv53
PGP SIGNATURE

Hi Volker,

Le 18 juin 08 11:52, Volker Lendecke a :
>
Just found in the SUN docs of the acl(2) system call that is
evolving or uncommitted. This is really brillant, because
according to their docs it means:
>
>No commitment is made about either source or binary
>compatibility of these interfaces from one Minor release to
>the next. Even the drastic incompatible change of removal of
>the interface in a Minor release is possible.
>
Can we use this AT ALL?

man -s5 attributes extract :

Evolving

An Evolving interface may eventually become Standard or
Stable but is still in transition.

Sun will make reasonable efforts to ensure compatibility
with previous releases as it evolves. When non-upwards
compatible changes become necessary, they will occur in
minor and major releases; such changes will be avoided
in micro releases whenever possible. If such a change is
necessary, it will be documented in the release notes
for the affected release, and when feasible, Sun will
provide migration aids for binary compatibility and con-
tinued source development.


Are you still afraid ?

Nicolas Dorfsman wrote:
>
Hi Volker,
>
Le 18 juin 08 11:52, Volker Lendecke a :
>>
>Just found in the SUN docs of the acl(2) system call that is
>evolving or uncommitted. This is really brillant, because
>according to their docs it means:
>>
No commitment is made about either source or binary
compatibility of these interfaces from one Minor release to
the next. Even the drastic incompatible change of removal of
the interface in a Minor release is possible.
>>
>Can we use this AT ALL?
>
man -s5 attributes extract :
>
Evolving
>
An Evolving interface may eventually become Standard or
Stable but is still in transition.
>
Sun will make reasonable efforts to ensure compatibility
with previous releases as it evolves. When non-upwards
compatible changes become necessary, they will occur in
minor and major releases; such changes will be avoided
in micro releases whenever possible. If such a change is
necessary, it will be documented in the release notes
for the affected release, and when feasible, Sun will
provide migration aids for binary compatibility and con-
tinued source development.
>
>
Are you still afraid ?

"Evolving" really mens the interface is uncommitted and can be changed
by Sun but in case of the acl(2) you should have on your mind the ace_t
structure is using layout of the "NFSv4 wire format" for the flags so
the RFC 3530 should be rewritten first in this case. Also the ZFS is
using the NFSv4 extended ACL interface so here is a lot of code in
Solaris which should be rewritten in such case :-)

Jiri