diff mbox series

[RFC] ext4: add link file support for {GET,SET}XATTR ioctl

Message ID 1548899232-30311-1-git-send-email-wshilong1991@gmail.com
State Rejected
Headers show
Series [RFC] ext4: add link file support for {GET,SET}XATTR ioctl | expand

Commit Message

Wang Shilong Jan. 31, 2019, 1:47 a.m. UTC 
From: Wang Shilong <wshilong@ddn.com>

Currently there is no way to change project ID of
symlink file itself, this is important to implement
Directory quota for an existed directory.

For example, for a newly created dir, we could just
set project <ID> and inherit attribute, newly created
symlink file will inherit it automatically.

We implment project changes by GET/SETXATTR ioctl,the
hook function only works for file or dir. To support
symlink file we could do by some ways that i could think of:

1) Add unlocked_ioctl for sys link files.
2) Add FS_IOC_FS[GET|SET]TXATTR_CHILD ioctl which
get/set the xattr of an inode by giving its parent
directory and its dentry name.

This patch try to use the second way to address this problem.
Child share parent @filep for mnt_want_write_file() for
security check which actually try to use sb, and use
for filesystem freeze purpose.

Cc: Andreas Dilger <adilger@dilger.ca>
Cc: Li Xi <lixi@ddn.com>
Signed-off-by: Wang Shilong <wshilong@ddn.com>
---
 fs/ext4/ext4.h                |   2 +
 fs/ext4/ioctl.c               | 148 +++++++++++++++++++++++++---------
 include/uapi/linux/fs.h       |  10 +++
 tools/include/uapi/linux/fs.h |  10 +++
 4 files changed, 132 insertions(+), 38 deletions(-)

Comments

Dave Chinner Jan. 31, 2019, 3:41 a.m. UTC | #1 
On Thu, Jan 31, 2019 at 10:47:12AM +0900, Wang Shilong wrote:
> From: Wang Shilong <wshilong@ddn.com>
> 
> Currently there is no way to change project ID of
> symlink file itself, this is important to implement
> Directory quota for an existed directory.

This seems like something open(O_PATH|O_NOFOLLOW) should allow.
from open(2):

	If pathname is a symbolic link and the O_NOFOLLOW flag is
	also specified, then the call returns a file  descriptor
	referring  to  the symbolic  link.  This  file  descriptor
	can  be used as the dirfd argument in calls to fchownat(2),
	fstatat(2), linkat(2), and read¿ linkat(2) with an empty
	pathname to have the calls operate on the symbolic link.

Changing the project id is the equivalent of fchownat().....

Cheers,

Dave.
Darrick Wong Jan. 31, 2019, 5:40 p.m. UTC | #2 
On Thu, Jan 31, 2019 at 02:41:06PM +1100, Dave Chinner wrote:
> On Thu, Jan 31, 2019 at 10:47:12AM +0900, Wang Shilong wrote:
> > From: Wang Shilong <wshilong@ddn.com>
> > 
> > Currently there is no way to change project ID of
> > symlink file itself, this is important to implement
> > Directory quota for an existed directory.
> 
> This seems like something open(O_PATH|O_NOFOLLOW) should allow.
> from open(2):

...but I thought O_PATH|O_NOFOLLOW file descriptions didn't allow ioctl
calls?

$ ln -sf urk /mnt/cow
$ xfs_io -c 'open -PL /mnt/cow' -c 'chproj 6'
setprojid: Bad file descriptor
$ ls -la /mnt/
lrwxrwxrwx 1 root root 3 Jan 31 09:30 /mnt/cow -> moo

> 	If pathname is a symbolic link and the O_NOFOLLOW flag is
> 	also specified, then the call returns a file  descriptor
> 	referring  to  the symbolic  link.  This  file  descriptor
> 	can  be used as the dirfd argument in calls to fchownat(2),
> 	fstatat(2), linkat(2), and read¿ linkat(2) with an empty
> 	pathname to have the calls operate on the symbolic link.
> 
> Changing the project id is the equivalent of fchownat().....

/me & others wonder (on the ext4 call) if maybe we should promote
project id to a vfs level concept?  i.e. store project id in struct
inode instead of the fs-specific inode structures.  Then we can use the
existing setattr infrastructure to persist those changes.

As for fchownat, how about a new flag that means "use the value in the
gid field to set the project id"?

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
Dave Chinner Jan. 31, 2019, 8:43 p.m. UTC | #3 
On Thu, Jan 31, 2019 at 09:40:59AM -0800, Darrick J. Wong wrote:
> On Thu, Jan 31, 2019 at 02:41:06PM +1100, Dave Chinner wrote:
> > On Thu, Jan 31, 2019 at 10:47:12AM +0900, Wang Shilong wrote:
> > > From: Wang Shilong <wshilong@ddn.com>
> > > 
> > > Currently there is no way to change project ID of
> > > symlink file itself, this is important to implement
> > > Directory quota for an existed directory.
> > 
> > This seems like something open(O_PATH|O_NOFOLLOW) should allow.
> > from open(2):
> 
> ...but I thought O_PATH|O_NOFOLLOW file descriptions didn't allow ioctl
> calls?

It doesn't, and that is the problem here.

> > 	If pathname is a symbolic link and the O_NOFOLLOW flag is
> > 	also specified, then the call returns a file  descriptor
> > 	referring  to  the symbolic  link.  This  file  descriptor
> > 	can  be used as the dirfd argument in calls to fchownat(2),
> > 	fstatat(2), linkat(2), and read¿ linkat(2) with an empty
> > 	pathname to have the calls operate on the symbolic link.
> > 
> > Changing the project id is the equivalent of fchownat().....
> 
> /me & others wonder (on the ext4 call) if maybe we should promote
> project id to a vfs level concept? 

It already is - see include/linux/kprojid.h.

That was done for user namespaces, despite my objections that
project IDs aren't something that user namespaces should swizzle
because they are filesystem controlled IDs and the init namespace
needs to use them to control container directory space usage,
therefore they are not available for use in user namespaces
contexts.....

> i.e. store project id in struct
> inode instead of the fs-specific inode structures.  Then we can use the
> existing setattr infrastructure to persist those changes.

Makes no difference to me where they are stored - settattr has to
call into the filesystem anyway to change them (transactions!) so it
doesn't need to be in the struct inode, just in the struct iattr
that is passed to the filesystems.

> As for fchownat, how about a new flag that means "use the value in the
> gid field to set the project id"?

Kinda what I was thinking, so we don't need a whole new syscall.
Just limit that flag usage to the init namespace and we're all good.

Cheers,

Dave.
Wang Shilong Feb. 14, 2019, 8:14 a.m. UTC | #4 
Hi Dave, Darrick,

      Thanks for good suggestions!
As my understanding, we could modify 

do_fchownat() to change project ID instead of
group id if AT_FCHOWN_PROJID(new flags) passed in.

Besides that we also need a new flag AT_FCHOWN_PROJ_INHERIT.
to set/clear inherit attributes? 

And for Project Get, we might need extend 'struct attr {'
to include project id.

What do you think?

Thanks,
Shilong
Darrick Wong Feb. 14, 2019, 3:54 p.m. UTC | #5 
On Thu, Feb 14, 2019 at 08:14:56AM +0000, Wang Shilong wrote:
> Hi Dave, Darrick,
> 
>       Thanks for good suggestions!
> As my understanding, we could modify 
> 
> do_fchownat() to change project ID instead of
> group id if AT_FCHOWN_PROJID(new flags) passed in.
> 
> Besides that we also need a new flag AT_FCHOWN_PROJ_INHERIT.
> to set/clear inherit attributes? 

That sounds reasonable enough to consider as a formal proposal.

> And for Project Get, we might need extend 'struct attr {'
> to include project id.

Let's add a new STATX_PROJID so that statx() callers can fetch it?

Also manpage updates and fstests cases...

--D

> What do you think?
> 
> Thanks,
> Shilong
diff mbox series

Patch

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 185a05d3257e..2468c64bb987 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -648,6 +648,8 @@  enum {
 
 #define EXT4_IOC_FSGETXATTR		FS_IOC_FSGETXATTR
 #define EXT4_IOC_FSSETXATTR		FS_IOC_FSSETXATTR
+#define EXT4_IOC_FSGETXATTR_CHILD	FS_IOC_FSGETXATTR_CHILD
+#define EXT4_IOC_FSSETXATTR_CHILD	FS_IOC_FSSETXATTR_CHILD
 
 #define EXT4_IOC_SHUTDOWN _IOR ('X', 125, __u32)
 
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index d37dafa1d133..0a75cda2dba1 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -20,6 +20,7 @@ 
 #include <linux/uaccess.h>
 #include <linux/delay.h>
 #include <linux/iversion.h>
+#include <linux/namei.h>
 #include "ext4_jbd2.h"
 #include "ext4.h"
 #include <linux/fsmap.h>
@@ -333,9 +334,8 @@  static int ext4_ioctl_setflags(struct inode *inode,
 }
 
 #ifdef CONFIG_QUOTA
-static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
+static int ext4_ioctl_setproject(struct inode *inode, __u32 projid)
 {
-	struct inode *inode = file_inode(filp);
 	struct super_block *sb = inode->i_sb;
 	struct ext4_inode_info *ei = EXT4_I(inode);
 	int err, rc;
@@ -419,7 +419,7 @@  static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
 	return err;
 }
 #else
-static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
+static int ext4_ioctl_setproject(struct inode *inode, __u32 projid)
 {
 	if (projid != EXT4_DEF_PROJID)
 		return -EOPNOTSUPP;
@@ -663,6 +663,59 @@  static int ext4_ioctl_check_project(struct inode *inode, struct fsxattr *fa)
 	return 0;
 }
 
+static void ext4_ioctl_fsgetxattr(struct inode *inode, struct fsxattr *fa,
+				  unsigned long arg)
+{
+	struct ext4_inode_info *ei = EXT4_I(inode);
+
+	memset(fa, 0, sizeof(struct fsxattr));
+	fa->fsx_xflags = ext4_iflags_to_xflags(ei->i_flags &
+					       EXT4_FL_USER_VISIBLE);
+
+	if (ext4_has_feature_project(inode->i_sb))
+		fa->fsx_projid = (__u32)from_kprojid(&init_user_ns,
+						     ei->i_projid);
+}
+
+static int ext4_ioctl_fssetxattr(struct file *filp, struct inode *cinode,
+				 struct fsxattr *fa)
+{
+	int err;
+	unsigned int flags;
+	struct ext4_inode_info *ei = EXT4_I(cinode);
+
+	/* Make sure caller has proper permission */
+	if (!inode_owner_or_capable(cinode))
+		return -EACCES;
+
+	if (fa->fsx_xflags & ~EXT4_SUPPORTED_FS_XFLAGS)
+		return -EOPNOTSUPP;
+
+	flags = ext4_xflags_to_iflags(fa->fsx_xflags);
+	if (ext4_mask_flags(cinode->i_mode, flags) != flags)
+		return -EOPNOTSUPP;
+
+	err = mnt_want_write_file(filp);
+	if (err)
+		return err;
+
+	inode_lock(cinode);
+	err = ext4_ioctl_check_project(cinode, fa);
+	if (err)
+		goto out;
+	flags = (ei->i_flags & ~EXT4_FL_XFLAG_VISIBLE) |
+		 (flags & EXT4_FL_XFLAG_VISIBLE);
+	err = ext4_ioctl_setflags(cinode, flags);
+	if (err)
+		goto out;
+	err = ext4_ioctl_setproject(cinode, fa->fsx_projid);
+out:
+	inode_unlock(cinode);
+	mnt_drop_write_file(filp);
+
+	return err;
+}
+
 long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	struct inode *inode = file_inode(filp);
@@ -1024,56 +1077,75 @@  long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	{
 		struct fsxattr fa;
 
-		memset(&fa, 0, sizeof(struct fsxattr));
-		fa.fsx_xflags = ext4_iflags_to_xflags(ei->i_flags & EXT4_FL_USER_VISIBLE);
+		ext4_ioctl_fsgetxattr(inode, &fa, arg);
 
-		if (ext4_has_feature_project(inode->i_sb)) {
-			fa.fsx_projid = (__u32)from_kprojid(&init_user_ns,
-				EXT4_I(inode)->i_projid);
-		}
+		if (copy_to_user((struct fsxattr __user *)arg, &fa,
+				 sizeof(fa)))
+			return -EFAULT;
+	}
+	case EXT4_IOC_FSGETXATTR_CHILD:
+	{
+		struct fsxattr_child fa;
+		struct dentry *dentry;
 
-		if (copy_to_user((struct fsxattr __user *)arg,
-				 &fa, sizeof(fa)))
+		if (copy_from_user(&fa, (struct fsxattr_child __user *)arg,
+				   sizeof(fa)))
+			return -EFAULT;
+
+		fa.fsxc_name[NAME_MAX] = '\0';
+		inode_lock(file_inode(filp));
+		dentry = lookup_one_len(fa.fsxc_name, file_dentry(filp),
+					strlen(fa.fsxc_name));
+		inode_unlock(file_inode(filp));
+		if (IS_ERR(dentry))
+			return PTR_ERR(dentry);
+
+		ext4_ioctl_fsgetxattr(d_inode(dentry), &fa.fsxc_fsx, arg);
+		dput(dentry);
+
+		if (copy_to_user((struct fsxattr_child __user *)arg, &fa,
+				 sizeof(fa)))
 			return -EFAULT;
-		return 0;
 	}
 	case EXT4_IOC_FSSETXATTR:
 	{
 		struct fsxattr fa;
-		int err;
 
 		if (copy_from_user(&fa, (struct fsxattr __user *)arg,
 				   sizeof(fa)))
 			return -EFAULT;
 
-		/* Make sure caller has proper permission */
-		if (!inode_owner_or_capable(inode))
-			return -EACCES;
-
-		if (fa.fsx_xflags & ~EXT4_SUPPORTED_FS_XFLAGS)
-			return -EOPNOTSUPP;
+		return ext4_ioctl_fssetxattr(filp, inode, &fa);
+	}
+	case EXT4_IOC_FSSETXATTR_CHILD:
+	{
+		struct fsxattr_child fa;
+		struct dentry *dentry;
+		int err;
 
-		flags = ext4_xflags_to_iflags(fa.fsx_xflags);
-		if (ext4_mask_flags(inode->i_mode, flags) != flags)
-			return -EOPNOTSUPP;
+		if (copy_from_user(&fa, (struct fsxattr_child __user *)arg,
+				   sizeof(fa)))
+			return -EFAULT;
 
-		err = mnt_want_write_file(filp);
-		if (err)
-			return err;
+		fa.fsxc_name[NAME_MAX] = '\0';
+		inode_lock(file_inode(filp));
+		dentry = lookup_one_len(fa.fsxc_name, file_dentry(filp),
+					strlen(fa.fsxc_name));
+		inode_unlock(file_inode(filp));
+		if (IS_ERR(dentry))
+			return PTR_ERR(dentry);
+		/* this didn't work since child will share
+		 * parent @filp to use mnt_want_write_file()
+		 */
+		if (file_inode(filp)->i_sb != d_inode(dentry)->i_sb) {
+			err = -EXDEV;
+			goto errout;
+		}
 
-		inode_lock(inode);
-		err = ext4_ioctl_check_project(inode, &fa);
-		if (err)
-			goto out;
-		flags = (ei->i_flags & ~EXT4_FL_XFLAG_VISIBLE) |
-			 (flags & EXT4_FL_XFLAG_VISIBLE);
-		err = ext4_ioctl_setflags(inode, flags);
-		if (err)
-			goto out;
-		err = ext4_ioctl_setproject(filp, fa.fsx_projid);
-out:
-		inode_unlock(inode);
-		mnt_drop_write_file(filp);
+		err = ext4_ioctl_fssetxattr(filp, d_inode(dentry),
+					    &fa.fsxc_fsx);
+errout:
+		dput(dentry);
 		return err;
 	}
 	case EXT4_IOC_SHUTDOWN:
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 121e82ce296b..e8f54126da2e 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -118,6 +118,14 @@  struct fsxattr {
 	unsigned char	fsx_pad[8];
 };
 
+/*
+ * Structure for FS_IOC_FSGETXATTR_CHILD and FS_IOC_FSSETXATTR_CHILD.
+ */
+struct fsxattr_child {
+	struct fsxattr	fsxc_fsx;
+	unsigned char	fsxc_name[NAME_MAX + 1]; /* child filename */
+};
+
 /*
  * Flags for the fsx_xflags field
  */
@@ -209,6 +217,8 @@  struct fsxattr {
 #define FS_IOC32_SETVERSION		_IOW('v', 2, int)
 #define FS_IOC_FSGETXATTR		_IOR('X', 31, struct fsxattr)
 #define FS_IOC_FSSETXATTR		_IOW('X', 32, struct fsxattr)
+#define FS_IOC_FSGETXATTR_CHILD		_IOR('X', 33, struct fsxattr_child)
+#define FS_IOC_FSSETXATTR_CHILD		_IOW('X', 34, struct fsxattr_child)
 #define FS_IOC_GETFSLABEL		_IOR(0x94, 49, char[FSLABEL_MAX])
 #define FS_IOC_SETFSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])
 
diff --git a/tools/include/uapi/linux/fs.h b/tools/include/uapi/linux/fs.h
index 121e82ce296b..e8f54126da2e 100644
--- a/tools/include/uapi/linux/fs.h
+++ b/tools/include/uapi/linux/fs.h
@@ -118,6 +118,14 @@  struct fsxattr {
 	unsigned char	fsx_pad[8];
 };
 
+/*
+ * Structure for FS_IOC_FSGETXATTR_CHILD and FS_IOC_FSSETXATTR_CHILD.
+ */
+struct fsxattr_child {
+	struct fsxattr	fsxc_fsx;
+	unsigned char	fsxc_name[NAME_MAX + 1]; /* child filename */
+};
+
 /*
  * Flags for the fsx_xflags field
  */
@@ -209,6 +217,8 @@  struct fsxattr {
 #define FS_IOC32_SETVERSION		_IOW('v', 2, int)
 #define FS_IOC_FSGETXATTR		_IOR('X', 31, struct fsxattr)
 #define FS_IOC_FSSETXATTR		_IOW('X', 32, struct fsxattr)
+#define FS_IOC_FSGETXATTR_CHILD		_IOR('X', 33, struct fsxattr_child)
+#define FS_IOC_FSSETXATTR_CHILD		_IOW('X', 34, struct fsxattr_child)
 #define FS_IOC_GETFSLABEL		_IOR(0x94, 49, char[FSLABEL_MAX])
 #define FS_IOC_SETFSLABEL		_IOW(0x94, 50, char[FSLABEL_MAX])