[2/3] xfs: reset page mappings after setting immutable
diff mbox series

Message ID 155379544747.24796.1807309281507099911.stgit@magnolia
State New
Headers show
Series
  • vfs: make immutable files actually immutable
Related show

Commit Message

Darrick J. Wong March 28, 2019, 5:50 p.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

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."

This means that we need to flush the page cache when setting the
immutable flag so that programs cannot continue to write to writable
mappings.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_ioctl.c |   63 +++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 20 deletions(-)

Comments

Dave Chinner March 28, 2019, 9:21 p.m. UTC | #1
On Thu, Mar 28, 2019 at 10:50:47AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> 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."
> 
> This means that we need to flush the page cache when setting the

nit: flush and invalidate the page cache.

> immutable flag so that programs cannot continue to write to writable
> mappings.

Do we even need to invalidate the page cache for this? i.e. we've
cleaned the pages so that any new write to them will fault,
that will see the immutable flag via ->page_mkwrite and then the
app should segv, right?

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_ioctl.c |   63 +++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 43 insertions(+), 20 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 6ecdbb3af7de..2bd1c5ab5008 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -998,6 +998,37 @@ xfs_diflags_to_linux(
>  #endif
>  }
>  
> +static int
> +xfs_ioctl_setattr_flush(
> +	struct xfs_inode	*ip,
> +	int			*join_flags)
> +{
> +	struct inode		*inode = VFS_I(ip);
> +	int			error;
> +
> +	if (S_ISDIR(inode->i_mode))
> +		return 0;
> +	if ((*join_flags) & (XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL))
> +		return 0;
> +
> +	/* lock, flush and invalidate mapping in preparation for flag change */
> +	xfs_ilock(ip, XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL);
> +	error = filemap_write_and_wait(inode->i_mapping);
> +	if (error)
> +		goto out_unlock;
> +	error = invalidate_inode_pages2(inode->i_mapping);
> +	if (error)
> +		goto out_unlock;
> +
> +	*join_flags = XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL;
> +	return 0;
> +
> +out_unlock:
> +	xfs_iunlock(ip, XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL);
> +	return error;
> +
> +}

Doesn't wait for direct IO to drain. Wouldn't it be better to do
this?

	lock()
	xfs_flush_unmap_range(ip, 0, XFS_SIZE(ip));
	unlock()

Otherwise looks ok.

Cheers,

Dave.
Darrick J. Wong April 5, 2019, 12:29 a.m. UTC | #2
On Fri, Mar 29, 2019 at 08:21:47AM +1100, Dave Chinner wrote:
> On Thu, Mar 28, 2019 at 10:50:47AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > 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."
> > 
> > This means that we need to flush the page cache when setting the
> 
> nit: flush and invalidate the page cache.
> 
> > immutable flag so that programs cannot continue to write to writable
> > mappings.
> 
> Do we even need to invalidate the page cache for this? i.e. we've
> cleaned the pages so that any new write to them will fault,
> that will see the immutable flag via ->page_mkwrite and then the
> app should segv, right?

Yeah, we only need to flush the pages.  No need to invalidate.

> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_ioctl.c |   63 +++++++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 43 insertions(+), 20 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 6ecdbb3af7de..2bd1c5ab5008 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -998,6 +998,37 @@ xfs_diflags_to_linux(
> >  #endif
> >  }
> >  
> > +static int
> > +xfs_ioctl_setattr_flush(
> > +	struct xfs_inode	*ip,
> > +	int			*join_flags)
> > +{
> > +	struct inode		*inode = VFS_I(ip);
> > +	int			error;
> > +
> > +	if (S_ISDIR(inode->i_mode))
> > +		return 0;
> > +	if ((*join_flags) & (XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL))
> > +		return 0;
> > +
> > +	/* lock, flush and invalidate mapping in preparation for flag change */
> > +	xfs_ilock(ip, XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL);
> > +	error = filemap_write_and_wait(inode->i_mapping);
> > +	if (error)
> > +		goto out_unlock;
> > +	error = invalidate_inode_pages2(inode->i_mapping);
> > +	if (error)
> > +		goto out_unlock;
> > +
> > +	*join_flags = XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL;
> > +	return 0;
> > +
> > +out_unlock:
> > +	xfs_iunlock(ip, XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL);
> > +	return error;
> > +
> > +}
> 
> Doesn't wait for direct IO to drain. Wouldn't it be better to do
> this?
> 
> 	lock()
> 	xfs_flush_unmap_range(ip, 0, XFS_SIZE(ip));
> 	unlock()

