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

Hi!
> >> +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.

It does test the permissions for the open() call. It's wrong to check
the errnos from set*() calls.

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

And as I described below we do not want to propagate results in LTP. We
should report PASS/FAIL as close to the call that we check as possible.
With that we avoid all possible bugs in result propagation.

> >> +		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().

-- 
Cyril Hrubis
[email protected]

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