[#LU-974] Lustre does not ignore umask when default ACL with mask is set

[LU-974] Lustre does not ignore umask when default ACL with mask is set Created: 09/Jan/12 Updated: 12/Mar/19 Resolved: 22/Dec/12
Status:	Resolved
Project:	Lustre
Component/s:	None
Affects Version/s:	Lustre 2.3.0, Lustre 2.1.3, Lustre 1.8.8, Lustre 1.8.7
Fix Version/s:	Lustre 2.4.0, Lustre 2.1.4

Type:

Bug

Priority:

Minor

Reporter:

Kent Engström (Inactive)

Assignee:

Lai Siyao

Resolution:

Fixed

Votes:

Labels:

None

Environment:

Lustre 1.8.7 on CentOS 5

Issue Links:

Duplicate
is duplicated by	~~LU-2185~~	setfacl not working correctly	Resolved

Severity:

Bugzilla ID:

23,194

Rank (Obsolete):

4667

Description

One of our users recently requested us to turn on ACLs on a new Lustre filesystem.
After doing so, we discovered that one of the features this user group needs,
does not seem to be supported by Lustre: setting a mask in the ACL that
overrides the umask of the user creating files/directories.

We thinks this is the same as https://bugzilla.lustre.org/show_bug.cgi?id=23194

We include logs showing behavious with Lustre, and for comparison NFS (backed by XFS)
and a local ext4 filesystem.

LUSTRE
======

Client: lustre-1.8.7.80-2.6.18_274.12.1.el5_g27e7ce7
(patchless build a few commits beyond 1.8.7-wc1 in order
to get the fix for ~~LU-645~~).

Server: lustre-1.8.5-2.6.18_194.17.1.el5_lustre.1.8.5.nsc1_201105161812
(1.8.5 with a single patch for ~~LU-274~~)

[kent@server ~]$ umask
0022

[kent@server ~]$ cd /nobackup/rossby16/kent

[kent@server kent]$ mkdir subdir

[kent@server kent]$ setfacl -R -d -m mask:007 subdir

[kent@server kent]$ getfacl subdir

file: subdir
owner: kent
group: nsc
user::rwx
group::r-x
other::r-x
default:user::rwx
default:group::r-x
default:mask::rwx
default:other::r-x

[kent@server kent]$ mkdir subdir/dir

[kent@server kent]$ touch subdir/file

[kent@server kent]$ ls -lrt subdir/
total 4
drwxr-xr-x+ 2 kent nsc 4096 Jan 9 14:45 dir
~~rw-r~~r-+ 1 kent nsc 0 Jan 9 14:45 file

[kent@server kent]$ ls -lrtd subdir
drwxr-xr-x+ 3 kent nsc 4096 Jan 9 14:45 subdir

COMPARISON: NFS BACKED BY XFS
=============================

Client/server: CentOS 5 kernel 2.6.18-274.12.1.el5

[kent@server ~]$ umask
0022

[kent@server ~]$ mkdir subdir

[kent@server ~]$ setfacl -R -d -m mask:007 subdir

[kent@server ~]$ getfacl subdir

file: subdir
owner: kent
group: nsc
user::rwx
group::r-x
other::r-x
default:user::rwx
default:group::r-x
default:mask::rwx
default:other::r-x

[kent@server ~]$ mkdir subdir/dir

[kent@server ~]$ touch subdir/file

[kent@server ~]$ ls -lrt subdir/
total 8
drwxrwxr-x+ 2 kent nsc 6 Jan 9 14:48 dir
~~rw-rw-r~~-+ 1 kent nsc 0 Jan 9 14:48 file

[kent@server ~]$ ls -lrtd subdir
drwxr-xr-x+ 3 kent nsc 27 Jan 9 14:48 subdir

COMPARISION: FEDORA 16
======================

Local ext4 filesystem on kernel 3.1.6-1.fc16.x86_64

[kent@workstation]~% umask
022

[kent@workstation]~% mkdir subdir

[kent@workstation]~% setfacl -R -d -m mask:007 subdir

[kent@workstation]~% getfacl subdir

file: subdir
owner: kent
group: kent
user::rwx
group::r-x
other::r-x
default:user::rwx
default:group::r-x
default:mask::rwx
default:other::r-x

[kent@workstation]~% mkdir subdir/dir

[kent@workstation]~% touch subdir/file

[kent@workstation]~% ls -lrt subdir/
total 12
drwxrwxr-x+ 2 kent kent 4096 Jan 9 14:50 dir
~~rw-rw-r~~-+ 1 kent kent 0 Jan 9 14:50 file

[kent@workstation]~% ls -lrtd subdir
drwxr-xr-x+ 3 kent kent 4096 Jan 9 14:50 subdir

Comments

Comment by Peter Jones [ 10/Jan/12 ]

Lai

Could you please look into this one?

Thanks

Peter

Comment by Lai Siyao [ 15/Jan/12 ]

You're correct, lustre client doesn't ignore umask when default ACL is set, but unfortunately MDS doesn't respect default ACL, so it needs change on MDS too. I'll provide a fix when local test passes.

Comment by Lai Siyao [ 16/Jan/12 ]

fix for 1.8 is on http://review.whamcloud.com/#change,1976
fix for 2.x is on http://review.whamcloud.com/#change,1972

Kent, would you verify the fix for 1.8?

Comment by Kent Engström (Inactive) [ 16/Jan/12 ]

Just to be sure: do we need to update the servers to in order to test/deploy this,
or does it suffice to update the client side?

Comment by Lai Siyao [ 16/Jan/12 ]

The fix is made on both client and mds code, so you need to update both. Since the code is still on review phase, you don't need to deploy it now, if all is okay, the fix will be landed to latest b1_8 and master branch, after that you can really deploy it, currently you can test it on a test system.

Comment by Kent Engström (Inactive) [ 17/Jan/12 ]

I tried to get a test server by building the code in http://review.whamcloud.com/#change,1976
(commit 4c2b70f664fe286f335f76395a0b165919c75a1d) on top of an up-to-date CentOS 5.7
retrofitted with kernel-2.6.18-274.3.1.el5 and related RPMs, following the instructions at
http://wiki.whamcloud.com/display/PUB/Walk-thru-+Build+Lustre+1.8+on+CentOS+5.5+or+5.6+from+Whamcloud+git

The build seems OK, but I get no lustre-ldiskfs*.rpm built.

Do you have any recommendations on how to proceed? Will I have to downgrade to CentOS 5.6 to build the
test server (not just downgrading the kernel and related RPMs)?

Comment by Kent Engström (Inactive) [ 17/Jan/12 ]

Now I found ~~LU-987~~. Applying the one-line fix from http://review.whamcloud.com/#change,1964
seems to solve this.

Comment by Peter Jones [ 17/Jan/12 ]

Thanks Kent. We had not had lu-987 reported on 1.8.x yet. Please let us know how you get on with your testing of this issue.

Comment by Kent Engström (Inactive) [ 17/Jan/12 ]

The fix is not correct. The umask should be ignored only when creating files/directories in a directory with a default ACL.
The current code seems to ignore the umask for all directories with ACLs even if they are only access ACLs with
no default part.

I am by far no export on POSIX ACL semantics. I'm trying to understand the expected behaviour by comparing
with other filesystems and reading http://www.suse.de/~agruen/acl/linux-acls/online/

Comment by Kent Engström (Inactive) [ 17/Jan/12 ]

It seems even more wrong when I test a bit more:

Creating a subdir in a directory without any ACLs set (ext4 on Fedora 16):

[kent@ceres]~% umask; getfacl .; mkdir subdir; getfacl subdir
077

file: .
owner: kent
group: kent
user::rwx
group::r-x
other::r-x

file: subdir
owner: kent
group: kent
user::rwx
group::---
other::---

Creating a subdir in a directory without any ACLs set (patched Lustre client against patched Lustre server):

[kent@n319 kent]$ umask; getfacl .; mkdir subdir; getfacl subdir
0077

file: .
owner: kent
group: nsc
user::rwx
group::r-x
other::r-x

file: subdir
owner: kent
group: nsc
user::rwx
group::r-x
other::r-x

As the directory I was in did not have any default ACL, the umask should have been in effect. The question is why I got
r-x for group and other. I the umask was totally ignored, shouldn't I have got rwx?

Comment by Lai Siyao [ 17/Jan/12 ]

Hi Kent, currently lustre client doesn't enforce its umask to server, that means, the umask MDS uses is the one it gets when it's started (it's often 022 by default), I suspect you changed it to 077 on client in this test. I checked NFS, it looks to be the same, could you verify the result for it? And I am not quite sure what's the proper way for this, if server should use the umask client sets, the code needs some fundamental changes.

Comment by Kent Engström (Inactive) [ 18/Jan/12 ]

Let's show the regession I'm talking about in my 17/Jan/12 11:13 AM comment, without setfacl/getfacl:

$ umask 0022; mkdir subdir; ls -ld . subdir; rmdir subdir
drwxr-xr-x 14 kent nsc 4096 Jan 18 09:33 .
drwxr-xr-x 2 kent nsc 4096 Jan 18 09:33 subdir

$ umask 0077; mkdir subdir; ls -ld . subdir; rmdir subdir
drwxr-xr-x 14 kent nsc 4096 Jan 18 09:33 .
drwx------ 2 kent nsc 4096 Jan 18 09:33 subdir

This is what I get on local ext4, NFS backed by XFS, and all our production Lustre file systems
(with or without ACLs enabled).

Now, on my test Lustre filesystem, with client/server using the patchset for ~~LU-974~~, I get:

[kent@ossspare kent]$ umask 0022; mkdir subdir; ls -ld . subdir; rmdir subdir
drwxr-xr-x 4 kent build 4096 Jan 18 10:35 .
drwxr-xr-x 2 kent kent 4096 Jan 18 10:35 subdir

[kent@ossspare kent]$ umask 0077; mkdir subdir; ls -ld . subdir; rmdir subdir
drwxr-xr-x 4 kent build 4096 Jan 18 10:36 .
drwxr-xr-x 2 kent kent 4096 Jan 18 10:36 subdir

Comment by Lai Siyao [ 18/Jan/12 ]

Ahh, I see what's going on now:

VFS ignores umask if acl enabled, and the disk filesystem (eg. ext4) will enforce umask if default acl doesn't exist.
nfs always enforece umask, but if default acl exists, it will update acl, and the final result is correct.
Lustre follows VFS' way, and it leaves it to ldiskfs to enforce umask if default acl doesn't exist, but unfortunately lustre client and server doesn't share umask for requests, so if client changes its umask, it will not reflect on server, and the result is incorrect.

I need think it over again on how to implement it correctly, because the way nfs does may cause extra RPCs, and if we follow the VFS way, client and server need to share umask, which will cause in-wire data change.

Comment by Lai Siyao [ 21/Jan/12 ]

Hi Kent, I've chosen to follow VFS's way, and updated the patches, would you verify it again?

Comment by Kent Engström (Inactive) [ 23/Jan/12 ]

Where do I get your updated patches? I looked at http://review.whamcloud.com/#change,1964 but see no update there.

A question: if this changes both client and server, how will it work when one uses combinations like
unfixed client -> fixed server, fixed client -> unfixed server etc?

Comment by Kent Engström (Inactive) [ 25/Jan/12 ]

ping See my 23/Jan/12 4:41 AM comment.

Comment by Peter Jones [ 25/Jan/12 ]

hi Kent

Lai is out of the office on vacation. I will see whether another engineer can answer in his absence

Peter

Comment by Peter Jones [ 25/Jan/12 ]

The b1_8 version of the patch is http://review.whamcloud.com/#change,1976
The master version of the patch is http://review.whamcloud.com/#change,1972

Comment by Kent Engström (Inactive) [ 25/Jan/12 ]

Peter, thanks for the code links. My other question can wait until Lai is back.

Comment by Peter Jones [ 25/Jan/12 ]

ok Kent - that will be on Monday

Comment by Lai Siyao [ 28/Jan/12 ]

Hi Kent, the fix changes both client and server, the result of different deployment is as below:

fixed client <-> fixed server: Okay!
fixed client <-> unfixed server: server always uses umask 022 in inode creation normally no matter default ACL with mask is set. The umask on client will be ignored.
unfixed client <~~> fixed server: umask always enforced, same as unfixed client <~~> unfixed server.

Comment by Kent Engström (Inactive) [ 07/Feb/12 ]

I've now tested your patch on a test server and it seems OK as far as our rather limit ACL testing is concerned.

If I understand your fixed-vs-unfixed list above, unfixed client towards fixed server should be no problem,
but I am not sure about the consequences of fixed client towards unfixed server above. When will the
behaviour differ from the case with unfixed-unfixed and how? We should be careful not to introduce security issues
(like setting too permissive rights on files and directories).

Comment by Lai Siyao [ 12/Feb/12 ]

Hi Kent, I updated the patch after my last comment, now for the fixed-vs-unfixed cases, only when both server and client get fixed, it will act correctly; while for other cases, it behaves the old way (umask always enforced no matter default ACL with mask set or not), so no new security issues introduced.

Comment by Kent Engström (Inactive) [ 13/Feb/12 ]

Would that updated patch be Patch Set 3 of http://review.whamcloud.com/#change,1976 ?

Comment by Lai Siyao [ 13/Feb/12 ]

Yes.

Comment by Kent Engström (Inactive) [ 16/Feb/12 ]

So, what will happen to this patch now? Will it be landed to b1_8 and master? When?

Comment by Peter Jones [ 16/Feb/12 ]

Kent

Yes, the patches should land for both b1_8 and master once the respective changesets have two positive inspections and have passed autotest.

Peter

Comment by Andreas Dilger [ 13/Mar/12 ]

The value assigned to OBD_CONNECT_UMASK in this patch has a potential conflict with OBD_CONNECT_GRANT_PARAM, which was just reserved on master and orion for a separate feature.

Fortunately, it is possible to change OBD_CONNECT_GRANT_PARAM because it is not currently in use on these branches, but it is lucky that there was not a more serious fallout from using OBD_CONNECT flags without reserving them on the other branches first. Patches are:

orion: http://review.whamcloud.com/2296
master: http://review.whamcloud.com/2297
b1_8: http://review.whamcloud.com/2298
b2_1: http://review.whamcloud.com/2299
b2_2: http://review.whamcloud.com/2301

Comment by Build Master (Inactive) [ 28/Mar/12 ]

Integrated in lustre-b2_2 » i686,client,el6,ofa #18
~~LU-974~~ protocol: change OBD_CONNECT_GRANT_PARAM (Revision 0aea0520956beb402c75cafe621b67fadff42d43)

Result = SUCCESS
Oleg Drokin : 0aea0520956beb402c75cafe621b67fadff42d43
Files :

lustre/ptlrpc/wiretest.c
lustre/include/lustre/lustre_idl.h
lustre/obdclass/lprocfs_status.c
lustre/utils/wiretest.c
lustre/utils/wirecheck.c

Comment by Build Master (Inactive) [ 28/Mar/12 ]

Integrated in lustre-b2_2 » i686,server,el6,ofa #18
~~LU-974~~ protocol: change OBD_CONNECT_GRANT_PARAM (Revision 0aea0520956beb402c75cafe621b67fadff42d43)

Result = SUCCESS
Oleg Drokin : 0aea0520956beb402c75cafe621b67fadff42d43
Files :

lustre/utils/wirecheck.c
lustre/utils/wiretest.c
lustre/obdclass/lprocfs_status.c
lustre/ptlrpc/wiretest.c
lustre/include/lustre/lustre_idl.h