Re: [f2fs-dev] [PATCH 5/5] vfs: don't allow writes to swap files

2019-06-25 Thread Al Viro 
On Tue, Jun 25, 2019 at 07:33:31PM -0700, Darrick J. Wong wrote:
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -236,6 +236,9 @@ int notify_change(struct dentry * dentry, struct iattr * 
> attr, struct inode **de
>   if (IS_IMMUTABLE(inode))
>   return -EPERM;
>  
> + if (IS_SWAPFILE(inode))
> + return -ETXTBSY;
> +
>   if ((ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID | ATTR_TIMES_SET)) &&
>   IS_APPEND(inode))
>   return -EPERM;

Er...  So why exactly is e.g. chmod(2) forbidden for swapfiles?  Or touch(1),
for that matter...

> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 596ac98051c5..1ca4ee8c2d60 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -3165,6 +3165,19 @@ SYSCALL_DEFINE2(swapon, const char __user *, 
> specialfile, int, swap_flags)
>   if (error)
>   goto bad_swap;
>  
> + /*
> +  * Flush any pending IO and dirty mappings before we start using this
> +  * swap file.
> +  */
> + if (S_ISREG(inode->i_mode)) {
> + inode->i_flags |= S_SWAPFILE;
> + error = inode_drain_writes(inode);
> + if (error) {
> + inode->i_flags &= ~S_SWAPFILE;
> + goto bad_swap;
> + }
> + }

Why are swap partitions any less worthy of protection?


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 1/5] vfs: create a generic checking and prep function for FS_IOC_SETFLAGS

2019-06-25 Thread Al Viro 
On Tue, Jun 25, 2019 at 07:32:10PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong 
> 
> Create a generic function to check incoming FS_IOC_SETFLAGS flag values
> and later prepare the inode for updates so that we can standardize the
> implementations that follow ext4's flag values.

Change in efivarfs behaviour deserves a note.  I'm not saying it's wrong,
but behaviour is changed there - no-op FS_IOC_SETFLAGS used to fail
without CAP_LINUX_IMMUTABLE...


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/5] vfs: create a generic checking function for FS_IOC_FSSETXATTR

2019-06-25 Thread Al Viro 
On Tue, Jun 25, 2019 at 07:32:18PM -0700, Darrick J. Wong wrote:
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -373,10 +373,9 @@ static int check_xflags(unsigned int flags)
>  static int btrfs_ioctl_fsgetxattr(struct file *file, void __user *arg)
>  {
>   struct btrfs_inode *binode = BTRFS_I(file_inode(file));
> - struct fsxattr fa;
> -
> - memset(&fa, 0, sizeof(fa));
> - fa.fsx_xflags = btrfs_inode_flags_to_xflags(binode->flags);
> + struct fsxattr fa = {
> + .fsx_xflags = btrfs_inode_flags_to_xflags(binode->flags),
> + };

Umm...  Sure, there's no padding, but still - you are going to copy that thing
to userland...  How about

static inline void simple_fill_fsxattr(struct fsxattr *fa, unsigned xflags)
{
memset(fa, 0, sizeof(*fa));
fa->fsx_xflags = xflags;
}

and let the compiler optimize the crap out?


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH 5/5] vfs: don't allow writes to swap files

2019-06-25 Thread Darrick J. Wong 
From: Darrick J. Wong 

Don't let userspace write to an active swap file because the kernel
effectively has a long term lease on the storage and things could get
seriously corrupted if we let this happen.

Signed-off-by: Darrick J. Wong 
---
 fs/attr.c |3 +++
 mm/filemap.c  |3 +++
 mm/memory.c   |4 +++-
 mm/mmap.c |2 ++
 mm/swapfile.c |   15 +--
 5 files changed, 24 insertions(+), 3 deletions(-)


diff --git a/fs/attr.c b/fs/attr.c
index 1fcfdcc5b367..42f4d4fb0631 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -236,6 +236,9 @@ int notify_change(struct dentry * dentry, struct iattr * 
attr, struct inode **de
if (IS_IMMUTABLE(inode))
return -EPERM;
 
+   if (IS_SWAPFILE(inode))
+   return -ETXTBSY;
+
if ((ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID | ATTR_TIMES_SET)) &&
IS_APPEND(inode))
return -EPERM;
diff --git a/mm/filemap.c b/mm/filemap.c
index dad85e10f5f8..fd80bc20e30a 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2938,6 +2938,9 @@ inline ssize_t generic_write_checks(struct kiocb *iocb, 
struct iov_iter *from)
if (IS_IMMUTABLE(inode))
return -EPERM;
 
+   if (IS_SWAPFILE(inode))
+   return -ETXTBSY;
+
if (!iov_iter_count(from))
return 0;
 
diff --git a/mm/memory.c b/mm/memory.c
index 4311cfdade90..c04c6a689995 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2235,7 +2235,9 @@ static vm_fault_t do_page_mkwrite(struct vm_fault *vmf)
 
vmf->flags = FAULT_FLAG_WRITE|FAULT_FLAG_MKWRITE;
 
