Linuxulator: fix symbol link file permissions returned by stat
Needs ReviewPublic
Actions

Authored by ankohuu_outlook.com on Jan 16 2021, 4:14 AM.

Details

Reviewers

None

Group Reviewers

Summary

According to the POSIX standard，symbolic links can always be read, except that the value of the file mode bits returned in the st_mode field of the stat structure is unspecified.

Linux implemented like this: stat() always returns st_mode 0777 for almost symbolic link files, but on FreeBSD returns its own real permission.
Some linux programs do obtain the symbol link file permissions in proc or sys folders and check whether it is equal to 777, so I think maybe linux_stat return 0777 better at this time.

BTW, I found two exceptions on Linux, I don’t know if there are any other exceptions.

lr-x------ /proc/xxx/fd/yyy

If file is open for reading, mode is S_IRUSR | S_IXUSR.
If file is open for writing, mode is S_IWUSR | S_IXUSR.
linux code 5.7-rc1:

static void tid_fd_update_inode(struct task_struct *task, struct inode *inode, fmode_t f_mode)
{
    task_dump_owner(task, 0, &inode->i_uid, &inode->i_gid);

    if (S_ISLNK(inode->i_mode)) {
        unsigned i_mode = S_IFLNK;
        if (f_mode & FMODE_READ)
            i_mode |= S_IRUSR | S_IXUSR;
        if (f_mode & FMODE_WRITE)
            i_mode |= S_IWUSR | S_IXUSR;
        inode->i_mode = i_mode;
    }
    security_task_to_inode(task, inode);
}

In this case, we let fdescfs handle it.

lr-------- /proc/aaa/map_files/bbb-ccc

If file is open for reading, mode is S_IRUSR.
If file is open for writing, mode is S_IWUSR.
linux code 5.7-rc1:

static struct dentry *
proc_map_files_instantiate(struct dentry *dentry, struct task_struct *task, const void *ptr)
{
    fmode_t mode = (fmode_t)(unsigned long)ptr;
    struct proc_inode *ei;
    struct inode *inode;

    inode = proc_pid_make_inode(dentry->d_sb, task, S_IFLNK |
                    ((mode & FMODE_READ ) ? S_IRUSR : 0) |
                    ((mode & FMODE_WRITE) ? S_IWUSR : 0));
    if (!inode)
        return ERR_PTR(-ENOENT);

    ei = PROC_I(inode);
    ei->op.proc_get_link = map_files_get_link;

    inode->i_op = &proc_map_files_link_inode_operations;
    inode->i_size = 64;

    d_set_d_op(dentry, &tid_map_files_dentry_operations);
    return d_splice_alias(inode, dentry);
}

In this case, we don't have such files and just skip.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 36268
Build 33157: arc lint + arc unit

Event Timeline

ankohuu_outlook.com created this revision.Jan 16 2021, 4:14 AM

Herald added subscribers: Contributor Reviews (src), imp. · View Herald TranscriptJan 16 2021, 4:14 AM

ankohuu_outlook.com requested review of this revision.Jan 16 2021, 4:14 AM

Harbormaster completed remote builds in B36257: Diff 82354.Jan 16 2021, 4:14 AM

ankohuu_outlook.com edited the summary of this revision. (Show Details)Jan 16 2021, 4:19 AM

kib added a subscriber: kib.Jan 16 2021, 7:56 AM

kib added inline comments.

sys/compat/linux/linux_stats.c
93	Why do you need to check for VV_READLINK? Isn't it already handled in the changed S_IF<MODE>, and S_ISLNK() would be false? I suggest to move this if() to 'else if' of vn_isdisk() check.

ankohuu_outlook.com marked an inline comment as done.Jan 16 2021, 4:13 PM

ankohuu_outlook.com added inline comments.

sys/compat/linux/linux_stats.c
93	Why do you need to check for VV_READLINK? Isn't it already handled in the changed S_IF<MODE>, and S_ISLNK() would be false? I suggest to move this if() to 'else if' of vn_isdisk() check.
93	Why do you need to check for VV_READLINK? Isn't it already handled in the changed S_IF<MODE>, and S_ISLNK() would be false? I suggest to move this if() to 'else if' of vn_isdisk() check.
93	Why do you need to check for VV_READLINK? I did this to find out the symlink files under the path /proc/self/fd, on Linux not all symlink permissions are 777 as I mentioned in the summary, files in /proc/self/fd follow rules below, if file is open for reading, mode is S_IRUSR \| S_IXUSR，if file is open for writing, mode is S_IWUSR \| S_IXUSR. In rc.d/linux we mount devfs to /compat/linux/dev with linrdlnk option, files under it have VV_READLINK flag, and /compat/linux/proc/self/fd is a link to /compat/linux/dev. Isn't it already handled in the changed S_IF<MODE>, and S_ISLNK() would be false? do you mean this code? if (vn_isdisk(vp)) { sb->st_mode &= ~S_IFMT; sb->st_mode \|= S_IFBLK; } I think only some VCHRs can change their mode to S_IFBLK, my logic only deals with VLNKs, they are mutually exclusive and will not affect each other I suggest to move this if() to 'else if' of vn_isdisk() check. Yes it will improve efficiency, I made some changes, please check

ankohuu_outlook.com marked an inline comment as done and 2 inline comments as not done.Jan 16 2021, 4:13 PM

call perm modify function only when file is symbol link
only call perm function in lstat hook
if fdescfs mounted in /compat/linux/dev, file perm generated by open flag

Harbormaster completed remote builds in B36268: Diff 82370.Jan 16 2021, 4:22 PM

kib added inline comments.Jan 16 2021, 4:27 PM

sys/compat/linux/linux_stats.c
93	No, I mean that VV_READLINK is irrelevant there. Look at the fdescfs code where it is set and used.

emaste added a subscriber: trasz.Jan 16 2021, 6:49 PM

emaste added a subscriber: emaste.

kib added inline comments.Jan 17 2021, 5:16 AM

sys/fs/fdescfs/fdesc_vnops.c
465	This is severe layering violation, fget() is before vnode lock in the global order. I think you do not get a warning from any debugging facility in kernel because fget() is mostly avoids locks ATM. Sill, this must not be done. I do not have only two possible suggestions: either do not do anything, or possibly cache file flags in fdescfs vnode v_data at the time of fdesc_allocvp(). There is fget-ed file around that time.