Project

General

Profile

Actions

Bug #14646

closed

vnode_valid_pn() always fails for symlinks

Added by Matt Barden 6 months ago. Updated 5 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
kernel
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:
External Bug:

Description

vnode_valid_pn() unconditionally passes the FOLLOW flag to lookuppnvp. The only effect the FOLLOW flag has on lookuppvnp is, if the final component of the path is a symlink, to cause it to follow that symlink and return its target, instead of the symlink itself ('intermediate' symlinks are always followed). This means that, when vnode_valid_pn is called on a vnode of type VLNK, the vnode of the target (rather than the symlink) will be returned, and so the following vn_compare() will fail. As a result vnodetopath() always fails for symlinks.

vnode_valid_pn() should only pass FOLLOW when the vp is not of type VLNK. This allows vnodetopath() to work on symlinks, while maintaining the old behavior (allowing vnodetopath to work even if the vp is the target of a symlink and its v_path contains a path to that symlink instead of a resolved path).

Issue can be reproduced with the attached kernel module and program.

cp vntest /usr/kernel/drv/amd64/ ; cp vntest.conf /usr/kernel/drv
add_drv vntest
./vntest_prog <path-to-symlink>

This is a prerequisite for 11873/11874.

Testing was done by running the test program against a variety of objects (symlinks, directories, files, devices) and verifying that the path output matches the input, as well as verifying that 'pwd' continues to work (which calls dogetcwd()->vnodetopath_common()).


Files

Makefile (1.37 KB) Makefile Matt Barden, 2022-04-18 05:17 PM
vntest.conf (31 Bytes) vntest.conf Matt Barden, 2022-04-18 05:17 PM
vntest.h (90 Bytes) vntest.h Matt Barden, 2022-04-18 05:17 PM
vntest.c (4.22 KB) vntest.c Matt Barden, 2022-05-04 03:38 PM
vntest_prog.c (1.26 KB) vntest_prog.c Matt Barden, 2022-05-04 03:38 PM

Related issues

Related to illumos gate - Bug #8376: cached v_path should be kept freshClosedPatrick Mooney2017-06-12

Actions
Blocks illumos gate - Bug #11873: SMB file access audit loggingIn ProgressMatt Barden

Actions
Blocks illumos gate - Feature #11874: NFS file access audit loggingIn ProgressMatt Barden

Actions
Actions #1

Updated by Matt Barden 6 months ago

Actions #2

Updated by Joshua M. Clulow 6 months ago

  • Blocks Bug #11873: SMB file access audit logging added
Actions #3

Updated by Joshua M. Clulow 6 months ago

Actions #4

Updated by Electric Monk 6 months ago

  • Gerrit CR set to 2117
Actions #5

Updated by Marcel Telka 6 months ago

  • Related to Bug #8376: cached v_path should be kept fresh added
Actions #6

Updated by Matt Barden 6 months ago

  • Description updated (diff)
Actions #7

Updated by Electric Monk 5 months ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit fad76a8aa8247c36be908a3a4de1674f5b444484

commit  fad76a8aa8247c36be908a3a4de1674f5b444484
Author: Matt Barden <mbarden@tintri.com>
Date:   2022-05-03T00:45:52.000Z

    14646 vnode_valid_pn() always fails for symlinks
    Reviewed by: Gordon Ross <Gordon.W.Ross@gmail.com>
    Reviewed by: Marcel Telka <marcel@telka.sk>
    Approved by: Robert Mustacchi <rm@fingolfin.org>

Actions #8

Updated by Matt Barden 5 months ago

During additional testing requested during RTI, I discovered, much to my surprise, that vnode_valid_pn() actually succeeds for certain links under /proc/<pid>/path. Namely, so long as the link isn't the 'a.out' link, and 'ls -l' shows a valid path, it succeeds.

Unlike typical link implementations, most procfs links define VOP_REALVP() as the target of the link. Additionally, The readlink() implementation in procfs constructs the target path at call time (whereas typical links store the target path in the link). For some, it simply returns a 0-length path; for others, it constructs it based on information in the node. However, for PATH-type links, readlink() is implemented as vnodetopath(VOP_REALVP(vnode)) - except for the 'a.out' object, where the final component needs to be added into the returned path.

To recap: vnode_valid_pn() succeeds when lookuppnvp() succeeds, and either vn_compare(old, new) or vnode_match(old, new) succeeds. When the FOLLOW flag is passed and the argument is of type VLNK, lookuppnvp will call VOP_READLINK() on the vnode. In procfs, this returns the path of the target, and lookuppnvp returns the target node, with the target's path in the rpnp argument. vn_compare() then calls VN_CMP() on the VOP_REALVP() of both the old and new vnodes. For procfs PATH-type links other than a.out, VOP_REALVP() is the target node, and so VN_CMP() succeeds. As such, vnodetopath() succeeds, returning the path of the target.
For /proc/pid/path/a.out, VOP_REALVP() is actually the containing directory, not the object itself, so vn_compare() fails, and vnodetopath returns with ENOENT. Similarly, some PATH-type links return a 0-length path, and vnodetopath() returns ENOENT.

As such, this change represents a behavior change for objects under /proc/pid/path; previously, vnodetopath() would return the path of the target, for fail with ENOENT; now, they return the path to the link itself.
Other behaviours (such as normal VOP operations - readlink, open, etc) are not changed, and so 'normal' consumers of these objects should be unaffected. For example, you can still 'cd /proc/pid/path/cwd', and /usr/bin/pwd will return the correct directory, and 'ls -l' still shows all of the paths correctly.

It is unclear whether this was ever intended behavior, or a happy coincidence where the implementation of procfs and vnodetopath() aligned. The code that became vnode_valid_pn(), as well as the implementation of readlink() in procfs, both pre-date OpenSolaris launch. Both vnodetopath_common() and vnode_match() make reference to other procfs links (links where v_type is VDIR but the VTYPE getattr is VLNK), but there are no code comments regarding these VLNKs. Additionally, vnodetopath() already does not work on every link of this type (notably a.out).

As these are VLNKs, any fallout of this change should be limited to kernel consumers of vnodetopath(). Userspace programs cannot ordinarily open objects of type VLNK (open(), for example, can only either follow the link, or return ELOOP if O_NOFOLLOW is specified), CWD shouldn't be a link for similar reasons, and this does not change the behavior of other VFS operations.
While most in-gate consumers of vnodetopath() are unlikely to be calling it on link objects under /proc/pid/path, we'll nonetheless be monitoring for reports of breakages.

Actions #9

Updated by Matt Barden 5 months ago

  • File deleted (vntest.c)
Actions #10

Updated by Matt Barden 5 months ago

  • File deleted (vntest_prog.c)
Actions #11

Updated by Matt Barden 5 months ago

Actions

Also available in: Atom PDF