[PATCH] xfs: check for inode size overflow in xfs_new_eof()

[PATCH] xfs: check for inode size overflow in xfs_new_eof()

[Brian Foster]

If we write to the maximum file offset (2^63-2), XFS fails to log the
inode size update when the page is flushed. For example:

$ xfs_io -fc "pwrite `echo "2^63-1-1" | bc` 1" /mnt/file
wrote 1/1 bytes at offset 9223372036854775806
1.000000 bytes, 1 ops; 0.0000 sec (22.711 KiB/sec and 23255.8140 ops/sec)
$ stat -c %s /mnt/file
9223372036854775807
$ umount /mnt ; mount <dev> /mnt/
$ stat -c %s /mnt/file
0

This occurs because XFS calculates the new file size as io_offset +
io_size, I/O occurs in block sized requests, and the maximum supported
file size is not block aligned. Therefore, a write to the max allowable
offset on a 4k blocksize fs results in a write of size 4k to offset
2^63-4096 (e.g., equivalent to round_down(2^63-1, 4096), or IOW the
offset of the block that contains the max file size). The offset plus
size calculation (2^63 - 4096 + 4096 == 2^63) overflows the signed
64-bit variable which goes negative and causes the > comparison to the
on-disk inode size to fail. This returns 0 from xfs_new_eof() and
results in no change to the inode on-disk.

Update xfs_new_eof() to explicitly detect overflow of the local
calculation and use the VFS inode size in this scenario. The VFS inode
size is capped to the maximum and thus XFS writes the correct inode size
to disk.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---

Hi all,

This was also discovered while playing around with xfs/071, though it
doesn't cause a failure. FWIW, I started off fixing this by converting
xfs_new_eof() to use xfs_ufsize_t. That worked, but this seemed more
explicit.

Brian

 fs/xfs/xfs_inode.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index c10e3fa..9af2882 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -102,7 +102,7 @@ xfs_new_eof(struct xfs_inode *ip, xfs_fsize_t new_size)
 {
 	xfs_fsize_t i_size = i_size_read(VFS_I(ip));
 
-	if (new_size > i_size)
+	if (new_size > i_size || new_size < 0)
 		new_size = i_size;
 	return new_size > ip->i_d.di_size ? new_size : 0;
 }
-- 
1.8.3.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: check for inode size overflow in xfs_new_eof()

[Dave Chinner]

On Tue, Sep 30, 2014 at 03:21:23PM -0400, Brian Foster wrote:
> If we write to the maximum file offset (2^63-2), XFS fails to log the
> inode size update when the page is flushed. For example:
> 
> $ xfs_io -fc "pwrite `echo "2^63-1-1" | bc` 1" /mnt/file
> wrote 1/1 bytes at offset 9223372036854775806
> 1.000000 bytes, 1 ops; 0.0000 sec (22.711 KiB/sec and 23255.8140 ops/sec)
> $ stat -c %s /mnt/file
> 9223372036854775807
> $ umount /mnt ; mount <dev> /mnt/
> $ stat -c %s /mnt/file
> 0
> 
> This occurs because XFS calculates the new file size as io_offset +
> io_size, I/O occurs in block sized requests, and the maximum supported
> file size is not block aligned. Therefore, a write to the max allowable
> offset on a 4k blocksize fs results in a write of size 4k to offset
> 2^63-4096 (e.g., equivalent to round_down(2^63-1, 4096), or IOW the
> offset of the block that contains the max file size). The offset plus
> size calculation (2^63 - 4096 + 4096 == 2^63) overflows the signed
> 64-bit variable which goes negative and causes the > comparison to the
> on-disk inode size to fail. This returns 0 from xfs_new_eof() and
> results in no change to the inode on-disk.
> 
> Update xfs_new_eof() to explicitly detect overflow of the local
> calculation and use the VFS inode size in this scenario. The VFS inode
> size is capped to the maximum and thus XFS writes the correct inode size
> to disk.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> 
> Hi all,
> 
> This was also discovered while playing around with xfs/071, though it
> doesn't cause a failure. FWIW, I started off fixing this by converting
> xfs_new_eof() to use xfs_ufsize_t. That worked, but this seemed more
> explicit.
> 
> Brian
> 
>  fs/xfs/xfs_inode.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index c10e3fa..9af2882 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -102,7 +102,7 @@ xfs_new_eof(struct xfs_inode *ip, xfs_fsize_t new_size)
>  {
>  	xfs_fsize_t i_size = i_size_read(VFS_I(ip));
>  
> -	if (new_size > i_size)
> +	if (new_size > i_size || new_size < 0)
>  		new_size = i_size;
>  	return new_size > ip->i_d.di_size ? new_size : 0;
>  }

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs