Page MenuHomeFreeBSD
Differential D29111

Add AT_EMPTY_PATH
ClosedPublic
Actions

Authored by kib on Mar 7 2021, 2:53 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 17, 5:23 AM
Unknown Object (File)
Mon, Dec 16, 5:21 AM
Unknown Object (File)
Fri, Dec 13, 6:25 PM
Unknown Object (File)
Fri, Dec 13, 5:50 AM
Unknown Object (File)
Thu, Dec 12, 1:42 PM
Unknown Object (File)
Wed, Dec 11, 10:28 PM
Unknown Object (File)
Mon, Dec 9, 7:45 AM
Unknown Object (File)
Sat, Dec 7, 5:08 AM
View All Files
Subscribers
2khramtsov_gmail.com
emaste
wulf

Details

Reviewers
trasz
emaste
markj
Commits
rG509124b62616: Add AT_EMPTY_PATH for several *at(2) syscalls
rGd51b4b0aac43: AT_RESOLVE_BENEATH is bsd-specific
rGc78e124535fe: link(2): correct descriptor name in AT_RESOLVE_BENEATH description
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
Lint Skipped
Unit
Tests Skipped

Event Timeline

kib requested review of this revision.Mar 7 2021, 2:53 PM
kib created this revision.
emaste added a subscriber: emaste.Mar 9 2021, 3:02 PM
kib updated this revision to Diff 85573.Mar 11 2021, 3:53 PM
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.

kib updated this revision to Diff 85591.Mar 11 2021, 7:11 PM

fstatat(2) needs locked leaf vnode.

kib updated this revision to Diff 85611.Mar 12 2021, 11:14 AM

Skip ENOTDIR for EMPTYPATH

trasz added a comment.Mar 12 2021, 4:25 PM

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

kib updated this revision to Diff 85638.Mar 12 2021, 4:41 PM
kib added reviewers: emaste, markj.

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

emaste added inline comments.Mar 13 2021, 9:47 PM
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 updated this revision to Diff 85709.Mar 14 2021, 7:57 AM
kib marked an inline comment as done.

Reword manual page update, by Ed.

emaste added a comment.Mar 15 2021, 2:57 AM

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

markj added a comment.Mar 15 2021, 3:59 PM

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

Why is the check done after resolution?

kib marked 2 inline comments as done.Mar 15 2021, 4:16 PM
In D29111#655384, @markj 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.

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

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

markj added a comment.Mar 15 2021, 4:51 PM
In D29111#655388, @kib wrote:
In D29111#655384, @markj 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

trasz mentioned this in D29250: Get rid of CTR(9) in linux_futex.c..Mar 15 2021, 10:33 PM
trasz added a comment.Mar 16 2021, 4:26 PM

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.

emaste added a comment.Mar 16 2021, 5:22 PM

Also, given how many Linux apps started to use those

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

markj added inline comments.Mar 17 2021, 5:01 PM
sys/kern/vfs_lookup.c
470

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.

585

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
470

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

kib updated this revision to Diff 85929.Mar 17 2021, 8:30 PM
kib marked an inline comment as done.

namei_emptypath()

kib mentioned this in D29323: Implement O_PATH.Mar 23 2021, 7:26 PM
markj added a comment.Mar 29 2021, 7:26 PM

Should faccessat(2) handle the flag as well?

sys/kern/vfs_lookup.c
467

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

620

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
620

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 updated this revision to Diff 86526.Mar 29 2021, 8:54 PM
kib marked an inline comment as done.

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

markj accepted this revision.Mar 30 2021, 2:17 PM

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
kib updated this revision to Diff 86561.Mar 30 2021, 4:30 PM

Update man pages for the affected syscalls.

This revision now requires review to proceed.Mar 30 2021, 4:30 PM
markj added inline comments.Mar 30 2021, 9:31 PM
lib/libc/sys/link.2
120 ↗(On Diff #86561)

I guess this should be fd1.

129 ↗(On Diff #86561)

name2?

131 ↗(On Diff #86561)

I believe it should be fd2.

kib updated this revision to Diff 86584.Mar 30 2021, 9:58 PM
kib marked 3 inline comments as done.

link(2) arg names fixes

markj accepted this revision.Mar 30 2021, 10:02 PM
This revision is now accepted and ready to land.Mar 30 2021, 10:02 PM
markj added inline comments.Apr 1 2021, 3:28 PM
sys/kern/vfs_lookup.c
322

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 updated this revision to Diff 86700.Apr 1 2021, 6:02 PM
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
markj added inline comments.Apr 9 2021, 6:21 PM
sys/kern/vfs_syscalls.c
1585

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

kib updated this revision to Diff 87157.Apr 9 2021, 9:37 PM
kib marked an inline comment as done.

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

wulf added a subscriber: wulf.Apr 11 2021, 2:20 PM
In D29111#654477, @trasz wrote:

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

markj accepted this revision.Apr 11 2021, 10:19 PM
This revision is now accepted and ready to land.Apr 11 2021, 10:19 PM
markj added inline comments.Apr 11 2021, 10:20 PM
lib/libc/sys/link.2
131 ↗(On Diff #86561)

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.

2khramtsov_gmail.com added a subscriber: 2khramtsov_gmail.com.Apr 14 2021, 7:23 PM
Closed by commit rGc78e124535fe: link(2): correct descriptor name in AT_RESOLVE_BENEATH description (authored by kib). · Explain WhyApr 15 2021, 9:50 AM
This revision was automatically updated to reflect the committed changes.
kib added a commit: rGc78e124535fe: link(2): correct descriptor name in AT_RESOLVE_BENEATH description.
kib added a commit: rGd51b4b0aac43: AT_RESOLVE_BENEATH is bsd-specific.
kib added a commit: rG509124b62616: Add AT_EMPTY_PATH for several *at(2) syscalls.
markj added inline comments.Apr 19 2021, 12:53 PM
sys/kern/vfs_syscalls.c
1563

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

Revision Contents
Changeset List

PathSize
sys/
kern/
32 lines
57 lines
sys/
5 lines
3 lines

Diff 85591

View Options

sys/kern/vfs_lookup.c

Loading...
View Options

sys/kern/vfs_syscalls.c

Loading...
View Options

sys/sys/fcntl.h

Loading...
View Options

sys/sys/namei.h

Loading...