Re: fs/acl: Add ACL_USER_OBJ permissions test

On Wed, 3 Jun 2026 19:31:40 +0530, Sachin Sant wrote:
> fs/acl: Add ACL_USER_OBJ permissions test
> fs/acl: Add ACL mask interaction tests
> fs/acl: Add ACL_OTHER permissions test
> fs/acl: Add default ACL inheritance test
> fs/acl: Add chmod/chown ACL interaction tests
> fs/acl: Add symlink ACL operations test
> fs/acl: Add extended attributes test
> fs/acl: Remove old shell-based ACL test

The overall approach is sound — replacing the old monolithic shell script
with focused C tests is the right direction, and the shared acl_lib.h is
well-structured. However there are a few issues that need addressing before
this is ready.

---
BLOCKING
---

[1] acl_link01.c: test is vacuous and does not verify symlink-following

The test creates TESTFILE with mode 0644, then sets:

  ACL_USER_OBJ = rw-
  ACL_GROUP_OBJ = r--
  ACL_OTHER     = r--

via TESTSYMLINK. That ACL is bit-for-bit identical to the minimal ACL
already implied by mode 0644, so acl_set_file() changes nothing.  Then
acl_get_file() is called on both TESTFILE and TESTSYMLINK; since both
resolve to the same inode, acl_cmp() is trivially 0 regardless of whether
the symlink was followed during the set call.

The test would pass even if acl_set_file() on a symlink were a no-op.

Fix: set a deliberately distinct ACL through the symlink — one that is
different from the file's initial state — and then verify the target file's
ACL reflects that change.  For example:

  SAFE_OPEN(TESTFILE, O_CREAT | O_WRONLY, 0600);

  acl = acl_init(3);
  add_acl_entry(acl, ACL_USER_OBJ, ACL_READ | ACL_WRITE | ACL_EXECUTE);
  add_acl_entry(acl, ACL_GROUP_OBJ, ACL_READ | ACL_WRITE);
  add_acl_entry(acl, ACL_OTHER, 0);

  /* Set the new ACL through the symlink */
  acl_set_file(TESTSYMLINK, ACL_TYPE_ACCESS, acl);

  /* Now read from the target and from the symlink */
  target_acl  = acl_get_file(TESTFILE, ACL_TYPE_ACCESS);
  symlink_acl = acl_get_file(TESTSYMLINK, ACL_TYPE_ACCESS);

  /* Both must match the ACL we set, and the target must differ from 0600 */

With a distinct ACL, a failure to follow the symlink during the set call
would be caught because the target file's mode/ACL would remain at 0600.

---

[2] acl_other01.c: correctness depends on distro-specific useradd behaviour

The test relies on user2_gid != user1_gid so that user2 falls through to
ACL_OTHER rather than matching ACL_GROUP_OBJ (the directory's owning
group is user1_gid).  Whether useradd creates a private group per user is
controlled by USERGROUPS_ENAB in /etc/login.defs.  On many distributions
this defaults to "yes", but it is not universal.  On a system where
USERGROUPS_ENAB=no, both users would share the same primary GID and user2
would match ACL_GROUP_OBJ (masked to --- by ACL_MASK), causing EACCES and
a false test failure.

Fix: explicitly request a user-private group:

  tst_cmd((const char *[]){"useradd", "-M", "-U", username, NULL}, ...);

The "-U" flag creates a group with the same name as the user, guaranteeing
a distinct GID even when USERGROUPS_ENAB=no.  Check that your target
useradd supports -U (it is part of shadow-utils and available on all
mainstream distros).

The same concern applies to acl_mask01.c which tests ACL_GROUP and
ACL_GROUP_OBJ interactions depending on user/group separation.

---
NON-BLOCKING
---

[3] acl_lib.h: create_user_if_needed() uses TCONF when TBROK is correct

  if (tst_cmd((const char *[]){"useradd", "-M", username, NULL},
              NULL, NULL, TST_CMD_PASS_RETVAL) != 0)
      tst_brk(TCONF, "Failed to create user %s", username);

The needs_cmds field already handles the "useradd not installed" case
with TCONF.  Once useradd is present but returns non-zero (e.g. PAM
policy, container user-namespace restrictions, nss failures), the test
cannot be set up — that is TBROK, not TCONF.  Using TCONF here silently
skips the test instead of surfacing the setup failure.

  tst_brk(TBROK, "Failed to create user %s", username);

---

[4] acl_lib.h: set_acl_file() should treat EOPNOTSUPP as TCONF

  static inline void set_acl_file(const char *path, acl_type_t type, acl_t acl)
  {
      ...
      if (acl_set_file(path, type, acl) == -1)
          tst_brk(TBROK | TERRNO, "acl_set_file(%s) failed", path);
  }

If POSIX ACL support is missing at the kernel level (CONFIG_FS_POSIX_ACL
not set) and the filesystem returns EOPNOTSUPP, all tests that go through
set_acl_file() will report TBROK instead of TCONF.  acl_link01.c already
handles this correctly inline; the same treatment should be in the shared
helper:

  if (acl_set_file(path, type, acl) == -1) {
      if (errno == EOPNOTSUPP)
          tst_brk(TCONF | TERRNO, "ACL not supported on %s", path);
      tst_brk(TBROK | TERRNO, "acl_set_file(%s) failed", path);
  }

---

[5] acl_inherit01.c: acl_get_file() result is never inspected

  file_acl = acl_get_file(TESTFILE, ACL_TYPE_ACCESS);
  if (!file_acl) {
      cleanup_testfile();
      tst_brk(TBROK | TERRNO, "acl_get_file failed");
  }
  safe_acl_free(file_acl);

The test states it verifies that "the file inherits the default ACL as
its access ACL" but the retrieved ACL object is freed immediately without
any content check.  The mode-bit verification (0444) is sufficient to
confirm the inherited ACL was applied for a minimal ACL, so either:

  (a) Remove the acl_get_file() block and add a comment explaining that
      for a minimal inherited ACL the mode bits are the canonical check, or

  (b) Iterate file_acl and verify that each entry matches the expected
      inherited permissions.

As written the block misleads the reader into thinking the ACL content
is being validated when it is not.

---

Sachin Sant <[email protected]>

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