* [PATCH 1/8] fs: add support to change project ID
2019-03-01 14:05 [PATCH 0/8] add generic interface to set/get project Wang Shilong
@ 2019-03-01 14:05 ` Wang Shilong
2019-03-03 21:53 ` Dave Chinner
2019-03-01 14:05 ` [PATCH 2/8] ext4: support project ID in ext4_setattr() Wang Shilong
` (7 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Wang Shilong @ 2019-03-01 14:05 UTC (permalink / raw)
To: linux-fsdevel, linux-ext4, linux-xfs, linux-f2fs-devel
Cc: lixi, adilger, Wang Shilong
From: Wang Shilong <wshilong@ddn.com>
From: Wang Shilong <wshilong@ddn.com>
Currently, Filesystem use FS_IOC_FS_SETXATTR ioctl
to change project ID of file. However we don't
support ioctl on symlink files, and it is desirable
to change symlink files' project ID just like uid/gid.
This patch try to reuse existed interface fchownat(),
use group id to set project ID if flag AT_FCHOWN_PROJID
passed in.
Signed-off-by: Wang Shilong <wshilong@ddn.com>
---
fs/attr.c | 26 ++++++++++++++++++++++++--
fs/open.c | 29 +++++++++++++++++++++++------
fs/quota/dquot.c | 23 +++++++++++++++++++++++
include/linux/fs.h | 3 +++
include/linux/quotaops.h | 9 +++++++++
include/uapi/linux/fcntl.h | 1 +
tools/include/uapi/linux/fcntl.h | 1 +
7 files changed, 84 insertions(+), 8 deletions(-)
diff --git a/fs/attr.c b/fs/attr.c
index d22e8187477f..c6b1c1132c8f 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -85,6 +85,28 @@ int setattr_prepare(struct dentry *dentry, struct iattr *attr)
if ((ia_valid & ATTR_GID) && !chgrp_ok(inode, attr->ia_gid))
return -EPERM;
+ /*
+ * Project Quota ID state is only allowed to change from within the init
+ * namespace. Enforce that restriction only if we are trying to change
+ * the quota ID state. Everything else is allowed in user namespaces.
+ */
+ if ((ia_valid & ATTR_PROJID) && current_user_ns() != &init_user_ns) {
+ kprojid_t projid;
+ int rc;
+
+ /*
+ * Filesystem like xfs does't have ->get_projid hook
+ * should check permission by themselves.
+ */
+ if (inode->i_sb->dq_op->get_projid) {
+ rc = inode->i_sb->dq_op->get_projid(inode, &projid);
+ if (rc)
+ return rc;
+ if (!projid_eq(projid, attr->ia_projid))
+ return -EPERM;
+ }
+ }
+
/* Make sure a caller can chmod. */
if (ia_valid & ATTR_MODE) {
if (!inode_owner_or_capable(inode))
@@ -232,8 +254,8 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
unsigned int ia_valid = attr->ia_valid;
WARN_ON_ONCE(!inode_is_locked(inode));
-
- if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID | ATTR_TIMES_SET)) {
+ if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID | ATTR_PROJID |
+ ATTR_TIMES_SET)) {
if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
return -EPERM;
}
diff --git a/fs/open.c b/fs/open.c
index 0285ce7dbd51..4e58c6ee23b3 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -597,7 +597,8 @@ SYSCALL_DEFINE2(chmod, const char __user *, filename, umode_t, mode)
return do_fchmodat(AT_FDCWD, filename, mode);
}
-static int chown_common(const struct path *path, uid_t user, gid_t group)
+static int chown_common(const struct path *path, uid_t user, gid_t group,
+ bool set_project)
{
struct inode *inode = path->dentry->d_inode;
struct inode *delegated_inode = NULL;
@@ -605,9 +606,11 @@ static int chown_common(const struct path *path, uid_t user, gid_t group)
struct iattr newattrs;
kuid_t uid;
kgid_t gid;
+ kprojid_t projid;
uid = make_kuid(current_user_ns(), user);
gid = make_kgid(current_user_ns(), group);
+ projid = make_kprojid(current_user_ns(), (projid_t)group);
retry_deleg:
newattrs.ia_valid = ATTR_CTIME;
@@ -620,13 +623,22 @@ static int chown_common(const struct path *path, uid_t user, gid_t group)
if (group != (gid_t) -1) {
if (!gid_valid(gid))
return -EINVAL;
- newattrs.ia_valid |= ATTR_GID;
- newattrs.ia_gid = gid;
+ if (!set_project) {
+ newattrs.ia_valid |= ATTR_GID;
+ newattrs.ia_gid = gid;
+ } else {
+ newattrs.ia_valid |= ATTR_PROJID;
+ newattrs.ia_projid = projid;
+ }
+ } else if (set_project) {
+ return -EINVAL;
}
if (!S_ISDIR(inode->i_mode))
newattrs.ia_valid |=
ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV;
inode_lock(inode);
+ if (set_project)
+ gid = make_kgid(current_user_ns(), (gid_t) -1);
error = security_path_chown(path, uid, gid);
if (!error)
error = notify_change(path->dentry, &newattrs, &delegated_inode);
@@ -645,10 +657,15 @@ int do_fchownat(int dfd, const char __user *filename, uid_t user, gid_t group,
struct path path;
int error = -EINVAL;
int lookup_flags;
+ bool set_project = false;
- if ((flag & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
+ if ((flag & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH |
+ AT_FCHOWN_PROJID)) != 0)
goto out;
+ if (flag & AT_FCHOWN_PROJID)
+ set_project = true;
+
lookup_flags = (flag & AT_SYMLINK_NOFOLLOW) ? 0 : LOOKUP_FOLLOW;
if (flag & AT_EMPTY_PATH)
lookup_flags |= LOOKUP_EMPTY;
@@ -659,7 +676,7 @@ int do_fchownat(int dfd, const char __user *filename, uid_t user, gid_t group,
error = mnt_want_write(path.mnt);
if (error)
goto out_release;
- error = chown_common(&path, user, group);
+ error = chown_common(&path, user, group, set_project);
mnt_drop_write(path.mnt);
out_release:
path_put(&path);
@@ -700,7 +717,7 @@ int ksys_fchown(unsigned int fd, uid_t user, gid_t group)
if (error)
goto out_fput;
audit_file(f.file);
- error = chown_common(&f.file->f_path, user, group);
+ error = chown_common(&f.file->f_path, user, group, false);
mnt_drop_write_file(f.file);
out_fput:
fdput(f);
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index fc20e06c56ba..46f39ee87312 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -2095,6 +2095,29 @@ int dquot_transfer(struct inode *inode, struct iattr *iattr)
}
transfer_to[GRPQUOTA] = dquot;
}
+
+ if (iattr->ia_valid & ATTR_PROJID) {
+ kprojid_t projid;
+
+ if (!inode->i_sb->dq_op->get_projid)
+ return -ENOTSUPP;
+
+ ret = inode->i_sb->dq_op->get_projid(inode, &projid);
+ if (ret)
+ return ret;
+ if (!projid_eq(iattr->ia_projid, projid)) {
+ dquot = dqget(sb, make_kqid_projid(iattr->ia_projid));
+ if (IS_ERR(dquot)) {
+ if (PTR_ERR(dquot) != -ESRCH) {
+ ret = PTR_ERR(dquot);
+ goto out_put;
+ }
+ dquot = NULL;
+ }
+ transfer_to[PRJQUOTA] = dquot;
+ }
+ }
+
ret = __dquot_transfer(inode, transfer_to);
out_put:
dqput_all(transfer_to);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 29d8e2cfed0e..2a878a2b90e3 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -37,6 +37,7 @@
#include <linux/uuid.h>
#include <linux/errseq.h>
#include <linux/ioprio.h>
+#include <linux/projid.h>
#include <asm/byteorder.h>
#include <uapi/linux/fs.h>
@@ -191,6 +192,7 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
#define ATTR_OPEN (1 << 15) /* Truncating from open(O_TRUNC) */
#define ATTR_TIMES_SET (1 << 16)
#define ATTR_TOUCH (1 << 17)
+#define ATTR_PROJID (1 << 18)
/*
* Whiteout is represented by a char device. The following constants define the
@@ -213,6 +215,7 @@ struct iattr {
umode_t ia_mode;
kuid_t ia_uid;
kgid_t ia_gid;
+ kprojid_t ia_projid;
loff_t ia_size;
struct timespec64 ia_atime;
struct timespec64 ia_mtime;
diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
index dc905a4ff8d7..84d3aeb43e2c 100644
--- a/include/linux/quotaops.h
+++ b/include/linux/quotaops.h
@@ -22,6 +22,15 @@ static inline struct quota_info *sb_dqopt(struct super_block *sb)
/* i_mutex must being held */
static inline bool is_quota_modification(struct inode *inode, struct iattr *ia)
{
+ if (ia->ia_valid & ATTR_PROJID && inode->i_sb->dq_op->get_projid) {
+ kprojid_t projid;
+ int rc;
+
+ rc = inode->i_sb->dq_op->get_projid(inode, &projid);
+ if (!rc && !projid_eq(projid, ia->ia_projid))
+ return true;
+ }
+
return (ia->ia_valid & ATTR_SIZE && ia->ia_size != inode->i_size) ||
(ia->ia_valid & ATTR_UID && !uid_eq(ia->ia_uid, inode->i_uid)) ||
(ia->ia_valid & ATTR_GID && !gid_eq(ia->ia_gid, inode->i_gid));
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 6448cdd9a350..712c60d7f727 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -90,5 +90,6 @@
#define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */
#define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */
+#define AT_FCHOWN_PROJID 0x40000000 /* Change project ID instead of group id */
#endif /* _UAPI_LINUX_FCNTL_H */
diff --git a/tools/include/uapi/linux/fcntl.h b/tools/include/uapi/linux/fcntl.h
index 6448cdd9a350..712c60d7f727 100644
--- a/tools/include/uapi/linux/fcntl.h
+++ b/tools/include/uapi/linux/fcntl.h
@@ -90,5 +90,6 @@
#define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */
#define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */
+#define AT_FCHOWN_PROJID 0x40000000 /* Change project ID instead of group id */
#endif /* _UAPI_LINUX_FCNTL_H */
--
2.19.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 1/8] fs: add support to change project ID
2019-03-01 14:05 ` [PATCH 1/8] fs: add support to change project ID Wang Shilong
@ 2019-03-03 21:53 ` Dave Chinner
2019-03-04 23:36 ` 答复: " Wang Shilong
0 siblings, 1 reply; 17+ messages in thread
From: Dave Chinner @ 2019-03-03 21:53 UTC (permalink / raw)
To: Wang Shilong
Cc: linux-fsdevel, linux-ext4, linux-xfs, linux-f2fs-devel, lixi,
adilger, Wang Shilong
On Fri, Mar 01, 2019 at 11:05:34PM +0900, Wang Shilong wrote:
> From: Wang Shilong <wshilong@ddn.com>
>
> From: Wang Shilong <wshilong@ddn.com>
>
> Currently, Filesystem use FS_IOC_FS_SETXATTR ioctl
> to change project ID of file. However we don't
> support ioctl on symlink files, and it is desirable
> to change symlink files' project ID just like uid/gid.
>
> This patch try to reuse existed interface fchownat(),
> use group id to set project ID if flag AT_FCHOWN_PROJID
> passed in.
>
> Signed-off-by: Wang Shilong <wshilong@ddn.com>
> ---
> fs/attr.c | 26 ++++++++++++++++++++++++--
> fs/open.c | 29 +++++++++++++++++++++++------
> fs/quota/dquot.c | 23 +++++++++++++++++++++++
> include/linux/fs.h | 3 +++
> include/linux/quotaops.h | 9 +++++++++
> include/uapi/linux/fcntl.h | 1 +
> tools/include/uapi/linux/fcntl.h | 1 +
> 7 files changed, 84 insertions(+), 8 deletions(-)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index d22e8187477f..c6b1c1132c8f 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -85,6 +85,28 @@ int setattr_prepare(struct dentry *dentry, struct iattr *attr)
> if ((ia_valid & ATTR_GID) && !chgrp_ok(inode, attr->ia_gid))
> return -EPERM;
>
> + /*
> + * Project Quota ID state is only allowed to change from within the init
> + * namespace. Enforce that restriction only if we are trying to change
> + * the quota ID state. Everything else is allowed in user namespaces.
> + */
> + if ((ia_valid & ATTR_PROJID) && current_user_ns() != &init_user_ns) {
> + kprojid_t projid;
> + int rc;
> +
> + /*
> + * Filesystem like xfs does't have ->get_projid hook
> + * should check permission by themselves.
> + */
> + if (inode->i_sb->dq_op->get_projid) {
> + rc = inode->i_sb->dq_op->get_projid(inode, &projid);
> + if (rc)
> + return rc;
> + if (!projid_eq(projid, attr->ia_projid))
> + return -EPERM;
> + }
> + }
That's a nasty landmine, and we shouldn't be making exceptions like
this in generic code. And, really, it makes no sense to me to be
checking if the projid is changing, either. If ATTR_PROJID is set,
and we aren't in the init_user_ns then reject it.
i.e. Callers should not set ATTR_PROJID if they aren't changing it,
not expect the infrastructure to silently ignore attempts to change
attributes they do not have permission to change when no change will
eventually occur.
> /* Make sure a caller can chmod. */
> if (ia_valid & ATTR_MODE) {
> if (!inode_owner_or_capable(inode))
> @@ -232,8 +254,8 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
> unsigned int ia_valid = attr->ia_valid;
>
> WARN_ON_ONCE(!inode_is_locked(inode));
> -
> - if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID | ATTR_TIMES_SET)) {
> + if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID | ATTR_PROJID |
> + ATTR_TIMES_SET)) {
> if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
> return -EPERM;
> }
> diff --git a/fs/open.c b/fs/open.c
> index 0285ce7dbd51..4e58c6ee23b3 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -597,7 +597,8 @@ SYSCALL_DEFINE2(chmod, const char __user *, filename, umode_t, mode)
> return do_fchmodat(AT_FDCWD, filename, mode);
> }
>
> -static int chown_common(const struct path *path, uid_t user, gid_t group)
> +static int chown_common(const struct path *path, uid_t user, gid_t group,
> + bool set_project)
Why not just pass the projid? This bleeds API definition into the
implementation. chown_common() shoul dbe able toset all three IDs in
one call as it is not restricted by the chownat() userspace API.
i.e.:
static int chown_common(const struct path *path, uid_t user, gid_t group, projid_t project)
and the IDs that aren't getting set should be passed with the value
of -1.
> {
> struct inode *inode = path->dentry->d_inode;
> struct inode *delegated_inode = NULL;
> @@ -605,9 +606,11 @@ static int chown_common(const struct path *path, uid_t user, gid_t group)
> struct iattr newattrs;
> kuid_t uid;
> kgid_t gid;
> + kprojid_t projid;
>
> uid = make_kuid(current_user_ns(), user);
> gid = make_kgid(current_user_ns(), group);
> + projid = make_kprojid(current_user_ns(), (projid_t)group);
This doesn't look right. project IDs are not to be mapped to the
current_user_ns - they should only be visible to the init_user_ns.
> retry_deleg:
> newattrs.ia_valid = ATTR_CTIME;
> @@ -620,13 +623,22 @@ static int chown_common(const struct path *path, uid_t user, gid_t group)
> if (group != (gid_t) -1) {
> if (!gid_valid(gid))
> return -EINVAL;
> - newattrs.ia_valid |= ATTR_GID;
> - newattrs.ia_gid = gid;
> + if (!set_project) {
> + newattrs.ia_valid |= ATTR_GID;
> + newattrs.ia_gid = gid;
> + } else {
> + newattrs.ia_valid |= ATTR_PROJID;
> + newattrs.ia_projid = projid;
> + }
> + } else if (set_project) {
> + return -EINVAL;
> }
> if (!S_ISDIR(inode->i_mode))
> newattrs.ia_valid |=
> ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV;
> inode_lock(inode);
> + if (set_project)
> + gid = make_kgid(current_user_ns(), (gid_t) -1);
This is cumbersome because you didn't pass the project ID as it's
own type. it also removes the gid_valid() check. Leave the group
code alone, then add:
if (projid != (projid_t)-1) {
if (current_user_ns() != &init_user_ns)
return -EPERM;
newattrs.ia_projid = make_kprojid(&init_user_ns, projid);
if (!projid_valid(newattrs.ia_projid))
return -EINVAL;
newattrs.ia_valid |= ATTR_PROJID;
}
This way callers of chown_common() can set all three types in one
call if need be.
> error = security_path_chown(path, uid, gid);
> if (!error)
> error = notify_change(path->dentry, &newattrs, &delegated_inode);
> @@ -645,10 +657,15 @@ int do_fchownat(int dfd, const char __user *filename, uid_t user, gid_t group,
> struct path path;
> int error = -EINVAL;
> int lookup_flags;
> + bool set_project = false;
>
> - if ((flag & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> + if ((flag & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH |
> + AT_FCHOWN_PROJID)) != 0)
> goto out;
>
> + if (flag & AT_FCHOWN_PROJID)
> + set_project = true;
> +
> lookup_flags = (flag & AT_SYMLINK_NOFOLLOW) ? 0 : LOOKUP_FOLLOW;
> if (flag & AT_EMPTY_PATH)
> lookup_flags |= LOOKUP_EMPTY;
> @@ -659,7 +676,7 @@ int do_fchownat(int dfd, const char __user *filename, uid_t user, gid_t group,
> error = mnt_want_write(path.mnt);
> if (error)
> goto out_release;
> - error = chown_common(&path, user, group);
> + error = chown_common(&path, user, group, set_project);
> mnt_drop_write(path.mnt);
/*
* If the project ID flag is set, the group field contains the
* Project ID, not a Group ID.
*/
if (flag & AT_FCHOWN_PROJID)
error = chown_common(&path, user, -1, group);
else
error = chown_common(&path, user, group, -1);
> out_release:
> path_put(&path);
> @@ -700,7 +717,7 @@ int ksys_fchown(unsigned int fd, uid_t user, gid_t group)
> if (error)
> goto out_fput;
> audit_file(f.file);
> - error = chown_common(&f.file->f_path, user, group);
> + error = chown_common(&f.file->f_path, user, group, false);
error = chown_common(&f.file->f_path, user, group, -1);
> mnt_drop_write_file(f.file);
> out_fput:
> fdput(f);
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index fc20e06c56ba..46f39ee87312 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -2095,6 +2095,29 @@ int dquot_transfer(struct inode *inode, struct iattr *iattr)
> }
> transfer_to[GRPQUOTA] = dquot;
> }
> +
> + if (iattr->ia_valid & ATTR_PROJID) {
> + kprojid_t projid;
> +
> + if (!inode->i_sb->dq_op->get_projid)
> + return -ENOTSUPP;
> +
> + ret = inode->i_sb->dq_op->get_projid(inode, &projid);
> + if (ret)
> + return ret;
> + if (!projid_eq(iattr->ia_projid, projid)) {
> + dquot = dqget(sb, make_kqid_projid(iattr->ia_projid));
> + if (IS_ERR(dquot)) {
> + if (PTR_ERR(dquot) != -ESRCH) {
> + ret = PTR_ERR(dquot);
> + goto out_put;
> + }
> + dquot = NULL;
> + }
> + transfer_to[PRJQUOTA] = dquot;
> + }
> + }
> +
> ret = __dquot_transfer(inode, transfer_to);
OK, no I see why this is such a mess. There's no project ID field
in the struct inode, which would get rid of the need to call
get_projid() to extract it from the inode quota interface.
Ok, I think this patch needs to be split up into system call
functionality and quota infrastructure, rather than dumping them
into the same patch. That way we can discuss them separately, and
have the conversion of whether we shoul dbemaking the project ID a
member of struct inode or not to simplify this code.
> diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
> index dc905a4ff8d7..84d3aeb43e2c 100644
> --- a/include/linux/quotaops.h
> +++ b/include/linux/quotaops.h
> @@ -22,6 +22,15 @@ static inline struct quota_info *sb_dqopt(struct super_block *sb)
> /* i_mutex must being held */
> static inline bool is_quota_modification(struct inode *inode, struct iattr *ia)
> {
> + if (ia->ia_valid & ATTR_PROJID && inode->i_sb->dq_op->get_projid) {
> + kprojid_t projid;
> + int rc;
> +
> + rc = inode->i_sb->dq_op->get_projid(inode, &projid);
> + if (!rc && !projid_eq(projid, ia->ia_projid))
> + return true;
> + }
> +
> return (ia->ia_valid & ATTR_SIZE && ia->ia_size != inode->i_size) ||
> (ia->ia_valid & ATTR_UID && !uid_eq(ia->ia_uid, inode->i_uid)) ||
> (ia->ia_valid & ATTR_GID && !gid_eq(ia->ia_gid, inode->i_gid));
Because the same issues keep coming up....
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index 6448cdd9a350..712c60d7f727 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -90,5 +90,6 @@
> #define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */
> #define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */
>
> +#define AT_FCHOWN_PROJID 0x40000000 /* Change project ID instead of group id */
>
> #endif /* _UAPI_LINUX_FCNTL_H */
> diff --git a/tools/include/uapi/linux/fcntl.h b/tools/include/uapi/linux/fcntl.h
> index 6448cdd9a350..712c60d7f727 100644
> --- a/tools/include/uapi/linux/fcntl.h
> +++ b/tools/include/uapi/linux/fcntl.h
> @@ -90,5 +90,6 @@
> #define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */
> #define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */
>
> +#define AT_FCHOWN_PROJID 0x40000000 /* Change project ID instead of group id */
What is the significance of this number? Why not just the next
highest flag bit in the sequence (i.e. 0x8000)?
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 17+ messages in thread* 答复: [PATCH 1/8] fs: add support to change project ID
2019-03-03 21:53 ` Dave Chinner
@ 2019-03-04 23:36 ` Wang Shilong
0 siblings, 0 replies; 17+ messages in thread
From: Wang Shilong @ 2019-03-04 23:36 UTC (permalink / raw)
To: Dave Chinner, Wang Shilong
Cc: linux-fsdevel, linux-ext4, linux-xfs, linux-f2fs-devel, Li Xi, adilger
Hi Dave,
Thanks very much for detailed review and good suggestions, will
refresh and send a V2 soon!
Thanks,
Shilong
________________________________________
发件人: Dave Chinner <david@fromorbit.com>
发送时间: 2019年3月4日 5:53
收件人: Wang Shilong
抄送: linux-fsdevel@vger.kernel.org; linux-ext4@vger.kernel.org; linux-xfs@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net; Li Xi; adilger@dilger.ca; Wang Shilong
主题: Re: [PATCH 1/8] fs: add support to change project ID
On Fri, Mar 01, 2019 at 11:05:34PM +0900, Wang Shilong wrote:
> From: Wang Shilong <wshilong@ddn.com>
>
> From: Wang Shilong <wshilong@ddn.com>
>
> Currently, Filesystem use FS_IOC_FS_SETXATTR ioctl
> to change project ID of file. However we don't
> support ioctl on symlink files, and it is desirable
> to change symlink files' project ID just like uid/gid.
>
> This patch try to reuse existed interface fchownat(),
> use group id to set project ID if flag AT_FCHOWN_PROJID
> passed in.
>
> Signed-off-by: Wang Shilong <wshilong@ddn.com>
> ---
> fs/attr.c | 26 ++++++++++++++++++++++++--
> fs/open.c | 29 +++++++++++++++++++++++------
> fs/quota/dquot.c | 23 +++++++++++++++++++++++
> include/linux/fs.h | 3 +++
> include/linux/quotaops.h | 9 +++++++++
> include/uapi/linux/fcntl.h | 1 +
> tools/include/uapi/linux/fcntl.h | 1 +
> 7 files changed, 84 insertions(+), 8 deletions(-)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index d22e8187477f..c6b1c1132c8f 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -85,6 +85,28 @@ int setattr_prepare(struct dentry *dentry, struct iattr *attr)
> if ((ia_valid & ATTR_GID) && !chgrp_ok(inode, attr->ia_gid))
> return -EPERM;
>
> + /*
> + * Project Quota ID state is only allowed to change from within the init
> + * namespace. Enforce that restriction only if we are trying to change
> + * the quota ID state. Everything else is allowed in user namespaces.
> + */
> + if ((ia_valid & ATTR_PROJID) && current_user_ns() != &init_user_ns) {
> + kprojid_t projid;
> + int rc;
> +
> + /*
> + * Filesystem like xfs does't have ->get_projid hook
> + * should check permission by themselves.
> + */
> + if (inode->i_sb->dq_op->get_projid) {
> + rc = inode->i_sb->dq_op->get_projid(inode, &projid);
> + if (rc)
> + return rc;
> + if (!projid_eq(projid, attr->ia_projid))
> + return -EPERM;
> + }
> + }
That's a nasty landmine, and we shouldn't be making exceptions like
this in generic code. And, really, it makes no sense to me to be
checking if the projid is changing, either. If ATTR_PROJID is set,
and we aren't in the init_user_ns then reject it.
i.e. Callers should not set ATTR_PROJID if they aren't changing it,
not expect the infrastructure to silently ignore attempts to change
attributes they do not have permission to change when no change will
eventually occur.
> /* Make sure a caller can chmod. */
> if (ia_valid & ATTR_MODE) {
> if (!inode_owner_or_capable(inode))
> @@ -232,8 +254,8 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
> unsigned int ia_valid = attr->ia_valid;
>
> WARN_ON_ONCE(!inode_is_locked(inode));
> -
> - if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID | ATTR_TIMES_SET)) {
> + if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID | ATTR_PROJID |
> + ATTR_TIMES_SET)) {
> if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
> return -EPERM;
> }
> diff --git a/fs/open.c b/fs/open.c
> index 0285ce7dbd51..4e58c6ee23b3 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -597,7 +597,8 @@ SYSCALL_DEFINE2(chmod, const char __user *, filename, umode_t, mode)
> return do_fchmodat(AT_FDCWD, filename, mode);
> }
>
> -static int chown_common(const struct path *path, uid_t user, gid_t group)
> +static int chown_common(const struct path *path, uid_t user, gid_t group,
> + bool set_project)
Why not just pass the projid? This bleeds API definition into the
implementation. chown_common() shoul dbe able toset all three IDs in
one call as it is not restricted by the chownat() userspace API.
i.e.:
static int chown_common(const struct path *path, uid_t user, gid_t group, projid_t project)
and the IDs that aren't getting set should be passed with the value
of -1.
> {
> struct inode *inode = path->dentry->d_inode;
> struct inode *delegated_inode = NULL;
> @@ -605,9 +606,11 @@ static int chown_common(const struct path *path, uid_t user, gid_t group)
> struct iattr newattrs;
> kuid_t uid;
> kgid_t gid;
> + kprojid_t projid;
>
> uid = make_kuid(current_user_ns(), user);
> gid = make_kgid(current_user_ns(), group);
> + projid = make_kprojid(current_user_ns(), (projid_t)group);
This doesn't look right. project IDs are not to be mapped to the
current_user_ns - they should only be visible to the init_user_ns.
> retry_deleg:
> newattrs.ia_valid = ATTR_CTIME;
> @@ -620,13 +623,22 @@ static int chown_common(const struct path *path, uid_t user, gid_t group)
> if (group != (gid_t) -1) {
> if (!gid_valid(gid))
> return -EINVAL;
> - newattrs.ia_valid |= ATTR_GID;
> - newattrs.ia_gid = gid;
> + if (!set_project) {
> + newattrs.ia_valid |= ATTR_GID;
> + newattrs.ia_gid = gid;
> + } else {
> + newattrs.ia_valid |= ATTR_PROJID;
> + newattrs.ia_projid = projid;
> + }
> + } else if (set_project) {
> + return -EINVAL;
> }
> if (!S_ISDIR(inode->i_mode))
> newattrs.ia_valid |=
> ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV;
> inode_lock(inode);
> + if (set_project)
> + gid = make_kgid(current_user_ns(), (gid_t) -1);
This is cumbersome because you didn't pass the project ID as it's
own type. it also removes the gid_valid() check. Leave the group
code alone, then add:
if (projid != (projid_t)-1) {
if (current_user_ns() != &init_user_ns)
return -EPERM;
newattrs.ia_projid = make_kprojid(&init_user_ns, projid);
if (!projid_valid(newattrs.ia_projid))
return -EINVAL;
newattrs.ia_valid |= ATTR_PROJID;
}
This way callers of chown_common() can set all three types in one
call if need be.
> error = security_path_chown(path, uid, gid);
> if (!error)
> error = notify_change(path->dentry, &newattrs, &delegated_inode);
> @@ -645,10 +657,15 @@ int do_fchownat(int dfd, const char __user *filename, uid_t user, gid_t group,
> struct path path;
> int error = -EINVAL;
> int lookup_flags;
> + bool set_project = false;
>
> - if ((flag & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> + if ((flag & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH |
> + AT_FCHOWN_PROJID)) != 0)
> goto out;
>
> + if (flag & AT_FCHOWN_PROJID)
> + set_project = true;
> +
> lookup_flags = (flag & AT_SYMLINK_NOFOLLOW) ? 0 : LOOKUP_FOLLOW;
> if (flag & AT_EMPTY_PATH)
> lookup_flags |= LOOKUP_EMPTY;
> @@ -659,7 +676,7 @@ int do_fchownat(int dfd, const char __user *filename, uid_t user, gid_t group,
> error = mnt_want_write(path.mnt);
> if (error)
> goto out_release;
> - error = chown_common(&path, user, group);
> + error = chown_common(&path, user, group, set_project);
> mnt_drop_write(path.mnt);
/*
* If the project ID flag is set, the group field contains the
* Project ID, not a Group ID.
*/
if (flag & AT_FCHOWN_PROJID)
error = chown_common(&path, user, -1, group);
else
error = chown_common(&path, user, group, -1);
> out_release:
> path_put(&path);
> @@ -700,7 +717,7 @@ int ksys_fchown(unsigned int fd, uid_t user, gid_t group)
> if (error)
> goto out_fput;
> audit_file(f.file);
> - error = chown_common(&f.file->f_path, user, group);
> + error = chown_common(&f.file->f_path, user, group, false);
error = chown_common(&f.file->f_path, user, group, -1);
> mnt_drop_write_file(f.file);
> out_fput:
> fdput(f);
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index fc20e06c56ba..46f39ee87312 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -2095,6 +2095,29 @@ int dquot_transfer(struct inode *inode, struct iattr *iattr)
> }
> transfer_to[GRPQUOTA] = dquot;
> }
> +
> + if (iattr->ia_valid & ATTR_PROJID) {
> + kprojid_t projid;
> +
> + if (!inode->i_sb->dq_op->get_projid)
> + return -ENOTSUPP;
> +
> + ret = inode->i_sb->dq_op->get_projid(inode, &projid);
> + if (ret)
> + return ret;
> + if (!projid_eq(iattr->ia_projid, projid)) {
> + dquot = dqget(sb, make_kqid_projid(iattr->ia_projid));
> + if (IS_ERR(dquot)) {
> + if (PTR_ERR(dquot) != -ESRCH) {
> + ret = PTR_ERR(dquot);
> + goto out_put;
> + }
> + dquot = NULL;
> + }
> + transfer_to[PRJQUOTA] = dquot;
> + }
> + }
> +
> ret = __dquot_transfer(inode, transfer_to);
OK, no I see why this is such a mess. There's no project ID field
in the struct inode, which would get rid of the need to call
get_projid() to extract it from the inode quota interface.
Ok, I think this patch needs to be split up into system call
functionality and quota infrastructure, rather than dumping them
into the same patch. That way we can discuss them separately, and
have the conversion of whether we shoul dbemaking the project ID a
member of struct inode or not to simplify this code.
> diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
> index dc905a4ff8d7..84d3aeb43e2c 100644
> --- a/include/linux/quotaops.h
> +++ b/include/linux/quotaops.h
> @@ -22,6 +22,15 @@ static inline struct quota_info *sb_dqopt(struct super_block *sb)
> /* i_mutex must being held */
> static inline bool is_quota_modification(struct inode *inode, struct iattr *ia)
> {
> + if (ia->ia_valid & ATTR_PROJID && inode->i_sb->dq_op->get_projid) {
> + kprojid_t projid;
> + int rc;
> +
> + rc = inode->i_sb->dq_op->get_projid(inode, &projid);
> + if (!rc && !projid_eq(projid, ia->ia_projid))
> + return true;
> + }
> +
> return (ia->ia_valid & ATTR_SIZE && ia->ia_size != inode->i_size) ||
> (ia->ia_valid & ATTR_UID && !uid_eq(ia->ia_uid, inode->i_uid)) ||
> (ia->ia_valid & ATTR_GID && !gid_eq(ia->ia_gid, inode->i_gid));
Because the same issues keep coming up....
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index 6448cdd9a350..712c60d7f727 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -90,5 +90,6 @@
> #define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */
> #define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */
>
> +#define AT_FCHOWN_PROJID 0x40000000 /* Change project ID instead of group id */
>
> #endif /* _UAPI_LINUX_FCNTL_H */
> diff --git a/tools/include/uapi/linux/fcntl.h b/tools/include/uapi/linux/fcntl.h
> index 6448cdd9a350..712c60d7f727 100644
> --- a/tools/include/uapi/linux/fcntl.h
> +++ b/tools/include/uapi/linux/fcntl.h
> @@ -90,5 +90,6 @@
> #define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */
> #define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */
>
> +#define AT_FCHOWN_PROJID 0x40000000 /* Change project ID instead of group id */
What is the significance of this number? Why not just the next
highest flag bit in the sequence (i.e. 0x8000)?
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/8] ext4: support project ID in ext4_setattr()
2019-03-01 14:05 [PATCH 0/8] add generic interface to set/get project Wang Shilong
2019-03-01 14:05 ` [PATCH 1/8] fs: add support to change project ID Wang Shilong
@ 2019-03-01 14:05 ` Wang Shilong
2019-03-01 14:05 ` [PATCH 3/8] f2fs: support project ID in f2fs_setattr() Wang Shilong
` (6 subsequent siblings)
8 siblings, 0 replies; 17+ messages in thread
From: Wang Shilong @ 2019-03-01 14:05 UTC (permalink / raw)
To: linux-fsdevel, linux-ext4, linux-xfs, linux-f2fs-devel
Cc: lixi, adilger, Wang Shilong
From: Wang Shilong <wshilong@ddn.com>
From: Wang Shilong <wshilong@ddn.com>
Signed-off-by: Wang Shilong <wshilong@ddn.com>
---
fs/ext4/inode.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 34d7e0703cc6..b6c451407dcd 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5537,10 +5537,12 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
return error;
}
if ((ia_valid & ATTR_UID && !uid_eq(attr->ia_uid, inode->i_uid)) ||
- (ia_valid & ATTR_GID && !gid_eq(attr->ia_gid, inode->i_gid))) {
+ (ia_valid & ATTR_GID && !gid_eq(attr->ia_gid, inode->i_gid)) ||
+ (ia_valid & ATTR_PROJID && !projid_eq(attr->ia_projid,
+ EXT4_I(inode)->i_projid))) {
handle_t *handle;
- /* (user+group)*(old+new) structure, inode write (sb,
+ /* (user+group+project)*(old+new) structure, inode write (sb,
* inode block, ? - but truncate inode update has it) */
handle = ext4_journal_start(inode, EXT4_HT_QUOTA,
(EXT4_MAXQUOTAS_INIT_BLOCKS(inode->i_sb) +
@@ -5567,6 +5569,8 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
inode->i_uid = attr->ia_uid;
if (attr->ia_valid & ATTR_GID)
inode->i_gid = attr->ia_gid;
+ if (attr->ia_valid & ATTR_PROJID)
+ EXT4_I(inode)->i_projid = attr->ia_projid;
error = ext4_mark_inode_dirty(handle, inode);
ext4_journal_stop(handle);
}
--
2.19.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 3/8] f2fs: support project ID in f2fs_setattr()
2019-03-01 14:05 [PATCH 0/8] add generic interface to set/get project Wang Shilong
2019-03-01 14:05 ` [PATCH 1/8] fs: add support to change project ID Wang Shilong
2019-03-01 14:05 ` [PATCH 2/8] ext4: support project ID in ext4_setattr() Wang Shilong
@ 2019-03-01 14:05 ` Wang Shilong
2019-03-01 14:05 ` [PATCH 4/8] xfs: support project ID in xfs_setattr() Wang Shilong
` (5 subsequent siblings)
8 siblings, 0 replies; 17+ messages in thread
From: Wang Shilong @ 2019-03-01 14:05 UTC (permalink / raw)
To: linux-fsdevel, linux-ext4, linux-xfs, linux-f2fs-devel
Cc: lixi, adilger, Wang Shilong
From: Wang Shilong <wshilong@ddn.com>
From: Wang Shilong <wshilong@ddn.com>
Signed-off-by: Wang Shilong <wshilong@ddn.com>
---
fs/f2fs/file.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index bba56b39dcc5..8eaca056e857 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -789,7 +789,9 @@ int f2fs_setattr(struct dentry *dentry, struct iattr *attr)
if ((attr->ia_valid & ATTR_UID &&
!uid_eq(attr->ia_uid, inode->i_uid)) ||
(attr->ia_valid & ATTR_GID &&
- !gid_eq(attr->ia_gid, inode->i_gid))) {
+ !gid_eq(attr->ia_gid, inode->i_gid)) ||
+ (attr->ia_valid & ATTR_PROJID &&
+ !projid_eq(attr->ia_projid, F2FS_I(inode)->i_projid))) {
f2fs_lock_op(F2FS_I_SB(inode));
err = dquot_transfer(inode, attr);
if (err) {
@@ -806,6 +808,8 @@ int f2fs_setattr(struct dentry *dentry, struct iattr *attr)
inode->i_uid = attr->ia_uid;
if (attr->ia_valid & ATTR_GID)
inode->i_gid = attr->ia_gid;
+ if (attr->ia_valid & ATTR_PROJID)
+ F2FS_I(inode)->i_projid = attr->ia_projid;
f2fs_mark_inode_dirty_sync(inode, true);
f2fs_unlock_op(F2FS_I_SB(inode));
}
--
2.19.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 4/8] xfs: support project ID in xfs_setattr()
2019-03-01 14:05 [PATCH 0/8] add generic interface to set/get project Wang Shilong
` (2 preceding siblings ...)
2019-03-01 14:05 ` [PATCH 3/8] f2fs: support project ID in f2fs_setattr() Wang Shilong
@ 2019-03-01 14:05 ` Wang Shilong
2019-03-01 15:49 ` Darrick J. Wong
2019-03-03 22:18 ` Dave Chinner
2019-03-01 14:05 ` [PATCH 5/8] fs: add project support to statx Wang Shilong
` (4 subsequent siblings)
8 siblings, 2 replies; 17+ messages in thread
From: Wang Shilong @ 2019-03-01 14:05 UTC (permalink / raw)
To: linux-fsdevel, linux-ext4, linux-xfs, linux-f2fs-devel
Cc: lixi, adilger, Wang Shilong
From: Wang Shilong <wshilong@ddn.com>
From: Wang Shilong <wshilong@ddn.com>
Signed-off-by: Wang Shilong <wshilong@ddn.com>
---
fs/xfs/xfs_iops.c | 51 +++++++++++++++++++++++++++++++++++++---------
fs/xfs/xfs_linux.h | 10 +++++++++
2 files changed, 51 insertions(+), 10 deletions(-)
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index f48ffd7a8d3e..c10466fe6ed4 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -589,7 +589,8 @@ xfs_vn_change_ok(
struct dentry *dentry,
struct iattr *iattr)
{
- struct xfs_mount *mp = XFS_I(d_inode(dentry))->i_mount;
+ struct xfs_inode *ip = XFS_I(d_inode(dentry));
+ struct xfs_mount *mp = ip->i_mount;
if (mp->m_flags & XFS_MOUNT_RDONLY)
return -EROFS;
@@ -597,6 +598,13 @@ xfs_vn_change_ok(
if (XFS_FORCED_SHUTDOWN(mp))
return -EIO;
+ if ((iattr->ia_valid & ATTR_PROJID) &&
+ current_user_ns() != &init_user_ns) {
+ if (!projid_eq(xfs_projid_to_kprojid(xfs_get_projid(ip)),
+ iattr->ia_projid))
+ return -EPERM;
+ }
+
return setattr_prepare(dentry, iattr);
}
@@ -619,8 +627,10 @@ xfs_setattr_nonsize(
int error;
kuid_t uid = GLOBAL_ROOT_UID, iuid = GLOBAL_ROOT_UID;
kgid_t gid = GLOBAL_ROOT_GID, igid = GLOBAL_ROOT_GID;
- struct xfs_dquot *udqp = NULL, *gdqp = NULL;
+ kprojid_t projid, iprojid;
+ struct xfs_dquot *udqp = NULL, *gdqp = NULL, *pdqp = NULL;
struct xfs_dquot *olddquot1 = NULL, *olddquot2 = NULL;
+ struct xfs_dquot *olddquot3 = NULL;
ASSERT((mask & ATTR_SIZE) == 0);
@@ -632,7 +642,7 @@ xfs_setattr_nonsize(
* If the IDs do change before we take the ilock, we're covered
* because the i_*dquot fields will get updated anyway.
*/
- if (XFS_IS_QUOTA_ON(mp) && (mask & (ATTR_UID|ATTR_GID))) {
+ if (XFS_IS_QUOTA_ON(mp) && (mask & (ATTR_UID|ATTR_GID|ATTR_PROJID))) {
uint qflags = 0;
if ((mask & ATTR_UID) && XFS_IS_UQUOTA_ON(mp)) {
@@ -647,18 +657,25 @@ xfs_setattr_nonsize(
} else {
gid = inode->i_gid;
}
+ if ((mask & ATTR_PROJID) && XFS_IS_PQUOTA_ON(mp)) {
+ projid = iattr->ia_projid;
+ qflags |= XFS_QMOPT_PQUOTA;
+ } else {
+ projid = xfs_projid_to_kprojid(xfs_get_projid(ip));
+ }
/*
- * We take a reference when we initialize udqp and gdqp,
+ * We take a reference when we initialize udqp,gdqp and pdqp,
* so it is important that we never blindly double trip on
* the same variable. See xfs_create() for an example.
*/
ASSERT(udqp == NULL);
ASSERT(gdqp == NULL);
+ ASSERT(pdqp == NULL);
error = xfs_qm_vop_dqalloc(ip, xfs_kuid_to_uid(uid),
xfs_kgid_to_gid(gid),
- xfs_get_projid(ip),
- qflags, &udqp, &gdqp, NULL);
+ xfs_kprojid_to_projid(projid),
+ qflags, &udqp, &gdqp, &pdqp);
if (error)
return error;
}
@@ -673,7 +690,7 @@ xfs_setattr_nonsize(
/*
* Change file ownership. Must be the owner or privileged.
*/
- if (mask & (ATTR_UID|ATTR_GID)) {
+ if (mask & (ATTR_UID|ATTR_GID|ATTR_PROJID)) {
/*
* These IDs could have changed since we last looked at them.
* But, we're assured that if the ownership did change
@@ -682,8 +699,10 @@ xfs_setattr_nonsize(
*/
iuid = inode->i_uid;
igid = inode->i_gid;
+ iprojid = xfs_projid_to_kprojid(xfs_get_projid(ip));
gid = (mask & ATTR_GID) ? iattr->ia_gid : igid;
uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;
+ projid = (mask & ATTR_PROJID) ? iattr->ia_projid : iprojid;
/*
* Do a quota reservation only if uid/gid is actually
@@ -691,10 +710,11 @@ xfs_setattr_nonsize(
*/
if (XFS_IS_QUOTA_RUNNING(mp) &&
((XFS_IS_UQUOTA_ON(mp) && !uid_eq(iuid, uid)) ||
- (XFS_IS_GQUOTA_ON(mp) && !gid_eq(igid, gid)))) {
+ (XFS_IS_GQUOTA_ON(mp) && !gid_eq(igid, gid)) ||
+ (XFS_IS_PQUOTA_ON(mp) && !projid_eq(iprojid, projid)))) {
ASSERT(tp);
error = xfs_qm_vop_chown_reserve(tp, ip, udqp, gdqp,
- NULL, capable(CAP_FOWNER) ?
+ pdqp, capable(CAP_FOWNER) ?
XFS_QMOPT_FORCE_RES : 0);
if (error) /* out of quota */
goto out_cancel;
@@ -704,7 +724,7 @@ xfs_setattr_nonsize(
/*
* Change file ownership. Must be the owner or privileged.
*/
- if (mask & (ATTR_UID|ATTR_GID)) {
+ if (mask & (ATTR_UID|ATTR_GID|ATTR_PROJID)) {
/*
* CAP_FSETID overrides the following restrictions:
*
@@ -741,6 +761,15 @@ xfs_setattr_nonsize(
ip->i_d.di_gid = xfs_kgid_to_gid(gid);
inode->i_gid = gid;
}
+ if (!projid_eq(iprojid, projid)) {
+ if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp)) {
+ ASSERT(mask & ATTR_PROJID);
+ ASSERT(pdqp);
+ olddquot3 = xfs_qm_vop_chown(tp, ip,
+ &ip->i_pdquot, pdqp);
+ }
+ xfs_set_projid(ip, xfs_kprojid_to_projid(projid));
+ }
}
if (mask & ATTR_MODE)
@@ -763,8 +792,10 @@ xfs_setattr_nonsize(
*/
xfs_qm_dqrele(olddquot1);
xfs_qm_dqrele(olddquot2);
+ xfs_qm_dqrele(olddquot3);
xfs_qm_dqrele(udqp);
xfs_qm_dqrele(gdqp);
+ xfs_qm_dqrele(pdqp);
if (error)
return error;
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index edbd5a210df2..80f5ea32823d 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -191,6 +191,16 @@ static inline kgid_t xfs_gid_to_kgid(uint32_t gid)
return make_kgid(&init_user_ns, gid);
}
+static inline uint32_t xfs_kprojid_to_projid(kprojid_t projid)
+{
+ return from_kprojid(&init_user_ns, projid);
+}
+
+static inline kprojid_t xfs_projid_to_kprojid(uint32_t projid)
+{
+ return make_kprojid(&init_user_ns, projid);
+}
+
static inline dev_t xfs_to_linux_dev_t(xfs_dev_t dev)
{
return MKDEV(sysv_major(dev) & 0x1ff, sysv_minor(dev));
--
2.19.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 4/8] xfs: support project ID in xfs_setattr()
2019-03-01 14:05 ` [PATCH 4/8] xfs: support project ID in xfs_setattr() Wang Shilong
@ 2019-03-01 15:49 ` Darrick J. Wong
2019-03-03 22:18 ` Dave Chinner
1 sibling, 0 replies; 17+ messages in thread
From: Darrick J. Wong @ 2019-03-01 15:49 UTC (permalink / raw)
To: Wang Shilong
Cc: linux-fsdevel, linux-ext4, linux-xfs, linux-f2fs-devel, lixi,
adilger, Wang Shilong
On Fri, Mar 01, 2019 at 11:05:37PM +0900, Wang Shilong wrote:
> From: Wang Shilong <wshilong@ddn.com>
>
> From: Wang Shilong <wshilong@ddn.com>
Needs a commit message here ^^^^
e.g. "Wire up XFS to the new ATTR_PROJID setattr functionality"
> Signed-off-by: Wang Shilong <wshilong@ddn.com>
> ---
> fs/xfs/xfs_iops.c | 51 +++++++++++++++++++++++++++++++++++++---------
> fs/xfs/xfs_linux.h | 10 +++++++++
> 2 files changed, 51 insertions(+), 10 deletions(-)
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index f48ffd7a8d3e..c10466fe6ed4 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -589,7 +589,8 @@ xfs_vn_change_ok(
> struct dentry *dentry,
> struct iattr *iattr)
> {
> - struct xfs_mount *mp = XFS_I(d_inode(dentry))->i_mount;
> + struct xfs_inode *ip = XFS_I(d_inode(dentry));
> + struct xfs_mount *mp = ip->i_mount;
>
> if (mp->m_flags & XFS_MOUNT_RDONLY)
> return -EROFS;
> @@ -597,6 +598,13 @@ xfs_vn_change_ok(
> if (XFS_FORCED_SHUTDOWN(mp))
> return -EIO;
>
> + if ((iattr->ia_valid & ATTR_PROJID) &&
> + current_user_ns() != &init_user_ns) {
> + if (!projid_eq(xfs_projid_to_kprojid(xfs_get_projid(ip)),
> + iattr->ia_projid))
> + return -EPERM;
> + }
> +
> return setattr_prepare(dentry, iattr);
> }
>
> @@ -619,8 +627,10 @@ xfs_setattr_nonsize(
> int error;
> kuid_t uid = GLOBAL_ROOT_UID, iuid = GLOBAL_ROOT_UID;
> kgid_t gid = GLOBAL_ROOT_GID, igid = GLOBAL_ROOT_GID;
> - struct xfs_dquot *udqp = NULL, *gdqp = NULL;
> + kprojid_t projid, iprojid;
> + struct xfs_dquot *udqp = NULL, *gdqp = NULL, *pdqp = NULL;
> struct xfs_dquot *olddquot1 = NULL, *olddquot2 = NULL;
> + struct xfs_dquot *olddquot3 = NULL;
>
> ASSERT((mask & ATTR_SIZE) == 0);
>
> @@ -632,7 +642,7 @@ xfs_setattr_nonsize(
> * If the IDs do change before we take the ilock, we're covered
> * because the i_*dquot fields will get updated anyway.
> */
> - if (XFS_IS_QUOTA_ON(mp) && (mask & (ATTR_UID|ATTR_GID))) {
> + if (XFS_IS_QUOTA_ON(mp) && (mask & (ATTR_UID|ATTR_GID|ATTR_PROJID))) {
> uint qflags = 0;
>
> if ((mask & ATTR_UID) && XFS_IS_UQUOTA_ON(mp)) {
> @@ -647,18 +657,25 @@ xfs_setattr_nonsize(
> } else {
> gid = inode->i_gid;
> }
> + if ((mask & ATTR_PROJID) && XFS_IS_PQUOTA_ON(mp)) {
> + projid = iattr->ia_projid;
> + qflags |= XFS_QMOPT_PQUOTA;
> + } else {
> + projid = xfs_projid_to_kprojid(xfs_get_projid(ip));
Hmm. Prior to this change, xfs never actually touched the kernel's kprojid
infrastructure, which is to say that I don't think we actually map the
xfs project ids into a "kprojid user-namespace pair". Does that cause a
user-visible change in how project ids work? Don't we need to change
getxattr and setxattr to perform the translation too? Or is everything
just fine the way it is in xfs without a new layer of mapping?
And if we /are/ deciding to wrap xfs project ids in kprojid now, I think
that ought to be a separate patch.
> + }
>
> /*
> - * We take a reference when we initialize udqp and gdqp,
> + * We take a reference when we initialize udqp,gdqp and pdqp,
space after the comma, please ^^^
--D
> * so it is important that we never blindly double trip on
> * the same variable. See xfs_create() for an example.
> */
> ASSERT(udqp == NULL);
> ASSERT(gdqp == NULL);
> + ASSERT(pdqp == NULL);
> error = xfs_qm_vop_dqalloc(ip, xfs_kuid_to_uid(uid),
> xfs_kgid_to_gid(gid),
> - xfs_get_projid(ip),
> - qflags, &udqp, &gdqp, NULL);
> + xfs_kprojid_to_projid(projid),
> + qflags, &udqp, &gdqp, &pdqp);
> if (error)
> return error;
> }
> @@ -673,7 +690,7 @@ xfs_setattr_nonsize(
> /*
> * Change file ownership. Must be the owner or privileged.
> */
> - if (mask & (ATTR_UID|ATTR_GID)) {
> + if (mask & (ATTR_UID|ATTR_GID|ATTR_PROJID)) {
> /*
> * These IDs could have changed since we last looked at them.
> * But, we're assured that if the ownership did change
> @@ -682,8 +699,10 @@ xfs_setattr_nonsize(
> */
> iuid = inode->i_uid;
> igid = inode->i_gid;
> + iprojid = xfs_projid_to_kprojid(xfs_get_projid(ip));
> gid = (mask & ATTR_GID) ? iattr->ia_gid : igid;
> uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;
> + projid = (mask & ATTR_PROJID) ? iattr->ia_projid : iprojid;
>
> /*
> * Do a quota reservation only if uid/gid is actually
> @@ -691,10 +710,11 @@ xfs_setattr_nonsize(
> */
> if (XFS_IS_QUOTA_RUNNING(mp) &&
> ((XFS_IS_UQUOTA_ON(mp) && !uid_eq(iuid, uid)) ||
> - (XFS_IS_GQUOTA_ON(mp) && !gid_eq(igid, gid)))) {
> + (XFS_IS_GQUOTA_ON(mp) && !gid_eq(igid, gid)) ||
> + (XFS_IS_PQUOTA_ON(mp) && !projid_eq(iprojid, projid)))) {
> ASSERT(tp);
> error = xfs_qm_vop_chown_reserve(tp, ip, udqp, gdqp,
> - NULL, capable(CAP_FOWNER) ?
> + pdqp, capable(CAP_FOWNER) ?
> XFS_QMOPT_FORCE_RES : 0);
> if (error) /* out of quota */
> goto out_cancel;
> @@ -704,7 +724,7 @@ xfs_setattr_nonsize(
> /*
> * Change file ownership. Must be the owner or privileged.
> */
> - if (mask & (ATTR_UID|ATTR_GID)) {
> + if (mask & (ATTR_UID|ATTR_GID|ATTR_PROJID)) {
> /*
> * CAP_FSETID overrides the following restrictions:
> *
> @@ -741,6 +761,15 @@ xfs_setattr_nonsize(
> ip->i_d.di_gid = xfs_kgid_to_gid(gid);
> inode->i_gid = gid;
> }
> + if (!projid_eq(iprojid, projid)) {
> + if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp)) {
> + ASSERT(mask & ATTR_PROJID);
> + ASSERT(pdqp);
> + olddquot3 = xfs_qm_vop_chown(tp, ip,
> + &ip->i_pdquot, pdqp);
> + }
> + xfs_set_projid(ip, xfs_kprojid_to_projid(projid));
> + }
> }
>
> if (mask & ATTR_MODE)
> @@ -763,8 +792,10 @@ xfs_setattr_nonsize(
> */
> xfs_qm_dqrele(olddquot1);
> xfs_qm_dqrele(olddquot2);
> + xfs_qm_dqrele(olddquot3);
> xfs_qm_dqrele(udqp);
> xfs_qm_dqrele(gdqp);
> + xfs_qm_dqrele(pdqp);
>
> if (error)
> return error;
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index edbd5a210df2..80f5ea32823d 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -191,6 +191,16 @@ static inline kgid_t xfs_gid_to_kgid(uint32_t gid)
> return make_kgid(&init_user_ns, gid);
> }
>
> +static inline uint32_t xfs_kprojid_to_projid(kprojid_t projid)
> +{
> + return from_kprojid(&init_user_ns, projid);
> +}
> +
> +static inline kprojid_t xfs_projid_to_kprojid(uint32_t projid)
> +{
> + return make_kprojid(&init_user_ns, projid);
> +}
> +
> static inline dev_t xfs_to_linux_dev_t(xfs_dev_t dev)
> {
> return MKDEV(sysv_major(dev) & 0x1ff, sysv_minor(dev));
> --
> 2.19.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 4/8] xfs: support project ID in xfs_setattr()
2019-03-01 14:05 ` [PATCH 4/8] xfs: support project ID in xfs_setattr() Wang Shilong
2019-03-01 15:49 ` Darrick J. Wong
@ 2019-03-03 22:18 ` Dave Chinner
1 sibling, 0 replies; 17+ messages in thread
From: Dave Chinner @ 2019-03-03 22:18 UTC (permalink / raw)
To: Wang Shilong
Cc: linux-fsdevel, linux-ext4, linux-xfs, linux-f2fs-devel, lixi,
adilger, Wang Shilong
On Fri, Mar 01, 2019 at 11:05:37PM +0900, Wang Shilong wrote:
> From: Wang Shilong <wshilong@ddn.com>
>
> From: Wang Shilong <wshilong@ddn.com>
>
> Signed-off-by: Wang Shilong <wshilong@ddn.com>
> ---
> fs/xfs/xfs_iops.c | 51 +++++++++++++++++++++++++++++++++++++---------
> fs/xfs/xfs_linux.h | 10 +++++++++
> 2 files changed, 51 insertions(+), 10 deletions(-)
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index f48ffd7a8d3e..c10466fe6ed4 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -589,7 +589,8 @@ xfs_vn_change_ok(
> struct dentry *dentry,
> struct iattr *iattr)
> {
> - struct xfs_mount *mp = XFS_I(d_inode(dentry))->i_mount;
> + struct xfs_inode *ip = XFS_I(d_inode(dentry));
> + struct xfs_mount *mp = ip->i_mount;
>
> if (mp->m_flags & XFS_MOUNT_RDONLY)
> return -EROFS;
> @@ -597,6 +598,13 @@ xfs_vn_change_ok(
> if (XFS_FORCED_SHUTDOWN(mp))
> return -EIO;
>
> + if ((iattr->ia_valid & ATTR_PROJID) &&
> + current_user_ns() != &init_user_ns) {
> + if (!projid_eq(xfs_projid_to_kprojid(xfs_get_projid(ip)),
> + iattr->ia_projid))
> + return -EPERM;
> + }
See previous comments about this.
> +
> return setattr_prepare(dentry, iattr);
> }
>
> @@ -619,8 +627,10 @@ xfs_setattr_nonsize(
> int error;
> kuid_t uid = GLOBAL_ROOT_UID, iuid = GLOBAL_ROOT_UID;
> kgid_t gid = GLOBAL_ROOT_GID, igid = GLOBAL_ROOT_GID;
> - struct xfs_dquot *udqp = NULL, *gdqp = NULL;
> + kprojid_t projid, iprojid;
So, uninitialised, unlink the uid/gids.
These shoul dbe GLOBAL_ROOT_PROJID, which probably should be:
#define GLOBAL_ROOT_PROJID KPROJIDT_INIT(0)
to match these:
#define XFS_PROJID_DEFAULT 0
#define F2FS_DEF_PROJID 0 /* default project ID */
#define EXT4_DEF_PROJID 0
> + struct xfs_dquot *udqp = NULL, *gdqp = NULL, *pdqp = NULL;
> struct xfs_dquot *olddquot1 = NULL, *olddquot2 = NULL;
> + struct xfs_dquot *olddquot3 = NULL;
>
> ASSERT((mask & ATTR_SIZE) == 0);
>
> @@ -632,7 +642,7 @@ xfs_setattr_nonsize(
> * If the IDs do change before we take the ilock, we're covered
> * because the i_*dquot fields will get updated anyway.
> */
> - if (XFS_IS_QUOTA_ON(mp) && (mask & (ATTR_UID|ATTR_GID))) {
> + if (XFS_IS_QUOTA_ON(mp) && (mask & (ATTR_UID|ATTR_GID|ATTR_PROJID))) {
> uint qflags = 0;
>
> if ((mask & ATTR_UID) && XFS_IS_UQUOTA_ON(mp)) {
> @@ -647,18 +657,25 @@ xfs_setattr_nonsize(
> } else {
> gid = inode->i_gid;
> }
> + if ((mask & ATTR_PROJID) && XFS_IS_PQUOTA_ON(mp)) {
> + projid = iattr->ia_projid;
> + qflags |= XFS_QMOPT_PQUOTA;
> + } else {
> + projid = xfs_projid_to_kprojid(xfs_get_projid(ip));
> + }
Hmmm. why would we convert the XFS on-disk project ID to a kernel
representation, only to immediately:
>
> /*
> - * We take a reference when we initialize udqp and gdqp,
> + * We take a reference when we initialize udqp,gdqp and pdqp,
> * so it is important that we never blindly double trip on
> * the same variable. See xfs_create() for an example.
> */
> ASSERT(udqp == NULL);
> ASSERT(gdqp == NULL);
> + ASSERT(pdqp == NULL);
> error = xfs_qm_vop_dqalloc(ip, xfs_kuid_to_uid(uid),
> xfs_kgid_to_gid(gid),
> - xfs_get_projid(ip),
> - qflags, &udqp, &gdqp, NULL);
> + xfs_kprojid_to_projid(projid),
> + qflags, &udqp, &gdqp, &pdqp);
Convert it back to the XFS on disk representation? Perhaps:
if ((mask & ATTR_PROJID) && XFS_IS_PQUOTA_ON(mp)) {
projid = xfs_kprojid_to_projid(iattr->ia_projid);
qflags |= XFS_QMOPT_PQUOTA;
} else {
projid = xfs_get_projid(ip);
}
Unless, of course, we promote the the project ID into the struct
inode so it matches uid and gid. This code is really telling me that
we should be promoting it before we make thse changes...
> @@ -673,7 +690,7 @@ xfs_setattr_nonsize(
> /*
> * Change file ownership. Must be the owner or privileged.
> */
> - if (mask & (ATTR_UID|ATTR_GID)) {
> + if (mask & (ATTR_UID|ATTR_GID|ATTR_PROJID)) {
> /*
> * These IDs could have changed since we last looked at them.
> * But, we're assured that if the ownership did change
> @@ -682,8 +699,10 @@ xfs_setattr_nonsize(
> */
> iuid = inode->i_uid;
> igid = inode->i_gid;
> + iprojid = xfs_projid_to_kprojid(xfs_get_projid(ip));
> gid = (mask & ATTR_GID) ? iattr->ia_gid : igid;
> uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;
> + projid = (mask & ATTR_PROJID) ? iattr->ia_projid : iprojid;
>
> /*
> * Do a quota reservation only if uid/gid is actually
> @@ -691,10 +710,11 @@ xfs_setattr_nonsize(
> */
> if (XFS_IS_QUOTA_RUNNING(mp) &&
> ((XFS_IS_UQUOTA_ON(mp) && !uid_eq(iuid, uid)) ||
> - (XFS_IS_GQUOTA_ON(mp) && !gid_eq(igid, gid)))) {
> + (XFS_IS_GQUOTA_ON(mp) && !gid_eq(igid, gid)) ||
> + (XFS_IS_PQUOTA_ON(mp) && !projid_eq(iprojid, projid)))) {
> ASSERT(tp);
> error = xfs_qm_vop_chown_reserve(tp, ip, udqp, gdqp,
> - NULL, capable(CAP_FOWNER) ?
> + pdqp, capable(CAP_FOWNER) ?
> XFS_QMOPT_FORCE_RES : 0);
> if (error) /* out of quota */
> goto out_cancel;
> @@ -704,7 +724,7 @@ xfs_setattr_nonsize(
> /*
> * Change file ownership. Must be the owner or privileged.
> */
> - if (mask & (ATTR_UID|ATTR_GID)) {
> + if (mask & (ATTR_UID|ATTR_GID|ATTR_PROJID)) {
> /*
> * CAP_FSETID overrides the following restrictions:
> *
> @@ -741,6 +761,15 @@ xfs_setattr_nonsize(
> ip->i_d.di_gid = xfs_kgid_to_gid(gid);
> inode->i_gid = gid;
> }
> + if (!projid_eq(iprojid, projid)) {
> + if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp)) {
> + ASSERT(mask & ATTR_PROJID);
> + ASSERT(pdqp);
> + olddquot3 = xfs_qm_vop_chown(tp, ip,
> + &ip->i_pdquot, pdqp);
> + }
> + xfs_set_projid(ip, xfs_kprojid_to_projid(projid));
> + }
> }
Ok, this adds another set of boilerplate code here. This is starting
to need some factoring work. Not needed for this patchset, though.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 5/8] fs: add project support to statx
2019-03-01 14:05 [PATCH 0/8] add generic interface to set/get project Wang Shilong
` (3 preceding siblings ...)
2019-03-01 14:05 ` [PATCH 4/8] xfs: support project ID in xfs_setattr() Wang Shilong
@ 2019-03-01 14:05 ` Wang Shilong
2019-03-03 23:01 ` Dave Chinner
2019-03-01 14:05 ` [PATCH 6/8] ext4: support project in ext4_getattr() Wang Shilong
` (3 subsequent siblings)
8 siblings, 1 reply; 17+ messages in thread
From: Wang Shilong @ 2019-03-01 14:05 UTC (permalink / raw)
To: linux-fsdevel, linux-ext4, linux-xfs, linux-f2fs-devel
Cc: lixi, adilger, Wang Shilong
From: Wang Shilong <wshilong@ddn.com>
From: Wang Shilong <wshilong@ddn.com>
Extend statx to support project ID and inherit attribute.
Signed-off-by: Wang Shilong <wshilong@ddn.com>
---
fs/stat.c | 1 +
include/linux/stat.h | 2 ++
include/uapi/linux/stat.h | 8 ++++++--
tools/include/uapi/linux/stat.h | 8 ++++++--
4 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/fs/stat.c b/fs/stat.c
index adbfcd86c81b..82d855c4647c 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -551,6 +551,7 @@ cp_statx(const struct kstat *stat, struct statx __user *buffer)
tmp.stx_rdev_minor = MINOR(stat->rdev);
tmp.stx_dev_major = MAJOR(stat->dev);
tmp.stx_dev_minor = MINOR(stat->dev);
+ tmp.stx_projid = (u32)from_kprojid(&init_user_ns, stat->projid);
return copy_to_user(buffer, &tmp, sizeof(tmp)) ? -EFAULT : 0;
}
diff --git a/include/linux/stat.h b/include/linux/stat.h
index 765573dc17d6..72c9d2ab5343 100644
--- a/include/linux/stat.h
+++ b/include/linux/stat.h
@@ -18,6 +18,7 @@
#include <linux/types.h>
#include <linux/time.h>
#include <linux/uidgid.h>
+#include <linux/projid.h>
#define KSTAT_QUERY_FLAGS (AT_STATX_SYNC_TYPE)
@@ -40,6 +41,7 @@ struct kstat {
dev_t rdev;
kuid_t uid;
kgid_t gid;
+ kprojid_t projid;
loff_t size;
struct timespec64 atime;
struct timespec64 mtime;
diff --git a/include/uapi/linux/stat.h b/include/uapi/linux/stat.h
index 7b35e98d3c58..56d35a2cbd0c 100644
--- a/include/uapi/linux/stat.h
+++ b/include/uapi/linux/stat.h
@@ -123,7 +123,9 @@ struct statx {
__u32 stx_dev_major; /* ID of device containing file [uncond] */
__u32 stx_dev_minor;
/* 0x90 */
- __u64 __spare2[14]; /* Spare space for future expansion */
+ __u32 stx_projid; /* Project ID of file */
+ __u32 __spare1[1];
+ __u64 __spare2[13]; /* Spare space for future expansion */
/* 0x100 */
};
@@ -148,7 +150,8 @@ struct statx {
#define STATX_BLOCKS 0x00000400U /* Want/got stx_blocks */
#define STATX_BASIC_STATS 0x000007ffU /* The stuff in the normal stat struct */
#define STATX_BTIME 0x00000800U /* Want/got stx_btime */
-#define STATX_ALL 0x00000fffU /* All currently supported flags */
+#define STATX_PROJID 0x00001000U /* Want/Got stx_projid */
+#define STATX_ALL 0x00001fffU /* All currently supported flags */
#define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */
/*
@@ -170,5 +173,6 @@ struct statx {
#define STATX_ATTR_AUTOMOUNT 0x00001000 /* Dir: Automount trigger */
+#define STATX_ATTR_PROJINHERIT 0x00002000 /* [I] File project inherit is set */
#endif /* _UAPI_LINUX_STAT_H */
diff --git a/tools/include/uapi/linux/stat.h b/tools/include/uapi/linux/stat.h
index 7b35e98d3c58..21b542b3b061 100644
--- a/tools/include/uapi/linux/stat.h
+++ b/tools/include/uapi/linux/stat.h
@@ -123,7 +123,9 @@ struct statx {
__u32 stx_dev_major; /* ID of device containing file [uncond] */
__u32 stx_dev_minor;
/* 0x90 */
- __u64 __spare2[14]; /* Spare space for future expansion */
+ __u32 stx_projid; /* Project ID of file */
+ __u32 __spare1[1];
+ __u64 __spare2[13]; /* Spare space for future expansion */
/* 0x100 */
};
@@ -148,7 +150,8 @@ struct statx {
#define STATX_BLOCKS 0x00000400U /* Want/got stx_blocks */
#define STATX_BASIC_STATS 0x000007ffU /* The stuff in the normal stat struct */
#define STATX_BTIME 0x00000800U /* Want/got stx_btime */
-#define STATX_ALL 0x00000fffU /* All currently supported flags */
+#define STATX_PROJID 0x00001000U /* Want/Got stx_projid */
+#define STATX_ALL 0x00001fffU /* All currently supported flags */
#define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */
/*
@@ -170,5 +173,6 @@ struct statx {
#define STATX_ATTR_AUTOMOUNT 0x00001000 /* Dir: Automount trigger */
+#define STATX_ATTR_PROJINHERIT 0x00002000 /* [I] File project inherit is set */
#endif /* _UAPI_LINUX_STAT_H */
--
2.19.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 5/8] fs: add project support to statx
2019-03-01 14:05 ` [PATCH 5/8] fs: add project support to statx Wang Shilong
@ 2019-03-03 23:01 ` Dave Chinner
0 siblings, 0 replies; 17+ messages in thread
From: Dave Chinner @ 2019-03-03 23:01 UTC (permalink / raw)
To: Wang Shilong
Cc: linux-fsdevel, linux-ext4, linux-xfs, linux-f2fs-devel, lixi,
adilger, Wang Shilong
On Fri, Mar 01, 2019 at 11:05:38PM +0900, Wang Shilong wrote:
> From: Wang Shilong <wshilong@ddn.com>
>
> From: Wang Shilong <wshilong@ddn.com>
>
> Extend statx to support project ID and inherit attribute.
>
> Signed-off-by: Wang Shilong <wshilong@ddn.com>
> ---
> fs/stat.c | 1 +
> include/linux/stat.h | 2 ++
> include/uapi/linux/stat.h | 8 ++++++--
> tools/include/uapi/linux/stat.h | 8 ++++++--
> 4 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/fs/stat.c b/fs/stat.c
> index adbfcd86c81b..82d855c4647c 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -551,6 +551,7 @@ cp_statx(const struct kstat *stat, struct statx __user *buffer)
> tmp.stx_rdev_minor = MINOR(stat->rdev);
> tmp.stx_dev_major = MAJOR(stat->dev);
> tmp.stx_dev_minor = MINOR(stat->dev);
> + tmp.stx_projid = (u32)from_kprojid(&init_user_ns, stat->projid);
If we are not in the init_user_ns, the project ID should be zero -
it should not be changeable or visible at all. I'm guessing the next
patches enforce this?
Regardless, the cast to (u32) should not be necessary.
Hmmmm.
/me looks at from_kprojid_munged() and thinks it needs to be nuked
from orbit. There is no such thing as an "overflow" project ID, and
65534 is a valid XFS project ID.
> diff --git a/tools/include/uapi/linux/stat.h b/tools/include/uapi/linux/stat.h
> index 7b35e98d3c58..21b542b3b061 100644
> --- a/tools/include/uapi/linux/stat.h
> +++ b/tools/include/uapi/linux/stat.h
> @@ -123,7 +123,9 @@ struct statx {
> __u32 stx_dev_major; /* ID of device containing file [uncond] */
> __u32 stx_dev_minor;
> /* 0x90 */
> - __u64 __spare2[14]; /* Spare space for future expansion */
> + __u32 stx_projid; /* Project ID of file */
> + __u32 __spare1[1];
> + __u64 __spare2[13]; /* Spare space for future expansion */
> /* 0x100 */
> };
>
> @@ -148,7 +150,8 @@ struct statx {
> #define STATX_BLOCKS 0x00000400U /* Want/got stx_blocks */
> #define STATX_BASIC_STATS 0x000007ffU /* The stuff in the normal stat struct */
> #define STATX_BTIME 0x00000800U /* Want/got stx_btime */
> -#define STATX_ALL 0x00000fffU /* All currently supported flags */
> +#define STATX_PROJID 0x00001000U /* Want/Got stx_projid */
> +#define STATX_ALL 0x00001fffU /* All currently supported flags */
> #define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */
>
> /*
> @@ -170,5 +173,6 @@ struct statx {
>
> #define STATX_ATTR_AUTOMOUNT 0x00001000 /* Dir: Automount trigger */
>
> +#define STATX_ATTR_PROJINHERIT 0x00002000 /* [I] File project inherit is set */
^^^^
The project ID inherit attribute is only valid for directories, not
files. Also, should probably be named STATX_ATTR_PROJID_INHERIT.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 6/8] ext4: support project in ext4_getattr()
2019-03-01 14:05 [PATCH 0/8] add generic interface to set/get project Wang Shilong
` (4 preceding siblings ...)
2019-03-01 14:05 ` [PATCH 5/8] fs: add project support to statx Wang Shilong
@ 2019-03-01 14:05 ` Wang Shilong
2019-03-01 14:05 ` [PATCH 7/8] f2fs: support project in f2fs_getattr() Wang Shilong
` (2 subsequent siblings)
8 siblings, 0 replies; 17+ messages in thread
From: Wang Shilong @ 2019-03-01 14:05 UTC (permalink / raw)
To: linux-fsdevel, linux-ext4, linux-xfs, linux-f2fs-devel
Cc: lixi, adilger, Wang Shilong
From: Wang Shilong <wshilong@ddn.com>
From: Wang Shilong <wshilong@ddn.com>
Signed-off-by: Wang Shilong <wshilong@ddn.com>
---
fs/ext4/inode.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index b6c451407dcd..cd7b3f997c3b 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5703,6 +5703,8 @@ int ext4_getattr(const struct path *path, struct kstat *stat,
struct ext4_inode_info *ei = EXT4_I(inode);
unsigned int flags;
+ stat->projid = ei->i_projid;
+
if (EXT4_FITS_IN_INODE(raw_inode, ei, i_crtime)) {
stat->result_mask |= STATX_BTIME;
stat->btime.tv_sec = ei->i_crtime.tv_sec;
@@ -5720,12 +5722,15 @@ int ext4_getattr(const struct path *path, struct kstat *stat,
stat->attributes |= STATX_ATTR_IMMUTABLE;
if (flags & EXT4_NODUMP_FL)
stat->attributes |= STATX_ATTR_NODUMP;
+ if (flags & EXT4_PROJINHERIT_FL)
+ stat->attributes |= STATX_ATTR_PROJINHERIT;
stat->attributes_mask |= (STATX_ATTR_APPEND |
STATX_ATTR_COMPRESSED |
STATX_ATTR_ENCRYPTED |
STATX_ATTR_IMMUTABLE |
- STATX_ATTR_NODUMP);
+ STATX_ATTR_NODUMP |
+ STATX_ATTR_PROJINHERIT);
generic_fillattr(inode, stat);
return 0;
--
2.19.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 7/8] f2fs: support project in f2fs_getattr()
2019-03-01 14:05 [PATCH 0/8] add generic interface to set/get project Wang Shilong
` (5 preceding siblings ...)
2019-03-01 14:05 ` [PATCH 6/8] ext4: support project in ext4_getattr() Wang Shilong
@ 2019-03-01 14:05 ` Wang Shilong
2019-03-01 14:05 ` [PATCH 8/8] xfs: support project in xfs_getattr() Wang Shilong
2019-03-03 21:11 ` [PATCH 0/8] add generic interface to set/get project Dave Chinner
8 siblings, 0 replies; 17+ messages in thread
From: Wang Shilong @ 2019-03-01 14:05 UTC (permalink / raw)
To: linux-fsdevel, linux-ext4, linux-xfs, linux-f2fs-devel
Cc: lixi, adilger, Wang Shilong
From: Wang Shilong <wshilong@ddn.com>
From: Wang Shilong <wshilong@ddn.com>
Signed-off-by: Wang Shilong <wshilong@ddn.com>
---
fs/f2fs/file.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 8eaca056e857..2db5883cc1b0 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -705,6 +705,7 @@ int f2fs_getattr(const struct path *path, struct kstat *stat,
stat->btime.tv_sec = fi->i_crtime.tv_sec;
stat->btime.tv_nsec = fi->i_crtime.tv_nsec;
}
+ stat->projid = fi->i_projid;
flags = fi->i_flags & F2FS_FL_USER_VISIBLE;
if (flags & F2FS_APPEND_FL)
@@ -717,12 +718,15 @@ int f2fs_getattr(const struct path *path, struct kstat *stat,
stat->attributes |= STATX_ATTR_IMMUTABLE;
if (flags & F2FS_NODUMP_FL)
stat->attributes |= STATX_ATTR_NODUMP;
+ if (flags & F2FS_PROJINHERIT_FL)
+ stat->attributes |= STATX_ATTR_PROJINHERIT;
stat->attributes_mask |= (STATX_ATTR_APPEND |
STATX_ATTR_COMPRESSED |
STATX_ATTR_ENCRYPTED |
STATX_ATTR_IMMUTABLE |
- STATX_ATTR_NODUMP);
+ STATX_ATTR_NODUMP |
+ STATX_ATTR_PROJINHERIT);
generic_fillattr(inode, stat);
--
2.19.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 8/8] xfs: support project in xfs_getattr()
2019-03-01 14:05 [PATCH 0/8] add generic interface to set/get project Wang Shilong
` (6 preceding siblings ...)
2019-03-01 14:05 ` [PATCH 7/8] f2fs: support project in f2fs_getattr() Wang Shilong
@ 2019-03-01 14:05 ` Wang Shilong
2019-03-01 15:39 ` Darrick J. Wong
2019-03-03 23:03 ` Dave Chinner
2019-03-03 21:11 ` [PATCH 0/8] add generic interface to set/get project Dave Chinner
8 siblings, 2 replies; 17+ messages in thread
From: Wang Shilong @ 2019-03-01 14:05 UTC (permalink / raw)
To: linux-fsdevel, linux-ext4, linux-xfs, linux-f2fs-devel
Cc: lixi, adilger, Wang Shilong
From: Wang Shilong <wshilong@ddn.com>
From: Wang Shilong <wshilong@ddn.com>
Signed-off-by: Wang Shilong <wshilong@ddn.com>
---
fs/xfs/xfs_iops.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index c10466fe6ed4..a2f8c0f048cf 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -501,6 +501,7 @@ xfs_vn_getattr(
if (XFS_FORCED_SHUTDOWN(mp))
return -EIO;
+ stat->projid = xfs_projid_to_kprojid(xfs_get_projid(ip));
stat->size = XFS_ISIZE(ip);
stat->dev = inode->i_sb->s_dev;
stat->mode = inode->i_mode;
@@ -528,6 +529,8 @@ xfs_vn_getattr(
stat->attributes |= STATX_ATTR_APPEND;
if (ip->i_d.di_flags & XFS_DIFLAG_NODUMP)
stat->attributes |= STATX_ATTR_NODUMP;
+ if (ip->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)
+ stat->attributes |= STATX_ATTR_PROJINHERIT;
switch (inode->i_mode & S_IFMT) {
case S_IFBLK:
--
2.19.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 8/8] xfs: support project in xfs_getattr()
2019-03-01 14:05 ` [PATCH 8/8] xfs: support project in xfs_getattr() Wang Shilong
@ 2019-03-01 15:39 ` Darrick J. Wong
2019-03-03 23:03 ` Dave Chinner
1 sibling, 0 replies; 17+ messages in thread
From: Darrick J. Wong @ 2019-03-01 15:39 UTC (permalink / raw)
To: Wang Shilong
Cc: linux-fsdevel, linux-ext4, linux-xfs, linux-f2fs-devel, lixi,
adilger, Wang Shilong
On Fri, Mar 01, 2019 at 11:05:41PM +0900, Wang Shilong wrote:
> From: Wang Shilong <wshilong@ddn.com>
>
> From: Wang Shilong <wshilong@ddn.com>
>
> Signed-off-by: Wang Shilong <wshilong@ddn.com>
> ---
> fs/xfs/xfs_iops.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index c10466fe6ed4..a2f8c0f048cf 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -501,6 +501,7 @@ xfs_vn_getattr(
> if (XFS_FORCED_SHUTDOWN(mp))
> return -EIO;
>
> + stat->projid = xfs_projid_to_kprojid(xfs_get_projid(ip));
I think we're supposed to set STATX_PROJID in the result mask if the
caller asks for it, right?
if (request_mask & STATX_PROJID) {
stat->projid = xfs_projid_to_kprojid(xfs_get_projid(ip));
stat->result_mask |= STATX_PROJID;
}
> stat->size = XFS_ISIZE(ip);
> stat->dev = inode->i_sb->s_dev;
> stat->mode = inode->i_mode;
> @@ -528,6 +529,8 @@ xfs_vn_getattr(
> stat->attributes |= STATX_ATTR_APPEND;
> if (ip->i_d.di_flags & XFS_DIFLAG_NODUMP)
> stat->attributes |= STATX_ATTR_NODUMP;
> + if (ip->i_d.di_flags & XFS_DIFLAG_PROJINHERIT)
> + stat->attributes |= STATX_ATTR_PROJINHERIT;
I think we also have to set STATX_ATTR_PROJINHERIT in the
attributes_mask, but ... heh, XFS doesn't do that at all. :(
--D
> switch (inode->i_mode & S_IFMT) {
> case S_IFBLK:
> --
> 2.19.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 8/8] xfs: support project in xfs_getattr()
2019-03-01 14:05 ` [PATCH 8/8] xfs: support project in xfs_getattr() Wang Shilong
2019-03-01 15:39 ` Darrick J. Wong
@ 2019-03-03 23:03 ` Dave Chinner
1 sibling, 0 replies; 17+ messages in thread
From: Dave Chinner @ 2019-03-03 23:03 UTC (permalink / raw)
To: Wang Shilong
Cc: linux-fsdevel, linux-ext4, linux-xfs, linux-f2fs-devel, lixi,
adilger, Wang Shilong
On Fri, Mar 01, 2019 at 11:05:41PM +0900, Wang Shilong wrote:
> From: Wang Shilong <wshilong@ddn.com>
>
> From: Wang Shilong <wshilong@ddn.com>
>
> Signed-off-by: Wang Shilong <wshilong@ddn.com>
> ---
> fs/xfs/xfs_iops.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index c10466fe6ed4..a2f8c0f048cf 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -501,6 +501,7 @@ xfs_vn_getattr(
> if (XFS_FORCED_SHUTDOWN(mp))
> return -EIO;
>
> + stat->projid = xfs_projid_to_kprojid(xfs_get_projid(ip));
Should only be set if the caller is in the init_user_ns.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/8] add generic interface to set/get project
2019-03-01 14:05 [PATCH 0/8] add generic interface to set/get project Wang Shilong
` (7 preceding siblings ...)
2019-03-01 14:05 ` [PATCH 8/8] xfs: support project in xfs_getattr() Wang Shilong
@ 2019-03-03 21:11 ` Dave Chinner
8 siblings, 0 replies; 17+ messages in thread
From: Dave Chinner @ 2019-03-03 21:11 UTC (permalink / raw)
To: Wang Shilong
Cc: linux-fsdevel, linux-ext4, linux-xfs, linux-f2fs-devel, lixi,
adilger, Wang Shilong
On Fri, Mar 01, 2019 at 11:05:33PM +0900, Wang Shilong wrote:
> From: Wang Shilong <wshilong@ddn.com>
>
> Currently, Filesystem use FS_IOC_FS_SETXATTR ioctl
> to change project ID of file. However we don't
> support ioctl on symlink files, and it is desirable
> to change symlink files' project ID just like uid/gid.
>
> This patch try to reuse existed interface fchownat(),
> use group id to set project ID if flag AT_FCHOWN_PROJID
> passed in.
>
> Also extend statx() calles to get symlink files' project
> ID and inherit attribute.
I'll mention the generic book-keeping stuff here:
- Series needs to be cc'd to linux-api@vger.kernel.org
- commit messages should be formatted similar to email. i.e. line
wrap at 68-72 columns, not 50
- all of the patches have duplicate "From:" lines in the commit
message.
- most of the patches are missing commit messages.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 17+ messages in thread