-   if (vmf->vma->vm_file && IS_IMMUTABLE(file_inode(vmf->vma->vm_file)))
+   if (vmf->vma->vm_file &&
+   (IS_IMMUTABLE(file_inode(vmf->vma->vm_file)) ||
+IS_SWAPFILE(file_inode(vmf->vma->vm_file
return VM_FAULT_SIGBUS;
 
ret = vmf->vma->vm_ops->page_mkwrite(vmf);
diff --git a/mm/mmap.c b/mm/mmap.c
index ac1e32205237..031807339869 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1488,6 +1488,8 @@ unsigned long do_mmap(struct file *file, unsigned long 
addr,
return -EACCES;
if (IS_IMMUTABLE(file_inode(file)))
return -EPERM;
+   if (IS_SWAPFILE(file_inode(file)))
+   return -ETXTBSY;
}
 
/*
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 596ac98051c5..1ca4ee8c2d60 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -3165,6 +3165,19 @@ SYSCALL_DEFINE2(swapon, const char __user *, 
specialfile, int, swap_flags)
if (error)
goto bad_swap;
 
+   /*
+* Flush any pending IO and dirty mappings before we start using this
+* swap file.
+*/
+   if (S_ISREG(inode->i_mode)) {
+   inode->i_flags |= S_SWAPFILE;
+   error = inode_drain_writes(inode);
+   if (error) {
+   inode->i_flags &= ~S_SWAPFILE;
+   goto bad_swap;
+   }
+   }
+
mutex_lock(&swapon_mutex);
prio = -1;
if (swap_flags & SWAP_FLAG_PREFER)
@@ -3185,8 +3198,6 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, 
int, swap_flags)
atomic_inc(&proc_poll_event);
wake_up_interruptible(&proc_poll_wait);
 
-   if (S_ISREG(inode->i_mode))
-   inode->i_flags |= S_SWAPFILE;
error = 0;
goto out;
 bad_swap:



___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH 3/5] vfs: flush and wait for io when setting the immutable flag via FSSETXATTR

2019-06-25 Thread Darrick J. Wong 
From: Darrick J. Wong 

When we're using FS_IOC_FSSETXATTR to set the immutable flag on a file,
we need to ensure that userspace can't continue to write the file after
the file becomes immutable.  To make that happen, we have to flush all
the dirty pagecache pages to disk to ensure that we can fail a page
fault on a mmap'd region, wait for pending directio to complete, and
hope the caller locked out any new writes by holding the inode lock.

XFS has more complex locking than other FSSETXATTR implementations so we
have to keep the checking and preparation code in different functions.

Signed-off-by: Darrick J. Wong 
---
 fs/btrfs/ioctl.c   |2 +
 fs/ext4/ioctl.c|2 +
 fs/f2fs/file.c |2 +
 fs/inode.c |   31 +++
 fs/xfs/xfs_ioctl.c |   71 +++-
 include/linux/fs.h |3 ++
 6 files changed, 90 insertions(+), 21 deletions(-)


diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 0f5af7c5f66b..bbd6d908900e 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -423,7 +423,7 @@ static int btrfs_ioctl_fssetxattr(struct file *file, void 
__user *arg)
old_flags = binode->flags;
old_i_flags = inode->i_flags;
 
-   ret = vfs_ioc_fssetxattr_check(inode, &old_fa, &fa);
+   ret = vfs_ioc_fssetxattr_prepare(inode, &old_fa, &fa);
if (ret)
goto out_unlock;
 
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 1e88c3af9a8d..146587c3fe8e 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -1109,7 +1109,7 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, 
unsigned long arg)
 
inode_lock(inode);
ext4_fill_fsxattr(inode, &old_fa);
-   err = vfs_ioc_fssetxattr_check(inode, &old_fa, &fa);
+   err = vfs_ioc_fssetxattr_prepare(inode, &old_fa, &fa);
if (err)
goto out;
flags = (ei->i_flags & ~EXT4_FL_XFLAG_VISIBLE) |
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index d6ed319388d6..af0fc040a15c 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2826,7 +2826,7 @@ static int f2fs_ioc_fssetxattr(struct file *filp, 
unsigned long arg)
inode_lock(inode);
 
f2fs_fill_fsxattr(inode, &old_fa);
-   err = vfs_ioc_fssetxattr_check(inode, &old_fa, &fa);
+   err = vfs_ioc_fssetxattr_prepare(inode, &old_fa, &fa);
if (err)
goto out;
flags = (fi->i_flags & ~F2FS_FL_XFLAG_VISIBLE) |
diff --git a/fs/inode.c b/fs/inode.c
index 65a412af3ffb..cf07378e5731 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2293,3 +2293,34 @@ int vfs_ioc_fssetxattr_check(struct inode *inode, const 
struct fsxattr *old_fa,
return 0;
 }
 EXPORT_SYMBOL(vfs_ioc_fssetxattr_check);
+
+/*
+ * Generic function to check FS_IOC_FSSETXATTR values and reject any invalid
+ * configurations.  If none are found, flush all pending IO and dirty mappings
+ * before setting S_IMMUTABLE on an inode.  If the flush fails we'll clear the
+ * flag before returning error.
+ *
+ * Note: the caller must hold whatever locks are necessary to block any other
+ * threads from starting a write to the file.
+ */
+int vfs_ioc_fssetxattr_prepare(struct inode *inode,
+  const struct fsxattr *old_fa,
+  struct fsxattr *fa)
+{
+   int ret;
+
+   ret = vfs_ioc_fssetxattr_check(inode, old_fa, fa);
+   if (ret)
+   return ret;
+
+   if (!S_ISREG(inode->i_mode) || IS_IMMUTABLE(inode) ||
+   !(fa->fsx_xflags & FS_XFLAG_IMMUTABLE))
+   return 0;
+
+   inode_set_flags(inode, S_IMMUTABLE, S_IMMUTABLE);
+   ret = inode_drain_writes(inode);
+   if (ret)
+   inode_set_flags(inode, 0, S_IMMUTABLE);
+   return ret;
+}
+EXPORT_SYMBOL(vfs_ioc_fssetxattr_prepare);
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 011657bd50ca..723550c8a2e4 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1058,6 +1058,30 @@ xfs_ioctl_setattr_xflags(
return 0;
 }
 
+/*
+ * If we're setting immutable on a regular file, we need to prevent new writes.
+ * Once we've done that, we must wait for all the other writes to complete.
+ *
+ * The caller must use @join_flags to release the locks which are held on @ip
+ * regardless of return value.
+ */
+static int
+xfs_ioctl_setattr_drain_writes(
+   struct xfs_inode*ip,
+   const struct fsxattr*fa,
+   int *join_flags)
+{
+   struct inode*inode = VFS_I(ip);
+
+   if (!S_ISREG(inode->i_mode) || !(fa->fsx_xflags & FS_XFLAG_IMMUTABLE))
+   return 0;
+
+   *join_flags = XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL;
+   xfs_ilock(ip, *join_flags);
+
+   return inode_drain_writes(inode);
+}
+
 /*
  * If we are changing DAX flags, we have to ensure the file is clean and any
  * cached objects in the address space are invalidated and removed. This
@@ -1065,6

[f2fs-dev] [PATCH 4/5] vfs: don't allow most setxattr to immutable files

2019-06-25 Thread Darrick J. Wong 
From: Darrick J. Wong 

The chattr manpage has this to say about immutable files:

"A file with the 'i' attribute cannot be modified: it cannot be deleted
or renamed, no link can be created to this file, most of the file's
metadata can not be modified, and the file can not be opened in write
mode."

However, we don't actually check the immutable flag in the setattr code,
which means that we can update inode flags and project ids and extent
size hints on supposedly immutable files.  Therefore, reject setflags
and fssetxattr calls on an immutable file if the file is immutable and
will remain that way.

Signed-off-by: Darrick J. Wong 
---
 fs/inode.c |   27 +++
 1 file changed, 27 insertions(+)


diff --git a/fs/inode.c b/fs/inode.c
index cf07378e5731..4261c709e50e 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2214,6 +2214,14 @@ int vfs_ioc_setflags_prepare(struct inode *inode, 
unsigned int oldflags,
!capable(CAP_LINUX_IMMUTABLE))
return -EPERM;
 
+   /*
+* We aren't allowed to change any other flags if the immutable flag is
+* already set and is not being unset.
+*/
+   if ((oldflags & FS_IMMUTABLE_FL) && (flags & FS_IMMUTABLE_FL) &&
+   oldflags != flags)
+   return -EPERM;
+
/*
 * Now that we're done checking the new flags, flush all pending IO and
 * dirty mappings before setting S_IMMUTABLE on an inode via
@@ -2284,6 +2292,25 @@ int vfs_ioc_fssetxattr_check(struct inode *inode, const 
struct fsxattr *old_fa,
!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)))
return -EINVAL;
 
+   /*
+* We aren't allowed to change any fields if the immutable flag is
+* already set and is not being unset.
+*/
+   if ((old_fa->fsx_xflags & FS_XFLAG_IMMUTABLE) &&
+   (fa->fsx_xflags & FS_XFLAG_IMMUTABLE)) {
+   if (old_fa->fsx_xflags != fa->fsx_xflags)
+   return -EPERM;
+   if (old_fa->fsx_projid != fa->fsx_projid)
+   return -EPERM;
+   if ((fa->fsx_xflags & (FS_XFLAG_EXTSIZE |
+  FS_XFLAG_EXTSZINHERIT)) &&
+   old_fa->fsx_extsize != fa->fsx_extsize)
+   return -EPERM;
+   if ((old_fa->fsx_xflags & FS_XFLAG_COWEXTSIZE) &&
+   old_fa->fsx_cowextsize != fa->fsx_cowextsize)
+   return -EPERM;
+   }
+
/* Extent size hints of zero turn off the flags. */
if (fa->fsx_extsize == 0)
fa->fsx_xflags &= ~(FS_XFLAG_EXTSIZE | FS_XFLAG_EXTSZINHERIT);



___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH v5 0/5] vfs: make immutable files actually immutable

2019-06-25 Thread Darrick J. Wong 
Hi all,

The chattr(1) manpage has this to say about the immutable bit that
system administrators can set on files:

"A file with the 'i' attribute cannot be modified: it cannot be deleted
or renamed, no link can be created to this file, most of the file's
metadata can not be modified, and the file can not be opened in write
mode."

Given the clause about how the file 'cannot be modified', it is
surprising that programs holding writable file descriptors can continue
to write to and truncate files after the immutable flag has been set,
but they cannot call other things such as utimes, fallocate, unlink,
link, setxattr, or reflink.

Since the immutable flag is only settable by administrators, resolve
this inconsistent behavior in favor of the documented behavior -- once
the flag is set, the file cannot be modified, period.  We presume that
administrators must be trusted to know what they're doing, and that
cutting off programs with writable fds will probably break them.

Therefore, add immutability checks to the relevant VFS functions, then
refactor the SETFLAGS and FSSETXATTR implementations to use common
argument checking functions so that we can then force pagefaults on all
the file data when setting immutability.

Note that various distro manpages points out the inconsistent behavior
of the various Linux filesystems w.r.t. immutable.  This fixes all that.

I also discovered that userspace programs can write and create writable
memory mappings to active swap files.  This is extremely bad because
this allows anyone with write privileges to corrupt system memory.  The
final patch in this series closes off that hole, at least for swap
files.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This has been lightly tested with fstests.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=immutable-files

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=immutable-files


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH 1/5] mm/fs: don't allow writes to immutable files

2019-06-25 Thread Darrick J. Wong 
From: Darrick J. Wong 

The chattr manpage has this to say about immutable files:

"A file with the 'i' attribute cannot be modified: it cannot be deleted
or renamed, no link can be created to this file, most of the file's
metadata can not be modified, and the file can not be opened in write
mode."

Once the flag is set, it is enforced for quite a few file operations,
such as fallocate, fpunch, fzero, rm, touch, open, etc.  However, we
don't check for immutability when doing a write(), a PROT_WRITE mmap(),
a truncate(), or a write to a previously established mmap.