But if we only need to filemap_write_and_wait, then calling flush_unmap
(which also calls truncate_pagecache_range) is too much work.

--D

> 
> Otherwise looks ok.
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Patch
diff mbox series

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 6ecdbb3af7de..2bd1c5ab5008 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -998,6 +998,37 @@  xfs_diflags_to_linux(
 #endif
 }
 
+static int
+xfs_ioctl_setattr_flush(
+	struct xfs_inode	*ip,
+	int			*join_flags)
+{
+	struct inode		*inode = VFS_I(ip);
+	int			error;
+
+	if (S_ISDIR(inode->i_mode))
+		return 0;
+	if ((*join_flags) & (XFS_IOLOCK_EXCL | XFS_MMAPLOCK_EXCL))
+		return 0;
+
+	/* lock, flush and invalidate mapping in preparation for flag change */
+	xfs_ilock(ip, XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL);
+	error = filemap_write_and_wait(inode->i_mapping);
+	if (error)
+		goto out_unlock;
+	error = invalidate_inode_pages2(inode->i_mapping);
+	if (error)
+		goto out_unlock;
+
+	*join_flags = XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL;
+	return 0;
+
+out_unlock:
+	xfs_iunlock(ip, XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL);
+	return error;
+
+}
+
 static int
 xfs_ioctl_setattr_xflags(
 	struct xfs_trans	*tp,
@@ -1067,7 +1098,6 @@  xfs_ioctl_setattr_dax_invalidate(
 {
 	struct inode		*inode = VFS_I(ip);
 	struct super_block	*sb = inode->i_sb;
-	int			error;
 
 	*join_flags = 0;
 
@@ -1092,25 +1122,7 @@  xfs_ioctl_setattr_dax_invalidate(
 	if (!(fa->fsx_xflags & FS_XFLAG_DAX) && !IS_DAX(inode))
 		return 0;
 
-	if (S_ISDIR(inode->i_mode))
-		return 0;
-
-	/* lock, flush and invalidate mapping in preparation for flag change */
-	xfs_ilock(ip, XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL);
-	error = filemap_write_and_wait(inode->i_mapping);
-	if (error)
-		goto out_unlock;
-	error = invalidate_inode_pages2(inode->i_mapping);
-	if (error)
-		goto out_unlock;
-
-	*join_flags = XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL;
-	return 0;
-
-out_unlock:
-	xfs_iunlock(ip, XFS_MMAPLOCK_EXCL | XFS_IOLOCK_EXCL);
-	return error;
-
+	return xfs_ioctl_setattr_flush(ip, join_flags);
 }
 
 /*
@@ -1356,6 +1368,17 @@  xfs_ioctl_setattr(
 	if (code)
 		goto error_free_dquots;
 
+	/*
+	 * If we are trying to set immutable then flush everything to disk to
+	 * force all writable memory mappings back through the pagefault
+	 * handler.
+	 */
+	if (!IS_IMMUTABLE(VFS_I(ip)) && (fa->fsx_xflags & FS_XFLAG_IMMUTABLE)) {
+		code = xfs_ioctl_setattr_flush(ip, &join_flags);
+		if (code)
+			goto error_free_dquots;
+	}
+
 	tp = xfs_ioctl_setattr_get_trans(ip, join_flags);
 	if (IS_ERR(tp)) {
 		code = PTR_ERR(tp);