Page MenuHomeFreeBSD

Add AT_EMPTY_PATH
ClosedPublic

Authored by kib on Mar 7 2021, 2:53 PM.

Details

Summary

It is currently allowed to fchownat(2), fchmodat(2), fchflagsat(2), utimensat(2), fstatat(2), and linkat(2).

For linkat(2), PRIV_VFS_FHOPEN privilege is required to exercise the flag. It allows to link any open file.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib requested review of this revision.Mar 7 2021, 2:53 PM
kib created this revision.
kib edited the summary of this revision. (Show Details)
kib edited the test plan for this revision. (Show Details)

Allow AT_EMPTY_PATH for fstatat(2).
Fix visibility issue.

fstatat(2) needs locked leaf vnode.

Skip ENOTDIR for EMPTYPATH

Seems to work perfectly for at least fstatat(2) and fchownat(2). Thanks for working on it!

kib added reviewers: emaste, markj.

Update man page for fstatat(2). Other will follow after this update is corrected.

lib/libc/sys/stat.2
114–120 ↗(On Diff #85638)

My take:

If the path argument is an empty string, operate on the file or directory referenced by the descriptor fd. If fd is equal to AT_FDCWD, operate on the current working directory.

"operate on" could be "return information about" if that's more clear.

To me it seems that "file or directory referenced..." is sufficiently clear that the "does not need to be a directory" sentence could be omitted, but I'm not certain - you could also leave it.

kib marked an inline comment as done.

Reword manual page update, by Ed.

Man page change looks fine to me, would be good to have a 2nd opinion from @trasz or manpages

What is the use-case for this? fstatat(fd, "", &sb, AT_EMPTY) seems equivalent to fstat(fd, &sb).

lib/libc/sys/stat.2
124 ↗(On Diff #85709)

Some mention of the required privileges seems useful.

sys/kern/vfs_syscalls.c
1585 ↗(On Diff #85709)

Why is the check done after resolution?

kib marked 2 inline comments as done.Mar 15 2021, 4:16 PM

What is the use-case for this? fstatat(fd, "", &sb, AT_EMPTY) seems equivalent to fstat(fd, &sb).

For fstatat(2) yes, but note linkat(2). The real user is linuxolator, anyway. Trasz@ needs this to make some linux program work.

lib/libc/sys/stat.2
124 ↗(On Diff #85709)

There is no required privilege for fstat(). For linkat() I will mention PRIV_VFS_FHOPEN

sys/kern/vfs_syscalls.c
1585 ↗(On Diff #85709)

Because I do not know if namei() would really use EMPTYPATH in advance, and I do not want to duplicate the logic.

In D29111#655388, @kib wrote:

What is the use-case for this? fstatat(fd, "", &sb, AT_EMPTY) seems equivalent to fstat(fd, &sb).

For fstatat(2) yes, but note linkat(2). The real user is linuxolator, anyway. Trasz@ needs this to make some linux program work.

I see, there is some context in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=197778

Also, given how many Linux apps started to use those (visible as a number of regressions in Ubuntu Focal compared to Ubuntu Bionic), it might soon come really handy when porting software.

Also, given how many Linux apps started to use those

Maybe happening via glibc though, not explicit AT_EMPTY_PATH in src?

sys/kern/vfs_lookup.c
472 ↗(On Diff #85709)

I think a comment explaining how this is handled by the caller would be useful. Else it looks like a bug.

I would go further and centralize error handling at the end of the function:

cnp->cn_nameptr = cnp->cn_pnbuf;
return (0);

fail:
if (error != ENOENT)
    namei_cleanup_cnp(cnp);
return (error);

Otherwise this is IMO too fragile.

587 ↗(On Diff #85709)

IMO it would be nicer to lift this into a separate function. There is too much indentation otherwise.

kib marked 4 inline comments as done.Mar 17 2021, 8:30 PM
kib added inline comments.
sys/kern/vfs_lookup.c
472 ↗(On Diff #85709)

I restructured the error handling to do cleanup in the caller instead. Also added namei_emptypath() and reduced indentations.

kib marked an inline comment as done.

namei_emptypath()

Should faccessat(2) handle the flag as well?

sys/kern/vfs_lookup.c
467 ↗(On Diff #85929)

The comment should be updated to note that EMPTYPATH is handled in the caller. Otherwise it looks like the code is wrong.

636 ↗(On Diff #85929)

Cleanup now needs to happen here too, no?

kib marked 2 inline comments as done.Mar 29 2021, 8:53 PM
kib added inline comments.
sys/kern/vfs_lookup.c
636 ↗(On Diff #85929)

I think that no. To get there, we must already pass namei_getpath() above, and then ENOENT or any other error is already handled.

Hmm, indeed, userspace might change the string between previous and this namei_getpath(). Ok.

kib marked an inline comment as done.

Enable for faccessat(2).
Handle one more namei_getpath() call.
Update comment.

Seems ok to me. I did some fuzzing of the change without any problems found so far.

This revision is now accepted and ready to land.Mar 30 2021, 2:17 PM

Update man pages for the affected syscalls.

This revision now requires review to proceed.Mar 30 2021, 4:30 PM
lib/libc/sys/link.2
120

I guess this should be fd1.

128

name2?

130

I believe it should be fd2.

kib marked 3 inline comments as done.

link(2) arg names fixes

This revision is now accepted and ready to land.Mar 30 2021, 10:02 PM
sys/kern/vfs_lookup.c
323 ↗(On Diff #86584)

I believe it should not be allowed: fooat(AT_FDCWD, "", AT_EMPTY_PATH) is an operation on the CWD, but the CWD is not a capability. fooat(AT_FDCWD) is always disallowed in capability mode.

kib marked an inline comment as done.

Remove XXX comment about AT_FDCWD for AT_EMPTY_PATH

This revision now requires review to proceed.Apr 1 2021, 6:02 PM
sys/kern/vfs_syscalls.c
1585 ↗(On Diff #85709)

Actually I am confused by this. Shouldn't we be handling NIRES_EMPTY path for the first path, not the second?

kib marked an inline comment as done.

Check privilege for the right lookup case in linkat(AT_EMPTY_PATH).

Seems to work perfectly for at least fstatat(2) and fchownat(2). Thanks for working on it!

faccessat(2) seems to work with O_PATH file descriptors too

This revision is now accepted and ready to land.Apr 11 2021, 10:19 PM
lib/libc/sys/link.2
130

Eh, sorry it should be name1 and fd1. I misunderstood the interface the first time I read this.

kib marked an inline comment as done.Apr 12 2021, 1:26 AM

I rebased O_PATH branch onto AT_EMPTY_PATH, so for now I will no longer update these reviews.
I plan to commit after Peter' tests.

sys/kern/vfs_syscalls.c
1563 ↗(On Diff #87157)

It looks like the committed version is still doing the priv_check() on the second path.