If a program has an open write fd to a file that the administrator
subsequently marks immutable, the program still can change the file
contents.  Weird!

The ability to write to an immutable file does not follow the manpage
promise that immutable files cannot be modified.  Worse yet it's
inconsistent with the behavior of other syscalls which don't allow
modifications of immutable files.

Therefore, add the necessary checks to make the write, mmap, and
truncate behavior consistent with what the manpage says and consistent
with other syscalls on filesystems which support IMMUTABLE.

Signed-off-by: Darrick J. Wong 
Reviewed-by: Jan Kara 
---
 fs/attr.c|   13 ++---
 mm/filemap.c |3 +++
 mm/memory.c  |3 +++
 mm/mmap.c|8 ++--
 4 files changed, 18 insertions(+), 9 deletions(-)


diff --git a/fs/attr.c b/fs/attr.c
index d22e8187477f..1fcfdcc5b367 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -233,19 +233,18 @@ int notify_change(struct dentry * dentry, struct iattr * 
attr, struct inode **de
 
WARN_ON_ONCE(!inode_is_locked(inode));
 
-   if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID | ATTR_TIMES_SET)) {
-   if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
-   return -EPERM;
-   }
+   if (IS_IMMUTABLE(inode))
+   return -EPERM;
+
+   if ((ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID | ATTR_TIMES_SET)) &&
+   IS_APPEND(inode))
+   return -EPERM;
 
/*
 * If utimes(2) and friends are called with times == NULL (or both
 * times are UTIME_NOW), then we need to check for write permission
 */
if (ia_valid & ATTR_TOUCH) {
-   if (IS_IMMUTABLE(inode))
-   return -EPERM;
-
if (!inode_owner_or_capable(inode)) {
error = inode_permission(inode, MAY_WRITE);
if (error)
diff --git a/mm/filemap.c b/mm/filemap.c
index aac71aef4c61..dad85e10f5f8 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2935,6 +2935,9 @@ inline ssize_t generic_write_checks(struct kiocb *iocb, 
struct iov_iter *from)
loff_t count;
int ret;
 
+   if (IS_IMMUTABLE(inode))
+   return -EPERM;
+
if (!iov_iter_count(from))
return 0;
 
diff --git a/mm/memory.c b/mm/memory.c
index ddf20bd0c317..4311cfdade90 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2235,6 +2235,9 @@ static vm_fault_t do_page_mkwrite(struct vm_fault *vmf)
 
vmf->flags = FAULT_FLAG_WRITE|FAULT_FLAG_MKWRITE;
 
+   if (vmf->vma->vm_file && IS_IMMUTABLE(file_inode(vmf->vma->vm_file)))
+   return VM_FAULT_SIGBUS;
+
ret = vmf->vma->vm_ops->page_mkwrite(vmf);
/* Restore original flags so that caller is not surprised */
vmf->flags = old_flags;
diff --git a/mm/mmap.c b/mm/mmap.c
index 7e8c3e8ae75f..ac1e32205237 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1483,8 +1483,12 @@ unsigned long do_mmap(struct file *file, unsigned long 
addr,
case MAP_SHARED_VALIDATE:
if (flags & ~flags_mask)
return -EOPNOTSUPP;
-   if ((prot&PROT_WRITE) && !(file->f_mode&FMODE_WRITE))
-   return -EACCES;
+   if (prot & PROT_WRITE) {
+   if (!(file->f_mode & FMODE_WRITE))
+   return -EACCES;
+   if (IS_IMMUTABLE(file_inode(file)))
+   return -EPERM;
+   }
 
/*
 * Make sure we don't allow writing to an append-only



___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH 2/5] vfs: flush and wait for io when setting the immutable flag via SETFLAGS

2019-06-25 Thread Darrick J. Wong 
From: Darrick J. Wong 

When we're using FS_IOC_SETFLAGS to set the immutable flag on a file, we
need to ensure that userspace can't continue to write the file after the
file becomes immutable.  To make that happen, we have to flush all the
dirty pagecache pages to disk to ensure that we can fail a page fault on
a mmap'd region, wait for pending directio to complete, and hope the
caller locked out any new writes by holding the inode lock.

Signed-off-by: Darrick J. Wong 
---
 fs/inode.c |   21 +++--
 include/linux/fs.h |   11 +++
 2 files changed, 30 insertions(+), 2 deletions(-)


diff --git a/fs/inode.c b/fs/inode.c
index f08711b34341..65a412af3ffb 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2193,7 +2193,8 @@ EXPORT_SYMBOL(current_time);
 
 /*
  * Generic function to check FS_IOC_SETFLAGS values and reject any invalid
- * configurations.
+ * configurations.  Once we're done, prepare the inode for whatever changes
+ * are coming down the pipeline.
  *
  * Note: the caller should be holding i_mutex, or else be sure that they have
  * exclusive access to the inode structure.
@@ -2201,6 +2202,8 @@ EXPORT_SYMBOL(current_time);
 int vfs_ioc_setflags_prepare(struct inode *inode, unsigned int oldflags,
 unsigned int flags)
 {
+   int ret;
+
/*
 * The IMMUTABLE and APPEND_ONLY flags can only be changed by
 * the relevant capability.
@@ -2211,7 +2214,21 @@ int vfs_ioc_setflags_prepare(struct inode *inode, 
unsigned int oldflags,
!capable(CAP_LINUX_IMMUTABLE))
return -EPERM;
 
-   return 0;
+   /*
+* Now that we're done checking the new flags, flush all pending IO and
+* dirty mappings before setting S_IMMUTABLE on an inode via
+* FS_IOC_SETFLAGS.  If the flush fails we'll clear the flag before
+* returning error.
+*/
+   if (!S_ISREG(inode->i_mode) || IS_IMMUTABLE(inode) ||
+   !(flags & FS_IMMUTABLE_FL))
+   return 0;
+
+   inode_set_flags(inode, S_IMMUTABLE, S_IMMUTABLE);
+   ret = inode_drain_writes(inode);
+   if (ret)
+   inode_set_flags(inode, 0, S_IMMUTABLE);
+   return ret;
 }
 EXPORT_SYMBOL(vfs_ioc_setflags_prepare);
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 48322bfd7299..51266c9dbadc 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3561,4 +3561,15 @@ int vfs_ioc_setflags_prepare(struct inode *inode, 
unsigned int oldflags,
 int vfs_ioc_fssetxattr_check(struct inode *inode, const struct fsxattr *old_fa,
 struct fsxattr *fa);
 
+/*
+ * Flush file data before changing attributes.  Caller must hold any locks
+ * required to prevent further writes to this file until we're done setting
+ * flags.
+ */
+static inline int inode_drain_writes(struct inode *inode)
+{
+   inode_dio_wait(inode);
+   return filemap_write_and_wait(inode->i_mapping);
+}
+
 #endif /* _LINUX_FS_H */



___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH 5/5] vfs: only allow FSSETXATTR to set DAX flag on files and dirs

2019-06-25 Thread Darrick J. Wong 
From: Darrick J. Wong 

The DAX flag only applies to files and directories, so don't let it get
set for other types of files.

Signed-off-by: Darrick J. Wong 
---
 fs/inode.c |8 
 1 file changed, 8 insertions(+)


diff --git a/fs/inode.c b/fs/inode.c
index 670d5408d022..f08711b34341 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2259,6 +2259,14 @@ int vfs_ioc_fssetxattr_check(struct inode *inode, const 
struct fsxattr *old_fa,
!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
return -EINVAL;
 
+   /*
+* It is only valid to set the DAX flag on regular files and
+* directories on filesystems.
+*/
+   if ((fa->fsx_xflags & FS_XFLAG_DAX) &&
+   !(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)))
+   return -EINVAL;
+
/* Extent size hints of zero turn off the flags. */
if (fa->fsx_extsize == 0)
fa->fsx_xflags &= ~(FS_XFLAG_EXTSIZE | FS_XFLAG_EXTSZINHERIT);



___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH 2/5] vfs: create a generic checking function for FS_IOC_FSSETXATTR

2019-06-25 Thread Darrick J. Wong 
From: Darrick J. Wong 

Create a generic checking function for the incoming FS_IOC_FSSETXATTR
fsxattr values so that we can standardize some of the implementation
behaviors.

Signed-off-by: Darrick J. Wong 
Reviewed-by: Jan Kara 
---
 fs/btrfs/ioctl.c   |   18 ++---
 fs/ext4/ioctl.c|   27 +---
 fs/f2fs/file.c |   28 ++---
 fs/inode.c |   23 +
 fs/xfs/xfs_ioctl.c |   70 ++--
 include/linux/fs.h |3 ++
 6 files changed, 112 insertions(+), 57 deletions(-)


diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index d3d9b4abb09b..0f5af7c5f66b 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -373,10 +373,9 @@ static int check_xflags(unsigned int flags)
 static int btrfs_ioctl_fsgetxattr(struct file *file, void __user *arg)
 {
struct btrfs_inode *binode = BTRFS_I(file_inode(file));
-   struct fsxattr fa;
-
-   memset(&fa, 0, sizeof(fa));
-   fa.fsx_xflags = btrfs_inode_flags_to_xflags(binode->flags);
+   struct fsxattr fa = {
+   .fsx_xflags = btrfs_inode_flags_to_xflags(binode->flags),
+   };
 
if (copy_to_user(arg, &fa, sizeof(fa)))
return -EFAULT;
@@ -391,6 +390,9 @@ static int btrfs_ioctl_fssetxattr(struct file *file, void 
__user *arg)
struct btrfs_root *root = binode->root;
struct btrfs_trans_handle *trans;
struct fsxattr fa;
+   struct fsxattr old_fa = {
+   .fsx_xflags = btrfs_inode_flags_to_xflags(binode->flags),
+   };
unsigned old_flags;
unsigned old_i_flags;
int ret = 0;
@@ -421,13 +423,9 @@ static int btrfs_ioctl_fssetxattr(struct file *file, void 
__user *arg)
old_flags = binode->flags;
old_i_flags = inode->i_flags;
 
-   /* We need the capabilities to change append-only or immutable inode */
-   if (((old_flags & (BTRFS_INODE_APPEND | BTRFS_INODE_IMMUTABLE)) ||
-(fa.fsx_xflags & (FS_XFLAG_APPEND | FS_XFLAG_IMMUTABLE))) &&
-   !capable(CAP_LINUX_IMMUTABLE)) {
-   ret = -EPERM;
+   ret = vfs_ioc_fssetxattr_check(inode, &old_fa, &fa);
+   if (ret)
goto out_unlock;
-   }
 
if (fa.fsx_xflags & FS_XFLAG_SYNC)
binode->flags |= BTRFS_INODE_SYNC;
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 272b6e44191b..ebcc173d1e7d 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -721,6 +721,17 @@ static int ext4_ioctl_check_project(struct inode *inode, 
struct fsxattr *fa)
return 0;
 }
 
+static void ext4_fill_fsxattr(struct inode *inode, struct fsxattr *fa)
+{
+   struct ext4_inode_info *ei = EXT4_I(inode);
+
+   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 = from_kprojid(&init_user_ns, ei->i_projid);
+}
+
 long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
struct inode *inode = file_inode(filp);
@@ -1087,15 +1098,9 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, 
unsigned long arg)
 
