Page MenuHomeFreeBSD

ext2fs: Add posix ACLs support
ClosedPublic

Authored by fsu on May 18 2017, 9:15 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 22 2024, 3:40 AM
Unknown Object (File)
Feb 16 2024, 11:59 PM
Unknown Object (File)
Feb 14 2024, 12:24 AM
Unknown Object (File)
Dec 28 2023, 9:55 PM
Unknown Object (File)
Dec 23 2023, 4:30 AM
Unknown Object (File)
Dec 22 2023, 8:44 PM
Unknown Object (File)
Dec 22 2023, 8:44 PM
Unknown Object (File)
Dec 22 2023, 8:44 PM
Subscribers

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 - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

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

sys/fs/ext2fs/ext2_acl.c
145

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

149

Use parenthesis with return, same on line 153.

sys/fs/ext2fs/ext2_acl.h
33

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

sys/fs/ext2fs/ext2_extattr.c
120

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

361

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

sys/fs/ext2fs/ext2_vnops.c
1215

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

sys/fs/ext2fs/ext2_acl.c
145

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

sys/fs/ext2fs/ext2_acl.c
145

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

sys/fs/ext2fs/ext2_acl.c
145

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.

sys/fs/ext2fs/ext2_acl.c
145

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.

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.

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.

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

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.

*_from_disk()/*_to_disk() refactoring

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.

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

This revision is now accepted and ready to land.May 26 2017, 1:39 AM

Looks good to me.

sys/fs/ext2fs/ext2_extattr.c
88

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

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.