[LTP] [PATCH v5 1/8] fs/acl: Add ACL_USER_OBJ permission test

[Sachin Sant]

On 08/06/26 5:27 pm, Cyril Hrubis wrote:
>> +
>> +	if (type == ACL_TYPE_ACCESS)
>> +		xattr_name = XATTR_NAME_POSIX_ACL_ACCESS;
>> +	else if (type == ACL_TYPE_DEFAULT)
>> +		xattr_name = XATTR_NAME_POSIX_ACL_DEFAULT;
>> +	else {
>> +		errno = EINVAL;
>> +		return NULL;
>> +	}
> We are using this in two places, maybe it's worth having an
> acl_type_to_name() functions and use that instead.
Makes sense. Will make the change.
>> +	header = (struct posix_acl_xattr_header *)buf;
>> +	if (le32_to_cpu(header->a_version) != POSIX_ACL_XATTR_VERSION) {
>> +		free(buf);
>> +		errno = EINVAL;
>> +		return NULL;
>> +	}
>> +
>> +	count = (size - sizeof(*header)) / sizeof(struct posix_acl_xattr_entry);
>> +	entries = (struct posix_acl_xattr_entry *)(buf + sizeof(*header));
> Maybe we can define a structure with both header and entries in order to
> avoid some of the casts here. Maybe this would work:
>
> struct acl_xattr {
> 	struct posix_acl_xattr_header header;
> 	struct posix_acl_xattr_entry entries[];
> };
>
> And then we could do:
>
> 	struct acl_xattr *ax = (void*)buf;
>
> 	if (le32_to_cpu(ax->header.a_version) == ACL_TYPE_ACCESS) {
> 		...
> 	}
>
> 	...
>
> 	for (i = 0; i < conut; i++) {
> 		uint16_t tag = le16_to_cpu(ax->entries[i].e_tag);
> 		...
>
> 	}
Okay, will check.
>> +	acl = acl_init();
>> +	if (!acl) {
>> +		free(buf);
>> +		return NULL;
>> +	}
>> +
>> +	for (i = 0; i < count; i++) {
>> +		uint16_t tag = le16_to_cpu(entries[i].e_tag);
>> +		uint16_t perm = le16_to_cpu(entries[i].e_perm);
>> +		uint32_t id = le32_to_cpu(entries[i].e_id);
>> +
>> +		if (acl_add_entry(acl, tag, perm, id) < 0) {
>> +			acl_free(acl);
>> +			free(buf);
>> +			return NULL;
>> +		}
>> +	}
>> +
>> +	free(buf);
>> +	return acl;
>> +}
>> +
>> +/*
>> + * Check if an ACL entry has a specific permission
>> + */
>> +static inline int acl_entry_has_perm(struct acl_entry *entry, uint16_t perm)
>> +{
>> +	return (entry->perm & perm) == perm;
>> +}
>> +
>> +/*
>> + * Check if an ACL entry has all rwx permissions
>> + */
>> +static inline int acl_entry_has_rwx(struct acl_entry *entry)
>> +{
>> +	return acl_entry_has_perm(entry, ACL_READ) &&
>> +	       acl_entry_has_perm(entry, ACL_WRITE) &&
>> +	       acl_entry_has_perm(entry, ACL_EXECUTE);
> Can't we pass all the bits to a single call of the function like
> acl_entry_has_perm(entry, ACL_READ | ACL_WRITE | ACL_EXECUTE) ?
Yes, will change.
>> +}
>> +
>> +/*
>> + * Find an ACL entry by tag type
>> + */
>> +static inline struct acl_entry *acl_find_entry(struct acl *acl, uint16_t tag,
>> +					       uint32_t id)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < acl->count; i++) {
>> +		if (acl->entries[i].tag == tag) {
>> +			if (tag == ACL_USER || tag == ACL_GROUP) {
>> +				if (acl->entries[i].id == id)
>> +					return &acl->entries[i];
>> +			} else {
>> +				return &acl->entries[i];
>> +			}
>> +		}
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +/*
>> + * Update ACL mask permissions
>> + */
>> +static inline int acl_set_mask_perms(struct acl *acl, uint16_t perm)
>> +{
>> +	struct acl_entry *mask_entry = acl_find_entry(acl, ACL_MASK, 0);
>> +
>> +	if (!mask_entry) {
>> +		errno = ENOENT;
>> +		return -1;
>> +	}
>> +
>> +	mask_entry->perm = perm;
>> +	return 0;
>> +}
>> +
>> +static inline int create_file_as(uid_t uid, gid_t gid, mode_t mode,
>> +				 int use_umask, mode_t mask)
>> +{
>> +	pid_t pid;
>> +	int status;
>> +
>> +	pid = SAFE_FORK();
>> +	if (!pid) {
>> +		int fd, err;
>> +
>> +		if (setgroups(0, NULL) == -1) {
>> +			err = errno;
>> +			_exit(err);
>> +		}
>> +
>> +		if (setgid(gid) == -1) {
>> +			err = errno;
>> +			_exit(err);
>> +		}
>> +
>> +		if (setuid(uid) == -1) {
>> +			err = errno;
>> +			_exit(err);
>> +		}
> These should be SAFE_MACROS().
Can you elaborate on this?
This code runs in a forked child process that intentionally tests 
permission scenarios.
The function expects these calls to potentially fail as part of testing 
ACL behavior.

The current pattern of capturing errno and exiting with it is correct for
communicating failure back to the parent.
SAFE_* macros would call tst_brk(TBROK) on failure, which is 
inappropriate in
a child process testing permissions.

The parent process checks the exit status to determine if the operation
succeeded or failed (line 442: return WEXITSTATUS(status))

>
>> +		if (use_umask)
>> +			umask(mask);
>> +
>> +		fd = open(TESTFILE, O_CREAT | O_WRONLY, mode);
>> +		if (fd >= 0) {
>> +			close(fd);
>> +			_exit(0);
>> +		}
> Generally in LTP we want to check the test conditions right after they
> happen. So this function should get expected errno as last parameter and
> we should check for the result right here with something as:
>
> 	if (errno)
> 		ST_EXP_FAIL()
> 	else
> 		TST_EXP_FD()
>
> With that we do need to propagate the return value manually, which is
> prone to errors, and do not need to process the child exit value
> manually, we can just let the library collect the children with
> tst_reap_children().
>
>> +		err = errno;
>> +		_exit(err);
>> +	}
>> +
>> +	SAFE_WAITPID(pid, &status, 0);
>> +
>> +	if (!WIFEXITED(status))
>> +		tst_brk(TBROK, "Child terminated abnormally");
>> +
>> +	return WEXITSTATUS(status);
>
>> +}
>> +
>> +static inline int try_create_as(uid_t uid, gid_t gid, mode_t mode)
>> +{
>> +	return create_file_as(uid, gid, mode, 0, 0);
>> +}
>> +
>> +static inline int create_with_umask_as(uid_t uid, gid_t gid, mode_t mode,
>> +					mode_t mask)
>> +{
>> +	return create_file_as(uid, gid, mode, 1, mask);
>> +}
>> +
>> +static inline void cleanup_test_paths(void)
>> +{
>> +	if (unlink(TESTSYMLINK) == -1 && errno != ENOENT)
>> +		tst_res(TWARN | TERRNO, "unlink(%s) failed", TESTSYMLINK);
>> +
>> +	if (unlink(TESTFILE) == -1 && errno != ENOENT)
>> +		tst_res(TWARN | TERRNO, "unlink(%s) failed", TESTFILE);
>> +
>> +	if (rmdir(TESTDIR) == -1 && errno != ENOENT)
>> +		tst_res(TWARN | TERRNO, "rmdir(%s) failed", TESTDIR);
> Maybe we need SAFE_TRY_UNLINK() and SAFE_TRY_RMDIR() that wouldn't TBROK
> on ENOENT and then we could use it here.
Good point. Let me check on this.
>> +}
>> +
>> +#endif /* ACL_LIB_H */
>> diff --git a/testcases/kernel/fs/acl/acl_user_obj01.c b/testcases/kernel/fs/acl/acl_user_obj01.c
>> new file mode 100644
>> index 000000000..01abfbf4e
>> --- /dev/null
>> +++ b/testcases/kernel/fs/acl/acl_user_obj01.c
>> @@ -0,0 +1,154 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (c) 2026 IBM
>> + *
>> + * Original shell test by Kai Zhao (ltcd3@cn.ibm.com)
>> + * Converted to C by Sachin Sant <sachinp@linux.ibm.com>
>> + */
>> +
>> +/*\
>> + * Test ACL_USER_OBJ permissions using direct xattr manipulation.
>> + *
>> + * Verify that owner permissions (ACL_USER_OBJ) correctly control access
>> + * to files and directories. The test validates that:
>> + * - ACL_USER_OBJ permissions are applied directly as the owner bits
>> + * - Setting ACL_USER_OBJ=rwx via setxattr() overrides a previous
>> + *   chmod restriction
>> + * - Owner permissions work independently of group and other permissions
>> + *
>> + * This test uses arbitrary UIDs without creating actual users, testing
>> + * only the kernel ACL implementation.
>> + */
>> +
>> +#include "acl_lib.h"
>> +
>> +#define TEST_UID 1000
>> +#define TEST_GID 1000
>> +
>> +/*
>> + * Test permission bits deny access.
>> + * Owner should be denied write access when mode is 0500.
>> + */
>> +static void test_deny_by_mode(void)
>> +{
>> +	int err;
>> +
>> +	tst_res(TINFO, "Testing permission bits deny access");
>> +	reset_test_path();
>> +
>> +	SAFE_CHOWN(TESTDIR, TEST_UID, TEST_GID);
>> +	SAFE_CHMOD(TESTDIR, 0500);
>> +
>> +	err = try_create_as(TEST_UID, TEST_GID, 0644);
>> +	if (!err) {
>> +		cleanup_testfile();
>> +		tst_res(TFAIL, "Created file without write permission");
>> +		return;
>> +	}
>> +
>> +	if (err != EACCES) {
>> +		errno = err;
>> +		tst_res(TFAIL | TERRNO, "Expected EACCES from owner create");
>> +		return;
>> +	}
>> +
>> +	tst_res(TPASS, "File creation denied by permission bits");
>> +}
>> +
>> +/*
>> + * Test ACL_USER_OBJ grants access.
>> + * Setting ACL_USER_OBJ=rwx should override previous chmod restriction.
>> + */
>> +static void test_grant_by_acl(void)
>> +{
>> +	struct acl *acl = NULL;
>> +	int err;
>> +
>> +	tst_res(TINFO, "Testing ACL_USER_OBJ grants access");
>> +	reset_test_path();
>> +
>> +	SAFE_CHOWN(TESTDIR, TEST_UID, TEST_GID);
>> +	SAFE_CHMOD(TESTDIR, 0500);
>> +
>> +	acl = acl_init();
>> +	if (!acl)
>> +		tst_brk(TBROK | TERRNO, "acl_init failed");
>> +
>> +	if (acl_add_entry(acl, ACL_USER_OBJ,
>> +			  ACL_READ | ACL_WRITE | ACL_EXECUTE, 0) < 0)
>> +		goto cleanup_acl;
>> +
>> +	if (acl_add_entry(acl, ACL_GROUP_OBJ, 0, 0) < 0)
>> +		goto cleanup_acl;
>> +
>> +	if (acl_add_entry(acl, ACL_OTHER, 0, 0) < 0)
>> +		goto cleanup_acl;
> Maybe we can just call SAFE_MALLOC() and tst_brk() internally in these
> functions so that we don't have to complicate the code here. We do call
> tst_brk() in cleanup_acl: anyways.
Thanks, yes make sense. Similar code pattern is used in multiple tests.
This should help simplify. Will check on this.

-- 
Thanks
- Sachin