case EXT4_IOC_FSGETXATTR:
{
-   struct fsxattr fa;
-
-   memset(&fa, 0, sizeof(struct fsxattr));
-   fa.fsx_xflags = ext4_iflags_to_xflags(ei->i_flags & 
EXT4_FL_USER_VISIBLE);
+   struct fsxattr fa = { 0 };
 
-   if (ext4_has_feature_project(inode->i_sb)) {
-   fa.fsx_projid = (__u32)from_kprojid(&init_user_ns,
-   EXT4_I(inode)->i_projid);
-   }
+   ext4_fill_fsxattr(inode, &fa);
 
if (copy_to_user((struct fsxattr __user *)arg,
 &fa, sizeof(fa)))
@@ -1104,7 +1109,7 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, 
unsigned long arg)
}
case EXT4_IOC_FSSETXATTR:
{
-   struct fsxattr fa;
+   struct fsxattr fa, old_fa = { 0 };
int err;
 
if (copy_from_user(&fa, (struct fsxattr __user *)arg,
@@ -1127,7 +1132,11 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, 
unsigned long arg)
return err;
 
inode_lock(inode);
+   ext4_fill_fsxattr(inode, &old_fa);
err = ext4_ioctl_check_project(inode, &fa);
+   if (err)
+   goto out;
+   err = vfs_ioc_fssetxattr_check(inode, &old_fa, &fa);
if (err)
goto out;
flags = (ei->i_flags & ~EXT4_FL_XFLAG_VISIBLE) |
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 845ae6f43ebc..555b970f7945 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2773,19 +2773,24 @@ static inline unsigned long f2fs_xflags_to_iflags(__u32 
xflags)
return iflags;
 }
 
-static int f2fs_ioc_fsget

[f2fs-dev] [PATCH 4/5] vfs: teach vfs_ioc_fssetxattr_check to check extent size hints

2019-06-25 Thread Darrick J. Wong 
From: Darrick J. Wong 

Move the extent size hint checks that aren't xfs-specific to the vfs.

Signed-off-by: Darrick J. Wong 
Reviewed-by: Jan Kara 
---
 fs/inode.c |   18 +
 fs/xfs/xfs_ioctl.c |   70 ++--
 2 files changed, 47 insertions(+), 41 deletions(-)


diff --git a/fs/inode.c b/fs/inode.c
index c4f8fb16f633..670d5408d022 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2247,6 +2247,24 @@ int vfs_ioc_fssetxattr_check(struct inode *inode, const 
struct fsxattr *old_fa,
return -EINVAL;
}
 
+   /* Check extent size hints. */
+   if ((fa->fsx_xflags & FS_XFLAG_EXTSIZE) && !S_ISREG(inode->i_mode))
+   return -EINVAL;
+
+   if ((fa->fsx_xflags & FS_XFLAG_EXTSZINHERIT) &&
+   !S_ISDIR(inode->i_mode))
+   return -EINVAL;
+
+   if ((fa->fsx_xflags & FS_XFLAG_COWEXTSIZE) &&
+   !S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
+   return -EINVAL;
+
+   /* Extent size hints of zero turn off the flags. */
+   if (fa->fsx_extsize == 0)
+   fa->fsx_xflags &= ~(FS_XFLAG_EXTSIZE | FS_XFLAG_EXTSZINHERIT);
+   if (fa->fsx_cowextsize == 0)
+   fa->fsx_xflags &= ~FS_XFLAG_COWEXTSIZE;
+
return 0;
 }
 EXPORT_SYMBOL(vfs_ioc_fssetxattr_check);
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index d8c02b9905a7..011657bd50ca 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1201,39 +1201,31 @@ xfs_ioctl_setattr_check_extsize(
struct fsxattr  *fa)
 {
struct xfs_mount*mp = ip->i_mount;
-
-   if ((fa->fsx_xflags & FS_XFLAG_EXTSIZE) && !S_ISREG(VFS_I(ip)->i_mode))
-   return -EINVAL;
-
-   if ((fa->fsx_xflags & FS_XFLAG_EXTSZINHERIT) &&
-   !S_ISDIR(VFS_I(ip)->i_mode))
-   return -EINVAL;
+   xfs_extlen_tsize;
+   xfs_fsblock_t   extsize_fsb;
 
if (S_ISREG(VFS_I(ip)->i_mode) && ip->i_d.di_nextents &&
((ip->i_d.di_extsize << mp->m_sb.sb_blocklog) != fa->fsx_extsize))
return -EINVAL;
 
-   if (fa->fsx_extsize != 0) {
-   xfs_extlen_tsize;
-   xfs_fsblock_t   extsize_fsb;
-
-   extsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_extsize);
-   if (extsize_fsb > MAXEXTLEN)
-   return -EINVAL;
+   if (fa->fsx_extsize == 0)
+   return 0;
 
-   if (XFS_IS_REALTIME_INODE(ip) ||
-   (fa->fsx_xflags & FS_XFLAG_REALTIME)) {
-   size = mp->m_sb.sb_rextsize << mp->m_sb.sb_blocklog;
-   } else {
-   size = mp->m_sb.sb_blocksize;
-   if (extsize_fsb > mp->m_sb.sb_agblocks / 2)
-   return -EINVAL;
-   }
+   extsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_extsize);
+   if (extsize_fsb > MAXEXTLEN)
+   return -EINVAL;
 
-   if (fa->fsx_extsize % size)
+   if (XFS_IS_REALTIME_INODE(ip) ||
+   (fa->fsx_xflags & FS_XFLAG_REALTIME)) {
+   size = mp->m_sb.sb_rextsize << mp->m_sb.sb_blocklog;
+   } else {
+   size = mp->m_sb.sb_blocksize;
+   if (extsize_fsb > mp->m_sb.sb_agblocks / 2)
return -EINVAL;
-   } else
-   fa->fsx_xflags &= ~(FS_XFLAG_EXTSIZE | FS_XFLAG_EXTSZINHERIT);
+   }
+
+   if (fa->fsx_extsize % size)
+   return -EINVAL;
 
