Page MenuHomeFreeBSD

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

Authored by khng on Aug 29 2021, 5:00 PM.
Tags
None
Referenced Files
F105715982: D31712.id94333.diff
Thu, Dec 19, 3:18 PM
F105715577: D31712.id94332.diff
Thu, Dec 19, 3:08 PM
F105715162: D31712.id94463.diff
Thu, Dec 19, 2:59 PM
F105714396: D31712.id94475.diff
Thu, Dec 19, 2:41 PM
F105709347: D31712.diff
Thu, Dec 19, 12:59 PM
Unknown Object (File)
Wed, Nov 27, 1:47 AM
Unknown Object (File)
Fri, Nov 22, 4:46 PM
Unknown Object (File)
Nov 17 2024, 8:09 AM
Subscribers

Details

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

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

khng requested review of this revision.Aug 29 2021, 5:00 PM

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.

Just checked the behavior of .fo_read/.fo_write.

khng edited the summary of this revision. (Show Details)

Make such change specific to vn_deallocate instead.

sys/kern/vfs_vnops.c
3576

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)

sys/kern/vfs_vnops.c
3576

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 retitled this revision from vfs: Use file_cred for VOP_DEALLOCATE when available to vfs: Use file_cred for VOP_DEALLOCATE in vn_deallocate if non-NULL.Aug 30 2021, 6:05 AM
  • 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.
sys/kern/vfs_vnops.c
3576

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.

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.

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

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.

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.

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@.

sys/kern/vfs_vnops.c
3535

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.

3553

cred = file_cred != NOCRED ? file_cred : active_cred;

khng marked an inline comment as done.
  • Comments
  • ternary instead.
This revision is now accepted and ready to land.Sep 1 2021, 12:11 PM