Skip to content

Commit

Permalink
Remove znode's z_uid/z_gid member
Browse files Browse the repository at this point in the history 
Remove duplicate z_uid/z_gid member which are also held in the
generic vfs inode struct. This is done by first removing the members
from struct znode and then using the KUID_TO_SUID/KGID_TO_SGID
macros to access the respective member from struct inode. In cases
where the uid/gids are being marshalled from/to disk, use the newly
introduced zfs_(uid|gid)_(read|write) functions to properly
save the uids rather than the internal kernel representation.

Signed-off-by: Nikolay Borisov <n.borisov.lkml@gmail.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#4685
Issue openzfs#227
  • Loading branch information
@behlendorf
Nikolay Borisov authored and behlendorf committed Jul 25, 2016
1 parent 82a1b2d commit 2c6abf1
Show file tree
Hide file tree
Showing 10 changed files with 69 additions and 54 deletions.
31 changes: 16 additions & 15 deletions include/sys/trace_acl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#define _TRACE_ACL_H

#include <linux/tracepoint.h>
#include <linux/vfs_compat.h>
#include <sys/types.h>

/*
Expand All @@ -56,15 +57,15 @@ DECLARE_EVENT_CLASS(zfs_ace_class,
__field(uint64_t, z_mapcnt)
__field(uint64_t, z_size)
__field(uint64_t, z_pflags)
__field(uint64_t, z_uid)
__field(uint64_t, z_gid)
__field(uint32_t, z_sync_cnt)
__field(mode_t, z_mode)
__field(boolean_t, z_is_sa)
__field(boolean_t, z_is_mapped)
__field(boolean_t, z_is_ctldir)
__field(boolean_t, z_is_stale)

__field(uint32_t, i_uid)
__field(uint32_t, i_gid)
__field(unsigned long, i_ino)
__field(unsigned int, i_nlink)
__field(u64, i_version)
Expand All @@ -91,15 +92,15 @@ DECLARE_EVENT_CLASS(zfs_ace_class,
__entry->z_mapcnt = zn->z_mapcnt;
__entry->z_size = zn->z_size;
__entry->z_pflags = zn->z_pflags;
__entry->z_uid = zn->z_uid;
__entry->z_gid = zn->z_gid;
__entry->z_sync_cnt = zn->z_sync_cnt;
__entry->z_mode = zn->z_mode;
__entry->z_is_sa = zn->z_is_sa;
__entry->z_is_mapped = zn->z_is_mapped;
__entry->z_is_ctldir = zn->z_is_ctldir;
__entry->z_is_stale = zn->z_is_stale;

__entry->i_uid = zfs_uid_read(ZTOI(zn));
__entry->i_gid = zfs_gid_read(ZTOI(zn));

This comment has been minimized.

Sign in to view

Copy link
@tuxoko

tuxoko Aug 3, 2016

@lorddoskias
use KUID

This comment has been minimized.

Sign in to view

Copy link
@lorddoskias

lorddoskias Aug 3, 2016 •
edited
Loading

I'd argue we want to see the UID that the user has set when performing the tracing, rather than the raw KUID.

This comment has been minimized.

Sign in to view

Copy link
@tuxoko

tuxoko Aug 3, 2016

This is not what user has set. The value user set already have been translated in the VFS layer.
__entry->i_ino = zn->z_inode.i_ino;
__entry->i_nlink = zn->z_inode.i_nlink;
__entry->i_version = zn->z_inode.i_version;
Expand All @@ -118,22 +119,22 @@ DECLARE_EVENT_CLASS(zfs_ace_class,
TP_printk("zn { id %llu unlinked %u atime_dirty %u "
"zn_prefetch %u moved %u blksz %u seq %u "
"mapcnt %llu size %llu pflags %llu "
"uid %llu gid %llu sync_cnt %u mode 0x%x is_sa %d "
"sync_cnt %u mode 0x%x is_sa %d "
"is_mapped %d is_ctldir %d is_stale %d inode { "
"ino %lu nlink %u version %llu size %lli blkbits %u "
"bytes %u mode 0x%x generation %x } } ace { type %u "
"flags %u access_mask %u } mask_matched %u",
"uid %u gid %u ino %lu nlink %u version %llu size %lli "
"blkbits %u bytes %u mode 0x%x generation %x } } "
"ace { type %u flags %u access_mask %u } mask_matched %u",
__entry->z_id, __entry->z_unlinked, __entry->z_atime_dirty,
__entry->z_zn_prefetch, __entry->z_moved, __entry->z_blksz,
__entry->z_seq, __entry->z_mapcnt, __entry->z_size,
__entry->z_pflags, __entry->z_uid,
__entry->z_gid, __entry->z_sync_cnt, __entry->z_mode,
__entry->z_pflags, __entry->z_sync_cnt, __entry->z_mode,
__entry->z_is_sa, __entry->z_is_mapped,
__entry->z_is_ctldir, __entry->z_is_stale, __entry->i_ino,
__entry->i_nlink, __entry->i_version, __entry->i_size,
__entry->i_blkbits, __entry->i_bytes, __entry->i_mode,
__entry->i_generation, __entry->z_type, __entry->z_flags,
__entry->z_access_mask, __entry->mask_matched)
__entry->z_is_ctldir, __entry->z_is_stale, __entry->i_uid,
__entry->i_gid, __entry->i_ino, __entry->i_nlink,
__entry->i_version, __entry->i_size, __entry->i_blkbits,
__entry->i_bytes, __entry->i_mode, __entry->i_generation,
__entry->z_type, __entry->z_flags, __entry->z_access_mask,
__entry->mask_matched)
);

#define DEFINE_ACE_EVENT(name) \
Expand Down
2 changes: 0 additions & 2 deletions include/sys/zfs_znode.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,6 @@ typedef struct znode {
uint64_t z_dnodesize; /* dnode size */
uint64_t z_size; /* file size (cached) */
uint64_t z_pflags; /* pflags (cached) */
uint64_t z_uid; /* uid fuid (cached) */
uint64_t z_gid; /* gid fuid (cached) */
uint32_t z_sync_cnt; /* synchronous open count */
mode_t z_mode; /* mode (cached) */
kmutex_t z_acl_lock; /* acl data lock */
Expand Down
27 changes: 17 additions & 10 deletions module/zfs/zfs_acl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
#include <sys/zap.h>
#include <sys/sa.h>
#include <sys/trace_acl.h>
#include <sys/zpl.h>
#include "fs/fs_subr.h"

