[PATCH] xfs: use xfs_vn_setattr_size to check on new size

[PATCH] xfs: use xfs_vn_setattr_size to check on new size

[Eryu Guan]

Commit 6552321831dc ("xfs: remove i_iolock and use i_rwsem in the
VFS inode instead") introduced a regression that truncate(2) doesn't
check on new size, so it succeeds even if the new size exceeds the
current resource limit. Because xfs_setattr_size() was used instead
of xfs_vn_setattr_size(), and the latter calls xfs_vn_change_ok()
first to do sanity check on permission and new size.

This is found by truncate03 test from ltp, and the following is a
simplified reproducer:

  #!/bin/bash
  dev=/dev/sda5
  mnt=/mnt/xfs

  mkfs -t xfs -f $dev
  mount $dev $mnt

  # set max file size to 16k
  ulimit -f 16
  truncate -s $((16 * 1024 + 1)) /mnt/xfs/testfile
  [ $? -eq 0 ] && echo "FAIL: truncate exceeded max file size"
  ulimit -f unlimited
  umount $mnt

Signed-off-by: Eryu Guan <eguan@redhat.com>
---
 fs/xfs/xfs_iops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index c962999..b930be0 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -988,7 +988,7 @@ xfs_vn_setattr(
 			return error;
 
 		xfs_ilock(ip, XFS_MMAPLOCK_EXCL);
-		error = xfs_setattr_size(ip, iattr);
+		error = xfs_vn_setattr_size(dentry, iattr);
 		xfs_iunlock(ip, XFS_MMAPLOCK_EXCL);
 	} else {
 		error = xfs_vn_setattr_nonsize(dentry, iattr);
-- 
2.9.3

Re: [PATCH] xfs: use xfs_vn_setattr_size to check on new size

[Christoph Hellwig]

On Thu, Dec 01, 2016 at 06:18:00PM +0800, Eryu Guan wrote:
> Commit 6552321831dc ("xfs: remove i_iolock and use i_rwsem in the
> VFS inode instead") introduced a regression that truncate(2) doesn't
> check on new size, so it succeeds even if the new size exceeds the
> current resource limit. Because xfs_setattr_size() was used instead
> of xfs_vn_setattr_size(), and the latter calls xfs_vn_change_ok()
> first to do sanity check on permission and new size.

Oops, looks like this was a merge error..

> 
> This is found by truncate03 test from ltp, and the following is a
> simplified reproducer:
> 
>   #!/bin/bash
>   dev=/dev/sda5
>   mnt=/mnt/xfs
> 
>   mkfs -t xfs -f $dev
>   mount $dev $mnt
> 
>   # set max file size to 16k
>   ulimit -f 16
>   truncate -s $((16 * 1024 + 1)) /mnt/xfs/testfile
>   [ $? -eq 0 ] && echo "FAIL: truncate exceeded max file size"
>   ulimit -f unlimited
>   umount $mnt

Can you add this to xfstests?

The patch looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] xfs: use xfs_vn_setattr_size to check on new size

[Eryu Guan]

On Thu, Dec 01, 2016 at 02:26:35AM -0800, Christoph Hellwig wrote:
> On Thu, Dec 01, 2016 at 06:18:00PM +0800, Eryu Guan wrote:
> > Commit 6552321831dc ("xfs: remove i_iolock and use i_rwsem in the
> > VFS inode instead") introduced a regression that truncate(2) doesn't
> > check on new size, so it succeeds even if the new size exceeds the
> > current resource limit. Because xfs_setattr_size() was used instead
> > of xfs_vn_setattr_size(), and the latter calls xfs_vn_change_ok()
> > first to do sanity check on permission and new size.
> 
> Oops, looks like this was a merge error..
> 
> > 
> > This is found by truncate03 test from ltp, and the following is a
> > simplified reproducer:
> > 
> >   #!/bin/bash
> >   dev=/dev/sda5
> >   mnt=/mnt/xfs
> > 
> >   mkfs -t xfs -f $dev
> >   mount $dev $mnt
> > 
> >   # set max file size to 16k
> >   ulimit -f 16
> >   truncate -s $((16 * 1024 + 1)) /mnt/xfs/testfile
> >   [ $? -eq 0 ] && echo "FAIL: truncate exceeded max file size"
> >   ulimit -f unlimited
> >   umount $mnt
> 
> Can you add this to xfstests?

Sure, I'll work on it.

> 
> The patch looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks!

Eryu