return 0;
 }
@@ -1259,6 +1251,8 @@ xfs_ioctl_setattr_check_cowextsize(
struct fsxattr  *fa)
 {
struct xfs_mount*mp = ip->i_mount;
+   xfs_extlen_tsize;
+   xfs_fsblock_t   cowextsize_fsb;
 
if (!(fa->fsx_xflags & FS_XFLAG_COWEXTSIZE))
return 0;
@@ -1267,25 +1261,19 @@ xfs_ioctl_setattr_check_cowextsize(
ip->i_d.di_version != 3)
return -EINVAL;
 
-   if (!S_ISREG(VFS_I(ip)->i_mode) && !S_ISDIR(VFS_I(ip)->i_mode))
-   return -EINVAL;
-
-   if (fa->fsx_cowextsize != 0) {
-   xfs_extlen_tsize;
-   xfs_fsblock_t   cowextsize_fsb;
+   if (fa->fsx_cowextsize == 0)
+   return 0;
 
-   cowextsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_cowextsize);
-   if (cowextsize_fsb > MAXEXTLEN)
-   return -EINVAL;
+   cowextsize_fsb = XFS_B_TO_FSB(mp, fa->fsx_cowextsize);
+   if (cowextsize_fsb > MAXEXTLEN)
+   return -EINVAL;
 
-   size = mp->m_sb.sb_blocksize;
-   if (cowextsize_fsb > mp->m_sb.sb_agblocks / 2)
-   return -EINVAL;
+   size = mp->m_sb.sb_blocksize;
+   if (cowextsize_fsb > mp->m_sb.sb_agblocks / 2)
+   return -EINVAL;
 
-   if (fa->fsx_cowextsize % size)
-   return -EINVAL;
-

[f2fs-dev] [PATCH 3/5] vfs: teach vfs_ioc_fssetxattr_check to check project id info

2019-06-25 Thread Darrick J. Wong 
From: Darrick J. Wong 

Standardize the project id checks for FSSETXATTR.

Signed-off-by: Darrick J. Wong 
Reviewed-by: Jan Kara 
---
 fs/ext4/ioctl.c|   27 ---
 fs/f2fs/file.c |   27 ---
 fs/inode.c |   13 +
 fs/xfs/xfs_ioctl.c |   15 ---
 4 files changed, 13 insertions(+), 69 deletions(-)


diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index ebcc173d1e7d..1e88c3af9a8d 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -697,30 +697,6 @@ static long ext4_ioctl_group_add(struct file *file,
return err;
 }
 
-static int ext4_ioctl_check_project(struct inode *inode, struct fsxattr *fa)
-{
-   /*
-* 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 (current_user_ns() == &init_user_ns)
-   return 0;
-
-   if (__kprojid_val(EXT4_I(inode)->i_projid) != fa->fsx_projid)
-   return -EINVAL;
-
-   if (ext4_test_inode_flag(inode, EXT4_INODE_PROJINHERIT)) {
-   if (!(fa->fsx_xflags & FS_XFLAG_PROJINHERIT))
-   return -EINVAL;
-   } else {
-   if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
-   return -EINVAL;
-   }
-
-   return 0;
-}
-
 static void ext4_fill_fsxattr(struct inode *inode, struct fsxattr *fa)
 {
struct ext4_inode_info *ei = EXT4_I(inode);
@@ -1133,9 +1109,6 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, 
unsigned long arg)
 
inode_lock(inode);
ext4_fill_fsxattr(inode, &old_fa);
-   err = ext4_ioctl_check_project(inode, &fa);
-   if (err)
-   goto out;
err = vfs_ioc_fssetxattr_check(inode, &old_fa, &fa);
if (err)
goto out;
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 555b970f7945..d6ed319388d6 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2797,30 +2797,6 @@ static int f2fs_ioc_fsgetxattr(struct file *filp, 
unsigned long arg)
return 0;
 }
 
-static int f2fs_ioctl_check_project(struct inode *inode, struct fsxattr *fa)
-{
-   /*
-* 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 (current_user_ns() == &init_user_ns)
-   return 0;
-
-   if (__kprojid_val(F2FS_I(inode)->i_projid) != fa->fsx_projid)
-   return -EINVAL;
-
-   if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL) {
-   if (!(fa->fsx_xflags & FS_XFLAG_PROJINHERIT))
-   return -EINVAL;
-   } else {
-   if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
-   return -EINVAL;
-   }
-
-   return 0;
-}
-
 static int f2fs_ioc_fssetxattr(struct file *filp, unsigned long arg)
 {
struct inode *inode = file_inode(filp);
@@ -2848,9 +2824,6 @@ static int f2fs_ioc_fssetxattr(struct file *filp, 
unsigned long arg)
return err;
 
inode_lock(inode);
-   err = f2fs_ioctl_check_project(inode, &fa);
-   if (err)
-   goto out;
 
f2fs_fill_fsxattr(inode, &old_fa);
err = vfs_ioc_fssetxattr_check(inode, &old_fa, &fa);
diff --git a/fs/inode.c b/fs/inode.c
index fdd6c5d3e48d..c4f8fb16f633 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2234,6 +2234,19 @@ int vfs_ioc_fssetxattr_check(struct inode *inode, const 
struct fsxattr *old_fa,
!capable(CAP_LINUX_IMMUTABLE))
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 (current_user_ns() != &init_user_ns) {
+   if (old_fa->fsx_projid != fa->fsx_projid)
+   return -EINVAL;
+   if ((old_fa->fsx_xflags ^ fa->fsx_xflags) &
+   FS_XFLAG_PROJINHERIT)
+   return -EINVAL;
+   }
+
return 0;
 }
 EXPORT_SYMBOL(vfs_ioc_fssetxattr_check);
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 08ec1e458865..d8c02b9905a7 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1299,21 +1299,6 @@ xfs_ioctl_setattr_check_projid(
if (fa->fsx_projid > (uint16_t)-1 &&
!xfs_sb_version_hasprojid32bit(&ip->i_mount->m_sb))
return -EINVAL;
-
-   /*
-* Project Quota ID state is only allowed to change from within the init
-* namespace. Enforce that restriction only if we are trying to change

[f2fs-dev] [PATCH 1/5] vfs: create a generic checking and prep function for FS_IOC_SETFLAGS

2019-06-25 Thread Darrick J. Wong 
From: Darrick J. Wong 

Create a generic function to check incoming FS_IOC_SETFLAGS flag values
and later prepare the inode for updates so that we can standardize the
implementations that follow ext4's flag values.

Signed-off-by: Darrick J. Wong 
Reviewed-by: Jan Kara 
Reviewed-by: Christoph Hellwig 
Acked-by: David Sterba 
---
 fs/btrfs/ioctl.c|   13 +
 fs/efivarfs/file.c  |   26 +-
 fs/ext2/ioctl.c |   16 
 fs/ext4/ioctl.c |   13 +++--
 fs/f2fs/file.c  |7 ---
 fs/gfs2/file.c  |   42 +-
 fs/hfsplus/ioctl.c  |   21 -
 fs/inode.c  |   24 
 fs/jfs/ioctl.c  |   22 +++---
 fs/nilfs2/ioctl.c   |9 ++---
 fs/ocfs2/ioctl.c|   13 +++--
 fs/orangefs/file.c  |   35 ++-
 fs/reiserfs/ioctl.c |   10 --
 fs/ubifs/ioctl.c|   13 +++--
 include/linux/fs.h  |3 +++
 15 files changed, 146 insertions(+), 121 deletions(-)


diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 6dafa857bbb9..d3d9b4abb09b 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -187,7 +187,7 @@ static int btrfs_ioctl_setflags(struct file *file, void 
__user *arg)
struct btrfs_inode *binode = BTRFS_I(inode);
struct btrfs_root *root = binode->root;
struct btrfs_trans_handle *trans;
-   unsigned int fsflags;
+   unsigned int fsflags, old_fsflags;
int ret;
const char *comp = NULL;
u32 binode_flags = binode->flags;
@@ -212,13 +212,10 @@ static int btrfs_ioctl_setflags(struct file *file, void 
__user *arg)
inode_lock(inode);
 
fsflags = btrfs_mask_fsflags_for_type(inode, fsflags);
-   if ((fsflags ^ btrfs_inode_flags_to_fsflags(binode->flags)) &
-   (FS_APPEND_FL | FS_IMMUTABLE_FL)) {
-   if (!capable(CAP_LINUX_IMMUTABLE)) {
-   ret = -EPERM;
-   goto out_unlock;
-   }
-   }
+   old_fsflags = btrfs_inode_flags_to_fsflags(binode->flags);
+   ret = vfs_ioc_setflags_prepare(inode, old_fsflags, fsflags);
+   if (ret)
+   goto out_unlock;
 
if (fsflags & FS_SYNC_FL)
binode_flags |= BTRFS_INODE_SYNC;
diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index 8e568428c88b..a3cc10b1bfe1 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -110,16 +110,22 @@ static ssize_t efivarfs_file_read(struct file *file, char 
__user *userbuf,
return size;
 }
 
-static int
-efivarfs_ioc_getxflags(struct file *file, void __user *arg)
+static inline unsigned int efivarfs_getflags(struct inode *inode)
 {
-   struct inode *inode = file->f_mapping->host;
unsigned int i_flags;
unsigned int flags = 0;
 
i_flags = inode->i_flags;
if (i_flags & S_IMMUTABLE)
flags |= FS_IMMUTABLE_FL;
+   return flags;
+}
+
+static int
+efivarfs_ioc_getxflags(struct file *file, void __user *arg)
+{
+   struct inode *inode = file->f_mapping->host;
+   unsigned int flags = efivarfs_getflags(inode);
 
if (copy_to_user(arg, &flags, sizeof(flags)))
return -EFAULT;
@@ -132,6 +138,7 @@ efivarfs_ioc_setxflags(struct file *file, void __user *arg)
struct inode *inode = file->f_mapping->host;
unsigned int flags;
unsigned int i_flags = 0;
+   unsigned int oldflags = efivarfs_getflags(inode);
int error;
 
if (!inode_owner_or_capable(inode))
@@ -143,9 +150,6 @@ efivarfs_ioc_setxflags(struct file *file, void __user *arg)
if (flags & ~FS_IMMUTABLE_FL)
return -EOPNOTSUPP;
 
-   if (!capable(CAP_LINUX_IMMUTABLE))
-   return -EPERM;
-
if (flags & FS_IMMUTABLE_FL)
i_flags |= S_IMMUTABLE;
 
@@ -155,12 +159,16 @@ efivarfs_ioc_setxflags(struct file *file, void __user 
*arg)
return error;
 
inode_lock(inode);
+
+   error = vfs_ioc_setflags_prepare(inode, oldflags, flags);
+   if (error)
+   goto out;
+
inode_set_flags(inode, i_flags, S_IMMUTABLE);
+out:
inode_unlock(inode);
-
mnt_drop_write_file(file);
-
-   return 0;
+   return error;
 }
 
 static long
diff --git a/fs/ext2/ioctl.c b/fs/ext2/ioctl.c
index 0367c0039e68..1b853fb0b163 100644
--- a/fs/ext2/ioctl.c
+++ b/fs/ext2/ioctl.c
@@ -60,18 +60,10 @@ long ext2_ioctl(struct file *filp, unsigned int cmd, 
unsigned long arg)
}
oldflags = ei->i_flags;
 
-   /*
-* The IMMUTABLE and APPEND_ONLY flags can only be changed by
-* the relevant capability.
-*
-* This test looks nicer. Thanks to Pauline Middelink
-*/
-   if ((flags ^ oldflags) & (EXT2_APPEND_FL | EXT2_IMMUTABLE_FL)) {
-

[f2fs-dev] [PATCH v3 0/5] vfs: clean up SETFLAGS and FSSETXATTR option processing

2019-06-25 Thread Darrick J. Wong 
Hi all,

The FS_IOC_SETFLAGS and FS_IOC_FSSETXATTR ioctls were promoted from ext4
and XFS, respectively, into the VFS.  However, we didn't promote any of
the parameter checking code from those filesystems, which lead to a mess
where each filesystem open-codes whatever parameter checks they want and
the behavior across filesystems is no longer consistent.

Therefore, create some generic checking functions in the VFS and remove
all the open-coded pieces in each filesystem.  This preserves the
current behavior where a filesystem can choose to ignore fields it
doesn't understand.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This has been lightly tested with fstests.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=file-ioctl-cleanups


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS

2019-06-25 Thread Darrick J. Wong 
On Tue, Jun 25, 2019 at 07:12:54PM +0200, David Sterba wrote:
> On Fri, Jun 21, 2019 at 04:56:21PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong 
> > 
> > Create a generic checking function for the incoming FS_IOC_SETFLAGS flag
> > values so that we can standardize the implementations that follow ext4's
> > flag values.
> 
> I checked a few samples what's the type of the flags, there are unsigned
> types while the proposed VFS functions take signed type.
> 
> > +int vfs_ioc_setflags_check(struct inode *inode, int oldflags, int flags);
> 
> Specifically ext4 uses unsigned type and his was the original API that
> got copied so I'd think that it should unsigned everywhere.

Yeah, I'll change it.

> >  fs/btrfs/ioctl.c|   13 +
> 
> For the btrfs bits
> 
> Acked-by: David Sterba 
> 
> and besides the signedness, the rest of the changes look good to me.

Thanks for the look around!  I'll have a new revision with all changes
out by the end of the day. :)

--D


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v4 0/7] vfs: make immutable files actually immutable

2019-06-25 Thread Darrick J. Wong 
On Tue, Jun 25, 2019 at 03:36:31AM -0700, Christoph Hellwig wrote:
> On Fri, Jun 21, 2019 at 04:56:50PM -0700, Darrick J. Wong wrote:
> > Hi all,
> > 
> > The chattr(1) manpage has this to say about the immutable bit that
> > system administrators can set on files:
> > 
> > "A file with the 'i' attribute cannot be modified: it cannot be deleted
> > or renamed, no link can be created to this file, most of the file's
> > metadata can not be modified, and the file can not be opened in write
> > mode."
> > 
> > Given the clause about how the file 'cannot be modified', it is
> > surprising that programs holding writable file descriptors can continue
> > to write to and truncate files after the immutable flag has been set,
> > but they cannot call other things such as utimes, fallocate, unlink,
> > link, setxattr, or reflink.
> 
> I still think living code beats documentation.  And as far as I can
> tell the immutable bit never behaved as documented or implemented
> in this series on Linux, and it originated on Linux.

The behavior has never been consistent -- since the beginning you can
keep write()ing to a fd after the file becomes immutable, but you can't
ftruncate() it.  I would really like to make the behavior consistent.
Since the authors of nearly every new system call and ioctl since the
late 1990s have interpreted S_IMMUTABLE to mean "immutable takes effect
everywhere immediately" I resolved the inconsistency in favor of that
interpretation.

I asked Ted what he thought that that userspace having the ability to
continue writing to an immutable file, and he thought it was an
implementation bug that had been there for 25 years.  Even he thought
that immutable should take effect immediately everywhere.

> If you want  hard cut off style immutable flag it should really be a
> new API, but I don't really see the point.  It isn't like the usual
> workload is to set the flag on a file actively in use.

FWIW Ted also thought that since it's rare for admins to set +i on a
file actively in use we could just change it without forcing everyone
onto a new api.

--D


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 16/16] f2fs: add fs-verity support

