vfs: Use file_cred for VOP_DEALLOCATE in vn_deallocate if non-NULL

[khng]

Summary

This changes vn_deallocate() to match the behavior of vn_rdwr() when
picking which ucred to use. That is, vn_deallocate() uses file_cred for
making VOP call if it is non-NULL, or use active_cred otherwise.

Sponsored by: The FreeBSD Foundation

[kib]

Why is it correct? fspacectl() is called by usermode, active_cred are current credentials. f_cred AKA file_cred are credentials of the thread that opened the file. So e.g. if root opened a file and passed fd around, fspacectl(fd) would be executed with root credentials.

vn_rdwr() is different, it is not called by syscall directly.

[khng]

Just checked the behavior of .fo_read/.fo_write.

[khng]

Make such change specific to vn_deallocate instead.

[khng]

Oops.

[kib]

sys/kern/vfs_vnops.c

3585

why passing fp->f_cred there at all?
from_fops is ugly and arguably not needed.
vn_deallocate() needs explanation about active_cre/file_cred in a comment, nobody would read man page for this (or in both)

[khng]

sys/kern/vfs_vnops.c

3585

The fp->f_cred was there to for mac_vnode_check_write's policy checking (although all the policy modules in base so far are not taking f_cred into consideration at all).

[khng]

• Use vn_deallocate_args to pass around most of the args to vn_deallocate_impl.
• Comment vn_deallocate(9)'s active_cred/file_cred preference.

[khng]

sys/kern/vfs_vnops.c

3585

A mental note to myself: Though mac_vnode_check_write()'s file_cred is not inspected by in base policy module, it is possibly used by outside world to implement capability semantic.

[kib]

sys/kern/vfs_vnops.c

191

Why do you need this?

For rw it was needed due to the vn_io_fault() structure, for vn_deallocate() it is pointless complication.

[khng]

The upper 16bit of ioflag is used to pass internal flag to vn_deallocate_impl.

[kib]

Don't do this either.

Right now I see the only place which uses vn_deallocate(), it is md(4). Do you envision any more places which would do that?
I suggest to change vn_deallocate_impl() to take three creds arg. One to pass to VOP_DEALLOCATE(), and two others active_cred/f_cred. Then the caller of vn_deallocate_impl() would decide directly what to pass to the vop, so vn_deallocate() should do the if (f_cred != NULL) dance, and vn_deallocate_impl() passes active_cred to vop. This seems to be most streamlined and natural implementation.

[khng]

In D31712#716746, @kib wrote:

Don't do this either.

Right now I see the only place which uses vn_deallocate(), it is md(4). Do you envision any more places which would do that?

My change to ctl(4) which translate UNMAP to VOP_DEALLOCATE for file-backed LUNS also calls vn_deallocate().

I suggest to change vn_deallocate_impl() to take three creds arg. One to pass to VOP_DEALLOCATE(), and two others active_cred/f_cred. Then the caller of vn_deallocate_impl() would decide directly what to pass to the vop, so vn_deallocate() should do the if (f_cred != NULL) dance, and vn_deallocate_impl() passes active_cred to vop. This seems to be most streamlined and natural implementation.

[khng]

vn_deallocate_impl takes three kinds of cred so it's the caller to decide which cred to pass to the VOP call. Suggested by kib@.

[kib]

sys/kern/vfs_vnops.c

3546

When I talked about commenting the use, I mean that it should explain that function is supposed to be used in the situations where the op is not triggered by a user request. In other words, currthread->td_ucred == active_cred is generally not the right credentials to account the operation to.

Your line above just repeats the code.

3563

cred = file_cred != NOCRED ? file_cred : active_cred;

[khng]

• Comments
• ternary instead.