Page MenuHomeFreeBSD
Differential D50531

vfs: Don't clobber namei flags in vn_open_cred()
ClosedPublic
Actions

Authored by markj on Mon, May 26, 1:57 PM.
Tags
None
Referenced Files
F119395084: D50531.id.diff
Sun, Jun 8, 10:22 AM
F119394204: D50531.diff
Sun, Jun 8, 10:09 AM
F119381708: D50531.id156107.diff
Sun, Jun 8, 7:12 AM
F119362286: D50531.id156055.diff
Sun, Jun 8, 2:27 AM
Unknown Object (File)
Thu, Jun 5, 11:03 AM
Unknown Object (File)
Tue, Jun 3, 5:21 PM
Unknown Object (File)
Fri, May 30, 5:27 PM
Unknown Object (File)
Wed, May 28, 5:54 PM
View All 11 Files
Subscribers
imp

Details

Reviewers
jamie
olce
kib
Commits
rGe05e33041c25: vfs: Don't clobber namei flags in vn_open_cred()
Summary

Otherwise NAMEILOOKUP is cleared. More generally it seems quite
surprising that the flags set by vn_open_cred() callers are not
automatically preserved. I see at least one bug due to this:
zfs_getextattr_dir()'s use of NOFOLLOW is ignored, as it doesn't also
specify O_NOFOLLOW.

After going through callers, I believe it's safe to add the flags this
way.

Fixes: 7587f6d4840f ("namei: Make stackable filesystems check harder for jail roots")

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 64439
Build 61323: arc lint + arc unit

Event Timeline

markj created this revision.Mon, May 26, 1:57 PM
Herald added a subscriber: imp. · View Herald TranscriptMon, May 26, 1:57 PM
markj requested review of this revision.Mon, May 26, 1:57 PM
Harbormaster completed remote builds in B64439: Diff 156049.Mon, May 26, 1:57 PM
markj added a child revision: D50532: vfs cache: Add NAMEILOOKUP to the whitelist of fastpath lookup flags.Mon, May 26, 1:57 PM
markj updated this revision to Diff 156055.Mon, May 26, 4:48 PM

Fix up handling of O_NOFOLLOW.

Harbormaster completed remote builds in B64442: Diff 156055.Mon, May 26, 4:48 PM
kib added inline comments.Mon, May 26, 4:52 PM
sys/kern/vfs_vnops.c
190

I suggest changing the open2nameif() signature to

static void open2nameif(int fmode, u_int vn_open_flags, struct nameidata *ndp);

and having the 'or' hidden in the function.

olce added a comment.Tue, May 27, 2:07 PM

Browsing some old notes, I had noticed that cn_flags were sometimes crushed, and concluded that VN_* flags probably were just workarounds for that problem, except for VN_OPEN_INVFS.

Now, not crushing some flags on O_CREAT might introduce subtle semantics change. An audit of all direct vn_open_cred() callers does not show any problem (but make me notice a bug that the change here will fix as a side-effect, see below). However, I have not audited callers of vn_open().

The bug you noticed in zfs_setextattr_dir() about NOFOLLOW being ignored is in fact only a conceptual one, as NOFOLLOW is in fact 0. However, it seems there is a real, similar bug in openat() but with FOLLOW.

sys/kern/vfs_vnops.c
190

I'm OK with open2nameif() doing the OR (and actually more, see the other inline comment), but not really with passing ndp, which obfuscates at calling points that this function is only meant to compute cn_flags.

I'd rather suggest passing a pointer to cn_flags instead of ndp.

354–355

I would put the O_FOLLOW handling into open2nameif().

markj updated this revision to Diff 156096.Tue, May 27, 4:24 PM
markj marked an inline comment as done.

Fix FOLLOW/NOFOLLOW handling.

Harbormaster completed remote builds in B64463: Diff 156096.Tue, May 27, 4:24 PM
markj added a comment.Tue, May 27, 4:28 PM
In D50531#1153900, @olce wrote:

Browsing some old notes, I had noticed that cn_flags were sometimes crushed, and concluded that VN_* flags probably were just workarounds for that problem, except for VN_OPEN_INVFS.

Now, not crushing some flags on O_CREAT might introduce subtle semantics change. An audit of all direct vn_open_cred() callers does not show any problem (but make me notice a bug that the change here will fix as a side-effect, see below). However, I have not audited callers of vn_open().

The bug you noticed in zfs_setextattr_dir() about NOFOLLOW being ignored is in fact only a conceptual one, as NOFOLLOW is in fact 0. However, it seems there is a real, similar bug in openat() but with FOLLOW.

openatfp() ORs FOLLOW into the cnflags unless O_NOFOLLOW is set, and zfs_setextattr_dir() does not specify O_NOFOLLOW. So its use of NOFOLLOW is silently getting converted to FOLLOW. Maybe it's not a real problem in practice, but it doesn't matter that NOFOLLOW is 0.

sys/kern/vfs_vnops.c
190

I did it a bit differently than both suggestions, such that open2nameif() continues to be a pure function.

kib added inline comments.Tue, May 27, 5:43 PM
sys/kern/vfs_vnops.c
274

IMO this is too convoluted. open2namei() already set FOLLOW in !O_NOFOLLOW. So to keep the existing behavior, isn't it enough to clear FOLLOW if O_EXCL is set, without two lines above that set FOLLOW?

markj updated this revision to Diff 156107.Tue, May 27, 6:14 PM
  • Simplify O_EXCL handling.
  • Preserve NOFOLLOW from callers, inadvertently lost in the previous revision.

Now, a caller of vn_open_cred() has to explicitly specify FOLLOW, else it gets NOFOLLOW
semantics. O_FOLLOW overrides FOLLOW.

markj marked an inline comment as done.Tue, May 27, 6:14 PM
Harbormaster completed remote builds in B64467: Diff 156107.Tue, May 27, 6:15 PM
kib accepted this revision.Tue, May 27, 7:43 PM
This revision is now accepted and ready to land.Tue, May 27, 7:43 PM
olce accepted this revision.Tue, May 27, 8:43 PM
In D50531#1153938, @markj wrote:

openatfp() ORs FOLLOW into the cnflags unless O_NOFOLLOW is set, and zfs_setextattr_dir() does not specify O_NOFOLLOW. So its use of NOFOLLOW is silently getting converted to FOLLOW. Maybe it's not a real problem in practice, but it doesn't matter that NOFOLLOW is 0.

You probably meant vn_open_cred() here. Yes, I forgot about the special handling of O_NOFOLLOW after calling open2nameif(), with indeed causes a real problem in zfs_setextattr_dir().

In D50531#1154012, @markj wrote:

O_FOLLOW overrides FOLLOW.

O_NOFOLLOW, yes. Doing so seems quite reasonable.

Closed by commit rGe05e33041c25: vfs: Don't clobber namei flags in vn_open_cred() (authored by markj). · Explain WhyWed, May 28, 3:43 PM
This revision was automatically updated to reflect the committed changes.
markj added a commit: rGe05e33041c25: vfs: Don't clobber namei flags in vn_open_cred().

Revision Contents
Changeset List

PathSize
sys/
kern/
5 lines

Diff 156049

View Options

sys/kern/vfs_vnops.c

Loading...