2019-06-25 Thread Eric Biggers 
Hi Chao, thanks for the review.

On Tue, Jun 25, 2019 at 03:55:57PM +0800, Chao Yu wrote:
> Hi Eric,
> 
> On 2019/6/21 4:50, Eric Biggers wrote:
> > +static int f2fs_begin_enable_verity(struct file *filp)
> > +{
> > +   struct inode *inode = file_inode(filp);
> > +   int err;
> > +
> 
> I think we'd better add condition here (under inode lock) to disallow enabling
> verity on atomic/volatile inode, as we may fail to write merkle tree data due 
> to
> atomic/volatile inode's special writeback method.
> 

Yes, I'll add the following:

if (f2fs_is_atomic_file(inode) || f2fs_is_volatile_file(inode))
return -EOPNOTSUPP;

> > +   err = f2fs_convert_inline_inode(inode);
> > +   if (err)
> > +   return err;
> > +
> > +   err = dquot_initialize(inode);
> > +   if (err)
> > +   return err;
> 
> We can get rid of dquot_initialize() here, since f2fs_file_open() ->
> dquot_file_open() should has initialized quota entry previously, right?

We still need it because dquot_file_open() only calls dquot_initialize() if the
file is being opened for writing.  But here the file descriptor is readonly.
I'll add a comment explaining this here and in the ext4 equivalent.

- Eric


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/4] vfs: create a generic checking function for FS_IOC_FSSETXATTR

