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
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 41251 Build 38140: arc lint + arc unit
Event Timeline
Comment Actions
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.
| sys/kern/vfs_vnops.c | ||
|---|---|---|
| 3575 | why passing fp->f_cred there at all? | |
| sys/kern/vfs_vnops.c | ||
|---|---|---|
| 3575 | 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). | |
Comment Actions
- 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 | ||
|---|---|---|
| 3575 | 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. | |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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 | ||
|---|---|---|
| 3541 | 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; | |