ext2fs: Add posix ACLs support

[fsu]

Test Plan

1. tools-posix.test:

$ mke2fs /dev/md0 # Where md0 mapped tp 4G file
$ mount -t ext2fs -o acls /dev/md0 /mnt
$ cd /mnt
$ /usr/src/tests/sys/acl/run /usr/src/tests/sys/acl/tools-posix.test
Expected only next fails, because ACLs does not supported for fifos:
[409] $ setfacl -m u:42:r,g:43:w fff -- failed
setfacl: fff: acl_get_file() failed: Operation not supported != ~
[410] $ getfacl fff -- failed

1. file: fff == # file: fff
2. owner: root == # owner: root
3. group: wheel == # group: wheel

user::rw- == user::rw-
group::r-- != user:42:r--
other::r-- != group::r--
~ != group:43:-w-
~ != mask::rw-
~ != other::r--
[421] $ ls -l fff | cut -d' ' -f1 -- failed
prw-r--r-- != prw-rw-r--+
[424] $ setfacl -bn fff -- failed

2. tuxera pjd test:

Download it from here http://www.tuxera.com/community/posix-test-suite/
Fix Makefile to use clang and change error codes in fstest.c, for example:
ENODATA => ENOATTR, EBADRQC => EINVAL
$ cp ./pjd-fstest-20090130-RC /mnt/
$ make
$ prove -r ./tests/xacl/
Expected:
./tests/xacl/00.t .. ok
./tests/xacl/01.t .. ok
./tests/xacl/02.t .. ok
./tests/xacl/03.t .. ok
./tests/xacl/04.t .. ok
./tests/xacl/05.t .. ok
./tests/xacl/06.t .. ok
All tests successful.
Files=7, Tests=7, 6 wallclock secs ( 0.38 usr 0.55 sys + 0.70 cusr 4.56 csys = 6.19 CPU)
Result: PASS

3. pjdfstest with and without -o acls mount option:

$ mount -t ext2fs /dev/md0 /mnt
$ cd /mnt
$ cp -r /usr/src/contrib/pjdfstest ./
$ cd /mnt/pjdfstest
$ make
$ prove -r ./tests > ~/ext2_no_acls_log.txt
$ cd ~
$ umount /dev/md0
$ mount -t ext2fs -o acls /dev/md0 /mnt
$ cd /mnt/pjdfstest
$ prove -r ./tests > ~/ext2_with_acls_log.txt
$ diff ~/ext2_no_acls_log.txt ~/ext2_with_acls_log.txt
Expected:
$ diff ~/ext2_no_acls_log.txt ~/ext2_with_acls_log.txt
342c342

< Files=220, Tests=9869, 2298 wallclock secs (18.03 usr 28.01 sys + 246.51 cusr 1676.73 csys = 1969.27 CPU)

Files=220, Tests=9869, 1776 wallclock secs (12.79 usr 23.96 sys + 197.99 cusr 1305.92 csys = 1540.66 CPU)

4. EA's fsx from https://reviews.freebsd.org/D10460, regression:

$ ./extattr_fsx_test.sh
Expected, that test will be passed.

[pfg]

Minor issues.. would like to see some feedback from others.

sys/fs/ext2fs/ext2_acl.c

144 ↗

