Re: [LTP] fs/acl: Add ACL_USER_OBJ permissions test

[linuxtestproject.agent@gmail.com]

Hi Sachin,

Thanks for the patch series. The approach — converting tacl_xattr.sh to
a modular C test suite — is a good improvement. I have some issues to
address before this can be merged.

Kernel version note: checked against stable 7.1. POSIX ACLs and xattrs
have been in-tree for many kernel generations; no .min_kver concerns.

---

[PATCH 1/8] fs/acl: Add ACL_USER_OBJ permissions test

> /*\
>  * Test ACL_USER_OBJ permissions.
>  *
>  * Verify that owner permissions (ACL_USER_OBJ) correctly control access
>  * to files and directories. The test validates that:
>  * - Traditional permission bits deny access when restrictive
>  * - ACL_USER_OBJ entries can grant access beyond permission bits

"ACL_USER_OBJ entries can grant access beyond permission bits" is
incorrect. ACL_USER_OBJ IS the owner permission bits — they are the same
inode field. When you call acl_set_file() with ACL_USER_OBJ=rwx, the
kernel updates the inode owner permission bits to rwx. There is nothing
"beyond" the permission bits happening.

The test logic itself is correct: after chmod(0500) denies write, calling
acl_set_file() with ACL_USER_OBJ=rwx grants write access because the
inode owner bits are overwritten. Please fix the description to reflect
that accurately, for example:

  - ACL_USER_OBJ permissions are applied directly as the owner bits
  - Setting ACL_USER_OBJ=rwx via acl_set_file() overrides a previous
    chmod restriction

---

[PATCH 6/8] fs/acl: Add symlink ACL operations test

> /*
>  * Note: Some filesystems may not support ACLs on symlinks and will
>  * return EOPNOTSUPP, which is treated as TCONF (test not applicable).
>  */

This note is wrong, and it propagates into the TCONF message below.

acl_set_file() is implemented on top of setxattr(2), which follows
symlinks. The test is not "setting ACLs on a symlink" — it is setting
ACLs on the *target* through a symlink path. Symlinks themselves never
hold ACLs on Linux.

EOPNOTSUPP from acl_set_file() means the *target* filesystem does not
support ACLs, not that "ACLs on symlinks are unsupported".

> 	if (errno == EOPNOTSUPP) {
> 		tst_res(TCONF,
> 			"ACL on symlinks not supported");

The message "ACL on symlinks not supported" repeats the same incorrect
framing. It should say something like:

  tst_res(TCONF, "ACL not supported by this filesystem");

Please fix both the block comment and the TCONF string.

The commit body has the same issue:

> Returns TCONF if ACL on symlinks is not supported by the filesystem.

Should be:

  Returns TCONF if ACL is not supported by the filesystem.

---

[PATCH 6/8] fs/acl: Add symlink ACL operations test
[PATCH 7/8] fs/acl: Add extended attributes test

Both acl_link01.c and xattr_test01.c call init_test_users() in setup()
and cleanup_test_users() in cleanup(), and both have:

  .needs_cmds = (struct tst_cmd[]) {
      {.cmd = "useradd"},
      {.cmd = "userdel"},
      {}
  }

Neither test uses test user UIDs/GIDs for its actual test operations.
The user creation happens only because reset_test_path() calls
SAFE_CHOWN(TESTDIR, user1_uid, user1_gid). For these two tests:

- acl_link01: creates file + symlink as root, sets/gets ACL as root;
  the ownership of TESTDIR is irrelevant to whether symlink ACL
  operations work.
- xattr_test01: sets/gets/removes xattrs as root; TESTDIR ownership
  is irrelevant to xattr operations.

The needs_cmds dependency will cause TCONF on environments without
useradd/userdel (some containers, embedded targets) even though the
tests do not actually require those users. Please either:

  a) Avoid calling reset_test_path() in these two tests and use a
     simpler path setup that does not chown (e.g. SAFE_MKDIR only), or
  b) Add a variant of reset_test_path() that skips the chown step.

---

Pre-existing issues (informational only, do not affect verdict):

None found.

---

Verdict: Needs revision

Issues requiring fixes before merge:
  1. acl_user_obj01.c: incorrect description "beyond permission bits"
  2. acl_link01.c: incorrect TCONF message and block comment re symlinks
  3. Commit 1bdd453 body: incorrect "on symlinks" wording
  4. acl_link01.c + xattr_test01.c: unnecessary test user creation and
     needs_cmds adding useradd/userdel dependency

LTP AI Reviewer

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp