* remove the di_uid/di_gid fields from the XFS icdinode
@ 2020-02-18 21:00 Christoph Hellwig
2020-02-18 21:00 ` [PATCH 1/3] xfs: ensure that the inode uid/gid match values match the icdinode ones Christoph Hellwig
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Christoph Hellwig @ 2020-02-18 21:00 UTC (permalink / raw)
To: linux-xfs; +Cc: linux-fsdevel
Hi all,
this series removes the duplicate di_uid/di_gid fields in the XFS
icdinode and alwasys uses the VFS inode fields. It also matches the
user namespace used by other file systems instead of always using
init_user_ns. For now that change is mostly theoretical, but it might
be more useful with some of the pending VFS changes in that area.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] xfs: ensure that the inode uid/gid match values match the icdinode ones
2020-02-18 21:00 remove the di_uid/di_gid fields from the XFS icdinode Christoph Hellwig
@ 2020-02-18 21:00 ` Christoph Hellwig
2020-02-19 10:32 ` Carlos Maiolino
` (2 more replies)
2020-02-18 21:00 ` [PATCH 2/3] xfs: remove the icdinode di_uid/di_gid members Christoph Hellwig
2020-02-18 21:00 ` [PATCH 3/3] xfs: remove the kuid/kgid conversion wrappers Christoph Hellwig
2 siblings, 3 replies; 16+ messages in thread
From: Christoph Hellwig @ 2020-02-18 21:00 UTC (permalink / raw)
To: linux-xfs; +Cc: linux-fsdevel
Instead of only synchronizing the uid/gid values in xfs_setup_inode,
ensure that they always match to prepare for removing the icdinode
fields.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_inode_buf.c | 2 ++
fs/xfs/xfs_icache.c | 4 ++++
fs/xfs/xfs_inode.c | 8 ++++++--
fs/xfs/xfs_iops.c | 3 ---
4 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 8afacfe4be0a..cc4efd34843a 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -223,7 +223,9 @@ xfs_inode_from_disk(
to->di_format = from->di_format;
to->di_uid = be32_to_cpu(from->di_uid);
+ inode->i_uid = xfs_uid_to_kuid(to->di_uid);
to->di_gid = be32_to_cpu(from->di_gid);
+ inode->i_gid = xfs_gid_to_kgid(to->di_gid);
to->di_flushiter = be16_to_cpu(from->di_flushiter);
/*
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 8dc2e5414276..a7be7a9e5c1a 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -289,6 +289,8 @@ xfs_reinit_inode(
uint64_t version = inode_peek_iversion(inode);
umode_t mode = inode->i_mode;
dev_t dev = inode->i_rdev;
+ kuid_t uid = inode->i_uid;
+ kgid_t gid = inode->i_gid;
error = inode_init_always(mp->m_super, inode);
@@ -297,6 +299,8 @@ xfs_reinit_inode(
inode_set_iversion_queried(inode, version);
inode->i_mode = mode;
inode->i_rdev = dev;
+ inode->i_uid = uid;
+ inode->i_gid = gid;
return error;
}
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index c5077e6326c7..938b0943bd95 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -812,15 +812,19 @@ xfs_ialloc(
inode->i_mode = mode;
set_nlink(inode, nlink);
- ip->i_d.di_uid = xfs_kuid_to_uid(current_fsuid());
- ip->i_d.di_gid = xfs_kgid_to_gid(current_fsgid());
+ inode->i_uid = current_fsuid();
+ ip->i_d.di_uid = xfs_kuid_to_uid(inode->i_uid);
inode->i_rdev = rdev;
ip->i_d.di_projid = prid;
if (pip && XFS_INHERIT_GID(pip)) {
+ inode->i_gid = VFS_I(pip)->i_gid;
ip->i_d.di_gid = pip->i_d.di_gid;
if ((VFS_I(pip)->i_mode & S_ISGID) && S_ISDIR(mode))
inode->i_mode |= S_ISGID;
+ } else {
+ inode->i_gid = current_fsgid();
+ ip->i_d.di_gid = xfs_kgid_to_gid(inode->i_gid);
}
/*
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 81f2f93caec0..b818b261918f 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -1304,9 +1304,6 @@ xfs_setup_inode(
/* make the inode look hashed for the writeback code */
inode_fake_hash(inode);
- inode->i_uid = xfs_uid_to_kuid(ip->i_d.di_uid);
- inode->i_gid = xfs_gid_to_kgid(ip->i_d.di_gid);
-
i_size_write(inode, ip->i_d.di_size);
xfs_diflags_to_iflags(inode, ip);
--
2.24.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] xfs: remove the icdinode di_uid/di_gid members
2020-02-18 21:00 remove the di_uid/di_gid fields from the XFS icdinode Christoph Hellwig
2020-02-18 21:00 ` [PATCH 1/3] xfs: ensure that the inode uid/gid match values match the icdinode ones Christoph Hellwig
@ 2020-02-18 21:00 ` Christoph Hellwig
2020-02-19 10:51 ` Carlos Maiolino
` (2 more replies)
2020-02-18 21:00 ` [PATCH 3/3] xfs: remove the kuid/kgid conversion wrappers Christoph Hellwig
2 siblings, 3 replies; 16+ messages in thread
From: Christoph Hellwig @ 2020-02-18 21:00 UTC (permalink / raw)
To: linux-xfs; +Cc: linux-fsdevel
Use the Linux inode i_uid/i_gid members everywhere and just convert
from/to the scalar value when reading or writing the on-disk inode.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_inode_buf.c | 10 ++++-----
fs/xfs/libxfs/xfs_inode_buf.h | 2 --
fs/xfs/xfs_dquot.c | 4 ++--
fs/xfs/xfs_inode.c | 14 ++++--------
fs/xfs/xfs_inode_item.c | 4 ++--
fs/xfs/xfs_ioctl.c | 6 +++---
fs/xfs/xfs_iops.c | 6 +-----
fs/xfs/xfs_itable.c | 4 ++--
fs/xfs/xfs_qm.c | 40 ++++++++++++++++++++++-------------
fs/xfs/xfs_quota.h | 4 ++--
fs/xfs/xfs_symlink.c | 4 +---
11 files changed, 46 insertions(+), 52 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index cc4efd34843a..bc72b575ceed 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -222,10 +222,8 @@ xfs_inode_from_disk(
}
to->di_format = from->di_format;
- to->di_uid = be32_to_cpu(from->di_uid);
- inode->i_uid = xfs_uid_to_kuid(to->di_uid);
- to->di_gid = be32_to_cpu(from->di_gid);
- inode->i_gid = xfs_gid_to_kgid(to->di_gid);
+ inode->i_uid = xfs_uid_to_kuid(be32_to_cpu(from->di_uid));
+ inode->i_gid = xfs_gid_to_kgid(be32_to_cpu(from->di_gid));
to->di_flushiter = be16_to_cpu(from->di_flushiter);
/*
@@ -278,8 +276,8 @@ xfs_inode_to_disk(
to->di_version = from->di_version;
to->di_format = from->di_format;
- to->di_uid = cpu_to_be32(from->di_uid);
- to->di_gid = cpu_to_be32(from->di_gid);
+ to->di_uid = cpu_to_be32(xfs_kuid_to_uid(inode->i_uid));
+ to->di_gid = cpu_to_be32(xfs_kgid_to_gid(inode->i_gid));
to->di_projid_lo = cpu_to_be16(from->di_projid & 0xffff);
to->di_projid_hi = cpu_to_be16(from->di_projid >> 16);
diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
index fd94b1078722..2683e1e2c4a6 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.h
+++ b/fs/xfs/libxfs/xfs_inode_buf.h
@@ -19,8 +19,6 @@ struct xfs_icdinode {
int8_t di_version; /* inode version */
int8_t di_format; /* format of di_c data */
uint16_t di_flushiter; /* incremented on flush */
- uint32_t di_uid; /* owner's user id */
- uint32_t di_gid; /* owner's group id */
uint32_t di_projid; /* owner's project id */
xfs_fsize_t di_size; /* number of bytes in file */
xfs_rfsblock_t di_nblocks; /* # of direct & btree blocks used */
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index d223e1ae90a6..3579de9306c1 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -829,9 +829,9 @@ xfs_qm_id_for_quotatype(
{
switch (type) {
case XFS_DQ_USER:
- return ip->i_d.di_uid;
+ return xfs_kuid_to_uid(VFS_I(ip)->i_uid);
case XFS_DQ_GROUP:
- return ip->i_d.di_gid;
+ return xfs_kgid_to_gid(VFS_I(ip)->i_gid);
case XFS_DQ_PROJ:
return ip->i_d.di_projid;
}
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 938b0943bd95..3324e1696354 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -813,18 +813,15 @@ xfs_ialloc(
inode->i_mode = mode;
set_nlink(inode, nlink);
inode->i_uid = current_fsuid();
- ip->i_d.di_uid = xfs_kuid_to_uid(inode->i_uid);
inode->i_rdev = rdev;
ip->i_d.di_projid = prid;
if (pip && XFS_INHERIT_GID(pip)) {
inode->i_gid = VFS_I(pip)->i_gid;
- ip->i_d.di_gid = pip->i_d.di_gid;
if ((VFS_I(pip)->i_mode & S_ISGID) && S_ISDIR(mode))
inode->i_mode |= S_ISGID;
} else {
inode->i_gid = current_fsgid();
- ip->i_d.di_gid = xfs_kgid_to_gid(inode->i_gid);
}
/*
@@ -832,9 +829,8 @@ xfs_ialloc(
* ID or one of the supplementary group IDs, the S_ISGID bit is cleared
* (and only if the irix_sgid_inherit compatibility variable is set).
*/
- if ((irix_sgid_inherit) &&
- (inode->i_mode & S_ISGID) &&
- (!in_group_p(xfs_gid_to_kgid(ip->i_d.di_gid))))
+ if (irix_sgid_inherit &&
+ (inode->i_mode & S_ISGID) && !in_group_p(inode->i_gid))
inode->i_mode &= ~S_ISGID;
ip->i_d.di_size = 0;
@@ -1162,8 +1158,7 @@ xfs_create(
/*
* Make sure that we have allocated dquot(s) on disk.
*/
- error = xfs_qm_vop_dqalloc(dp, xfs_kuid_to_uid(current_fsuid()),
- xfs_kgid_to_gid(current_fsgid()), prid,
+ error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
&udqp, &gdqp, &pdqp);
if (error)
@@ -1313,8 +1308,7 @@ xfs_create_tmpfile(
/*
* Make sure that we have allocated dquot(s) on disk.
*/
- error = xfs_qm_vop_dqalloc(dp, xfs_kuid_to_uid(current_fsuid()),
- xfs_kgid_to_gid(current_fsgid()), prid,
+ error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
&udqp, &gdqp, &pdqp);
if (error)
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 8bd5d0de6321..83d7914556ef 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -308,8 +308,8 @@ xfs_inode_to_log_dinode(
to->di_version = from->di_version;
to->di_format = from->di_format;
- to->di_uid = from->di_uid;
- to->di_gid = from->di_gid;
+ to->di_uid = xfs_kuid_to_uid(inode->i_uid);
+ to->di_gid = xfs_kgid_to_gid(inode->i_gid);
to->di_projid_lo = from->di_projid & 0xffff;
to->di_projid_hi = from->di_projid >> 16;
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index d42de92cb283..0f85bedc5977 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1434,9 +1434,9 @@ xfs_ioctl_setattr(
* because the i_*dquot fields will get updated anyway.
*/
if (XFS_IS_QUOTA_ON(mp)) {
- code = xfs_qm_vop_dqalloc(ip, ip->i_d.di_uid,
- ip->i_d.di_gid, fa->fsx_projid,
- XFS_QMOPT_PQUOTA, &udqp, NULL, &pdqp);
+ code = xfs_qm_vop_dqalloc(ip, VFS_I(ip)->i_uid,
+ VFS_I(ip)->i_gid, fa->fsx_projid,
+ XFS_QMOPT_PQUOTA, &udqp, NULL, &pdqp);
if (code)
return code;
}
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index b818b261918f..a5b7c3100a2f 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -692,9 +692,7 @@ xfs_setattr_nonsize(
*/
ASSERT(udqp == NULL);
ASSERT(gdqp == NULL);
- error = xfs_qm_vop_dqalloc(ip, xfs_kuid_to_uid(uid),
- xfs_kgid_to_gid(gid),
- ip->i_d.di_projid,
+ error = xfs_qm_vop_dqalloc(ip, uid, gid, ip->i_d.di_projid,
qflags, &udqp, &gdqp, NULL);
if (error)
return error;
@@ -763,7 +761,6 @@ xfs_setattr_nonsize(
olddquot1 = xfs_qm_vop_chown(tp, ip,
&ip->i_udquot, udqp);
}
- ip->i_d.di_uid = xfs_kuid_to_uid(uid);
inode->i_uid = uid;
}
if (!gid_eq(igid, gid)) {
@@ -775,7 +772,6 @@ xfs_setattr_nonsize(
olddquot2 = xfs_qm_vop_chown(tp, ip,
&ip->i_gdquot, gdqp);
}
- ip->i_d.di_gid = xfs_kgid_to_gid(gid);
inode->i_gid = gid;
}
}
diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index 4b31c29b7e6b..497db4160283 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -86,8 +86,8 @@ xfs_bulkstat_one_int(
*/
buf->bs_projectid = ip->i_d.di_projid;
buf->bs_ino = ino;
- buf->bs_uid = dic->di_uid;
- buf->bs_gid = dic->di_gid;
+ buf->bs_uid = xfs_kuid_to_uid(inode->i_uid);
+ buf->bs_gid = xfs_kgid_to_gid(inode->i_gid);
buf->bs_size = dic->di_size;
buf->bs_nlink = inode->i_nlink;
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 0b0909657bad..54dda7d982c9 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -326,16 +326,18 @@ xfs_qm_dqattach_locked(
ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
if (XFS_IS_UQUOTA_ON(mp) && !ip->i_udquot) {
- error = xfs_qm_dqattach_one(ip, ip->i_d.di_uid, XFS_DQ_USER,
- doalloc, &ip->i_udquot);
+ error = xfs_qm_dqattach_one(ip,
+ xfs_kuid_to_uid(VFS_I(ip)->i_uid),
+ XFS_DQ_USER, doalloc, &ip->i_udquot);
if (error)
goto done;
ASSERT(ip->i_udquot);
}
if (XFS_IS_GQUOTA_ON(mp) && !ip->i_gdquot) {
- error = xfs_qm_dqattach_one(ip, ip->i_d.di_gid, XFS_DQ_GROUP,
- doalloc, &ip->i_gdquot);
+ error = xfs_qm_dqattach_one(ip,
+ xfs_kgid_to_gid(VFS_I(ip)->i_gid),
+ XFS_DQ_GROUP, doalloc, &ip->i_gdquot);
if (error)
goto done;
ASSERT(ip->i_gdquot);
@@ -1613,8 +1615,8 @@ xfs_qm_dqfree_one(
int
xfs_qm_vop_dqalloc(
struct xfs_inode *ip,
- xfs_dqid_t uid,
- xfs_dqid_t gid,
+ kuid_t uid,
+ kgid_t gid,
prid_t prid,
uint flags,
struct xfs_dquot **O_udqpp,
@@ -1622,6 +1624,7 @@ xfs_qm_vop_dqalloc(
struct xfs_dquot **O_pdqpp)
{
struct xfs_mount *mp = ip->i_mount;
+ struct inode *inode = VFS_I(ip);
struct xfs_dquot *uq = NULL;
struct xfs_dquot *gq = NULL;
struct xfs_dquot *pq = NULL;
@@ -1635,7 +1638,7 @@ xfs_qm_vop_dqalloc(
xfs_ilock(ip, lockflags);
if ((flags & XFS_QMOPT_INHERIT) && XFS_INHERIT_GID(ip))
- gid = ip->i_d.di_gid;
+ gid = inode->i_gid;
/*
* Attach the dquot(s) to this inode, doing a dquot allocation
@@ -1650,7 +1653,7 @@ xfs_qm_vop_dqalloc(
}
if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp)) {
- if (ip->i_d.di_uid != uid) {
+ if (!uid_eq(inode->i_uid, uid)) {
/*
* What we need is the dquot that has this uid, and
* if we send the inode to dqget, the uid of the inode
@@ -1661,7 +1664,8 @@ xfs_qm_vop_dqalloc(
* holding ilock.
*/
xfs_iunlock(ip, lockflags);
- error = xfs_qm_dqget(mp, uid, XFS_DQ_USER, true, &uq);
+ error = xfs_qm_dqget(mp, xfs_kuid_to_uid(uid),
+ XFS_DQ_USER, true, &uq);
if (error) {
ASSERT(error != -ENOENT);
return error;
@@ -1682,9 +1686,10 @@ xfs_qm_vop_dqalloc(
}
}
if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
- if (ip->i_d.di_gid != gid) {
+ if (!gid_eq(inode->i_gid, gid)) {
xfs_iunlock(ip, lockflags);
- error = xfs_qm_dqget(mp, gid, XFS_DQ_GROUP, true, &gq);
+ error = xfs_qm_dqget(mp, xfs_kgid_to_gid(gid),
+ XFS_DQ_GROUP, true, &gq);
if (error) {
ASSERT(error != -ENOENT);
goto error_rele;
@@ -1810,7 +1815,8 @@ xfs_qm_vop_chown_reserve(
XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
if (XFS_IS_UQUOTA_ON(mp) && udqp &&
- ip->i_d.di_uid != be32_to_cpu(udqp->q_core.d_id)) {
+ xfs_kuid_to_uid(VFS_I(ip)->i_uid) !=
+ be32_to_cpu(udqp->q_core.d_id)) {
udq_delblks = udqp;
/*
* If there are delayed allocation blocks, then we have to
@@ -1823,7 +1829,8 @@ xfs_qm_vop_chown_reserve(
}
}
if (XFS_IS_GQUOTA_ON(ip->i_mount) && gdqp &&
- ip->i_d.di_gid != be32_to_cpu(gdqp->q_core.d_id)) {
+ xfs_kgid_to_gid(VFS_I(ip)->i_gid) !=
+ be32_to_cpu(gdqp->q_core.d_id)) {
gdq_delblks = gdqp;
if (delblks) {
ASSERT(ip->i_gdquot);
@@ -1920,14 +1927,17 @@ xfs_qm_vop_create_dqattach(
if (udqp && XFS_IS_UQUOTA_ON(mp)) {
ASSERT(ip->i_udquot == NULL);
- ASSERT(ip->i_d.di_uid == be32_to_cpu(udqp->q_core.d_id));
+ ASSERT(xfs_kuid_to_uid(VFS_I(ip)->i_uid) ==
+ be32_to_cpu(udqp->q_core.d_id));
ip->i_udquot = xfs_qm_dqhold(udqp);
xfs_trans_mod_dquot(tp, udqp, XFS_TRANS_DQ_ICOUNT, 1);
}
if (gdqp && XFS_IS_GQUOTA_ON(mp)) {
ASSERT(ip->i_gdquot == NULL);
- ASSERT(ip->i_d.di_gid == be32_to_cpu(gdqp->q_core.d_id));
+ ASSERT(xfs_kgid_to_gid(VFS_I(ip)->i_gid) ==
+ be32_to_cpu(gdqp->q_core.d_id));
+
ip->i_gdquot = xfs_qm_dqhold(gdqp);
xfs_trans_mod_dquot(tp, gdqp, XFS_TRANS_DQ_ICOUNT, 1);
}
diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
index efe42ae7a2f3..aa8fc1f55fbd 100644
--- a/fs/xfs/xfs_quota.h
+++ b/fs/xfs/xfs_quota.h
@@ -86,7 +86,7 @@ extern int xfs_trans_reserve_quota_bydquots(struct xfs_trans *,
struct xfs_mount *, struct xfs_dquot *,
struct xfs_dquot *, struct xfs_dquot *, int64_t, long, uint);
-extern int xfs_qm_vop_dqalloc(struct xfs_inode *, xfs_dqid_t, xfs_dqid_t,
+extern int xfs_qm_vop_dqalloc(struct xfs_inode *, kuid_t, kgid_t,
prid_t, uint, struct xfs_dquot **, struct xfs_dquot **,
struct xfs_dquot **);
extern void xfs_qm_vop_create_dqattach(struct xfs_trans *, struct xfs_inode *,
@@ -109,7 +109,7 @@ extern void xfs_qm_unmount_quotas(struct xfs_mount *);
#else
static inline int
-xfs_qm_vop_dqalloc(struct xfs_inode *ip, xfs_dqid_t uid, xfs_dqid_t gid,
+xfs_qm_vop_dqalloc(struct xfs_inode *ip, kuid_t kuid, kgid_t kgid,
prid_t prid, uint flags, struct xfs_dquot **udqp,
struct xfs_dquot **gdqp, struct xfs_dquot **pdqp)
{
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index d762d42ed0ff..ea42e25ec1bf 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -182,9 +182,7 @@ xfs_symlink(
/*
* Make sure that we have allocated dquot(s) on disk.
*/
- error = xfs_qm_vop_dqalloc(dp,
- xfs_kuid_to_uid(current_fsuid()),
- xfs_kgid_to_gid(current_fsgid()), prid,
+ error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
&udqp, &gdqp, &pdqp);
if (error)
--
2.24.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] xfs: remove the kuid/kgid conversion wrappers
2020-02-18 21:00 remove the di_uid/di_gid fields from the XFS icdinode Christoph Hellwig
2020-02-18 21:00 ` [PATCH 1/3] xfs: ensure that the inode uid/gid match values match the icdinode ones Christoph Hellwig
2020-02-18 21:00 ` [PATCH 2/3] xfs: remove the icdinode di_uid/di_gid members Christoph Hellwig
@ 2020-02-18 21:00 ` Christoph Hellwig
2020-02-19 11:05 ` Carlos Maiolino
` (2 more replies)
2 siblings, 3 replies; 16+ messages in thread
From: Christoph Hellwig @ 2020-02-18 21:00 UTC (permalink / raw)
To: linux-xfs; +Cc: linux-fsdevel
Remove the XFS wrappers for converting from and to the kuid/kgid types.
Mostly this means switching to VFS i_{u,g}id_{read,write} helpers, but
in a few spots the calls to the conversion functions is open coded.
To match the use of sb->s_user_ns in the helpers and other file systems,
sb->s_user_ns is also used in the quota code. The ACL code already does
the conversion in a grotty layering violation in the VFS xattr code,
so it keeps using init_user_ns for the identity mapping.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_inode_buf.c | 8 ++++----
fs/xfs/xfs_acl.c | 12 ++++++++----
fs/xfs/xfs_dquot.c | 4 ++--
fs/xfs/xfs_inode_item.c | 4 ++--
fs/xfs/xfs_itable.c | 4 ++--
fs/xfs/xfs_linux.h | 26 --------------------------
fs/xfs/xfs_qm.c | 23 +++++++++--------------
7 files changed, 27 insertions(+), 54 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index bc72b575ceed..17e88a8c8353 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -222,8 +222,8 @@ xfs_inode_from_disk(
}
to->di_format = from->di_format;
- inode->i_uid = xfs_uid_to_kuid(be32_to_cpu(from->di_uid));
- inode->i_gid = xfs_gid_to_kgid(be32_to_cpu(from->di_gid));
+ i_uid_write(inode, be32_to_cpu(from->di_uid));
+ i_gid_write(inode, be32_to_cpu(from->di_gid));
to->di_flushiter = be16_to_cpu(from->di_flushiter);
/*
@@ -276,8 +276,8 @@ xfs_inode_to_disk(
to->di_version = from->di_version;
to->di_format = from->di_format;
- to->di_uid = cpu_to_be32(xfs_kuid_to_uid(inode->i_uid));
- to->di_gid = cpu_to_be32(xfs_kgid_to_gid(inode->i_gid));
+ to->di_uid = cpu_to_be32(i_uid_read(inode));
+ to->di_gid = cpu_to_be32(i_gid_read(inode));
to->di_projid_lo = cpu_to_be16(from->di_projid & 0xffff);
to->di_projid_hi = cpu_to_be16(from->di_projid >> 16);
diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index cd743fad8478..e7314b525b19 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -67,10 +67,12 @@ xfs_acl_from_disk(
switch (acl_e->e_tag) {
case ACL_USER:
- acl_e->e_uid = xfs_uid_to_kuid(be32_to_cpu(ace->ae_id));
+ acl_e->e_uid = make_kuid(&init_user_ns,
+ be32_to_cpu(ace->ae_id));
break;
case ACL_GROUP:
- acl_e->e_gid = xfs_gid_to_kgid(be32_to_cpu(ace->ae_id));
+ acl_e->e_gid = make_kgid(&init_user_ns,
+ be32_to_cpu(ace->ae_id));
break;
case ACL_USER_OBJ:
case ACL_GROUP_OBJ:
@@ -103,10 +105,12 @@ xfs_acl_to_disk(struct xfs_acl *aclp, const struct posix_acl *acl)
ace->ae_tag = cpu_to_be32(acl_e->e_tag);
switch (acl_e->e_tag) {
case ACL_USER:
- ace->ae_id = cpu_to_be32(xfs_kuid_to_uid(acl_e->e_uid));
+ ace->ae_id = cpu_to_be32(
+ from_kuid(&init_user_ns, acl_e->e_uid));
break;
case ACL_GROUP:
- ace->ae_id = cpu_to_be32(xfs_kgid_to_gid(acl_e->e_gid));
+ ace->ae_id = cpu_to_be32(
+ from_kgid(&init_user_ns, acl_e->e_gid));
break;
default:
ace->ae_id = cpu_to_be32(ACL_UNDEFINED_ID);
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 3579de9306c1..711376ca269f 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -829,9 +829,9 @@ xfs_qm_id_for_quotatype(
{
switch (type) {
case XFS_DQ_USER:
- return xfs_kuid_to_uid(VFS_I(ip)->i_uid);
+ return i_uid_read(VFS_I(ip));
case XFS_DQ_GROUP:
- return xfs_kgid_to_gid(VFS_I(ip)->i_gid);
+ return i_gid_read(VFS_I(ip));
case XFS_DQ_PROJ:
return ip->i_d.di_projid;
}
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 83d7914556ef..f021b55a0301 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -308,8 +308,8 @@ xfs_inode_to_log_dinode(
to->di_version = from->di_version;
to->di_format = from->di_format;
- to->di_uid = xfs_kuid_to_uid(inode->i_uid);
- to->di_gid = xfs_kgid_to_gid(inode->i_gid);
+ to->di_uid = i_uid_read(inode);
+ to->di_gid = i_gid_read(inode);
to->di_projid_lo = from->di_projid & 0xffff;
to->di_projid_hi = from->di_projid >> 16;
diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index 497db4160283..d10660469884 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -86,8 +86,8 @@ xfs_bulkstat_one_int(
*/
buf->bs_projectid = ip->i_d.di_projid;
buf->bs_ino = ino;
- buf->bs_uid = xfs_kuid_to_uid(inode->i_uid);
- buf->bs_gid = xfs_kgid_to_gid(inode->i_gid);
+ buf->bs_uid = i_uid_read(inode);
+ buf->bs_gid = i_gid_read(inode);
buf->bs_size = dic->di_size;
buf->bs_nlink = inode->i_nlink;
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index 8738bb03f253..bc43cd98697b 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -163,32 +163,6 @@ struct xstats {
extern struct xstats xfsstats;
-/* Kernel uid/gid conversion. These are used to convert to/from the on disk
- * uid_t/gid_t types to the kuid_t/kgid_t types that the kernel uses internally.
- * The conversion here is type only, the value will remain the same since we
- * are converting to the init_user_ns. The uid is later mapped to a particular
- * user namespace value when crossing the kernel/user boundary.
- */
-static inline uint32_t xfs_kuid_to_uid(kuid_t uid)
-{
- return from_kuid(&init_user_ns, uid);
-}
-
-static inline kuid_t xfs_uid_to_kuid(uint32_t uid)
-{
- return make_kuid(&init_user_ns, uid);
-}
-
-static inline uint32_t xfs_kgid_to_gid(kgid_t gid)
-{
- return from_kgid(&init_user_ns, gid);
-}
-
-static inline kgid_t xfs_gid_to_kgid(uint32_t gid)
-{
- return make_kgid(&init_user_ns, gid);
-}
-
static inline dev_t xfs_to_linux_dev_t(xfs_dev_t dev)
{
return MKDEV(sysv_major(dev) & 0x1ff, sysv_minor(dev));
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 54dda7d982c9..de1d2c606c14 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -326,8 +326,7 @@ xfs_qm_dqattach_locked(
ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
if (XFS_IS_UQUOTA_ON(mp) && !ip->i_udquot) {
- error = xfs_qm_dqattach_one(ip,
- xfs_kuid_to_uid(VFS_I(ip)->i_uid),
+ error = xfs_qm_dqattach_one(ip, i_uid_read(VFS_I(ip)),
XFS_DQ_USER, doalloc, &ip->i_udquot);
if (error)
goto done;
@@ -335,8 +334,7 @@ xfs_qm_dqattach_locked(
}
if (XFS_IS_GQUOTA_ON(mp) && !ip->i_gdquot) {
- error = xfs_qm_dqattach_one(ip,
- xfs_kgid_to_gid(VFS_I(ip)->i_gid),
+ error = xfs_qm_dqattach_one(ip, i_gid_read(VFS_I(ip)),
XFS_DQ_GROUP, doalloc, &ip->i_gdquot);
if (error)
goto done;
@@ -1625,6 +1623,7 @@ xfs_qm_vop_dqalloc(
{
struct xfs_mount *mp = ip->i_mount;
struct inode *inode = VFS_I(ip);
+ struct user_namespace *user_ns = inode->i_sb->s_user_ns;
struct xfs_dquot *uq = NULL;
struct xfs_dquot *gq = NULL;
struct xfs_dquot *pq = NULL;
@@ -1664,7 +1663,7 @@ xfs_qm_vop_dqalloc(
* holding ilock.
*/
xfs_iunlock(ip, lockflags);
- error = xfs_qm_dqget(mp, xfs_kuid_to_uid(uid),
+ error = xfs_qm_dqget(mp, from_kuid(user_ns, uid),
XFS_DQ_USER, true, &uq);
if (error) {
ASSERT(error != -ENOENT);
@@ -1688,7 +1687,7 @@ xfs_qm_vop_dqalloc(
if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
if (!gid_eq(inode->i_gid, gid)) {
xfs_iunlock(ip, lockflags);
- error = xfs_qm_dqget(mp, xfs_kgid_to_gid(gid),
+ error = xfs_qm_dqget(mp, from_kgid(user_ns, gid),
XFS_DQ_GROUP, true, &gq);
if (error) {
ASSERT(error != -ENOENT);
@@ -1815,8 +1814,7 @@ xfs_qm_vop_chown_reserve(
XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
if (XFS_IS_UQUOTA_ON(mp) && udqp &&
- xfs_kuid_to_uid(VFS_I(ip)->i_uid) !=
- be32_to_cpu(udqp->q_core.d_id)) {
+ i_uid_read(VFS_I(ip)) != be32_to_cpu(udqp->q_core.d_id)) {
udq_delblks = udqp;
/*
* If there are delayed allocation blocks, then we have to
@@ -1829,8 +1827,7 @@ xfs_qm_vop_chown_reserve(
}
}
if (XFS_IS_GQUOTA_ON(ip->i_mount) && gdqp &&
- xfs_kgid_to_gid(VFS_I(ip)->i_gid) !=
- be32_to_cpu(gdqp->q_core.d_id)) {
+ i_gid_read(VFS_I(ip)) != be32_to_cpu(gdqp->q_core.d_id)) {
gdq_delblks = gdqp;
if (delblks) {
ASSERT(ip->i_gdquot);
@@ -1927,16 +1924,14 @@ xfs_qm_vop_create_dqattach(
if (udqp && XFS_IS_UQUOTA_ON(mp)) {
ASSERT(ip->i_udquot == NULL);
- ASSERT(xfs_kuid_to_uid(VFS_I(ip)->i_uid) ==
- be32_to_cpu(udqp->q_core.d_id));
+ ASSERT(i_uid_read(VFS_I(ip)) == be32_to_cpu(udqp->q_core.d_id));
ip->i_udquot = xfs_qm_dqhold(udqp);
xfs_trans_mod_dquot(tp, udqp, XFS_TRANS_DQ_ICOUNT, 1);
}
if (gdqp && XFS_IS_GQUOTA_ON(mp)) {
ASSERT(ip->i_gdquot == NULL);
- ASSERT(xfs_kgid_to_gid(VFS_I(ip)->i_gid) ==
- be32_to_cpu(gdqp->q_core.d_id));
+ ASSERT(i_gid_read(VFS_I(ip)) == be32_to_cpu(gdqp->q_core.d_id));
ip->i_gdquot = xfs_qm_dqhold(gdqp);
xfs_trans_mod_dquot(tp, gdqp, XFS_TRANS_DQ_ICOUNT, 1);
--
2.24.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] xfs: ensure that the inode uid/gid match values match the icdinode ones
2020-02-18 21:00 ` [PATCH 1/3] xfs: ensure that the inode uid/gid match values match the icdinode ones Christoph Hellwig
@ 2020-02-19 10:32 ` Carlos Maiolino
2020-02-19 14:26 ` Brian Foster
2020-02-19 14:47 ` Chandan Rajendra
2 siblings, 0 replies; 16+ messages in thread
From: Carlos Maiolino @ 2020-02-19 10:32 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, linux-fsdevel
On Tue, Feb 18, 2020 at 01:00:18PM -0800, Christoph Hellwig wrote:
> Instead of only synchronizing the uid/gid values in xfs_setup_inode,
> ensure that they always match to prepare for removing the icdinode
> fields.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> fs/xfs/libxfs/xfs_inode_buf.c | 2 ++
> fs/xfs/xfs_icache.c | 4 ++++
> fs/xfs/xfs_inode.c | 8 ++++++--
> fs/xfs/xfs_iops.c | 3 ---
> 4 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 8afacfe4be0a..cc4efd34843a 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -223,7 +223,9 @@ xfs_inode_from_disk(
>
> to->di_format = from->di_format;
> to->di_uid = be32_to_cpu(from->di_uid);
> + inode->i_uid = xfs_uid_to_kuid(to->di_uid);
> to->di_gid = be32_to_cpu(from->di_gid);
> + inode->i_gid = xfs_gid_to_kgid(to->di_gid);
> to->di_flushiter = be16_to_cpu(from->di_flushiter);
>
> /*
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 8dc2e5414276..a7be7a9e5c1a 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -289,6 +289,8 @@ xfs_reinit_inode(
> uint64_t version = inode_peek_iversion(inode);
> umode_t mode = inode->i_mode;
> dev_t dev = inode->i_rdev;
> + kuid_t uid = inode->i_uid;
> + kgid_t gid = inode->i_gid;
>
> error = inode_init_always(mp->m_super, inode);
>
> @@ -297,6 +299,8 @@ xfs_reinit_inode(
> inode_set_iversion_queried(inode, version);
> inode->i_mode = mode;
> inode->i_rdev = dev;
> + inode->i_uid = uid;
> + inode->i_gid = gid;
> return error;
> }
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c5077e6326c7..938b0943bd95 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -812,15 +812,19 @@ xfs_ialloc(
>
> inode->i_mode = mode;
> set_nlink(inode, nlink);
> - ip->i_d.di_uid = xfs_kuid_to_uid(current_fsuid());
> - ip->i_d.di_gid = xfs_kgid_to_gid(current_fsgid());
> + inode->i_uid = current_fsuid();
> + ip->i_d.di_uid = xfs_kuid_to_uid(inode->i_uid);
> inode->i_rdev = rdev;
> ip->i_d.di_projid = prid;
>
> if (pip && XFS_INHERIT_GID(pip)) {
> + inode->i_gid = VFS_I(pip)->i_gid;
> ip->i_d.di_gid = pip->i_d.di_gid;
> if ((VFS_I(pip)->i_mode & S_ISGID) && S_ISDIR(mode))
> inode->i_mode |= S_ISGID;
> + } else {
> + inode->i_gid = current_fsgid();
> + ip->i_d.di_gid = xfs_kgid_to_gid(inode->i_gid);
> }
>
> /*
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 81f2f93caec0..b818b261918f 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -1304,9 +1304,6 @@ xfs_setup_inode(
> /* make the inode look hashed for the writeback code */
> inode_fake_hash(inode);
>
> - inode->i_uid = xfs_uid_to_kuid(ip->i_d.di_uid);
> - inode->i_gid = xfs_gid_to_kgid(ip->i_d.di_gid);
> -
> i_size_write(inode, ip->i_d.di_size);
> xfs_diflags_to_iflags(inode, ip);
>
> --
> 2.24.1
>
--
Carlos
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] xfs: remove the icdinode di_uid/di_gid members
2020-02-18 21:00 ` [PATCH 2/3] xfs: remove the icdinode di_uid/di_gid members Christoph Hellwig
@ 2020-02-19 10:51 ` Carlos Maiolino
2020-02-19 14:26 ` Brian Foster
2020-02-19 16:25 ` Chandan Rajendra
2 siblings, 0 replies; 16+ messages in thread
From: Carlos Maiolino @ 2020-02-19 10:51 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, linux-fsdevel
On Tue, Feb 18, 2020 at 01:00:19PM -0800, Christoph Hellwig wrote:
> Use the Linux inode i_uid/i_gid members everywhere and just convert
> from/to the scalar value when reading or writing the on-disk inode.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> fs/xfs/libxfs/xfs_inode_buf.c | 10 ++++-----
> fs/xfs/libxfs/xfs_inode_buf.h | 2 --
> fs/xfs/xfs_dquot.c | 4 ++--
> fs/xfs/xfs_inode.c | 14 ++++--------
> fs/xfs/xfs_inode_item.c | 4 ++--
> fs/xfs/xfs_ioctl.c | 6 +++---
> fs/xfs/xfs_iops.c | 6 +-----
> fs/xfs/xfs_itable.c | 4 ++--
> fs/xfs/xfs_qm.c | 40 ++++++++++++++++++++++-------------
> fs/xfs/xfs_quota.h | 4 ++--
> fs/xfs/xfs_symlink.c | 4 +---
> 11 files changed, 46 insertions(+), 52 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index cc4efd34843a..bc72b575ceed 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -222,10 +222,8 @@ xfs_inode_from_disk(
> }
>
> to->di_format = from->di_format;
> - to->di_uid = be32_to_cpu(from->di_uid);
> - inode->i_uid = xfs_uid_to_kuid(to->di_uid);
> - to->di_gid = be32_to_cpu(from->di_gid);
> - inode->i_gid = xfs_gid_to_kgid(to->di_gid);
> + inode->i_uid = xfs_uid_to_kuid(be32_to_cpu(from->di_uid));
> + inode->i_gid = xfs_gid_to_kgid(be32_to_cpu(from->di_gid));
> to->di_flushiter = be16_to_cpu(from->di_flushiter);
>
> /*
> @@ -278,8 +276,8 @@ xfs_inode_to_disk(
>
> to->di_version = from->di_version;
> to->di_format = from->di_format;
> - to->di_uid = cpu_to_be32(from->di_uid);
> - to->di_gid = cpu_to_be32(from->di_gid);
> + to->di_uid = cpu_to_be32(xfs_kuid_to_uid(inode->i_uid));
> + to->di_gid = cpu_to_be32(xfs_kgid_to_gid(inode->i_gid));
> to->di_projid_lo = cpu_to_be16(from->di_projid & 0xffff);
> to->di_projid_hi = cpu_to_be16(from->di_projid >> 16);
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
> index fd94b1078722..2683e1e2c4a6 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.h
> +++ b/fs/xfs/libxfs/xfs_inode_buf.h
> @@ -19,8 +19,6 @@ struct xfs_icdinode {
> int8_t di_version; /* inode version */
> int8_t di_format; /* format of di_c data */
> uint16_t di_flushiter; /* incremented on flush */
> - uint32_t di_uid; /* owner's user id */
> - uint32_t di_gid; /* owner's group id */
> uint32_t di_projid; /* owner's project id */
> xfs_fsize_t di_size; /* number of bytes in file */
> xfs_rfsblock_t di_nblocks; /* # of direct & btree blocks used */
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index d223e1ae90a6..3579de9306c1 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -829,9 +829,9 @@ xfs_qm_id_for_quotatype(
> {
> switch (type) {
> case XFS_DQ_USER:
> - return ip->i_d.di_uid;
> + return xfs_kuid_to_uid(VFS_I(ip)->i_uid);
> case XFS_DQ_GROUP:
> - return ip->i_d.di_gid;
> + return xfs_kgid_to_gid(VFS_I(ip)->i_gid);
> case XFS_DQ_PROJ:
> return ip->i_d.di_projid;
> }
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 938b0943bd95..3324e1696354 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -813,18 +813,15 @@ xfs_ialloc(
> inode->i_mode = mode;
> set_nlink(inode, nlink);
> inode->i_uid = current_fsuid();
> - ip->i_d.di_uid = xfs_kuid_to_uid(inode->i_uid);
> inode->i_rdev = rdev;
> ip->i_d.di_projid = prid;
>
> if (pip && XFS_INHERIT_GID(pip)) {
> inode->i_gid = VFS_I(pip)->i_gid;
> - ip->i_d.di_gid = pip->i_d.di_gid;
> if ((VFS_I(pip)->i_mode & S_ISGID) && S_ISDIR(mode))
> inode->i_mode |= S_ISGID;
> } else {
> inode->i_gid = current_fsgid();
> - ip->i_d.di_gid = xfs_kgid_to_gid(inode->i_gid);
> }
>
> /*
> @@ -832,9 +829,8 @@ xfs_ialloc(
> * ID or one of the supplementary group IDs, the S_ISGID bit is cleared
> * (and only if the irix_sgid_inherit compatibility variable is set).
> */
> - if ((irix_sgid_inherit) &&
> - (inode->i_mode & S_ISGID) &&
> - (!in_group_p(xfs_gid_to_kgid(ip->i_d.di_gid))))
> + if (irix_sgid_inherit &&
> + (inode->i_mode & S_ISGID) && !in_group_p(inode->i_gid))
> inode->i_mode &= ~S_ISGID;
>
> ip->i_d.di_size = 0;
> @@ -1162,8 +1158,7 @@ xfs_create(
> /*
> * Make sure that we have allocated dquot(s) on disk.
> */
> - error = xfs_qm_vop_dqalloc(dp, xfs_kuid_to_uid(current_fsuid()),
> - xfs_kgid_to_gid(current_fsgid()), prid,
> + error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
> XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> &udqp, &gdqp, &pdqp);
> if (error)
> @@ -1313,8 +1308,7 @@ xfs_create_tmpfile(
> /*
> * Make sure that we have allocated dquot(s) on disk.
> */
> - error = xfs_qm_vop_dqalloc(dp, xfs_kuid_to_uid(current_fsuid()),
> - xfs_kgid_to_gid(current_fsgid()), prid,
> + error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
> XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> &udqp, &gdqp, &pdqp);
> if (error)
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 8bd5d0de6321..83d7914556ef 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -308,8 +308,8 @@ xfs_inode_to_log_dinode(
>
> to->di_version = from->di_version;
> to->di_format = from->di_format;
> - to->di_uid = from->di_uid;
> - to->di_gid = from->di_gid;
> + to->di_uid = xfs_kuid_to_uid(inode->i_uid);
> + to->di_gid = xfs_kgid_to_gid(inode->i_gid);
> to->di_projid_lo = from->di_projid & 0xffff;
> to->di_projid_hi = from->di_projid >> 16;
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index d42de92cb283..0f85bedc5977 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1434,9 +1434,9 @@ xfs_ioctl_setattr(
> * because the i_*dquot fields will get updated anyway.
> */
> if (XFS_IS_QUOTA_ON(mp)) {
> - code = xfs_qm_vop_dqalloc(ip, ip->i_d.di_uid,
> - ip->i_d.di_gid, fa->fsx_projid,
> - XFS_QMOPT_PQUOTA, &udqp, NULL, &pdqp);
> + code = xfs_qm_vop_dqalloc(ip, VFS_I(ip)->i_uid,
> + VFS_I(ip)->i_gid, fa->fsx_projid,
> + XFS_QMOPT_PQUOTA, &udqp, NULL, &pdqp);
> if (code)
> return code;
> }
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index b818b261918f..a5b7c3100a2f 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -692,9 +692,7 @@ xfs_setattr_nonsize(
> */
> ASSERT(udqp == NULL);
> ASSERT(gdqp == NULL);
> - error = xfs_qm_vop_dqalloc(ip, xfs_kuid_to_uid(uid),
> - xfs_kgid_to_gid(gid),
> - ip->i_d.di_projid,
> + error = xfs_qm_vop_dqalloc(ip, uid, gid, ip->i_d.di_projid,
> qflags, &udqp, &gdqp, NULL);
> if (error)
> return error;
> @@ -763,7 +761,6 @@ xfs_setattr_nonsize(
> olddquot1 = xfs_qm_vop_chown(tp, ip,
> &ip->i_udquot, udqp);
> }
> - ip->i_d.di_uid = xfs_kuid_to_uid(uid);
> inode->i_uid = uid;
> }
> if (!gid_eq(igid, gid)) {
> @@ -775,7 +772,6 @@ xfs_setattr_nonsize(
> olddquot2 = xfs_qm_vop_chown(tp, ip,
> &ip->i_gdquot, gdqp);
> }
> - ip->i_d.di_gid = xfs_kgid_to_gid(gid);
> inode->i_gid = gid;
> }
> }
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 4b31c29b7e6b..497db4160283 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -86,8 +86,8 @@ xfs_bulkstat_one_int(
> */
> buf->bs_projectid = ip->i_d.di_projid;
> buf->bs_ino = ino;
> - buf->bs_uid = dic->di_uid;
> - buf->bs_gid = dic->di_gid;
> + buf->bs_uid = xfs_kuid_to_uid(inode->i_uid);
> + buf->bs_gid = xfs_kgid_to_gid(inode->i_gid);
> buf->bs_size = dic->di_size;
>
> buf->bs_nlink = inode->i_nlink;
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 0b0909657bad..54dda7d982c9 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -326,16 +326,18 @@ xfs_qm_dqattach_locked(
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>
> if (XFS_IS_UQUOTA_ON(mp) && !ip->i_udquot) {
> - error = xfs_qm_dqattach_one(ip, ip->i_d.di_uid, XFS_DQ_USER,
> - doalloc, &ip->i_udquot);
> + error = xfs_qm_dqattach_one(ip,
> + xfs_kuid_to_uid(VFS_I(ip)->i_uid),
> + XFS_DQ_USER, doalloc, &ip->i_udquot);
> if (error)
> goto done;
> ASSERT(ip->i_udquot);
> }
>
> if (XFS_IS_GQUOTA_ON(mp) && !ip->i_gdquot) {
> - error = xfs_qm_dqattach_one(ip, ip->i_d.di_gid, XFS_DQ_GROUP,
> - doalloc, &ip->i_gdquot);
> + error = xfs_qm_dqattach_one(ip,
> + xfs_kgid_to_gid(VFS_I(ip)->i_gid),
> + XFS_DQ_GROUP, doalloc, &ip->i_gdquot);
> if (error)
> goto done;
> ASSERT(ip->i_gdquot);
> @@ -1613,8 +1615,8 @@ xfs_qm_dqfree_one(
> int
> xfs_qm_vop_dqalloc(
> struct xfs_inode *ip,
> - xfs_dqid_t uid,
> - xfs_dqid_t gid,
> + kuid_t uid,
> + kgid_t gid,
> prid_t prid,
> uint flags,
> struct xfs_dquot **O_udqpp,
> @@ -1622,6 +1624,7 @@ xfs_qm_vop_dqalloc(
> struct xfs_dquot **O_pdqpp)
> {
> struct xfs_mount *mp = ip->i_mount;
> + struct inode *inode = VFS_I(ip);
> struct xfs_dquot *uq = NULL;
> struct xfs_dquot *gq = NULL;
> struct xfs_dquot *pq = NULL;
> @@ -1635,7 +1638,7 @@ xfs_qm_vop_dqalloc(
> xfs_ilock(ip, lockflags);
>
> if ((flags & XFS_QMOPT_INHERIT) && XFS_INHERIT_GID(ip))
> - gid = ip->i_d.di_gid;
> + gid = inode->i_gid;
>
> /*
> * Attach the dquot(s) to this inode, doing a dquot allocation
> @@ -1650,7 +1653,7 @@ xfs_qm_vop_dqalloc(
> }
>
> if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp)) {
> - if (ip->i_d.di_uid != uid) {
> + if (!uid_eq(inode->i_uid, uid)) {
> /*
> * What we need is the dquot that has this uid, and
> * if we send the inode to dqget, the uid of the inode
> @@ -1661,7 +1664,8 @@ xfs_qm_vop_dqalloc(
> * holding ilock.
> */
> xfs_iunlock(ip, lockflags);
> - error = xfs_qm_dqget(mp, uid, XFS_DQ_USER, true, &uq);
> + error = xfs_qm_dqget(mp, xfs_kuid_to_uid(uid),
> + XFS_DQ_USER, true, &uq);
> if (error) {
> ASSERT(error != -ENOENT);
> return error;
> @@ -1682,9 +1686,10 @@ xfs_qm_vop_dqalloc(
> }
> }
> if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
> - if (ip->i_d.di_gid != gid) {
> + if (!gid_eq(inode->i_gid, gid)) {
> xfs_iunlock(ip, lockflags);
> - error = xfs_qm_dqget(mp, gid, XFS_DQ_GROUP, true, &gq);
> + error = xfs_qm_dqget(mp, xfs_kgid_to_gid(gid),
> + XFS_DQ_GROUP, true, &gq);
> if (error) {
> ASSERT(error != -ENOENT);
> goto error_rele;
> @@ -1810,7 +1815,8 @@ xfs_qm_vop_chown_reserve(
> XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
>
> if (XFS_IS_UQUOTA_ON(mp) && udqp &&
> - ip->i_d.di_uid != be32_to_cpu(udqp->q_core.d_id)) {
> + xfs_kuid_to_uid(VFS_I(ip)->i_uid) !=
> + be32_to_cpu(udqp->q_core.d_id)) {
> udq_delblks = udqp;
> /*
> * If there are delayed allocation blocks, then we have to
> @@ -1823,7 +1829,8 @@ xfs_qm_vop_chown_reserve(
> }
> }
> if (XFS_IS_GQUOTA_ON(ip->i_mount) && gdqp &&
> - ip->i_d.di_gid != be32_to_cpu(gdqp->q_core.d_id)) {
> + xfs_kgid_to_gid(VFS_I(ip)->i_gid) !=
> + be32_to_cpu(gdqp->q_core.d_id)) {
> gdq_delblks = gdqp;
> if (delblks) {
> ASSERT(ip->i_gdquot);
> @@ -1920,14 +1927,17 @@ xfs_qm_vop_create_dqattach(
>
> if (udqp && XFS_IS_UQUOTA_ON(mp)) {
> ASSERT(ip->i_udquot == NULL);
> - ASSERT(ip->i_d.di_uid == be32_to_cpu(udqp->q_core.d_id));
> + ASSERT(xfs_kuid_to_uid(VFS_I(ip)->i_uid) ==
> + be32_to_cpu(udqp->q_core.d_id));
>
> ip->i_udquot = xfs_qm_dqhold(udqp);
> xfs_trans_mod_dquot(tp, udqp, XFS_TRANS_DQ_ICOUNT, 1);
> }
> if (gdqp && XFS_IS_GQUOTA_ON(mp)) {
> ASSERT(ip->i_gdquot == NULL);
> - ASSERT(ip->i_d.di_gid == be32_to_cpu(gdqp->q_core.d_id));
> + ASSERT(xfs_kgid_to_gid(VFS_I(ip)->i_gid) ==
> + be32_to_cpu(gdqp->q_core.d_id));
> +
> ip->i_gdquot = xfs_qm_dqhold(gdqp);
> xfs_trans_mod_dquot(tp, gdqp, XFS_TRANS_DQ_ICOUNT, 1);
> }
> diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
> index efe42ae7a2f3..aa8fc1f55fbd 100644
> --- a/fs/xfs/xfs_quota.h
> +++ b/fs/xfs/xfs_quota.h
> @@ -86,7 +86,7 @@ extern int xfs_trans_reserve_quota_bydquots(struct xfs_trans *,
> struct xfs_mount *, struct xfs_dquot *,
> struct xfs_dquot *, struct xfs_dquot *, int64_t, long, uint);
>
> -extern int xfs_qm_vop_dqalloc(struct xfs_inode *, xfs_dqid_t, xfs_dqid_t,
> +extern int xfs_qm_vop_dqalloc(struct xfs_inode *, kuid_t, kgid_t,
> prid_t, uint, struct xfs_dquot **, struct xfs_dquot **,
> struct xfs_dquot **);
> extern void xfs_qm_vop_create_dqattach(struct xfs_trans *, struct xfs_inode *,
> @@ -109,7 +109,7 @@ extern void xfs_qm_unmount_quotas(struct xfs_mount *);
>
> #else
> static inline int
> -xfs_qm_vop_dqalloc(struct xfs_inode *ip, xfs_dqid_t uid, xfs_dqid_t gid,
> +xfs_qm_vop_dqalloc(struct xfs_inode *ip, kuid_t kuid, kgid_t kgid,
> prid_t prid, uint flags, struct xfs_dquot **udqp,
> struct xfs_dquot **gdqp, struct xfs_dquot **pdqp)
> {
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index d762d42ed0ff..ea42e25ec1bf 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -182,9 +182,7 @@ xfs_symlink(
> /*
> * Make sure that we have allocated dquot(s) on disk.
> */
> - error = xfs_qm_vop_dqalloc(dp,
> - xfs_kuid_to_uid(current_fsuid()),
> - xfs_kgid_to_gid(current_fsgid()), prid,
> + error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
> XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> &udqp, &gdqp, &pdqp);
> if (error)
> --
> 2.24.1
>
--
Carlos
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] xfs: remove the kuid/kgid conversion wrappers
2020-02-18 21:00 ` [PATCH 3/3] xfs: remove the kuid/kgid conversion wrappers Christoph Hellwig
@ 2020-02-19 11:05 ` Carlos Maiolino
2020-02-19 14:26 ` Brian Foster
2020-02-21 1:26 ` Darrick J. Wong
2 siblings, 0 replies; 16+ messages in thread
From: Carlos Maiolino @ 2020-02-19 11:05 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, linux-fsdevel
On Tue, Feb 18, 2020 at 01:00:20PM -0800, Christoph Hellwig wrote:
> Remove the XFS wrappers for converting from and to the kuid/kgid types.
> Mostly this means switching to VFS i_{u,g}id_{read,write} helpers, but
> in a few spots the calls to the conversion functions is open coded.
> To match the use of sb->s_user_ns in the helpers and other file systems,
> sb->s_user_ns is also used in the quota code. The ACL code already does
> the conversion in a grotty layering violation in the VFS xattr code,
> so it keeps using init_user_ns for the identity mapping.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> fs/xfs/libxfs/xfs_inode_buf.c | 8 ++++----
> fs/xfs/xfs_acl.c | 12 ++++++++----
> fs/xfs/xfs_dquot.c | 4 ++--
> fs/xfs/xfs_inode_item.c | 4 ++--
> fs/xfs/xfs_itable.c | 4 ++--
> fs/xfs/xfs_linux.h | 26 --------------------------
> fs/xfs/xfs_qm.c | 23 +++++++++--------------
> 7 files changed, 27 insertions(+), 54 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index bc72b575ceed..17e88a8c8353 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -222,8 +222,8 @@ xfs_inode_from_disk(
> }
>
> to->di_format = from->di_format;
> - inode->i_uid = xfs_uid_to_kuid(be32_to_cpu(from->di_uid));
> - inode->i_gid = xfs_gid_to_kgid(be32_to_cpu(from->di_gid));
> + i_uid_write(inode, be32_to_cpu(from->di_uid));
> + i_gid_write(inode, be32_to_cpu(from->di_gid));
> to->di_flushiter = be16_to_cpu(from->di_flushiter);
>
> /*
> @@ -276,8 +276,8 @@ xfs_inode_to_disk(
>
> to->di_version = from->di_version;
> to->di_format = from->di_format;
> - to->di_uid = cpu_to_be32(xfs_kuid_to_uid(inode->i_uid));
> - to->di_gid = cpu_to_be32(xfs_kgid_to_gid(inode->i_gid));
> + to->di_uid = cpu_to_be32(i_uid_read(inode));
> + to->di_gid = cpu_to_be32(i_gid_read(inode));
> to->di_projid_lo = cpu_to_be16(from->di_projid & 0xffff);
> to->di_projid_hi = cpu_to_be16(from->di_projid >> 16);
>
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index cd743fad8478..e7314b525b19 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -67,10 +67,12 @@ xfs_acl_from_disk(
>
> switch (acl_e->e_tag) {
> case ACL_USER:
> - acl_e->e_uid = xfs_uid_to_kuid(be32_to_cpu(ace->ae_id));
> + acl_e->e_uid = make_kuid(&init_user_ns,
> + be32_to_cpu(ace->ae_id));
> break;
> case ACL_GROUP:
> - acl_e->e_gid = xfs_gid_to_kgid(be32_to_cpu(ace->ae_id));
> + acl_e->e_gid = make_kgid(&init_user_ns,
> + be32_to_cpu(ace->ae_id));
> break;
> case ACL_USER_OBJ:
> case ACL_GROUP_OBJ:
> @@ -103,10 +105,12 @@ xfs_acl_to_disk(struct xfs_acl *aclp, const struct posix_acl *acl)
> ace->ae_tag = cpu_to_be32(acl_e->e_tag);
> switch (acl_e->e_tag) {
> case ACL_USER:
> - ace->ae_id = cpu_to_be32(xfs_kuid_to_uid(acl_e->e_uid));
> + ace->ae_id = cpu_to_be32(
> + from_kuid(&init_user_ns, acl_e->e_uid));
> break;
> case ACL_GROUP:
> - ace->ae_id = cpu_to_be32(xfs_kgid_to_gid(acl_e->e_gid));
> + ace->ae_id = cpu_to_be32(
> + from_kgid(&init_user_ns, acl_e->e_gid));
> break;
> default:
> ace->ae_id = cpu_to_be32(ACL_UNDEFINED_ID);
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 3579de9306c1..711376ca269f 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -829,9 +829,9 @@ xfs_qm_id_for_quotatype(
> {
> switch (type) {
> case XFS_DQ_USER:
> - return xfs_kuid_to_uid(VFS_I(ip)->i_uid);
> + return i_uid_read(VFS_I(ip));
> case XFS_DQ_GROUP:
> - return xfs_kgid_to_gid(VFS_I(ip)->i_gid);
> + return i_gid_read(VFS_I(ip));
> case XFS_DQ_PROJ:
> return ip->i_d.di_projid;
> }
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 83d7914556ef..f021b55a0301 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -308,8 +308,8 @@ xfs_inode_to_log_dinode(
>
> to->di_version = from->di_version;
> to->di_format = from->di_format;
> - to->di_uid = xfs_kuid_to_uid(inode->i_uid);
> - to->di_gid = xfs_kgid_to_gid(inode->i_gid);
> + to->di_uid = i_uid_read(inode);
> + to->di_gid = i_gid_read(inode);
> to->di_projid_lo = from->di_projid & 0xffff;
> to->di_projid_hi = from->di_projid >> 16;
>
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 497db4160283..d10660469884 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -86,8 +86,8 @@ xfs_bulkstat_one_int(
> */
> buf->bs_projectid = ip->i_d.di_projid;
> buf->bs_ino = ino;
> - buf->bs_uid = xfs_kuid_to_uid(inode->i_uid);
> - buf->bs_gid = xfs_kgid_to_gid(inode->i_gid);
> + buf->bs_uid = i_uid_read(inode);
> + buf->bs_gid = i_gid_read(inode);
> buf->bs_size = dic->di_size;
>
> buf->bs_nlink = inode->i_nlink;
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index 8738bb03f253..bc43cd98697b 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -163,32 +163,6 @@ struct xstats {
>
> extern struct xstats xfsstats;
>
> -/* Kernel uid/gid conversion. These are used to convert to/from the on disk
> - * uid_t/gid_t types to the kuid_t/kgid_t types that the kernel uses internally.
> - * The conversion here is type only, the value will remain the same since we
> - * are converting to the init_user_ns. The uid is later mapped to a particular
> - * user namespace value when crossing the kernel/user boundary.
> - */
> -static inline uint32_t xfs_kuid_to_uid(kuid_t uid)
> -{
> - return from_kuid(&init_user_ns, uid);
> -}
> -
> -static inline kuid_t xfs_uid_to_kuid(uint32_t uid)
> -{
> - return make_kuid(&init_user_ns, uid);
> -}
> -
> -static inline uint32_t xfs_kgid_to_gid(kgid_t gid)
> -{
> - return from_kgid(&init_user_ns, gid);
> -}
> -
> -static inline kgid_t xfs_gid_to_kgid(uint32_t gid)
> -{
> - return make_kgid(&init_user_ns, gid);
> -}
> -
> static inline dev_t xfs_to_linux_dev_t(xfs_dev_t dev)
> {
> return MKDEV(sysv_major(dev) & 0x1ff, sysv_minor(dev));
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 54dda7d982c9..de1d2c606c14 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -326,8 +326,7 @@ xfs_qm_dqattach_locked(
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>
> if (XFS_IS_UQUOTA_ON(mp) && !ip->i_udquot) {
> - error = xfs_qm_dqattach_one(ip,
> - xfs_kuid_to_uid(VFS_I(ip)->i_uid),
> + error = xfs_qm_dqattach_one(ip, i_uid_read(VFS_I(ip)),
> XFS_DQ_USER, doalloc, &ip->i_udquot);
> if (error)
> goto done;
> @@ -335,8 +334,7 @@ xfs_qm_dqattach_locked(
> }
>
> if (XFS_IS_GQUOTA_ON(mp) && !ip->i_gdquot) {
> - error = xfs_qm_dqattach_one(ip,
> - xfs_kgid_to_gid(VFS_I(ip)->i_gid),
> + error = xfs_qm_dqattach_one(ip, i_gid_read(VFS_I(ip)),
> XFS_DQ_GROUP, doalloc, &ip->i_gdquot);
> if (error)
> goto done;
> @@ -1625,6 +1623,7 @@ xfs_qm_vop_dqalloc(
> {
> struct xfs_mount *mp = ip->i_mount;
> struct inode *inode = VFS_I(ip);
> + struct user_namespace *user_ns = inode->i_sb->s_user_ns;
> struct xfs_dquot *uq = NULL;
> struct xfs_dquot *gq = NULL;
> struct xfs_dquot *pq = NULL;
> @@ -1664,7 +1663,7 @@ xfs_qm_vop_dqalloc(
> * holding ilock.
> */
> xfs_iunlock(ip, lockflags);
> - error = xfs_qm_dqget(mp, xfs_kuid_to_uid(uid),
> + error = xfs_qm_dqget(mp, from_kuid(user_ns, uid),
> XFS_DQ_USER, true, &uq);
> if (error) {
> ASSERT(error != -ENOENT);
> @@ -1688,7 +1687,7 @@ xfs_qm_vop_dqalloc(
> if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
> if (!gid_eq(inode->i_gid, gid)) {
> xfs_iunlock(ip, lockflags);
> - error = xfs_qm_dqget(mp, xfs_kgid_to_gid(gid),
> + error = xfs_qm_dqget(mp, from_kgid(user_ns, gid),
> XFS_DQ_GROUP, true, &gq);
> if (error) {
> ASSERT(error != -ENOENT);
> @@ -1815,8 +1814,7 @@ xfs_qm_vop_chown_reserve(
> XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
>
> if (XFS_IS_UQUOTA_ON(mp) && udqp &&
> - xfs_kuid_to_uid(VFS_I(ip)->i_uid) !=
> - be32_to_cpu(udqp->q_core.d_id)) {
> + i_uid_read(VFS_I(ip)) != be32_to_cpu(udqp->q_core.d_id)) {
> udq_delblks = udqp;
> /*
> * If there are delayed allocation blocks, then we have to
> @@ -1829,8 +1827,7 @@ xfs_qm_vop_chown_reserve(
> }
> }
> if (XFS_IS_GQUOTA_ON(ip->i_mount) && gdqp &&
> - xfs_kgid_to_gid(VFS_I(ip)->i_gid) !=
> - be32_to_cpu(gdqp->q_core.d_id)) {
> + i_gid_read(VFS_I(ip)) != be32_to_cpu(gdqp->q_core.d_id)) {
> gdq_delblks = gdqp;
> if (delblks) {
> ASSERT(ip->i_gdquot);
> @@ -1927,16 +1924,14 @@ xfs_qm_vop_create_dqattach(
>
> if (udqp && XFS_IS_UQUOTA_ON(mp)) {
> ASSERT(ip->i_udquot == NULL);
> - ASSERT(xfs_kuid_to_uid(VFS_I(ip)->i_uid) ==
> - be32_to_cpu(udqp->q_core.d_id));
> + ASSERT(i_uid_read(VFS_I(ip)) == be32_to_cpu(udqp->q_core.d_id));
>
> ip->i_udquot = xfs_qm_dqhold(udqp);
> xfs_trans_mod_dquot(tp, udqp, XFS_TRANS_DQ_ICOUNT, 1);
> }
> if (gdqp && XFS_IS_GQUOTA_ON(mp)) {
> ASSERT(ip->i_gdquot == NULL);
> - ASSERT(xfs_kgid_to_gid(VFS_I(ip)->i_gid) ==
> - be32_to_cpu(gdqp->q_core.d_id));
> + ASSERT(i_gid_read(VFS_I(ip)) == be32_to_cpu(gdqp->q_core.d_id));
>
> ip->i_gdquot = xfs_qm_dqhold(gdqp);
> xfs_trans_mod_dquot(tp, gdqp, XFS_TRANS_DQ_ICOUNT, 1);
> --
> 2.24.1
>
--
Carlos
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] xfs: ensure that the inode uid/gid match values match the icdinode ones
2020-02-18 21:00 ` [PATCH 1/3] xfs: ensure that the inode uid/gid match values match the icdinode ones Christoph Hellwig
2020-02-19 10:32 ` Carlos Maiolino
@ 2020-02-19 14:26 ` Brian Foster
2020-02-19 14:47 ` Chandan Rajendra
2 siblings, 0 replies; 16+ messages in thread
From: Brian Foster @ 2020-02-19 14:26 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, linux-fsdevel
On Tue, Feb 18, 2020 at 01:00:18PM -0800, Christoph Hellwig wrote:
> Instead of only synchronizing the uid/gid values in xfs_setup_inode,
> ensure that they always match to prepare for removing the icdinode
> fields.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
The patch subject looks like it has an extra "match" in there. Otherwise
looks Ok to me:
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/libxfs/xfs_inode_buf.c | 2 ++
> fs/xfs/xfs_icache.c | 4 ++++
> fs/xfs/xfs_inode.c | 8 ++++++--
> fs/xfs/xfs_iops.c | 3 ---
> 4 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 8afacfe4be0a..cc4efd34843a 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -223,7 +223,9 @@ xfs_inode_from_disk(
>
> to->di_format = from->di_format;
> to->di_uid = be32_to_cpu(from->di_uid);
> + inode->i_uid = xfs_uid_to_kuid(to->di_uid);
> to->di_gid = be32_to_cpu(from->di_gid);
> + inode->i_gid = xfs_gid_to_kgid(to->di_gid);
> to->di_flushiter = be16_to_cpu(from->di_flushiter);
>
> /*
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 8dc2e5414276..a7be7a9e5c1a 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -289,6 +289,8 @@ xfs_reinit_inode(
> uint64_t version = inode_peek_iversion(inode);
> umode_t mode = inode->i_mode;
> dev_t dev = inode->i_rdev;
> + kuid_t uid = inode->i_uid;
> + kgid_t gid = inode->i_gid;
>
> error = inode_init_always(mp->m_super, inode);
>
> @@ -297,6 +299,8 @@ xfs_reinit_inode(
> inode_set_iversion_queried(inode, version);
> inode->i_mode = mode;
> inode->i_rdev = dev;
> + inode->i_uid = uid;
> + inode->i_gid = gid;
> return error;
> }
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c5077e6326c7..938b0943bd95 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -812,15 +812,19 @@ xfs_ialloc(
>
> inode->i_mode = mode;
> set_nlink(inode, nlink);
> - ip->i_d.di_uid = xfs_kuid_to_uid(current_fsuid());
> - ip->i_d.di_gid = xfs_kgid_to_gid(current_fsgid());
> + inode->i_uid = current_fsuid();
> + ip->i_d.di_uid = xfs_kuid_to_uid(inode->i_uid);
> inode->i_rdev = rdev;
> ip->i_d.di_projid = prid;
>
> if (pip && XFS_INHERIT_GID(pip)) {
> + inode->i_gid = VFS_I(pip)->i_gid;
> ip->i_d.di_gid = pip->i_d.di_gid;
> if ((VFS_I(pip)->i_mode & S_ISGID) && S_ISDIR(mode))
> inode->i_mode |= S_ISGID;
> + } else {
> + inode->i_gid = current_fsgid();
> + ip->i_d.di_gid = xfs_kgid_to_gid(inode->i_gid);
> }
>
> /*
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 81f2f93caec0..b818b261918f 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -1304,9 +1304,6 @@ xfs_setup_inode(
> /* make the inode look hashed for the writeback code */
> inode_fake_hash(inode);
>
> - inode->i_uid = xfs_uid_to_kuid(ip->i_d.di_uid);
> - inode->i_gid = xfs_gid_to_kgid(ip->i_d.di_gid);
> -
> i_size_write(inode, ip->i_d.di_size);
> xfs_diflags_to_iflags(inode, ip);
>
> --
> 2.24.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] xfs: remove the icdinode di_uid/di_gid members
2020-02-18 21:00 ` [PATCH 2/3] xfs: remove the icdinode di_uid/di_gid members Christoph Hellwig
2020-02-19 10:51 ` Carlos Maiolino
@ 2020-02-19 14:26 ` Brian Foster
2020-02-19 16:25 ` Chandan Rajendra
2 siblings, 0 replies; 16+ messages in thread
From: Brian Foster @ 2020-02-19 14:26 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, linux-fsdevel
On Tue, Feb 18, 2020 at 01:00:19PM -0800, Christoph Hellwig wrote:
> Use the Linux inode i_uid/i_gid members everywhere and just convert
> from/to the scalar value when reading or writing the on-disk inode.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/libxfs/xfs_inode_buf.c | 10 ++++-----
> fs/xfs/libxfs/xfs_inode_buf.h | 2 --
> fs/xfs/xfs_dquot.c | 4 ++--
> fs/xfs/xfs_inode.c | 14 ++++--------
> fs/xfs/xfs_inode_item.c | 4 ++--
> fs/xfs/xfs_ioctl.c | 6 +++---
> fs/xfs/xfs_iops.c | 6 +-----
> fs/xfs/xfs_itable.c | 4 ++--
> fs/xfs/xfs_qm.c | 40 ++++++++++++++++++++++-------------
> fs/xfs/xfs_quota.h | 4 ++--
> fs/xfs/xfs_symlink.c | 4 +---
> 11 files changed, 46 insertions(+), 52 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index cc4efd34843a..bc72b575ceed 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -222,10 +222,8 @@ xfs_inode_from_disk(
> }
>
> to->di_format = from->di_format;
> - to->di_uid = be32_to_cpu(from->di_uid);
> - inode->i_uid = xfs_uid_to_kuid(to->di_uid);
> - to->di_gid = be32_to_cpu(from->di_gid);
> - inode->i_gid = xfs_gid_to_kgid(to->di_gid);
> + inode->i_uid = xfs_uid_to_kuid(be32_to_cpu(from->di_uid));
> + inode->i_gid = xfs_gid_to_kgid(be32_to_cpu(from->di_gid));
> to->di_flushiter = be16_to_cpu(from->di_flushiter);
>
> /*
> @@ -278,8 +276,8 @@ xfs_inode_to_disk(
>
> to->di_version = from->di_version;
> to->di_format = from->di_format;
> - to->di_uid = cpu_to_be32(from->di_uid);
> - to->di_gid = cpu_to_be32(from->di_gid);
> + to->di_uid = cpu_to_be32(xfs_kuid_to_uid(inode->i_uid));
> + to->di_gid = cpu_to_be32(xfs_kgid_to_gid(inode->i_gid));
> to->di_projid_lo = cpu_to_be16(from->di_projid & 0xffff);
> to->di_projid_hi = cpu_to_be16(from->di_projid >> 16);
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
> index fd94b1078722..2683e1e2c4a6 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.h
> +++ b/fs/xfs/libxfs/xfs_inode_buf.h
> @@ -19,8 +19,6 @@ struct xfs_icdinode {
> int8_t di_version; /* inode version */
> int8_t di_format; /* format of di_c data */
> uint16_t di_flushiter; /* incremented on flush */
> - uint32_t di_uid; /* owner's user id */
> - uint32_t di_gid; /* owner's group id */
> uint32_t di_projid; /* owner's project id */
> xfs_fsize_t di_size; /* number of bytes in file */
> xfs_rfsblock_t di_nblocks; /* # of direct & btree blocks used */
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index d223e1ae90a6..3579de9306c1 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -829,9 +829,9 @@ xfs_qm_id_for_quotatype(
> {
> switch (type) {
> case XFS_DQ_USER:
> - return ip->i_d.di_uid;
> + return xfs_kuid_to_uid(VFS_I(ip)->i_uid);
> case XFS_DQ_GROUP:
> - return ip->i_d.di_gid;
> + return xfs_kgid_to_gid(VFS_I(ip)->i_gid);
> case XFS_DQ_PROJ:
> return ip->i_d.di_projid;
> }
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 938b0943bd95..3324e1696354 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -813,18 +813,15 @@ xfs_ialloc(
> inode->i_mode = mode;
> set_nlink(inode, nlink);
> inode->i_uid = current_fsuid();
> - ip->i_d.di_uid = xfs_kuid_to_uid(inode->i_uid);
> inode->i_rdev = rdev;
> ip->i_d.di_projid = prid;
>
> if (pip && XFS_INHERIT_GID(pip)) {
> inode->i_gid = VFS_I(pip)->i_gid;
> - ip->i_d.di_gid = pip->i_d.di_gid;
> if ((VFS_I(pip)->i_mode & S_ISGID) && S_ISDIR(mode))
> inode->i_mode |= S_ISGID;
> } else {
> inode->i_gid = current_fsgid();
> - ip->i_d.di_gid = xfs_kgid_to_gid(inode->i_gid);
> }
>
> /*
> @@ -832,9 +829,8 @@ xfs_ialloc(
> * ID or one of the supplementary group IDs, the S_ISGID bit is cleared
> * (and only if the irix_sgid_inherit compatibility variable is set).
> */
> - if ((irix_sgid_inherit) &&
> - (inode->i_mode & S_ISGID) &&
> - (!in_group_p(xfs_gid_to_kgid(ip->i_d.di_gid))))
> + if (irix_sgid_inherit &&
> + (inode->i_mode & S_ISGID) && !in_group_p(inode->i_gid))
> inode->i_mode &= ~S_ISGID;
>
> ip->i_d.di_size = 0;
> @@ -1162,8 +1158,7 @@ xfs_create(
> /*
> * Make sure that we have allocated dquot(s) on disk.
> */
> - error = xfs_qm_vop_dqalloc(dp, xfs_kuid_to_uid(current_fsuid()),
> - xfs_kgid_to_gid(current_fsgid()), prid,
> + error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
> XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> &udqp, &gdqp, &pdqp);
> if (error)
> @@ -1313,8 +1308,7 @@ xfs_create_tmpfile(
> /*
> * Make sure that we have allocated dquot(s) on disk.
> */
> - error = xfs_qm_vop_dqalloc(dp, xfs_kuid_to_uid(current_fsuid()),
> - xfs_kgid_to_gid(current_fsgid()), prid,
> + error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
> XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> &udqp, &gdqp, &pdqp);
> if (error)
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 8bd5d0de6321..83d7914556ef 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -308,8 +308,8 @@ xfs_inode_to_log_dinode(
>
> to->di_version = from->di_version;
> to->di_format = from->di_format;
> - to->di_uid = from->di_uid;
> - to->di_gid = from->di_gid;
> + to->di_uid = xfs_kuid_to_uid(inode->i_uid);
> + to->di_gid = xfs_kgid_to_gid(inode->i_gid);
> to->di_projid_lo = from->di_projid & 0xffff;
> to->di_projid_hi = from->di_projid >> 16;
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index d42de92cb283..0f85bedc5977 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1434,9 +1434,9 @@ xfs_ioctl_setattr(
> * because the i_*dquot fields will get updated anyway.
> */
> if (XFS_IS_QUOTA_ON(mp)) {
> - code = xfs_qm_vop_dqalloc(ip, ip->i_d.di_uid,
> - ip->i_d.di_gid, fa->fsx_projid,
> - XFS_QMOPT_PQUOTA, &udqp, NULL, &pdqp);
> + code = xfs_qm_vop_dqalloc(ip, VFS_I(ip)->i_uid,
> + VFS_I(ip)->i_gid, fa->fsx_projid,
> + XFS_QMOPT_PQUOTA, &udqp, NULL, &pdqp);
> if (code)
> return code;
> }
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index b818b261918f..a5b7c3100a2f 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -692,9 +692,7 @@ xfs_setattr_nonsize(
> */
> ASSERT(udqp == NULL);
> ASSERT(gdqp == NULL);
> - error = xfs_qm_vop_dqalloc(ip, xfs_kuid_to_uid(uid),
> - xfs_kgid_to_gid(gid),
> - ip->i_d.di_projid,
> + error = xfs_qm_vop_dqalloc(ip, uid, gid, ip->i_d.di_projid,
> qflags, &udqp, &gdqp, NULL);
> if (error)
> return error;
> @@ -763,7 +761,6 @@ xfs_setattr_nonsize(
> olddquot1 = xfs_qm_vop_chown(tp, ip,
> &ip->i_udquot, udqp);
> }
> - ip->i_d.di_uid = xfs_kuid_to_uid(uid);
> inode->i_uid = uid;
> }
> if (!gid_eq(igid, gid)) {
> @@ -775,7 +772,6 @@ xfs_setattr_nonsize(
> olddquot2 = xfs_qm_vop_chown(tp, ip,
> &ip->i_gdquot, gdqp);
> }
> - ip->i_d.di_gid = xfs_kgid_to_gid(gid);
> inode->i_gid = gid;
> }
> }
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 4b31c29b7e6b..497db4160283 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -86,8 +86,8 @@ xfs_bulkstat_one_int(
> */
> buf->bs_projectid = ip->i_d.di_projid;
> buf->bs_ino = ino;
> - buf->bs_uid = dic->di_uid;
> - buf->bs_gid = dic->di_gid;
> + buf->bs_uid = xfs_kuid_to_uid(inode->i_uid);
> + buf->bs_gid = xfs_kgid_to_gid(inode->i_gid);
> buf->bs_size = dic->di_size;
>
> buf->bs_nlink = inode->i_nlink;
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 0b0909657bad..54dda7d982c9 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -326,16 +326,18 @@ xfs_qm_dqattach_locked(
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>
> if (XFS_IS_UQUOTA_ON(mp) && !ip->i_udquot) {
> - error = xfs_qm_dqattach_one(ip, ip->i_d.di_uid, XFS_DQ_USER,
> - doalloc, &ip->i_udquot);
> + error = xfs_qm_dqattach_one(ip,
> + xfs_kuid_to_uid(VFS_I(ip)->i_uid),
> + XFS_DQ_USER, doalloc, &ip->i_udquot);
> if (error)
> goto done;
> ASSERT(ip->i_udquot);
> }
>
> if (XFS_IS_GQUOTA_ON(mp) && !ip->i_gdquot) {
> - error = xfs_qm_dqattach_one(ip, ip->i_d.di_gid, XFS_DQ_GROUP,
> - doalloc, &ip->i_gdquot);
> + error = xfs_qm_dqattach_one(ip,
> + xfs_kgid_to_gid(VFS_I(ip)->i_gid),
> + XFS_DQ_GROUP, doalloc, &ip->i_gdquot);
> if (error)
> goto done;
> ASSERT(ip->i_gdquot);
> @@ -1613,8 +1615,8 @@ xfs_qm_dqfree_one(
> int
> xfs_qm_vop_dqalloc(
> struct xfs_inode *ip,
> - xfs_dqid_t uid,
> - xfs_dqid_t gid,
> + kuid_t uid,
> + kgid_t gid,
> prid_t prid,
> uint flags,
> struct xfs_dquot **O_udqpp,
> @@ -1622,6 +1624,7 @@ xfs_qm_vop_dqalloc(
> struct xfs_dquot **O_pdqpp)
> {
> struct xfs_mount *mp = ip->i_mount;
> + struct inode *inode = VFS_I(ip);
> struct xfs_dquot *uq = NULL;
> struct xfs_dquot *gq = NULL;
> struct xfs_dquot *pq = NULL;
> @@ -1635,7 +1638,7 @@ xfs_qm_vop_dqalloc(
> xfs_ilock(ip, lockflags);
>
> if ((flags & XFS_QMOPT_INHERIT) && XFS_INHERIT_GID(ip))
> - gid = ip->i_d.di_gid;
> + gid = inode->i_gid;
>
> /*
> * Attach the dquot(s) to this inode, doing a dquot allocation
> @@ -1650,7 +1653,7 @@ xfs_qm_vop_dqalloc(
> }
>
> if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp)) {
> - if (ip->i_d.di_uid != uid) {
> + if (!uid_eq(inode->i_uid, uid)) {
> /*
> * What we need is the dquot that has this uid, and
> * if we send the inode to dqget, the uid of the inode
> @@ -1661,7 +1664,8 @@ xfs_qm_vop_dqalloc(
> * holding ilock.
> */
> xfs_iunlock(ip, lockflags);
> - error = xfs_qm_dqget(mp, uid, XFS_DQ_USER, true, &uq);
> + error = xfs_qm_dqget(mp, xfs_kuid_to_uid(uid),
> + XFS_DQ_USER, true, &uq);
> if (error) {
> ASSERT(error != -ENOENT);
> return error;
> @@ -1682,9 +1686,10 @@ xfs_qm_vop_dqalloc(
> }
> }
> if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
> - if (ip->i_d.di_gid != gid) {
> + if (!gid_eq(inode->i_gid, gid)) {
> xfs_iunlock(ip, lockflags);
> - error = xfs_qm_dqget(mp, gid, XFS_DQ_GROUP, true, &gq);
> + error = xfs_qm_dqget(mp, xfs_kgid_to_gid(gid),
> + XFS_DQ_GROUP, true, &gq);
> if (error) {
> ASSERT(error != -ENOENT);
> goto error_rele;
> @@ -1810,7 +1815,8 @@ xfs_qm_vop_chown_reserve(
> XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
>
> if (XFS_IS_UQUOTA_ON(mp) && udqp &&
> - ip->i_d.di_uid != be32_to_cpu(udqp->q_core.d_id)) {
> + xfs_kuid_to_uid(VFS_I(ip)->i_uid) !=
> + be32_to_cpu(udqp->q_core.d_id)) {
> udq_delblks = udqp;
> /*
> * If there are delayed allocation blocks, then we have to
> @@ -1823,7 +1829,8 @@ xfs_qm_vop_chown_reserve(
> }
> }
> if (XFS_IS_GQUOTA_ON(ip->i_mount) && gdqp &&
> - ip->i_d.di_gid != be32_to_cpu(gdqp->q_core.d_id)) {
> + xfs_kgid_to_gid(VFS_I(ip)->i_gid) !=
> + be32_to_cpu(gdqp->q_core.d_id)) {
> gdq_delblks = gdqp;
> if (delblks) {
> ASSERT(ip->i_gdquot);
> @@ -1920,14 +1927,17 @@ xfs_qm_vop_create_dqattach(
>
> if (udqp && XFS_IS_UQUOTA_ON(mp)) {
> ASSERT(ip->i_udquot == NULL);
> - ASSERT(ip->i_d.di_uid == be32_to_cpu(udqp->q_core.d_id));
> + ASSERT(xfs_kuid_to_uid(VFS_I(ip)->i_uid) ==
> + be32_to_cpu(udqp->q_core.d_id));
>
> ip->i_udquot = xfs_qm_dqhold(udqp);
> xfs_trans_mod_dquot(tp, udqp, XFS_TRANS_DQ_ICOUNT, 1);
> }
> if (gdqp && XFS_IS_GQUOTA_ON(mp)) {
> ASSERT(ip->i_gdquot == NULL);
> - ASSERT(ip->i_d.di_gid == be32_to_cpu(gdqp->q_core.d_id));
> + ASSERT(xfs_kgid_to_gid(VFS_I(ip)->i_gid) ==
> + be32_to_cpu(gdqp->q_core.d_id));
> +
> ip->i_gdquot = xfs_qm_dqhold(gdqp);
> xfs_trans_mod_dquot(tp, gdqp, XFS_TRANS_DQ_ICOUNT, 1);
> }
> diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
> index efe42ae7a2f3..aa8fc1f55fbd 100644
> --- a/fs/xfs/xfs_quota.h
> +++ b/fs/xfs/xfs_quota.h
> @@ -86,7 +86,7 @@ extern int xfs_trans_reserve_quota_bydquots(struct xfs_trans *,
> struct xfs_mount *, struct xfs_dquot *,
> struct xfs_dquot *, struct xfs_dquot *, int64_t, long, uint);
>
> -extern int xfs_qm_vop_dqalloc(struct xfs_inode *, xfs_dqid_t, xfs_dqid_t,
> +extern int xfs_qm_vop_dqalloc(struct xfs_inode *, kuid_t, kgid_t,
> prid_t, uint, struct xfs_dquot **, struct xfs_dquot **,
> struct xfs_dquot **);
> extern void xfs_qm_vop_create_dqattach(struct xfs_trans *, struct xfs_inode *,
> @@ -109,7 +109,7 @@ extern void xfs_qm_unmount_quotas(struct xfs_mount *);
>
> #else
> static inline int
> -xfs_qm_vop_dqalloc(struct xfs_inode *ip, xfs_dqid_t uid, xfs_dqid_t gid,
> +xfs_qm_vop_dqalloc(struct xfs_inode *ip, kuid_t kuid, kgid_t kgid,
> prid_t prid, uint flags, struct xfs_dquot **udqp,
> struct xfs_dquot **gdqp, struct xfs_dquot **pdqp)
> {
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index d762d42ed0ff..ea42e25ec1bf 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -182,9 +182,7 @@ xfs_symlink(
> /*
> * Make sure that we have allocated dquot(s) on disk.
> */
> - error = xfs_qm_vop_dqalloc(dp,
> - xfs_kuid_to_uid(current_fsuid()),
> - xfs_kgid_to_gid(current_fsgid()), prid,
> + error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
> XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> &udqp, &gdqp, &pdqp);
> if (error)
> --
> 2.24.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] xfs: remove the kuid/kgid conversion wrappers
2020-02-18 21:00 ` [PATCH 3/3] xfs: remove the kuid/kgid conversion wrappers Christoph Hellwig
2020-02-19 11:05 ` Carlos Maiolino
@ 2020-02-19 14:26 ` Brian Foster
2020-02-21 1:26 ` Darrick J. Wong
2 siblings, 0 replies; 16+ messages in thread
From: Brian Foster @ 2020-02-19 14:26 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, linux-fsdevel
On Tue, Feb 18, 2020 at 01:00:20PM -0800, Christoph Hellwig wrote:
> Remove the XFS wrappers for converting from and to the kuid/kgid types.
> Mostly this means switching to VFS i_{u,g}id_{read,write} helpers, but
> in a few spots the calls to the conversion functions is open coded.
> To match the use of sb->s_user_ns in the helpers and other file systems,
> sb->s_user_ns is also used in the quota code. The ACL code already does
> the conversion in a grotty layering violation in the VFS xattr code,
> so it keeps using init_user_ns for the identity mapping.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/libxfs/xfs_inode_buf.c | 8 ++++----
> fs/xfs/xfs_acl.c | 12 ++++++++----
> fs/xfs/xfs_dquot.c | 4 ++--
> fs/xfs/xfs_inode_item.c | 4 ++--
> fs/xfs/xfs_itable.c | 4 ++--
> fs/xfs/xfs_linux.h | 26 --------------------------
> fs/xfs/xfs_qm.c | 23 +++++++++--------------
> 7 files changed, 27 insertions(+), 54 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index bc72b575ceed..17e88a8c8353 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -222,8 +222,8 @@ xfs_inode_from_disk(
> }
>
> to->di_format = from->di_format;
> - inode->i_uid = xfs_uid_to_kuid(be32_to_cpu(from->di_uid));
> - inode->i_gid = xfs_gid_to_kgid(be32_to_cpu(from->di_gid));
> + i_uid_write(inode, be32_to_cpu(from->di_uid));
> + i_gid_write(inode, be32_to_cpu(from->di_gid));
> to->di_flushiter = be16_to_cpu(from->di_flushiter);
>
> /*
> @@ -276,8 +276,8 @@ xfs_inode_to_disk(
>
> to->di_version = from->di_version;
> to->di_format = from->di_format;
> - to->di_uid = cpu_to_be32(xfs_kuid_to_uid(inode->i_uid));
> - to->di_gid = cpu_to_be32(xfs_kgid_to_gid(inode->i_gid));
> + to->di_uid = cpu_to_be32(i_uid_read(inode));
> + to->di_gid = cpu_to_be32(i_gid_read(inode));
> to->di_projid_lo = cpu_to_be16(from->di_projid & 0xffff);
> to->di_projid_hi = cpu_to_be16(from->di_projid >> 16);
>
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index cd743fad8478..e7314b525b19 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -67,10 +67,12 @@ xfs_acl_from_disk(
>
> switch (acl_e->e_tag) {
> case ACL_USER:
> - acl_e->e_uid = xfs_uid_to_kuid(be32_to_cpu(ace->ae_id));
> + acl_e->e_uid = make_kuid(&init_user_ns,
> + be32_to_cpu(ace->ae_id));
> break;
> case ACL_GROUP:
> - acl_e->e_gid = xfs_gid_to_kgid(be32_to_cpu(ace->ae_id));
> + acl_e->e_gid = make_kgid(&init_user_ns,
> + be32_to_cpu(ace->ae_id));
> break;
> case ACL_USER_OBJ:
> case ACL_GROUP_OBJ:
> @@ -103,10 +105,12 @@ xfs_acl_to_disk(struct xfs_acl *aclp, const struct posix_acl *acl)
> ace->ae_tag = cpu_to_be32(acl_e->e_tag);
> switch (acl_e->e_tag) {
> case ACL_USER:
> - ace->ae_id = cpu_to_be32(xfs_kuid_to_uid(acl_e->e_uid));
> + ace->ae_id = cpu_to_be32(
> + from_kuid(&init_user_ns, acl_e->e_uid));
> break;
> case ACL_GROUP:
> - ace->ae_id = cpu_to_be32(xfs_kgid_to_gid(acl_e->e_gid));
> + ace->ae_id = cpu_to_be32(
> + from_kgid(&init_user_ns, acl_e->e_gid));
> break;
> default:
> ace->ae_id = cpu_to_be32(ACL_UNDEFINED_ID);
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 3579de9306c1..711376ca269f 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -829,9 +829,9 @@ xfs_qm_id_for_quotatype(
> {
> switch (type) {
> case XFS_DQ_USER:
> - return xfs_kuid_to_uid(VFS_I(ip)->i_uid);
> + return i_uid_read(VFS_I(ip));
> case XFS_DQ_GROUP:
> - return xfs_kgid_to_gid(VFS_I(ip)->i_gid);
> + return i_gid_read(VFS_I(ip));
> case XFS_DQ_PROJ:
> return ip->i_d.di_projid;
> }
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 83d7914556ef..f021b55a0301 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -308,8 +308,8 @@ xfs_inode_to_log_dinode(
>
> to->di_version = from->di_version;
> to->di_format = from->di_format;
> - to->di_uid = xfs_kuid_to_uid(inode->i_uid);
> - to->di_gid = xfs_kgid_to_gid(inode->i_gid);
> + to->di_uid = i_uid_read(inode);
> + to->di_gid = i_gid_read(inode);
> to->di_projid_lo = from->di_projid & 0xffff;
> to->di_projid_hi = from->di_projid >> 16;
>
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 497db4160283..d10660469884 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -86,8 +86,8 @@ xfs_bulkstat_one_int(
> */
> buf->bs_projectid = ip->i_d.di_projid;
> buf->bs_ino = ino;
> - buf->bs_uid = xfs_kuid_to_uid(inode->i_uid);
> - buf->bs_gid = xfs_kgid_to_gid(inode->i_gid);
> + buf->bs_uid = i_uid_read(inode);
> + buf->bs_gid = i_gid_read(inode);
> buf->bs_size = dic->di_size;
>
> buf->bs_nlink = inode->i_nlink;
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index 8738bb03f253..bc43cd98697b 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -163,32 +163,6 @@ struct xstats {
>
> extern struct xstats xfsstats;
>
> -/* Kernel uid/gid conversion. These are used to convert to/from the on disk
> - * uid_t/gid_t types to the kuid_t/kgid_t types that the kernel uses internally.
> - * The conversion here is type only, the value will remain the same since we
> - * are converting to the init_user_ns. The uid is later mapped to a particular
> - * user namespace value when crossing the kernel/user boundary.
> - */
> -static inline uint32_t xfs_kuid_to_uid(kuid_t uid)
> -{
> - return from_kuid(&init_user_ns, uid);
> -}
> -
> -static inline kuid_t xfs_uid_to_kuid(uint32_t uid)
> -{
> - return make_kuid(&init_user_ns, uid);
> -}
> -
> -static inline uint32_t xfs_kgid_to_gid(kgid_t gid)
> -{
> - return from_kgid(&init_user_ns, gid);
> -}
> -
> -static inline kgid_t xfs_gid_to_kgid(uint32_t gid)
> -{
> - return make_kgid(&init_user_ns, gid);
> -}
> -
> static inline dev_t xfs_to_linux_dev_t(xfs_dev_t dev)
> {
> return MKDEV(sysv_major(dev) & 0x1ff, sysv_minor(dev));
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 54dda7d982c9..de1d2c606c14 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -326,8 +326,7 @@ xfs_qm_dqattach_locked(
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>
> if (XFS_IS_UQUOTA_ON(mp) && !ip->i_udquot) {
> - error = xfs_qm_dqattach_one(ip,
> - xfs_kuid_to_uid(VFS_I(ip)->i_uid),
> + error = xfs_qm_dqattach_one(ip, i_uid_read(VFS_I(ip)),
> XFS_DQ_USER, doalloc, &ip->i_udquot);
> if (error)
> goto done;
> @@ -335,8 +334,7 @@ xfs_qm_dqattach_locked(
> }
>
> if (XFS_IS_GQUOTA_ON(mp) && !ip->i_gdquot) {
> - error = xfs_qm_dqattach_one(ip,
> - xfs_kgid_to_gid(VFS_I(ip)->i_gid),
> + error = xfs_qm_dqattach_one(ip, i_gid_read(VFS_I(ip)),
> XFS_DQ_GROUP, doalloc, &ip->i_gdquot);
> if (error)
> goto done;
> @@ -1625,6 +1623,7 @@ xfs_qm_vop_dqalloc(
> {
> struct xfs_mount *mp = ip->i_mount;
> struct inode *inode = VFS_I(ip);
> + struct user_namespace *user_ns = inode->i_sb->s_user_ns;
> struct xfs_dquot *uq = NULL;
> struct xfs_dquot *gq = NULL;
> struct xfs_dquot *pq = NULL;
> @@ -1664,7 +1663,7 @@ xfs_qm_vop_dqalloc(
> * holding ilock.
> */
> xfs_iunlock(ip, lockflags);
> - error = xfs_qm_dqget(mp, xfs_kuid_to_uid(uid),
> + error = xfs_qm_dqget(mp, from_kuid(user_ns, uid),
> XFS_DQ_USER, true, &uq);
> if (error) {
> ASSERT(error != -ENOENT);
> @@ -1688,7 +1687,7 @@ xfs_qm_vop_dqalloc(
> if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
> if (!gid_eq(inode->i_gid, gid)) {
> xfs_iunlock(ip, lockflags);
> - error = xfs_qm_dqget(mp, xfs_kgid_to_gid(gid),
> + error = xfs_qm_dqget(mp, from_kgid(user_ns, gid),
> XFS_DQ_GROUP, true, &gq);
> if (error) {
> ASSERT(error != -ENOENT);
> @@ -1815,8 +1814,7 @@ xfs_qm_vop_chown_reserve(
> XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
>
> if (XFS_IS_UQUOTA_ON(mp) && udqp &&
> - xfs_kuid_to_uid(VFS_I(ip)->i_uid) !=
> - be32_to_cpu(udqp->q_core.d_id)) {
> + i_uid_read(VFS_I(ip)) != be32_to_cpu(udqp->q_core.d_id)) {
> udq_delblks = udqp;
> /*
> * If there are delayed allocation blocks, then we have to
> @@ -1829,8 +1827,7 @@ xfs_qm_vop_chown_reserve(
> }
> }
> if (XFS_IS_GQUOTA_ON(ip->i_mount) && gdqp &&
> - xfs_kgid_to_gid(VFS_I(ip)->i_gid) !=
> - be32_to_cpu(gdqp->q_core.d_id)) {
> + i_gid_read(VFS_I(ip)) != be32_to_cpu(gdqp->q_core.d_id)) {
> gdq_delblks = gdqp;
> if (delblks) {
> ASSERT(ip->i_gdquot);
> @@ -1927,16 +1924,14 @@ xfs_qm_vop_create_dqattach(
>
> if (udqp && XFS_IS_UQUOTA_ON(mp)) {
> ASSERT(ip->i_udquot == NULL);
> - ASSERT(xfs_kuid_to_uid(VFS_I(ip)->i_uid) ==
> - be32_to_cpu(udqp->q_core.d_id));
> + ASSERT(i_uid_read(VFS_I(ip)) == be32_to_cpu(udqp->q_core.d_id));
>
> ip->i_udquot = xfs_qm_dqhold(udqp);
> xfs_trans_mod_dquot(tp, udqp, XFS_TRANS_DQ_ICOUNT, 1);
> }
> if (gdqp && XFS_IS_GQUOTA_ON(mp)) {
> ASSERT(ip->i_gdquot == NULL);
> - ASSERT(xfs_kgid_to_gid(VFS_I(ip)->i_gid) ==
> - be32_to_cpu(gdqp->q_core.d_id));
> + ASSERT(i_gid_read(VFS_I(ip)) == be32_to_cpu(gdqp->q_core.d_id));
>
> ip->i_gdquot = xfs_qm_dqhold(gdqp);
> xfs_trans_mod_dquot(tp, gdqp, XFS_TRANS_DQ_ICOUNT, 1);
> --
> 2.24.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] xfs: ensure that the inode uid/gid match values match the icdinode ones
2020-02-18 21:00 ` [PATCH 1/3] xfs: ensure that the inode uid/gid match values match the icdinode ones Christoph Hellwig
2020-02-19 10:32 ` Carlos Maiolino
2020-02-19 14:26 ` Brian Foster
@ 2020-02-19 14:47 ` Chandan Rajendra
2 siblings, 0 replies; 16+ messages in thread
From: Chandan Rajendra @ 2020-02-19 14:47 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, linux-fsdevel
On Wednesday, February 19, 2020 2:30 AM Christoph Hellwig wrote:
> Instead of only synchronizing the uid/gid values in xfs_setup_inode,
> ensure that they always match to prepare for removing the icdinode
> fields.
>
The changes indeed keep the uid and gid values the same across icdinode and
vfs inode.
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/libxfs/xfs_inode_buf.c | 2 ++
> fs/xfs/xfs_icache.c | 4 ++++
> fs/xfs/xfs_inode.c | 8 ++++++--
> fs/xfs/xfs_iops.c | 3 ---
> 4 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 8afacfe4be0a..cc4efd34843a 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -223,7 +223,9 @@ xfs_inode_from_disk(
>
> to->di_format = from->di_format;
> to->di_uid = be32_to_cpu(from->di_uid);
> + inode->i_uid = xfs_uid_to_kuid(to->di_uid);
> to->di_gid = be32_to_cpu(from->di_gid);
> + inode->i_gid = xfs_gid_to_kgid(to->di_gid);
> to->di_flushiter = be16_to_cpu(from->di_flushiter);
>
> /*
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 8dc2e5414276..a7be7a9e5c1a 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -289,6 +289,8 @@ xfs_reinit_inode(
> uint64_t version = inode_peek_iversion(inode);
> umode_t mode = inode->i_mode;
> dev_t dev = inode->i_rdev;
> + kuid_t uid = inode->i_uid;
> + kgid_t gid = inode->i_gid;
>
> error = inode_init_always(mp->m_super, inode);
>
> @@ -297,6 +299,8 @@ xfs_reinit_inode(
> inode_set_iversion_queried(inode, version);
> inode->i_mode = mode;
> inode->i_rdev = dev;
> + inode->i_uid = uid;
> + inode->i_gid = gid;
> return error;
> }
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c5077e6326c7..938b0943bd95 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -812,15 +812,19 @@ xfs_ialloc(
>
> inode->i_mode = mode;
> set_nlink(inode, nlink);
> - ip->i_d.di_uid = xfs_kuid_to_uid(current_fsuid());
> - ip->i_d.di_gid = xfs_kgid_to_gid(current_fsgid());
> + inode->i_uid = current_fsuid();
> + ip->i_d.di_uid = xfs_kuid_to_uid(inode->i_uid);
> inode->i_rdev = rdev;
> ip->i_d.di_projid = prid;
>
> if (pip && XFS_INHERIT_GID(pip)) {
> + inode->i_gid = VFS_I(pip)->i_gid;
> ip->i_d.di_gid = pip->i_d.di_gid;
> if ((VFS_I(pip)->i_mode & S_ISGID) && S_ISDIR(mode))
> inode->i_mode |= S_ISGID;
> + } else {
> + inode->i_gid = current_fsgid();
> + ip->i_d.di_gid = xfs_kgid_to_gid(inode->i_gid);
> }
>
> /*
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 81f2f93caec0..b818b261918f 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -1304,9 +1304,6 @@ xfs_setup_inode(
> /* make the inode look hashed for the writeback code */
> inode_fake_hash(inode);
>
> - inode->i_uid = xfs_uid_to_kuid(ip->i_d.di_uid);
> - inode->i_gid = xfs_gid_to_kgid(ip->i_d.di_gid);
> -
> i_size_write(inode, ip->i_d.di_size);
> xfs_diflags_to_iflags(inode, ip);
>
>
--
chandan
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] xfs: remove the icdinode di_uid/di_gid members
2020-02-18 21:00 ` [PATCH 2/3] xfs: remove the icdinode di_uid/di_gid members Christoph Hellwig
2020-02-19 10:51 ` Carlos Maiolino
2020-02-19 14:26 ` Brian Foster
@ 2020-02-19 16:25 ` Chandan Rajendra
2 siblings, 0 replies; 16+ messages in thread
From: Chandan Rajendra @ 2020-02-19 16:25 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, linux-fsdevel
On Wednesday, February 19, 2020 2:30 AM Christoph Hellwig wrote:
> Use the Linux inode i_uid/i_gid members everywhere and just convert
> from/to the scalar value when reading or writing the on-disk inode.
>
The conversion b/w kuid and on-disk uid is correct.
Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/libxfs/xfs_inode_buf.c | 10 ++++-----
> fs/xfs/libxfs/xfs_inode_buf.h | 2 --
> fs/xfs/xfs_dquot.c | 4 ++--
> fs/xfs/xfs_inode.c | 14 ++++--------
> fs/xfs/xfs_inode_item.c | 4 ++--
> fs/xfs/xfs_ioctl.c | 6 +++---
> fs/xfs/xfs_iops.c | 6 +-----
> fs/xfs/xfs_itable.c | 4 ++--
> fs/xfs/xfs_qm.c | 40 ++++++++++++++++++++++-------------
> fs/xfs/xfs_quota.h | 4 ++--
> fs/xfs/xfs_symlink.c | 4 +---
> 11 files changed, 46 insertions(+), 52 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index cc4efd34843a..bc72b575ceed 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -222,10 +222,8 @@ xfs_inode_from_disk(
> }
>
> to->di_format = from->di_format;
> - to->di_uid = be32_to_cpu(from->di_uid);
> - inode->i_uid = xfs_uid_to_kuid(to->di_uid);
> - to->di_gid = be32_to_cpu(from->di_gid);
> - inode->i_gid = xfs_gid_to_kgid(to->di_gid);
> + inode->i_uid = xfs_uid_to_kuid(be32_to_cpu(from->di_uid));
> + inode->i_gid = xfs_gid_to_kgid(be32_to_cpu(from->di_gid));
> to->di_flushiter = be16_to_cpu(from->di_flushiter);
>
> /*
> @@ -278,8 +276,8 @@ xfs_inode_to_disk(
>
> to->di_version = from->di_version;
> to->di_format = from->di_format;
> - to->di_uid = cpu_to_be32(from->di_uid);
> - to->di_gid = cpu_to_be32(from->di_gid);
> + to->di_uid = cpu_to_be32(xfs_kuid_to_uid(inode->i_uid));
> + to->di_gid = cpu_to_be32(xfs_kgid_to_gid(inode->i_gid));
> to->di_projid_lo = cpu_to_be16(from->di_projid & 0xffff);
> to->di_projid_hi = cpu_to_be16(from->di_projid >> 16);
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
> index fd94b1078722..2683e1e2c4a6 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.h
> +++ b/fs/xfs/libxfs/xfs_inode_buf.h
> @@ -19,8 +19,6 @@ struct xfs_icdinode {
> int8_t di_version; /* inode version */
> int8_t di_format; /* format of di_c data */
> uint16_t di_flushiter; /* incremented on flush */
> - uint32_t di_uid; /* owner's user id */
> - uint32_t di_gid; /* owner's group id */
> uint32_t di_projid; /* owner's project id */
> xfs_fsize_t di_size; /* number of bytes in file */
> xfs_rfsblock_t di_nblocks; /* # of direct & btree blocks used */
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index d223e1ae90a6..3579de9306c1 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -829,9 +829,9 @@ xfs_qm_id_for_quotatype(
> {
> switch (type) {
> case XFS_DQ_USER:
> - return ip->i_d.di_uid;
> + return xfs_kuid_to_uid(VFS_I(ip)->i_uid);
> case XFS_DQ_GROUP:
> - return ip->i_d.di_gid;
> + return xfs_kgid_to_gid(VFS_I(ip)->i_gid);
> case XFS_DQ_PROJ:
> return ip->i_d.di_projid;
> }
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 938b0943bd95..3324e1696354 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -813,18 +813,15 @@ xfs_ialloc(
> inode->i_mode = mode;
> set_nlink(inode, nlink);
> inode->i_uid = current_fsuid();
> - ip->i_d.di_uid = xfs_kuid_to_uid(inode->i_uid);
> inode->i_rdev = rdev;
> ip->i_d.di_projid = prid;
>
> if (pip && XFS_INHERIT_GID(pip)) {
> inode->i_gid = VFS_I(pip)->i_gid;
> - ip->i_d.di_gid = pip->i_d.di_gid;
> if ((VFS_I(pip)->i_mode & S_ISGID) && S_ISDIR(mode))
> inode->i_mode |= S_ISGID;
> } else {
> inode->i_gid = current_fsgid();
> - ip->i_d.di_gid = xfs_kgid_to_gid(inode->i_gid);
> }
>
> /*
> @@ -832,9 +829,8 @@ xfs_ialloc(
> * ID or one of the supplementary group IDs, the S_ISGID bit is cleared
> * (and only if the irix_sgid_inherit compatibility variable is set).
> */
> - if ((irix_sgid_inherit) &&
> - (inode->i_mode & S_ISGID) &&
> - (!in_group_p(xfs_gid_to_kgid(ip->i_d.di_gid))))
> + if (irix_sgid_inherit &&
> + (inode->i_mode & S_ISGID) && !in_group_p(inode->i_gid))
> inode->i_mode &= ~S_ISGID;
>
> ip->i_d.di_size = 0;
> @@ -1162,8 +1158,7 @@ xfs_create(
> /*
> * Make sure that we have allocated dquot(s) on disk.
> */
> - error = xfs_qm_vop_dqalloc(dp, xfs_kuid_to_uid(current_fsuid()),
> - xfs_kgid_to_gid(current_fsgid()), prid,
> + error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
> XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> &udqp, &gdqp, &pdqp);
> if (error)
> @@ -1313,8 +1308,7 @@ xfs_create_tmpfile(
> /*
> * Make sure that we have allocated dquot(s) on disk.
> */
> - error = xfs_qm_vop_dqalloc(dp, xfs_kuid_to_uid(current_fsuid()),
> - xfs_kgid_to_gid(current_fsgid()), prid,
> + error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
> XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> &udqp, &gdqp, &pdqp);
> if (error)
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 8bd5d0de6321..83d7914556ef 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -308,8 +308,8 @@ xfs_inode_to_log_dinode(
>
> to->di_version = from->di_version;
> to->di_format = from->di_format;
> - to->di_uid = from->di_uid;
> - to->di_gid = from->di_gid;
> + to->di_uid = xfs_kuid_to_uid(inode->i_uid);
> + to->di_gid = xfs_kgid_to_gid(inode->i_gid);
> to->di_projid_lo = from->di_projid & 0xffff;
> to->di_projid_hi = from->di_projid >> 16;
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index d42de92cb283..0f85bedc5977 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1434,9 +1434,9 @@ xfs_ioctl_setattr(
> * because the i_*dquot fields will get updated anyway.
> */
> if (XFS_IS_QUOTA_ON(mp)) {
> - code = xfs_qm_vop_dqalloc(ip, ip->i_d.di_uid,
> - ip->i_d.di_gid, fa->fsx_projid,
> - XFS_QMOPT_PQUOTA, &udqp, NULL, &pdqp);
> + code = xfs_qm_vop_dqalloc(ip, VFS_I(ip)->i_uid,
> + VFS_I(ip)->i_gid, fa->fsx_projid,
> + XFS_QMOPT_PQUOTA, &udqp, NULL, &pdqp);
> if (code)
> return code;
> }
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index b818b261918f..a5b7c3100a2f 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -692,9 +692,7 @@ xfs_setattr_nonsize(
> */
> ASSERT(udqp == NULL);
> ASSERT(gdqp == NULL);
> - error = xfs_qm_vop_dqalloc(ip, xfs_kuid_to_uid(uid),
> - xfs_kgid_to_gid(gid),
> - ip->i_d.di_projid,
> + error = xfs_qm_vop_dqalloc(ip, uid, gid, ip->i_d.di_projid,
> qflags, &udqp, &gdqp, NULL);
> if (error)
> return error;
> @@ -763,7 +761,6 @@ xfs_setattr_nonsize(
> olddquot1 = xfs_qm_vop_chown(tp, ip,
> &ip->i_udquot, udqp);
> }
> - ip->i_d.di_uid = xfs_kuid_to_uid(uid);
> inode->i_uid = uid;
> }
> if (!gid_eq(igid, gid)) {
> @@ -775,7 +772,6 @@ xfs_setattr_nonsize(
> olddquot2 = xfs_qm_vop_chown(tp, ip,
> &ip->i_gdquot, gdqp);
> }
> - ip->i_d.di_gid = xfs_kgid_to_gid(gid);
> inode->i_gid = gid;
> }
> }
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 4b31c29b7e6b..497db4160283 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -86,8 +86,8 @@ xfs_bulkstat_one_int(
> */
> buf->bs_projectid = ip->i_d.di_projid;
> buf->bs_ino = ino;
> - buf->bs_uid = dic->di_uid;
> - buf->bs_gid = dic->di_gid;
> + buf->bs_uid = xfs_kuid_to_uid(inode->i_uid);
> + buf->bs_gid = xfs_kgid_to_gid(inode->i_gid);
> buf->bs_size = dic->di_size;
>
> buf->bs_nlink = inode->i_nlink;
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 0b0909657bad..54dda7d982c9 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -326,16 +326,18 @@ xfs_qm_dqattach_locked(
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>
> if (XFS_IS_UQUOTA_ON(mp) && !ip->i_udquot) {
> - error = xfs_qm_dqattach_one(ip, ip->i_d.di_uid, XFS_DQ_USER,
> - doalloc, &ip->i_udquot);
> + error = xfs_qm_dqattach_one(ip,
> + xfs_kuid_to_uid(VFS_I(ip)->i_uid),
> + XFS_DQ_USER, doalloc, &ip->i_udquot);
> if (error)
> goto done;
> ASSERT(ip->i_udquot);
> }
>
> if (XFS_IS_GQUOTA_ON(mp) && !ip->i_gdquot) {
> - error = xfs_qm_dqattach_one(ip, ip->i_d.di_gid, XFS_DQ_GROUP,
> - doalloc, &ip->i_gdquot);
> + error = xfs_qm_dqattach_one(ip,
> + xfs_kgid_to_gid(VFS_I(ip)->i_gid),
> + XFS_DQ_GROUP, doalloc, &ip->i_gdquot);
> if (error)
> goto done;
> ASSERT(ip->i_gdquot);
> @@ -1613,8 +1615,8 @@ xfs_qm_dqfree_one(
> int
> xfs_qm_vop_dqalloc(
> struct xfs_inode *ip,
> - xfs_dqid_t uid,
> - xfs_dqid_t gid,
> + kuid_t uid,
> + kgid_t gid,
> prid_t prid,
> uint flags,
> struct xfs_dquot **O_udqpp,
> @@ -1622,6 +1624,7 @@ xfs_qm_vop_dqalloc(
> struct xfs_dquot **O_pdqpp)
> {
> struct xfs_mount *mp = ip->i_mount;
> + struct inode *inode = VFS_I(ip);
> struct xfs_dquot *uq = NULL;
> struct xfs_dquot *gq = NULL;
> struct xfs_dquot *pq = NULL;
> @@ -1635,7 +1638,7 @@ xfs_qm_vop_dqalloc(
> xfs_ilock(ip, lockflags);
>
> if ((flags & XFS_QMOPT_INHERIT) && XFS_INHERIT_GID(ip))
> - gid = ip->i_d.di_gid;
> + gid = inode->i_gid;
>
> /*
> * Attach the dquot(s) to this inode, doing a dquot allocation
> @@ -1650,7 +1653,7 @@ xfs_qm_vop_dqalloc(
> }
>
> if ((flags & XFS_QMOPT_UQUOTA) && XFS_IS_UQUOTA_ON(mp)) {
> - if (ip->i_d.di_uid != uid) {
> + if (!uid_eq(inode->i_uid, uid)) {
> /*
> * What we need is the dquot that has this uid, and
> * if we send the inode to dqget, the uid of the inode
> @@ -1661,7 +1664,8 @@ xfs_qm_vop_dqalloc(
> * holding ilock.
> */
> xfs_iunlock(ip, lockflags);
> - error = xfs_qm_dqget(mp, uid, XFS_DQ_USER, true, &uq);
> + error = xfs_qm_dqget(mp, xfs_kuid_to_uid(uid),
> + XFS_DQ_USER, true, &uq);
> if (error) {
> ASSERT(error != -ENOENT);
> return error;
> @@ -1682,9 +1686,10 @@ xfs_qm_vop_dqalloc(
> }
> }
> if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
> - if (ip->i_d.di_gid != gid) {
> + if (!gid_eq(inode->i_gid, gid)) {
> xfs_iunlock(ip, lockflags);
> - error = xfs_qm_dqget(mp, gid, XFS_DQ_GROUP, true, &gq);
> + error = xfs_qm_dqget(mp, xfs_kgid_to_gid(gid),
> + XFS_DQ_GROUP, true, &gq);
> if (error) {
> ASSERT(error != -ENOENT);
> goto error_rele;
> @@ -1810,7 +1815,8 @@ xfs_qm_vop_chown_reserve(
> XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
>
> if (XFS_IS_UQUOTA_ON(mp) && udqp &&
> - ip->i_d.di_uid != be32_to_cpu(udqp->q_core.d_id)) {
> + xfs_kuid_to_uid(VFS_I(ip)->i_uid) !=
> + be32_to_cpu(udqp->q_core.d_id)) {
> udq_delblks = udqp;
> /*
> * If there are delayed allocation blocks, then we have to
> @@ -1823,7 +1829,8 @@ xfs_qm_vop_chown_reserve(
> }
> }
> if (XFS_IS_GQUOTA_ON(ip->i_mount) && gdqp &&
> - ip->i_d.di_gid != be32_to_cpu(gdqp->q_core.d_id)) {
> + xfs_kgid_to_gid(VFS_I(ip)->i_gid) !=
> + be32_to_cpu(gdqp->q_core.d_id)) {
> gdq_delblks = gdqp;
> if (delblks) {
> ASSERT(ip->i_gdquot);
> @@ -1920,14 +1927,17 @@ xfs_qm_vop_create_dqattach(
>
> if (udqp && XFS_IS_UQUOTA_ON(mp)) {
> ASSERT(ip->i_udquot == NULL);
> - ASSERT(ip->i_d.di_uid == be32_to_cpu(udqp->q_core.d_id));
> + ASSERT(xfs_kuid_to_uid(VFS_I(ip)->i_uid) ==
> + be32_to_cpu(udqp->q_core.d_id));
>
> ip->i_udquot = xfs_qm_dqhold(udqp);
> xfs_trans_mod_dquot(tp, udqp, XFS_TRANS_DQ_ICOUNT, 1);
> }
> if (gdqp && XFS_IS_GQUOTA_ON(mp)) {
> ASSERT(ip->i_gdquot == NULL);
> - ASSERT(ip->i_d.di_gid == be32_to_cpu(gdqp->q_core.d_id));
> + ASSERT(xfs_kgid_to_gid(VFS_I(ip)->i_gid) ==
> + be32_to_cpu(gdqp->q_core.d_id));
> +
> ip->i_gdquot = xfs_qm_dqhold(gdqp);
> xfs_trans_mod_dquot(tp, gdqp, XFS_TRANS_DQ_ICOUNT, 1);
> }
> diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
> index efe42ae7a2f3..aa8fc1f55fbd 100644
> --- a/fs/xfs/xfs_quota.h
> +++ b/fs/xfs/xfs_quota.h
> @@ -86,7 +86,7 @@ extern int xfs_trans_reserve_quota_bydquots(struct xfs_trans *,
> struct xfs_mount *, struct xfs_dquot *,
> struct xfs_dquot *, struct xfs_dquot *, int64_t, long, uint);
>
> -extern int xfs_qm_vop_dqalloc(struct xfs_inode *, xfs_dqid_t, xfs_dqid_t,
> +extern int xfs_qm_vop_dqalloc(struct xfs_inode *, kuid_t, kgid_t,
> prid_t, uint, struct xfs_dquot **, struct xfs_dquot **,
> struct xfs_dquot **);
> extern void xfs_qm_vop_create_dqattach(struct xfs_trans *, struct xfs_inode *,
> @@ -109,7 +109,7 @@ extern void xfs_qm_unmount_quotas(struct xfs_mount *);
>
> #else
> static inline int
> -xfs_qm_vop_dqalloc(struct xfs_inode *ip, xfs_dqid_t uid, xfs_dqid_t gid,
> +xfs_qm_vop_dqalloc(struct xfs_inode *ip, kuid_t kuid, kgid_t kgid,
> prid_t prid, uint flags, struct xfs_dquot **udqp,
> struct xfs_dquot **gdqp, struct xfs_dquot **pdqp)
> {
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index d762d42ed0ff..ea42e25ec1bf 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -182,9 +182,7 @@ xfs_symlink(
> /*
> * Make sure that we have allocated dquot(s) on disk.
> */
> - error = xfs_qm_vop_dqalloc(dp,
> - xfs_kuid_to_uid(current_fsuid()),
> - xfs_kgid_to_gid(current_fsgid()), prid,
> + error = xfs_qm_vop_dqalloc(dp, current_fsuid(), current_fsgid(), prid,
> XFS_QMOPT_QUOTALL | XFS_QMOPT_INHERIT,
> &udqp, &gdqp, &pdqp);
> if (error)
>
--
chandan
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] xfs: remove the kuid/kgid conversion wrappers
2020-02-18 21:00 ` [PATCH 3/3] xfs: remove the kuid/kgid conversion wrappers Christoph Hellwig
2020-02-19 11:05 ` Carlos Maiolino
2020-02-19 14:26 ` Brian Foster
@ 2020-02-21 1:26 ` Darrick J. Wong
2020-02-21 15:54 ` Christoph Hellwig
2 siblings, 1 reply; 16+ messages in thread
From: Darrick J. Wong @ 2020-02-21 1:26 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, linux-fsdevel
On Tue, Feb 18, 2020 at 01:00:20PM -0800, Christoph Hellwig wrote:
> Remove the XFS wrappers for converting from and to the kuid/kgid types.
> Mostly this means switching to VFS i_{u,g}id_{read,write} helpers, but
> in a few spots the calls to the conversion functions is open coded.
> To match the use of sb->s_user_ns in the helpers and other file systems,
> sb->s_user_ns is also used in the quota code. The ACL code already does
> the conversion in a grotty layering violation in the VFS xattr code,
> so it keeps using init_user_ns for the identity mapping.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/libxfs/xfs_inode_buf.c | 8 ++++----
> fs/xfs/xfs_acl.c | 12 ++++++++----
> fs/xfs/xfs_dquot.c | 4 ++--
> fs/xfs/xfs_inode_item.c | 4 ++--
> fs/xfs/xfs_itable.c | 4 ++--
> fs/xfs/xfs_linux.h | 26 --------------------------
> fs/xfs/xfs_qm.c | 23 +++++++++--------------
> 7 files changed, 27 insertions(+), 54 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index bc72b575ceed..17e88a8c8353 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -222,8 +222,8 @@ xfs_inode_from_disk(
> }
>
> to->di_format = from->di_format;
> - inode->i_uid = xfs_uid_to_kuid(be32_to_cpu(from->di_uid));
Hmm. I'm not up on my userns-fu, but right now this is effectively:
inode->i_uid = make_kuid(&init_user_ns, be32_to_cpu(from->di_uid));
> - inode->i_gid = xfs_gid_to_kgid(be32_to_cpu(from->di_gid));
> + i_uid_write(inode, be32_to_cpu(from->di_uid));
Whereas this is:
inode->i_uid = make_kuid(inode->i_sb->s_user_ns, be32_to_cpu(...));
What happens if s_user_ns != init_user_ns? Isn't this a behavior
change? Granted, it looks like many of the other filesystems use
i_uid_write so maybe we're the ones who are doing it wrong...?
> + i_gid_write(inode, be32_to_cpu(from->di_gid));
> to->di_flushiter = be16_to_cpu(from->di_flushiter);
>
> /*
> @@ -276,8 +276,8 @@ xfs_inode_to_disk(
>
> to->di_version = from->di_version;
> to->di_format = from->di_format;
> - to->di_uid = cpu_to_be32(xfs_kuid_to_uid(inode->i_uid));
> - to->di_gid = cpu_to_be32(xfs_kgid_to_gid(inode->i_gid));
> + to->di_uid = cpu_to_be32(i_uid_read(inode));
> + to->di_gid = cpu_to_be32(i_gid_read(inode));
> to->di_projid_lo = cpu_to_be16(from->di_projid & 0xffff);
> to->di_projid_hi = cpu_to_be16(from->di_projid >> 16);
>
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index cd743fad8478..e7314b525b19 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -67,10 +67,12 @@ xfs_acl_from_disk(
>
> switch (acl_e->e_tag) {
> case ACL_USER:
> - acl_e->e_uid = xfs_uid_to_kuid(be32_to_cpu(ace->ae_id));
> + acl_e->e_uid = make_kuid(&init_user_ns,
> + be32_to_cpu(ace->ae_id));
And I'm assuming that the "gross layering violation in the vfs xattr
code" is why it's init_user_ns here?
--D
> break;
> case ACL_GROUP:
> - acl_e->e_gid = xfs_gid_to_kgid(be32_to_cpu(ace->ae_id));
> + acl_e->e_gid = make_kgid(&init_user_ns,
> + be32_to_cpu(ace->ae_id));
> break;
> case ACL_USER_OBJ:
> case ACL_GROUP_OBJ:
> @@ -103,10 +105,12 @@ xfs_acl_to_disk(struct xfs_acl *aclp, const struct posix_acl *acl)
> ace->ae_tag = cpu_to_be32(acl_e->e_tag);
> switch (acl_e->e_tag) {
> case ACL_USER:
> - ace->ae_id = cpu_to_be32(xfs_kuid_to_uid(acl_e->e_uid));
> + ace->ae_id = cpu_to_be32(
> + from_kuid(&init_user_ns, acl_e->e_uid));
> break;
> case ACL_GROUP:
> - ace->ae_id = cpu_to_be32(xfs_kgid_to_gid(acl_e->e_gid));
> + ace->ae_id = cpu_to_be32(
> + from_kgid(&init_user_ns, acl_e->e_gid));
> break;
> default:
> ace->ae_id = cpu_to_be32(ACL_UNDEFINED_ID);
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 3579de9306c1..711376ca269f 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -829,9 +829,9 @@ xfs_qm_id_for_quotatype(
> {
> switch (type) {
> case XFS_DQ_USER:
> - return xfs_kuid_to_uid(VFS_I(ip)->i_uid);
> + return i_uid_read(VFS_I(ip));
> case XFS_DQ_GROUP:
> - return xfs_kgid_to_gid(VFS_I(ip)->i_gid);
> + return i_gid_read(VFS_I(ip));
> case XFS_DQ_PROJ:
> return ip->i_d.di_projid;
> }
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 83d7914556ef..f021b55a0301 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -308,8 +308,8 @@ xfs_inode_to_log_dinode(
>
> to->di_version = from->di_version;
> to->di_format = from->di_format;
> - to->di_uid = xfs_kuid_to_uid(inode->i_uid);
> - to->di_gid = xfs_kgid_to_gid(inode->i_gid);
> + to->di_uid = i_uid_read(inode);
> + to->di_gid = i_gid_read(inode);
> to->di_projid_lo = from->di_projid & 0xffff;
> to->di_projid_hi = from->di_projid >> 16;
>
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 497db4160283..d10660469884 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -86,8 +86,8 @@ xfs_bulkstat_one_int(
> */
> buf->bs_projectid = ip->i_d.di_projid;
> buf->bs_ino = ino;
> - buf->bs_uid = xfs_kuid_to_uid(inode->i_uid);
> - buf->bs_gid = xfs_kgid_to_gid(inode->i_gid);
> + buf->bs_uid = i_uid_read(inode);
> + buf->bs_gid = i_gid_read(inode);
> buf->bs_size = dic->di_size;
>
> buf->bs_nlink = inode->i_nlink;
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index 8738bb03f253..bc43cd98697b 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -163,32 +163,6 @@ struct xstats {
>
> extern struct xstats xfsstats;
>
> -/* Kernel uid/gid conversion. These are used to convert to/from the on disk
> - * uid_t/gid_t types to the kuid_t/kgid_t types that the kernel uses internally.
> - * The conversion here is type only, the value will remain the same since we
> - * are converting to the init_user_ns. The uid is later mapped to a particular
> - * user namespace value when crossing the kernel/user boundary.
> - */
> -static inline uint32_t xfs_kuid_to_uid(kuid_t uid)
> -{
> - return from_kuid(&init_user_ns, uid);
> -}
> -
> -static inline kuid_t xfs_uid_to_kuid(uint32_t uid)
> -{
> - return make_kuid(&init_user_ns, uid);
> -}
> -
> -static inline uint32_t xfs_kgid_to_gid(kgid_t gid)
> -{
> - return from_kgid(&init_user_ns, gid);
> -}
> -
> -static inline kgid_t xfs_gid_to_kgid(uint32_t gid)
> -{
> - return make_kgid(&init_user_ns, gid);
> -}
> -
> static inline dev_t xfs_to_linux_dev_t(xfs_dev_t dev)
> {
> return MKDEV(sysv_major(dev) & 0x1ff, sysv_minor(dev));
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 54dda7d982c9..de1d2c606c14 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -326,8 +326,7 @@ xfs_qm_dqattach_locked(
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>
> if (XFS_IS_UQUOTA_ON(mp) && !ip->i_udquot) {
> - error = xfs_qm_dqattach_one(ip,
> - xfs_kuid_to_uid(VFS_I(ip)->i_uid),
> + error = xfs_qm_dqattach_one(ip, i_uid_read(VFS_I(ip)),
> XFS_DQ_USER, doalloc, &ip->i_udquot);
> if (error)
> goto done;
> @@ -335,8 +334,7 @@ xfs_qm_dqattach_locked(
> }
>
> if (XFS_IS_GQUOTA_ON(mp) && !ip->i_gdquot) {
> - error = xfs_qm_dqattach_one(ip,
> - xfs_kgid_to_gid(VFS_I(ip)->i_gid),
> + error = xfs_qm_dqattach_one(ip, i_gid_read(VFS_I(ip)),
> XFS_DQ_GROUP, doalloc, &ip->i_gdquot);
> if (error)
> goto done;
> @@ -1625,6 +1623,7 @@ xfs_qm_vop_dqalloc(
> {
> struct xfs_mount *mp = ip->i_mount;
> struct inode *inode = VFS_I(ip);
> + struct user_namespace *user_ns = inode->i_sb->s_user_ns;
> struct xfs_dquot *uq = NULL;
> struct xfs_dquot *gq = NULL;
> struct xfs_dquot *pq = NULL;
> @@ -1664,7 +1663,7 @@ xfs_qm_vop_dqalloc(
> * holding ilock.
> */
> xfs_iunlock(ip, lockflags);
> - error = xfs_qm_dqget(mp, xfs_kuid_to_uid(uid),
> + error = xfs_qm_dqget(mp, from_kuid(user_ns, uid),
> XFS_DQ_USER, true, &uq);
> if (error) {
> ASSERT(error != -ENOENT);
> @@ -1688,7 +1687,7 @@ xfs_qm_vop_dqalloc(
> if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
> if (!gid_eq(inode->i_gid, gid)) {
> xfs_iunlock(ip, lockflags);
> - error = xfs_qm_dqget(mp, xfs_kgid_to_gid(gid),
> + error = xfs_qm_dqget(mp, from_kgid(user_ns, gid),
> XFS_DQ_GROUP, true, &gq);
> if (error) {
> ASSERT(error != -ENOENT);
> @@ -1815,8 +1814,7 @@ xfs_qm_vop_chown_reserve(
> XFS_QMOPT_RES_RTBLKS : XFS_QMOPT_RES_REGBLKS;
>
> if (XFS_IS_UQUOTA_ON(mp) && udqp &&
> - xfs_kuid_to_uid(VFS_I(ip)->i_uid) !=
> - be32_to_cpu(udqp->q_core.d_id)) {
> + i_uid_read(VFS_I(ip)) != be32_to_cpu(udqp->q_core.d_id)) {
> udq_delblks = udqp;
> /*
> * If there are delayed allocation blocks, then we have to
> @@ -1829,8 +1827,7 @@ xfs_qm_vop_chown_reserve(
> }
> }
> if (XFS_IS_GQUOTA_ON(ip->i_mount) && gdqp &&
> - xfs_kgid_to_gid(VFS_I(ip)->i_gid) !=
> - be32_to_cpu(gdqp->q_core.d_id)) {
> + i_gid_read(VFS_I(ip)) != be32_to_cpu(gdqp->q_core.d_id)) {
> gdq_delblks = gdqp;
> if (delblks) {
> ASSERT(ip->i_gdquot);
> @@ -1927,16 +1924,14 @@ xfs_qm_vop_create_dqattach(
>
> if (udqp && XFS_IS_UQUOTA_ON(mp)) {
> ASSERT(ip->i_udquot == NULL);
> - ASSERT(xfs_kuid_to_uid(VFS_I(ip)->i_uid) ==
> - be32_to_cpu(udqp->q_core.d_id));
> + ASSERT(i_uid_read(VFS_I(ip)) == be32_to_cpu(udqp->q_core.d_id));
>
> ip->i_udquot = xfs_qm_dqhold(udqp);
> xfs_trans_mod_dquot(tp, udqp, XFS_TRANS_DQ_ICOUNT, 1);
> }
> if (gdqp && XFS_IS_GQUOTA_ON(mp)) {
> ASSERT(ip->i_gdquot == NULL);
> - ASSERT(xfs_kgid_to_gid(VFS_I(ip)->i_gid) ==
> - be32_to_cpu(gdqp->q_core.d_id));
> + ASSERT(i_gid_read(VFS_I(ip)) == be32_to_cpu(gdqp->q_core.d_id));
>
> ip->i_gdquot = xfs_qm_dqhold(gdqp);
> xfs_trans_mod_dquot(tp, gdqp, XFS_TRANS_DQ_ICOUNT, 1);
> --
> 2.24.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] xfs: remove the kuid/kgid conversion wrappers
2020-02-21 1:26 ` Darrick J. Wong
@ 2020-02-21 15:54 ` Christoph Hellwig
2020-02-21 16:19 ` Darrick J. Wong
0 siblings, 1 reply; 16+ messages in thread
From: Christoph Hellwig @ 2020-02-21 15:54 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Christoph Hellwig, linux-xfs, linux-fsdevel
On Thu, Feb 20, 2020 at 05:26:16PM -0800, Darrick J. Wong wrote:
> > to->di_format = from->di_format;
> > - inode->i_uid = xfs_uid_to_kuid(be32_to_cpu(from->di_uid));
>
> Hmm. I'm not up on my userns-fu, but right now this is effectively:
>
> inode->i_uid = make_kuid(&init_user_ns, be32_to_cpu(from->di_uid));
>
> > - inode->i_gid = xfs_gid_to_kgid(be32_to_cpu(from->di_gid));
> > + i_uid_write(inode, be32_to_cpu(from->di_uid));
>
> Whereas this is:
>
> inode->i_uid = make_kuid(inode->i_sb->s_user_ns, be32_to_cpu(...));
Yes. Which is intentional and mentioned in the commit log.
>
> What happens if s_user_ns != init_user_ns? Isn't this a behavior
> change? Granted, it looks like many of the other filesystems use
> i_uid_write so maybe we're the ones who are doing it wrong...?
In that case the uid gets translated. Which is intentional as it is
done everywhere else and XFS is the ugly ducking out that fails
to properly take the user_ns into account.
> > --- a/fs/xfs/xfs_acl.c
> > +++ b/fs/xfs/xfs_acl.c
> > @@ -67,10 +67,12 @@ xfs_acl_from_disk(
> >
> > switch (acl_e->e_tag) {
> > case ACL_USER:
> > - acl_e->e_uid = xfs_uid_to_kuid(be32_to_cpu(ace->ae_id));
> > + acl_e->e_uid = make_kuid(&init_user_ns,
> > + be32_to_cpu(ace->ae_id));
>
> And I'm assuming that the "gross layering violation in the vfs xattr
> code" is why it's init_user_ns here?
Yes. The generic xattr code checks if the attr is one of the ACL ones
in common code before calling into the fs and already translates them,
causing a giant mess.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] xfs: remove the kuid/kgid conversion wrappers
2020-02-21 15:54 ` Christoph Hellwig
@ 2020-02-21 16:19 ` Darrick J. Wong
2020-02-21 16:29 ` Christoph Hellwig
0 siblings, 1 reply; 16+ messages in thread
From: Darrick J. Wong @ 2020-02-21 16:19 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, linux-fsdevel
On Fri, Feb 21, 2020 at 04:54:50PM +0100, Christoph Hellwig wrote:
> On Thu, Feb 20, 2020 at 05:26:16PM -0800, Darrick J. Wong wrote:
> > > to->di_format = from->di_format;
> > > - inode->i_uid = xfs_uid_to_kuid(be32_to_cpu(from->di_uid));
> >
> > Hmm. I'm not up on my userns-fu, but right now this is effectively:
> >
> > inode->i_uid = make_kuid(&init_user_ns, be32_to_cpu(from->di_uid));
> >
> > > - inode->i_gid = xfs_gid_to_kgid(be32_to_cpu(from->di_gid));
> > > + i_uid_write(inode, be32_to_cpu(from->di_uid));
> >
> > Whereas this is:
> >
> > inode->i_uid = make_kuid(inode->i_sb->s_user_ns, be32_to_cpu(...));
>
> Yes. Which is intentional and mentioned in the commit log.
>
> >
> > What happens if s_user_ns != init_user_ns? Isn't this a behavior
> > change? Granted, it looks like many of the other filesystems use
> > i_uid_write so maybe we're the ones who are doing it wrong...?
>
> In that case the uid gets translated. Which is intentional as it is
> done everywhere else and XFS is the ugly ducking out that fails
> to properly take the user_ns into account.
Ok, we were doing it wrong. Should this series have fixed that as the
first patch (so that we could push it into old kernels) followed by the
actual icdinode field removal?
(Granted nobody seems to have complained...)
> > > --- a/fs/xfs/xfs_acl.c
> > > +++ b/fs/xfs/xfs_acl.c
> > > @@ -67,10 +67,12 @@ xfs_acl_from_disk(
> > >
> > > switch (acl_e->e_tag) {
> > > case ACL_USER:
> > > - acl_e->e_uid = xfs_uid_to_kuid(be32_to_cpu(ace->ae_id));
> > > + acl_e->e_uid = make_kuid(&init_user_ns,
> > > + be32_to_cpu(ace->ae_id));
> >
> > And I'm assuming that the "gross layering violation in the vfs xattr
> > code" is why it's init_user_ns here?
>
> Yes. The generic xattr code checks if the attr is one of the ACL ones
> in common code before calling into the fs and already translates them,
> causing a giant mess.
Got it.
--D
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] xfs: remove the kuid/kgid conversion wrappers
2020-02-21 16:19 ` Darrick J. Wong
@ 2020-02-21 16:29 ` Christoph Hellwig
0 siblings, 0 replies; 16+ messages in thread
From: Christoph Hellwig @ 2020-02-21 16:29 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Christoph Hellwig, linux-xfs, linux-fsdevel
On Fri, Feb 21, 2020 at 08:19:43AM -0800, Darrick J. Wong wrote:
> Ok, we were doing it wrong. Should this series have fixed that as the
> first patch (so that we could push it into old kernels) followed by the
> actual icdinode field removal?
Maybe I could squeeze that in after patch 1. Before that we'd get weird
mismatch, which is how I arrived at the current series.
> (Granted nobody seems to have complained...)
Right now the use case seems entirely theoretical, but there are multiple
series out on fsdevel that aim to change that.
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-02-21 16:29 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-18 21:00 remove the di_uid/di_gid fields from the XFS icdinode Christoph Hellwig
2020-02-18 21:00 ` [PATCH 1/3] xfs: ensure that the inode uid/gid match values match the icdinode ones Christoph Hellwig
2020-02-19 10:32 ` Carlos Maiolino
2020-02-19 14:26 ` Brian Foster
2020-02-19 14:47 ` Chandan Rajendra
2020-02-18 21:00 ` [PATCH 2/3] xfs: remove the icdinode di_uid/di_gid members Christoph Hellwig
2020-02-19 10:51 ` Carlos Maiolino
2020-02-19 14:26 ` Brian Foster
2020-02-19 16:25 ` Chandan Rajendra
2020-02-18 21:00 ` [PATCH 3/3] xfs: remove the kuid/kgid conversion wrappers Christoph Hellwig
2020-02-19 11:05 ` Carlos Maiolino
2020-02-19 14:26 ` Brian Foster
2020-02-21 1:26 ` Darrick J. Wong
2020-02-21 15:54 ` Christoph Hellwig
2020-02-21 16:19 ` Darrick J. Wong
2020-02-21 16:29 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).