#define ALLOW ACE_ACCESS_ALLOWED_ACE_TYPE
Expand Down Expand Up @@ -1166,7 +1167,8 @@ zfs_acl_chown_setattr(znode_t *zp)
error = zfs_acl_node_read(zp, B_TRUE, &aclp, B_FALSE);
if (error == 0 && aclp->z_acl_count > 0)
zp->z_mode = zfs_mode_compute(zp->z_mode, aclp,
&zp->z_pflags, zp->z_uid, zp->z_gid);
&zp->z_pflags, KUID_TO_SUID(ZTOI(zp)->i_uid),
KGID_TO_SGID(ZTOI(zp)->i_gid));

/*
* Some ZFS implementations (ZEVO) create neither a ZNODE_ACL
Expand Down Expand Up @@ -1324,7 +1326,7 @@ zfs_aclset_common(znode_t *zp, zfs_acl_t *aclp, cred_t *cr, dmu_tx_t *tx)
mode = zp->z_mode;

mode = zfs_mode_compute(mode, aclp, &zp->z_pflags,
zp->z_uid, zp->z_gid);
zfs_uid_read(ZTOI(zp)), zfs_gid_read(ZTOI(zp)));

This comment has been minimized.

Sign in to view

Copy link
@tuxoko

tuxoko Aug 3, 2016

@lorddoskias
Use KUID

zp->z_mode = mode;
SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_MODE(zsb), NULL,
Expand Down Expand Up @@ -1778,7 +1780,7 @@ zfs_acl_ids_create(znode_t *dzp, int flag, vattr_t *vap, cred_t *cr,
(uint64_t)vap->va_gid,
cr, ZFS_GROUP, &acl_ids->z_fuidp);
gid = vap->va_gid;
if (acl_ids->z_fgid != dzp->z_gid &&
if (acl_ids->z_fgid != KGID_TO_SGID(ZTOI(dzp)->i_gid) &&
!groupmember(vap->va_gid, cr) &&
secpolicy_vnode_create_gid(cr) != 0)
acl_ids->z_fgid = 0;
Expand All @@ -1788,7 +1790,8 @@ zfs_acl_ids_create(znode_t *dzp, int flag, vattr_t *vap, cred_t *cr,
char *domain;
uint32_t rid;

acl_ids->z_fgid = dzp->z_gid;
acl_ids->z_fgid = KGID_TO_SGID(
ZTOI(dzp)->i_gid);
gid = zfs_fuid_map_id(zsb, acl_ids->z_fgid,
cr, ZFS_GROUP);

Expand Down Expand Up @@ -2340,7 +2343,8 @@ zfs_has_access(znode_t *zp, cred_t *cr)
if (zfs_zaccess_aces_check(zp, &have, B_TRUE, cr) != 0) {
uid_t owner;

owner = zfs_fuid_map_id(ZTOZSB(zp), zp->z_uid, cr, ZFS_OWNER);
owner = zfs_fuid_map_id(ZTOZSB(zp),
KUID_TO_SUID(ZTOI(zp)->i_uid), cr, ZFS_OWNER);
return (secpolicy_vnode_any_access(cr, ZTOI(zp), owner) == 0);
}
return (B_TRUE);
Expand Down Expand Up @@ -2418,12 +2422,13 @@ zfs_fastaccesschk_execute(znode_t *zdp, cred_t *cr)
return (0);
}

if (FUID_INDEX(zdp->z_uid) != 0 || FUID_INDEX(zdp->z_gid) != 0) {
if (KUID_TO_SUID(ZTOI(zdp)->i_uid) != 0 ||
KGID_TO_SGID(ZTOI(zdp)->i_gid) != 0) {
mutex_exit(&zdp->z_acl_lock);
goto slow;
}

if (uid == zdp->z_uid) {
if (uid == KUID_TO_SUID(ZTOI(zdp)->i_uid)) {
owner = B_TRUE;
if (zdp->z_mode & S_IXUSR) {
mutex_exit(&zdp->z_acl_lock);
Expand All @@ -2433,7 +2438,7 @@ zfs_fastaccesschk_execute(znode_t *zdp, cred_t *cr)
goto slow;
}
}
if (groupmember(zdp->z_gid, cr)) {
if (groupmember(KGID_TO_SGID(ZTOI(zdp)->i_gid), cr)) {
groupmbr = B_TRUE;
if (zdp->z_mode & S_IXGRP) {
mutex_exit(&zdp->z_acl_lock);
Expand Down Expand Up @@ -2513,7 +2518,8 @@ zfs_zaccess(znode_t *zp, int mode, int flags, boolean_t skipaclchk, cred_t *cr)
}
}

owner = zfs_fuid_map_id(ZTOZSB(zp), zp->z_uid, cr, ZFS_OWNER);
owner = zfs_fuid_map_id(ZTOZSB(zp), KUID_TO_SUID(ZTOI(zp)->i_uid),
cr, ZFS_OWNER);
/*
* Map the bits required to the standard inode flags
* S_IRUSR|S_IWUSR|S_IXUSR in the needed_bits. Map the bits
Expand Down Expand Up @@ -2642,7 +2648,8 @@ zfs_delete_final_check(znode_t *zp, znode_t *dzp,
int error;
uid_t downer;

downer = zfs_fuid_map_id(ZTOZSB(dzp), dzp->z_uid, cr, ZFS_OWNER);
downer = zfs_fuid_map_id(ZTOZSB(dzp), KUID_TO_SUID(ZTOI(dzp)->i_uid),
cr, ZFS_OWNER);

error = secpolicy_vnode_access2(cr, ZTOI(dzp),
downer, available_perms, S_IWUSR|S_IXUSR);
Expand Down
2 changes: 0 additions & 2 deletions module/zfs/zfs_ctldir.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -479,8 +479,6 @@ zfsctl_inode_alloc(zfs_sb_t *zsb, uint64_t id,
zp->z_mapcnt = 0;
zp->z_size = 0;
zp->z_pflags = 0;
zp->z_uid = 0;
zp->z_gid = 0;
zp->z_mode = 0;
zp->z_sync_cnt = 0;
zp->z_is_mapped = B_FALSE;
Expand Down
6 changes: 4 additions & 2 deletions module/zfs/zfs_dir.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1104,8 +1104,10 @@ zfs_sticky_remove_access(znode_t *zdp, znode_t *zp, cred_t *cr)
if ((zdp->z_mode & S_ISVTX) == 0)
return (0);

downer = zfs_fuid_map_id(zsb, zdp->z_uid, cr, ZFS_OWNER);
fowner = zfs_fuid_map_id(zsb, zp->z_uid, cr, ZFS_OWNER);
downer = zfs_fuid_map_id(zsb, KUID_TO_SUID(ZTOI(zdp)->i_uid),
cr, ZFS_OWNER);
fowner = zfs_fuid_map_id(zsb, KUID_TO_SUID(ZTOI(zp)->i_uid),
cr, ZFS_OWNER);

if ((uid = crgetuid(cr)) == downer || uid == fowner ||
(S_ISDIR(ZTOI(zp)->i_mode) &&
Expand Down
6 changes: 4 additions & 2 deletions module/zfs/zfs_fuid.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -387,8 +387,10 @@ zfs_fuid_find_by_idx(zfs_sb_t *zsb, uint32_t idx)
void
zfs_fuid_map_ids(znode_t *zp, cred_t *cr, uid_t *uidp, uid_t *gidp)
{
*uidp = zfs_fuid_map_id(ZTOZSB(zp), zp->z_uid, cr, ZFS_OWNER);
*gidp = zfs_fuid_map_id(ZTOZSB(zp), zp->z_gid, cr, ZFS_GROUP);
*uidp = zfs_fuid_map_id(ZTOZSB(zp), KUID_TO_SUID(ZTOI(zp)->i_uid),
cr, ZFS_OWNER);
*gidp = zfs_fuid_map_id(ZTOZSB(zp), KGID_TO_SGID(ZTOI(zp)->i_gid),
cr, ZFS_GROUP);
}

uid_t
Expand Down
12 changes: 6 additions & 6 deletions module/zfs/zfs_log.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -282,13 +282,13 @@ zfs_log_create(zilog_t *zilog, dmu_tx_t *tx, uint64_t txtype,
/* Store dnode slot count in 8 bits above object id. */
LR_FOID_SET_SLOTS(lr->lr_foid, zp->z_dnodesize >> DNODE_SHIFT);
lr->lr_mode = zp->z_mode;
if (!IS_EPHEMERAL(zp->z_uid)) {
lr->lr_uid = (uint64_t)zp->z_uid;
if (!IS_EPHEMERAL(KUID_TO_SUID(ZTOI(zp)->i_uid))) {
lr->lr_uid = (uint64_t)KUID_TO_SUID(ZTOI(zp)->i_uid);
} else {
lr->lr_uid = fuidp->z_fuid_owner;
}
if (!IS_EPHEMERAL(zp->z_gid)) {
lr->lr_gid = (uint64_t)zp->z_gid;
if (!IS_EPHEMERAL(KGID_TO_SGID(ZTOI(zp)->i_gid))) {
lr->lr_gid = (uint64_t)KGID_TO_SGID(ZTOI(zp)->i_gid);
} else {
lr->lr_gid = fuidp->z_fuid_group;
}
Expand Down Expand Up @@ -407,8 +407,8 @@ zfs_log_symlink(zilog_t *zilog, dmu_tx_t *tx, uint64_t txtype,
lr = (lr_create_t *)&itx->itx_lr;
lr->lr_doid = dzp->z_id;
lr->lr_foid = zp->z_id;
lr->lr_uid = zp->z_uid;
lr->lr_gid = zp->z_gid;
lr->lr_uid = KUID_TO_SUID(ZTOI(zp)->i_uid);
lr->lr_gid = KGID_TO_SGID(ZTOI(zp)->i_gid);
lr->lr_mode = zp->z_mode;
(void) sa_lookup(zp->z_sa_hdl, SA_ZPL_GEN(ZTOZSB(zp)), &lr->lr_gen,
sizeof (uint64_t));
Expand Down
3 changes: 2 additions & 1 deletion module/zfs/zfs_vfsops.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -627,10 +627,11 @@ zfs_owner_overquota(zfs_sb_t *zsb, znode_t *zp, boolean_t isgroup)
{
uint64_t fuid;
uint64_t quotaobj;
struct inode *ip = ZTOI(zp);

quotaobj = isgroup ? zsb->z_groupquota_obj : zsb->z_userquota_obj;

fuid = isgroup ? zp->z_gid : zp->z_uid;
fuid = isgroup ? KGID_TO_SGID(ip->i_gid) : KUID_TO_SUID(ip->i_uid);

if (quotaobj == 0 || zsb->z_replay)
return (B_FALSE);
Expand Down
18 changes: 10 additions & 8 deletions module/zfs/zfs_vnops.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,7 @@ zfs_write(struct inode *ip, uio_t *uio, int ioflag, cred_t *cr)
int count = 0;
sa_bulk_attr_t bulk[4];
uint64_t mtime[2], ctime[2];
uint32_t uid;
ASSERTV(int iovcnt = uio->uio_iovcnt);

/*
Expand Down Expand Up @@ -862,11 +863,12 @@ zfs_write(struct inode *ip, uio_t *uio, int ioflag, cred_t *cr)
* user 0 is not an ephemeral uid.
*/
mutex_enter(&zp->z_acl_lock);
uid = KUID_TO_SUID(ip->i_uid);
if ((zp->z_mode & (S_IXUSR | (S_IXUSR >> 3) |
(S_IXUSR >> 6))) != 0 &&
(zp->z_mode & (S_ISUID | S_ISGID)) != 0 &&
secpolicy_vnode_setid_retain(cr,
(zp->z_mode & S_ISUID) != 0 && zp->z_uid == 0) != 0) {
((zp->z_mode & S_ISUID) != 0 && uid == 0)) != 0) {
uint64_t newmode;
zp->z_mode &= ~(S_ISUID | S_ISGID);
newmode = zp->z_mode;
Expand Down Expand Up @@ -2844,7 +2846,7 @@ zfs_setattr(struct inode *ip, vattr_t *vap, int flags, cred_t *cr)
if (mask & ATTR_UID) {
new_uid = zfs_fuid_create(zsb,
(uint64_t)vap->va_uid, cr, ZFS_OWNER, &fuidp);
if (new_uid != zp->z_uid &&
if (new_uid != KUID_TO_SUID(ZTOI(zp)->i_uid) &&
zfs_fuid_overquota(zsb, B_FALSE, new_uid)) {
if (attrzp)
iput(ZTOI(attrzp));
Expand All @@ -2856,7 +2858,7 @@ zfs_setattr(struct inode *ip, vattr_t *vap, int flags, cred_t *cr)
if (mask & ATTR_GID) {
new_gid = zfs_fuid_create(zsb, (uint64_t)vap->va_gid,
cr, ZFS_GROUP, &fuidp);
if (new_gid != zp->z_gid &&
if (new_gid != KGID_TO_SGID(ZTOI(zp)->i_gid) &&
zfs_fuid_overquota(zsb, B_TRUE, new_gid)) {
if (attrzp)
iput(ZTOI(attrzp));
Expand Down Expand Up @@ -2950,24 +2952,24 @@ zfs_setattr(struct inode *ip, vattr_t *vap, int flags, cred_t *cr)
if (mask & ATTR_UID) {
SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_UID(zsb), NULL,
&new_uid, sizeof (new_uid));

This comment has been minimized.

Sign in to view

Copy link
@tuxoko

tuxoko Aug 3, 2016

@lorddoskias
Use zfs_uid_read

This comment has been minimized.

Sign in to view

Copy link
@lorddoskias

lorddoskias Aug 3, 2016

Can you elaborate what you mean here? Since new_uid is set by zfs_fuid_create which just returns the uid being passed to the vap structure?

This comment has been minimized.

Sign in to view

Copy link
@tuxoko

tuxoko Aug 3, 2016

I've already told you we need to use uid_read/write between inode and SA. Now that we are using s_user_ns rather than init_user_ns, this is especially important.
zp->z_uid = new_uid;
ZTOI(zp)->i_uid = SUID_TO_KUID(new_uid);
if (attrzp) {
SA_ADD_BULK_ATTR(xattr_bulk, xattr_count,
SA_ZPL_UID(zsb), NULL, &new_uid,

This comment has been minimized.

Sign in to view

Copy link
@tuxoko

tuxoko Aug 3, 2016

@lorddoskias
Use zfs_uid_read
sizeof (new_uid));
attrzp->z_uid = new_uid;
ZTOI(attrzp)->i_uid = SUID_TO_KUID(new_uid);
}
}

if (mask & ATTR_GID) {
SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_GID(zsb),
NULL, &new_gid, sizeof (new_gid));

This comment has been minimized.

Sign in to view

Copy link
@tuxoko

tuxoko Aug 3, 2016

@lorddoskias
Use zfs_gid_read
zp->z_gid = new_gid;
ZTOI(zp)->i_gid = SGID_TO_KGID(new_gid);
if (attrzp) {
SA_ADD_BULK_ATTR(xattr_bulk, xattr_count,
SA_ZPL_GID(zsb), NULL, &new_gid,

This comment has been minimized.

Sign in to view

Copy link
@tuxoko

tuxoko Aug 3, 2016

@lorddoskias
Use zfs_gid_read
sizeof (new_gid));
attrzp->z_gid = new_gid;
ZTOI(attrzp)->i_gid = SGID_TO_KGID(new_gid);
}
}
if (!(mask & ATTR_MODE)) {
Expand Down Expand Up @@ -3847,7 +3849,7 @@ zfs_link(struct inode *tdip, struct inode *sip, char *name, cred_t *cr,
return (SET_ERROR(EINVAL));
}

owner = zfs_fuid_map_id(zsb, szp->z_uid, cr, ZFS_OWNER);
owner = zfs_fuid_map_id(zsb, KUID_TO_SUID(sip->i_uid), cr, ZFS_OWNER);
if (owner != crgetuid(cr) && secpolicy_basic_link(cr) != 0) {
ZFS_EXIT(zsb);
return (SET_ERROR(EPERM));
Expand Down
Loading

0 comments on commit 2c6abf1

Please sign in to comment.