2019-06-25 Thread David Sterba 
On Tue, Jun 25, 2019 at 10:16:16AM -0700, Darrick J. Wong wrote:
> On Tue, Jun 25, 2019 at 07:02:48PM +0200, David Sterba wrote:
> > On Tue, Jun 25, 2019 at 03:57:25AM -0700, Christoph Hellwig wrote:
> > > On Fri, Jun 21, 2019 at 04:56:29PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong 
> > > > 
> > > > Create a generic checking function for the incoming FS_IOC_FSSETXATTR
> > > > fsxattr values so that we can standardize some of the implementation
> > > > behaviors.
> > > > 
> > > > Signed-off-by: Darrick J. Wong 
> > > > Reviewed-by: Jan Kara 
> > > > ---
> > > >  fs/btrfs/ioctl.c   |   21 +---
> > > >  fs/ext4/ioctl.c|   27 ++--
> > > >  fs/f2fs/file.c |   26 ++-
> > > >  fs/inode.c |   17 +
> > > >  fs/xfs/xfs_ioctl.c |   70 
> > > > ++--
> > > >  include/linux/fs.h |3 ++
> > > >  6 files changed, 111 insertions(+), 53 deletions(-)
> > > > 
> > > > 
> > > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > > > index f408aa93b0cf..7ddda5b4b6a6 100644
> > > > --- a/fs/btrfs/ioctl.c
> > > > +++ b/fs/btrfs/ioctl.c
> > > > @@ -366,6 +366,13 @@ static int check_xflags(unsigned int flags)
> > > > return 0;
> > > >  }
> > > >  
> > > > +static void __btrfs_ioctl_fsgetxattr(struct btrfs_inode *binode,
> > > > +struct fsxattr *fa)
> > > > +{
> > > > +   memset(fa, 0, sizeof(*fa));
> > > > +   fa->fsx_xflags = btrfs_inode_flags_to_xflags(binode->flags);
> > > 
> > > Is there really much of a point in this helper? Epeciall as
> > > the zeroing could easily be done in the variable declaration
> > > line using
> > > 
> > >   struct fsxattr fa = { };
> > 
> > Agreed, not counting the initialization the wrapper is merely another
> > name for btrfs_inode_flags_to_xflags. I also find it slightly confusing
> > that __btrfs_ioctl_fsgetxattr name is too close to the ioctl callback
> > implementation btrfs_ioctl_fsgetxattr but only does some initialization.
> 
> Ok; it's easily enough changed to:
> 
>   struct fsxattr old_fa = {
>   .fsx_xflags = btrfs_inode_flags_to_xflags(binode->flags),
>   };

Works for me, thanks.


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/4] vfs: create a generic checking function for FS_IOC_FSSETXATTR

2019-06-25 Thread Darrick J. Wong 
On Tue, Jun 25, 2019 at 07:02:48PM +0200, David Sterba wrote:
> On Tue, Jun 25, 2019 at 03:57:25AM -0700, Christoph Hellwig wrote:
> > On Fri, Jun 21, 2019 at 04:56:29PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong 
> > > 
> > > Create a generic checking function for the incoming FS_IOC_FSSETXATTR
> > > fsxattr values so that we can standardize some of the implementation
> > > behaviors.
> > > 
> > > Signed-off-by: Darrick J. Wong 
> > > Reviewed-by: Jan Kara 
> > > ---
> > >  fs/btrfs/ioctl.c   |   21 +---
> > >  fs/ext4/ioctl.c|   27 ++--
> > >  fs/f2fs/file.c |   26 ++-
> > >  fs/inode.c |   17 +
> > >  fs/xfs/xfs_ioctl.c |   70 
> > > ++--
> > >  include/linux/fs.h |3 ++
> > >  6 files changed, 111 insertions(+), 53 deletions(-)
> > > 
> > > 
> > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > > index f408aa93b0cf..7ddda5b4b6a6 100644
> > > --- a/fs/btrfs/ioctl.c
> > > +++ b/fs/btrfs/ioctl.c
> > > @@ -366,6 +366,13 @@ static int check_xflags(unsigned int flags)
> > >   return 0;
> > >  }
> > >  
> > > +static void __btrfs_ioctl_fsgetxattr(struct btrfs_inode *binode,
> > > +  struct fsxattr *fa)
> > > +{
> > > + memset(fa, 0, sizeof(*fa));
> > > + fa->fsx_xflags = btrfs_inode_flags_to_xflags(binode->flags);
> > 
> > Is there really much of a point in this helper? Epeciall as
> > the zeroing could easily be done in the variable declaration
> > line using
> > 
> > struct fsxattr fa = { };
> 
> Agreed, not counting the initialization the wrapper is merely another
> name for btrfs_inode_flags_to_xflags. I also find it slightly confusing
> that __btrfs_ioctl_fsgetxattr name is too close to the ioctl callback
> implementation btrfs_ioctl_fsgetxattr but only does some initialization.

Ok; it's easily enough changed to:

struct fsxattr old_fa = {
.fsx_xflags = btrfs_inode_flags_to_xflags(binode->flags),
};

--D


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS

2019-06-25 Thread David Sterba 
On Fri, Jun 21, 2019 at 04:56:21PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong 
> 
> Create a generic checking function for the incoming FS_IOC_SETFLAGS flag
> values so that we can standardize the implementations that follow ext4's
> flag values.

I checked a few samples what's the type of the flags, there are unsigned
types while the proposed VFS functions take signed type.

> +int vfs_ioc_setflags_check(struct inode *inode, int oldflags, int flags);

Specifically ext4 uses unsigned type and his was the original API that
got copied so I'd think that it should unsigned everywhere.

>  fs/btrfs/ioctl.c|   13 +

For the btrfs bits

Acked-by: David Sterba 

and besides the signedness, the rest of the changes look good to me.


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/4] vfs: create a generic checking function for FS_IOC_FSSETXATTR

2019-06-25 Thread David Sterba 
On Tue, Jun 25, 2019 at 03:57:25AM -0700, Christoph Hellwig wrote:
> On Fri, Jun 21, 2019 at 04:56:29PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong 
> > 
> > Create a generic checking function for the incoming FS_IOC_FSSETXATTR
> > fsxattr values so that we can standardize some of the implementation
> > behaviors.
> > 
> > Signed-off-by: Darrick J. Wong 
> > Reviewed-by: Jan Kara 
> > ---
> >  fs/btrfs/ioctl.c   |   21 +---
> >  fs/ext4/ioctl.c|   27 ++--
> >  fs/f2fs/file.c |   26 ++-
> >  fs/inode.c |   17 +
> >  fs/xfs/xfs_ioctl.c |   70 
> > ++--
> >  include/linux/fs.h |3 ++
> >  6 files changed, 111 insertions(+), 53 deletions(-)
> > 
> > 
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index f408aa93b0cf..7ddda5b4b6a6 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -366,6 +366,13 @@ static int check_xflags(unsigned int flags)
> > return 0;
> >  }
> >  
> > +static void __btrfs_ioctl_fsgetxattr(struct btrfs_inode *binode,
> > +struct fsxattr *fa)
> > +{
> > +   memset(fa, 0, sizeof(*fa));
> > +   fa->fsx_xflags = btrfs_inode_flags_to_xflags(binode->flags);
> 
> Is there really much of a point in this helper? Epeciall as
> the zeroing could easily be done in the variable declaration
> line using
> 
>   struct fsxattr fa = { };

Agreed, not counting the initialization the wrapper is merely another
name for btrfs_inode_flags_to_xflags. I also find it slightly confusing
that __btrfs_ioctl_fsgetxattr name is too close to the ioctl callback
implementation btrfs_ioctl_fsgetxattr but only does some initialization.


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 4/4] vfs: teach vfs_ioc_fssetxattr_check to check extent size hints

2019-06-25 Thread Christoph Hellwig 
On Fri, Jun 21, 2019 at 04:56:45PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong 
> 
> Move the extent size hint checks that aren't xfs-specific to the vfs.
> 
> Signed-off-by: Darrick J. Wong 
> Reviewed-by: Jan Kara 

Looks good,

Reviewed-by: Christoph Hellwig 


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 3/4] fs: teach vfs_ioc_fssetxattr_check to check project id info

2019-06-25 Thread Christoph Hellwig 
On Fri, Jun 21, 2019 at 04:56:37PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong 
> 
> Standardize the project id checks for FSSETXATTR.
> 
> Signed-off-by: Darrick J. Wong 
> Reviewed-by: Jan Kara 

