Page MenuHomeFreeBSD

ext2fs: Add posix ACLs support
ClosedPublic

Authored by fsu on May 18 2017, 9:15 PM.

Details

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

  1. 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

  1. 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)

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

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

fsu created this revision.May 18 2017, 9:15 PM
pfg edited edge metadata.May 18 2017, 11:04 PM

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 updated this revision to Diff 28551.May 19 2017, 7:39 AM

Fix found remarks.

fsu added inline comments.May 19 2017, 7:40 AM
sys/fs/ext2fs/ext2_acl.c
144 ↗(On Diff #28539)

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

pfg added inline comments.May 19 2017, 2:42 PM
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 added inline comments.May 19 2017, 5:58 PM
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 added inline comments.May 19 2017, 6:06 PM
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 added a comment.May 19 2017, 6:21 PM

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 added a comment.May 19 2017, 6:49 PM
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 added a comment.May 19 2017, 7:10 PM

:) 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 added a comment.May 19 2017, 10:34 PM
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 updated this revision to Diff 28653.May 22 2017, 8:28 AM

*_from_disk()/*_to_disk() refactoring

kevlo edited edge metadata.May 25 2017, 8:35 AM

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 updated this revision to Diff 28771.May 25 2017, 9:31 AM

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

kevlo accepted this revision.May 26 2017, 1:39 AM
This revision is now accepted and ready to land.May 26 2017, 1:39 AM
pfg accepted this revision.May 27 2017, 8:18 PM

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 updated this revision to Diff 28939.May 28 2017, 3:09 PM
fsu edited edge metadata.

Fix minor style.

This revision now requires review to proceed.May 28 2017, 3:09 PM
This revision was automatically updated to reflect the committed changes.