(On Diff #28539)

size is unsigned, perhaps there is a risk for it to become negative here?

148 ↗

(On Diff #28539)

Use parenthesis with return, same on line 153.

sys/fs/ext2fs/ext2_acl.h

32 ↗

(On Diff #28539)

Not sure if this is right or not:
#define is always followed by a TAB.

sys/fs/ext2fs/ext2_extattr.c

120 ↗

(On Diff #28539)

s/fail/failure/ Not sure if a panic is appropriate here.

361 ↗

(On Diff #28539)

The 0 == looks better at the end of the comparison, but perhpas just IMO.

sys/fs/ext2fs/ext2_vnops.c

1215 ↗

(On Diff #28539)

Not sure what this means but ext2 doesn't support softupdates.

[fsu]

Fix found remarks.

[fsu]

sys/fs/ext2fs/ext2_acl.c

144 ↗

(On Diff #28539)

It should be impossible, because the acl header should present in all cases.

[pfg]

sys/fs/ext2fs/ext2_acl.c

144 ↗

(On Diff #28539)

Prceisely, perhaps I read it wrong but, what happens in line 144 and the case where someone specified
size < sizeof(struct ext2_acl_header) ?

[fsu]

sys/fs/ext2fs/ext2_acl.c

144 ↗

(On Diff #28539)

Sorry, may be my previous answer was too short:
ext4_acl_from_disk(char *value, size_t size, struct acl *acl)
{
const char *end = value + size;
int n, count;

if (!value || size < sizeof(struct ext2_acl_header))

		return (EINVAL);

if (((struct ext2_acl_header *)value)->a_version != EXT4_ACL_VERSION)

		return (EINVAL);

value = value + sizeof(struct ext2_acl_header);
count = ext2_acl_count(size)

...
So, as you can see it is impossible
to call ext2_acl_count() with size value less than sizeof(struct ext2_acl_header).
If you talking about somebady...
I hope somebady understands what he doing because it is internal (static) function.
Also, if it is critical for you, I can change size_t to signed and add appropriate check.

[pfg]

sys/fs/ext2fs/ext2_acl.c

144 ↗

(On Diff #28539)

OK .. I see now, sorry: I was looking at the local function only.

Perhaps it would be clearer if we pass the subtracted value instead of doing the subtraction within ext2_acl_count()., but it's OK as it is.

[fsu]

The reason, why I prefer to leave these functions as is, because it is mostly copy-paste from ext4 linux sources, and if somebody will try to compare it with original, it will be simpler.

[pfg]

In D10807#224256, @thisisadrgreenthumb_gmail.com wrote:

The reason, why I prefer to leave these functions as is, because it is mostly copy-paste from ext4 linux sources, and if somebody will try to compare it with original, it will be simpler.

This is *very* bad we ave no interest in sharing code. You should NEVER EVER copy-paste from linux or we will have licensing issues. Please change the code.

[fsu]

:) May be I said about copy-paste too roughly, but I will lie if I say that I do not use the linux source code.
Ok, let be closer to question, let compare ext4_acl_from_disk() above and from here:
http://elixir.free-electrons.com/linux/latest/source/fs/ext4/acl.c#L16
Could you answer is it copy-paste o not?
So, if it is copy-paste, I will try to rewrite it, but I am not sure that it is possible to do it correctly because it is function with straightforward logic.

[pfg]

In D10807#224273, @thisisadrgreenthumb_gmail.com wrote:

:) May be I said about copy-paste too roughly, but I will lie if I say that I do not use the linux source code.

Well, concerning licensing I have to have some level of paranoia. Note that I have always suggested using the UFS code as a basis. There are some things that can only be done in only one way: in those cases it is OK to look at the linux code, understand it, and then implement it. For the others you have to look for alternative implementations and eventually that can also leads to improvements.

Ok, let be closer to question, let compare ext4_acl_from_disk() above and from here:
http://elixir.free-electrons.com/linux/latest/source/fs/ext4/acl.c#L16
Could you answer is it copy-paste o not?
So, if it is copy-paste, I will try to rewrite it, but I am not sure that it is possible to do it correctly because it is function with straightforward logic.

I prefer to conserve a level of "safe ignorance": I particularly don't want to evaluate if code here or there is copied as that may sound like legal advice and IANAL. I do think that where there are opportunities to improve the code we should do it and the argument of "linux does this way" leads to bad engineering.

[fsu]

*_from_disk()/*_to_disk() refactoring

[kevlo]

I tested on amd64, and it worked fine. One minor bug, you should add
"fs/ext2fs/ext2_acl.c optional ext2fs" to sys/conf/files, thanks.

[fsu]

Thanks for testing.
Add ext2_acl.c sys/conf/files.

[pfg]

Looks good to me.

sys/fs/ext2fs/ext2_extattr.c

88 ↗

(On Diff #28771)

Minor style: Finish the sentence with a period, You can also use XXX to highlight this is missing in the implementation.

[fsu]

Fix minor style.