Looks good,

Reviewed-by: Christoph Hellwig 


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/4] vfs: create a generic checking function for FS_IOC_FSSETXATTR

2019-06-25 Thread Christoph Hellwig 
On Fri, Jun 21, 2019 at 04:56:29PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong 
> 
> Create a generic checking function for the incoming FS_IOC_FSSETXATTR
> fsxattr values so that we can standardize some of the implementation
> behaviors.
> 
> Signed-off-by: Darrick J. Wong 
> Reviewed-by: Jan Kara 
> ---
>  fs/btrfs/ioctl.c   |   21 +---
>  fs/ext4/ioctl.c|   27 ++--
>  fs/f2fs/file.c |   26 ++-
>  fs/inode.c |   17 +
>  fs/xfs/xfs_ioctl.c |   70 
> ++--
>  include/linux/fs.h |3 ++
>  6 files changed, 111 insertions(+), 53 deletions(-)
> 
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index f408aa93b0cf..7ddda5b4b6a6 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -366,6 +366,13 @@ static int check_xflags(unsigned int flags)
>   return 0;
>  }
>  
> +static void __btrfs_ioctl_fsgetxattr(struct btrfs_inode *binode,
> +  struct fsxattr *fa)
> +{
> + memset(fa, 0, sizeof(*fa));
> + fa->fsx_xflags = btrfs_inode_flags_to_xflags(binode->flags);

Is there really much of a point in this helper? Epeciall as
the zeroing could easily be done in the variable declaration
line using

struct fsxattr fa = { };

> + memset(fa, 0, sizeof(struct fsxattr));
> + fa->fsx_xflags = ext4_iflags_to_xflags(ei->i_flags & 
> EXT4_FL_USER_VISIBLE);

Overly lone line.

> + if (ext4_has_feature_project(inode->i_sb)) {
> + fa->fsx_projid = (__u32)from_kprojid(&init_user_ns,
> + ei->i_projid);

The cast here looks bogus.  Same comment for f2fs.


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 1/4] vfs: create a generic checking function for FS_IOC_SETFLAGS

2019-06-25 Thread Christoph Hellwig 
On Fri, Jun 21, 2019 at 04:56:21PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong 
> 
> Create a generic checking function for the incoming FS_IOC_SETFLAGS flag
> values so that we can standardize the implementations that follow ext4's
> flag values.
> 
> Signed-off-by: Darrick J. Wong 
> Reviewed-by: Jan Kara 

Looks fine:

Reviewed-by: Christoph Hellwig 


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v4 0/7] vfs: make immutable files actually immutable

2019-06-25 Thread Christoph Hellwig 
On Fri, Jun 21, 2019 at 04:56:50PM -0700, Darrick J. Wong wrote:
> Hi all,
> 
> The chattr(1) manpage has this to say about the immutable bit that
> system administrators can set on files:
> 
> "A file with the 'i' attribute cannot be modified: it cannot be deleted
> or renamed, no link can be created to this file, most of the file's
> metadata can not be modified, and the file can not be opened in write
> mode."
> 
> Given the clause about how the file 'cannot be modified', it is
> surprising that programs holding writable file descriptors can continue
> to write to and truncate files after the immutable flag has been set,
> but they cannot call other things such as utimes, fallocate, unlink,
> link, setxattr, or reflink.

I still think living code beats documentation.  And as far as I can
tell the immutable bit never behaved as documented or implemented
in this series on Linux, and it originated on Linux.

If you want  hard cut off style immutable flag it should really be a
new API, but I don't really see the point.  It isn't like the usual
workload is to set the flag on a file actively in use.


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v5 16/16] f2fs: add fs-verity support

2019-06-25 Thread Chao Yu 
Hi Eric,

On 2019/6/21 4:50, Eric Biggers wrote:
> +static int f2fs_begin_enable_verity(struct file *filp)
> +{
> + struct inode *inode = file_inode(filp);
> + int err;
> +

I think we'd better add condition here (under inode lock) to disallow enabling
verity on atomic/volatile inode, as we may fail to write merkle tree data due to
atomic/volatile inode's special writeback method.

> + err = f2fs_convert_inline_inode(inode);
> + if (err)
> + return err;
> +
> + err = dquot_initialize(inode);
> + if (err)
> + return err;

We can get rid of dquot_initialize() here, since f2fs_file_open() ->
dquot_file_open() should has initialized quota entry previously, right?

Thanks,

> +
> + set_inode_flag(inode, FI_VERITY_IN_PROGRESS);
> + return 0;
> +}
> +


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [Ocfs2-devel] [PATCH 2/7] vfs: flush and wait for io when setting the immutable flag via SETFLAGS

2019-06-25 Thread Jan Kara 
On Mon 24-06-19 20:04:39, Darrick J. Wong wrote:
> On Mon, Jun 24, 2019 at 02:58:17PM -0700, Darrick J. Wong wrote:
> > On Mon, Jun 24, 2019 at 01:37:37PM +0200, Jan Kara wrote:
> > > On Fri 21-06-19 16:57:07, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong 
> > > > 
> > > > When we're using FS_IOC_SETFLAGS to set the immutable flag on a file, we
> > > > need to ensure that userspace can't continue to write the file after the
> > > > file becomes immutable.  To make that happen, we have to flush all the
> > > > dirty pagecache pages to disk to ensure that we can fail a page fault on
> > > > a mmap'd region, wait for pending directio to complete, and hope the
> > > > caller locked out any new writes by holding the inode lock.
> > > > 
> > > > Signed-off-by: Darrick J. Wong 
> > > 
> > > Seeing the way this worked out, is there a reason to have separate
> > > vfs_ioc_setflags_flush_data() instead of folding the functionality in
> > > vfs_ioc_setflags_check() (possibly renaming it to
> > > vfs_ioc_setflags_prepare() to indicate it does already some changes)? I
> > > don't see any place that would need these two separated...
> > 
> > XFS needs them to be separated.
> > 
> > If we even /think/ that we're going to be setting the immutable flag
> > then we need to grab the IOLOCK and the MMAPLOCK to prevent further
> > writes while we drain all the directio writes and dirty data.  IO
> > completions for the write draining can take the ILOCK, which means that
> > we can't have grabbed it yet.
> > 
> > Next, we grab the ILOCK so we can check the new flags against the inode
> > and then update the inode core.
> > 
> > For most filesystems I think it suffices to inode_lock and then do both,
> > though.
> 
> Heh, lol, that applies to fssetxattr, not to setflags, because xfs
> setflags implementation open-codes the relevant fssetxattr pieces.
> So for setflags we can combine both parts into a single _prepare
> function.

Yeah. Also for fssetxattr we could use the prepare helper at least for
ext4, f2fs, and btrfs where the situation isn't so complex as for xfs to
save some boilerplate code.

Honza

> > > > +/*
> > > > + * Flush all pending IO and dirty mappings before setting S_IMMUTABLE 
> > > > on an
> > > > + * inode via FS_IOC_SETFLAGS.  If the flush fails we'll clear the flag 
> > > > before
> > > > + * returning error.
> > > > + *
> > > > + * Note: the caller should be holding i_mutex, or else be sure that
> > > > + * they have exclusive access to the inode structure.
> > > > + */
> > > > +static inline int vfs_ioc_setflags_flush_data(struct inode *inode, int 
> > > > flags)
> > > > +{
> > > > +   int ret;
> > > > +
> > > > +   if (!vfs_ioc_setflags_need_flush(inode, flags))
> > > > +   return 0;
> > > > +
> > > > +   inode_set_flags(inode, S_IMMUTABLE, S_IMMUTABLE);
> > > > +   ret = inode_flush_data(inode);
> > > > +   if (ret)
> > > > +   inode_set_flags(inode, 0, S_IMMUTABLE);
> > > > +   return ret;
> > > > +}
> > > 
> > > Also this sets S_IMMUTABLE whenever vfs_ioc_setflags_need_flush() returns
> > > true. That is currently the right thing but seems like a landmine waiting
> > > to trip? So I'd just drop the vfs_ioc_setflags_need_flush() abstraction to
> > > make it clear what's going on.
> > 
> > Ok.
> > 
> > --D
> > 
> > > 
> > >   Honza
> > > -- 
> > > Jan Kara 
> > > SUSE Labs, CR
> > 
> > ___
> > Ocfs2-devel mailing list
> > ocfs2-de...@oss.oracle.com
> > https://oss.oracle.com/mailman/listinfo/ocfs2-devel
-- 
Jan Kara 
SUSE Labs, CR


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel