Commit 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes") replaced
xfs_zero_remaining_bytes() with calls to iomap helpers.
Unfortunately the new iomap helpers don't enforce that [pos,count) lies
strictly on [0,i_size). This causes fallocate(mode=PUNCH_HOLE|KEEP_SIZE)
calls touching [i_size & ~PAGE_MASK, OFF_T_MAX] to round i_size up to
the nearest multiple of PAGE_SIZE:
calvinow@vm-disks/generic-xfs-1 ~$ dd if=/dev/urandom of=test bs=2048 count=1
calvinow@vm-disks/generic-xfs-1 ~$ stat test
Size: 2048 Blocks: 8 IO Block: 4096 regular file
calvinow@vm-disks/generic-xfs-1 ~$ fallocate -n -l 2048 -o 2048 -p test
calvinow@vm-disks/generic-xfs-1 ~$ stat test
Size: 4096 Blocks: 8 IO Block: 4096 regular file
Fix this by reintroducing the checks xfs_zero_remaining_bytes() did
against i_size into xfs_zero_range().
Reported-by: Aaron Gao <[email protected]>
Fixes: 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes")
Cc: Christoph Hellwig <[email protected]>
Cc: <[email protected]> # 4.8+
Signed-off-by: Calvin Owens <[email protected]>
---
fs/xfs/xfs_file.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 35703a8..da7cd27 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -58,6 +58,17 @@ xfs_zero_range(
xfs_off_t count,
bool *did_zero)
{
+ /*
+ * Avoid doing I/O beyond eof - it's not necessary
+ * since nothing can read beyond eof. The space will
+ * be zeroed when the file is extended anyway.
+ */
+ if (pos >= XFS_ISIZE(ip))
+ return 0;
+
+ if ((pos + count) >= XFS_ISIZE(ip))
+ count = XFS_ISIZE(ip) - pos - 1;
+
return iomap_zero_range(VFS_I(ip), pos, count, NULL, &xfs_iomap_ops);
}
--
2.9.3
> Fix this by reintroducing the checks xfs_zero_remaining_bytes() did
> against i_size into xfs_zero_range().
Sorry this is wrong: I missed that xfs_zero_range() has another caller that
depends on the behavior I'm changing. I'll send a v2 with the same hunk at
the bottom of xfs_free_file_space() instead.
Thanks,
Calvin
When punching past EOF on XFS, fallocate(mode=PUNCH_HOLE|KEEP_SIZE) will
round the file size up to the nearest multiple of PAGE_SIZE:
calvinow@vm-disks/generic-xfs-1 ~$ dd if=/dev/urandom of=test bs=2048 count=1
calvinow@vm-disks/generic-xfs-1 ~$ stat test
Size: 2048 Blocks: 8 IO Block: 4096 regular file
calvinow@vm-disks/generic-xfs-1 ~$ fallocate -n -l 2048 -o 2048 -p test
calvinow@vm-disks/generic-xfs-1 ~$ stat test
Size: 4096 Blocks: 8 IO Block: 4096 regular file
Commit 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes") replaced
xfs_zero_remaining_bytes() with calls to iomap helpers. The new helpers
don't enforce that [pos,offset) lies strictly on [0,i_size) when being
called from xfs_free_file_space(), so by "leaking" these ranges into
xfs_zero_range() we get this buggy behavior.
Fix this by reintroducing the checks xfs_zero_remaining_bytes() did
against i_size at the bottom of xfs_free_file_space().
Reported-by: Aaron Gao <[email protected]>
Fixes: 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes")
Cc: Christoph Hellwig <[email protected]>
Cc: <[email protected]> # 4.8+
Signed-off-by: Calvin Owens <[email protected]>
---
fs/xfs/xfs_bmap_util.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 8b75dce..0796ebc 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1309,6 +1309,17 @@ xfs_free_file_space(
}
/*
+ * Avoid doing I/O beyond eof - it's not necessary
+ * since nothing can read beyond eof. The space will
+ * be zeroed when the file is extended anyway.
+ */
+ if (offset >= XFS_ISIZE(ip))
+ return 0;
+
+ if ((offset + len) >= XFS_ISIZE(ip))
+ len = XFS_ISIZE(ip) - offset - 1;
+
+ /*
* Now that we've unmap all full blocks we'll have to zero out any
* partial block at the beginning and/or end. xfs_zero_range is
* smart enough to skip any holes, including those we just created.
--
2.9.3
On Sun, Mar 19, 2017 at 09:54:51PM -0700, Calvin Owens wrote:
> When punching past EOF on XFS, fallocate(mode=PUNCH_HOLE|KEEP_SIZE) will
> round the file size up to the nearest multiple of PAGE_SIZE:
>
> calvinow@vm-disks/generic-xfs-1 ~$ dd if=/dev/urandom of=test bs=2048 count=1
> calvinow@vm-disks/generic-xfs-1 ~$ stat test
> Size: 2048 Blocks: 8 IO Block: 4096 regular file
> calvinow@vm-disks/generic-xfs-1 ~$ fallocate -n -l 2048 -o 2048 -p test
> calvinow@vm-disks/generic-xfs-1 ~$ stat test
> Size: 4096 Blocks: 8 IO Block: 4096 regular file
>
> Commit 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes") replaced
> xfs_zero_remaining_bytes() with calls to iomap helpers. The new helpers
> don't enforce that [pos,offset) lies strictly on [0,i_size) when being
> called from xfs_free_file_space(), so by "leaking" these ranges into
> xfs_zero_range() we get this buggy behavior.
>
> Fix this by reintroducing the checks xfs_zero_remaining_bytes() did
> against i_size at the bottom of xfs_free_file_space().
>
> Reported-by: Aaron Gao <[email protected]>
> Fixes: 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes")
> Cc: Christoph Hellwig <[email protected]>
> Cc: <[email protected]> # 4.8+
> Signed-off-by: Calvin Owens <[email protected]>
> ---
> fs/xfs/xfs_bmap_util.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 8b75dce..0796ebc 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1309,6 +1309,17 @@ xfs_free_file_space(
> }
>
> /*
> + * Avoid doing I/O beyond eof - it's not necessary
> + * since nothing can read beyond eof. The space will
> + * be zeroed when the file is extended anyway.
> + */
I'd suggest to update the comment below with this information and move
the following bits down below it as well.
> + if (offset >= XFS_ISIZE(ip))
> + return 0;
> +
> + if ((offset + len) >= XFS_ISIZE(ip))
> + len = XFS_ISIZE(ip) - offset - 1;
> +
This looks like an off-by-one. Do you mean the following?
if (offset + len > XFS_ISIZE(ip))
len = XFS_ISIZE(ip) - offset;
Brian
> + /*
> * Now that we've unmap all full blocks we'll have to zero out any
> * partial block at the beginning and/or end. xfs_zero_range is
> * smart enough to skip any holes, including those we just created.
> --
> 2.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/21/2017 04:39 AM, Brian Foster wrote:
> On Sun, Mar 19, 2017 at 09:54:51PM -0700, Calvin Owens wrote:
>> When punching past EOF on XFS, fallocate(mode=PUNCH_HOLE|KEEP_SIZE) will
>> round the file size up to the nearest multiple of PAGE_SIZE:
>>
>> calvinow@vm-disks/generic-xfs-1 ~$ dd if=/dev/urandom of=test bs=2048 count=1
>> calvinow@vm-disks/generic-xfs-1 ~$ stat test
>> Size: 2048 Blocks: 8 IO Block: 4096 regular file
>> calvinow@vm-disks/generic-xfs-1 ~$ fallocate -n -l 2048 -o 2048 -p test
>> calvinow@vm-disks/generic-xfs-1 ~$ stat test
>> Size: 4096 Blocks: 8 IO Block: 4096 regular file
>>
>> Commit 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes") replaced
>> xfs_zero_remaining_bytes() with calls to iomap helpers. The new helpers
>> don't enforce that [pos,offset) lies strictly on [0,i_size) when being
>> called from xfs_free_file_space(), so by "leaking" these ranges into
>> xfs_zero_range() we get this buggy behavior.
>>
>> Fix this by reintroducing the checks xfs_zero_remaining_bytes() did
>> against i_size at the bottom of xfs_free_file_space().
>>
>> Reported-by: Aaron Gao <[email protected]>
>> Fixes: 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes")
>> Cc: Christoph Hellwig <[email protected]>
>> Cc: <[email protected]> # 4.8+
>> Signed-off-by: Calvin Owens <[email protected]>
>> ---
>> fs/xfs/xfs_bmap_util.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
>> index 8b75dce..0796ebc 100644
>> --- a/fs/xfs/xfs_bmap_util.c
>> +++ b/fs/xfs/xfs_bmap_util.c
>> @@ -1309,6 +1309,17 @@ xfs_free_file_space(
>> }
>>
>> /*
>> + * Avoid doing I/O beyond eof - it's not necessary
>> + * since nothing can read beyond eof. The space will
>> + * be zeroed when the file is extended anyway.
>> + */
>
> I'd suggest to update the comment below with this information and move
> the following bits down below it as well.
Will do.
>> + if (offset >= XFS_ISIZE(ip))
>> + return 0;
>> +
>> + if ((offset + len) >= XFS_ISIZE(ip))
>> + len = XFS_ISIZE(ip) - offset - 1;
>> +
>
> This looks like an off-by-one. Do you mean the following?
>
> if (offset + len > XFS_ISIZE(ip))
> len = XFS_ISIZE(ip) - offset;
It's not an off-by-one (it's self-consistent), but your way makes more
sense, I'll fix it ;)
Thanks,
Calvin
> Brian
>
>> + /*
>> * Now that we've unmap all full blocks we'll have to zero out any
>> * partial block at the beginning and/or end. xfs_zero_range is
>> * smart enough to skip any holes, including those we just created.
>> --
>> 2.9.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
When punching past EOF on XFS, fallocate(mode=PUNCH_HOLE|KEEP_SIZE) will
round the file size up to the nearest multiple of PAGE_SIZE:
calvinow@vm-disks/generic-xfs-1 ~$ dd if=/dev/urandom of=test bs=2048 count=1
calvinow@vm-disks/generic-xfs-1 ~$ stat test
Size: 2048 Blocks: 8 IO Block: 4096 regular file
calvinow@vm-disks/generic-xfs-1 ~$ fallocate -n -l 2048 -o 2048 -p test
calvinow@vm-disks/generic-xfs-1 ~$ stat test
Size: 4096 Blocks: 8 IO Block: 4096 regular file
Commit 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes") replaced
xfs_zero_remaining_bytes() with calls to iomap helpers. The new helpers
don't enforce that [pos,offset) lies strictly on [0,i_size) when being
called from xfs_free_file_space(), so by "leaking" these ranges into
xfs_zero_range() we get this buggy behavior.
Fix this by reintroducing the checks xfs_zero_remaining_bytes() did
against i_size at the bottom of xfs_free_file_space().
Reported-by: Aaron Gao <[email protected]>
Fixes: 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes")
Cc: Christoph Hellwig <[email protected]>
Cc: Brian Foster <[email protected]>
Cc: <[email protected]> # 4.8+
Signed-off-by: Calvin Owens <[email protected]>
---
fs/xfs/xfs_bmap_util.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 8b75dce..828532c 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1311,8 +1311,16 @@ xfs_free_file_space(
/*
* Now that we've unmap all full blocks we'll have to zero out any
* partial block at the beginning and/or end. xfs_zero_range is
- * smart enough to skip any holes, including those we just created.
+ * smart enough to skip any holes, including those we just created,
+ * but we must take care not to zero beyond EOF and enlarge i_size.
*/
+
+ if (offset >= XFS_ISIZE(ip))
+ return 0;
+
+ if (offset + len > XFS_ISIZE(ip))
+ len = XFS_ISIZE(ip) - offset;
+
return xfs_zero_range(ip, offset, len, NULL);
}
--
2.9.3
Looks fine,
Reviewed-by: Christoph Hellwig <[email protected]>
On Thu, Mar 30, 2017 at 09:18:00PM -0700, Calvin Owens wrote:
> When punching past EOF on XFS, fallocate(mode=PUNCH_HOLE|KEEP_SIZE) will
> round the file size up to the nearest multiple of PAGE_SIZE:
>
> calvinow@vm-disks/generic-xfs-1 ~$ dd if=/dev/urandom of=test bs=2048 count=1
> calvinow@vm-disks/generic-xfs-1 ~$ stat test
> Size: 2048 Blocks: 8 IO Block: 4096 regular file
> calvinow@vm-disks/generic-xfs-1 ~$ fallocate -n -l 2048 -o 2048 -p test
> calvinow@vm-disks/generic-xfs-1 ~$ stat test
> Size: 4096 Blocks: 8 IO Block: 4096 regular file
>
> Commit 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes") replaced
> xfs_zero_remaining_bytes() with calls to iomap helpers. The new helpers
> don't enforce that [pos,offset) lies strictly on [0,i_size) when being
> called from xfs_free_file_space(), so by "leaking" these ranges into
> xfs_zero_range() we get this buggy behavior.
>
> Fix this by reintroducing the checks xfs_zero_remaining_bytes() did
> against i_size at the bottom of xfs_free_file_space().
>
> Reported-by: Aaron Gao <[email protected]>
> Fixes: 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes")
> Cc: Christoph Hellwig <[email protected]>
> Cc: Brian Foster <[email protected]>
> Cc: <[email protected]> # 4.8+
> Signed-off-by: Calvin Owens <[email protected]>
> ---
> fs/xfs/xfs_bmap_util.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 8b75dce..828532c 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1311,8 +1311,16 @@ xfs_free_file_space(
> /*
> * Now that we've unmap all full blocks we'll have to zero out any
> * partial block at the beginning and/or end. xfs_zero_range is
> - * smart enough to skip any holes, including those we just created.
> + * smart enough to skip any holes, including those we just created,
> + * but we must take care not to zero beyond EOF and enlarge i_size.
> */
> +
> + if (offset >= XFS_ISIZE(ip))
> + return 0;
> +
> + if (offset + len > XFS_ISIZE(ip))
> + len = XFS_ISIZE(ip) - offset;
> +
I'll pick this up for 4.12.
Reviewed-by: Darrick J. Wong <[email protected]>
--D
> return xfs_zero_range(ip, offset, len, NULL);
> }
>
> --
> 2.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 03, 2017 at 10:15:31AM -0700, Darrick J. Wong wrote:
> On Thu, Mar 30, 2017 at 09:18:00PM -0700, Calvin Owens wrote:
> > When punching past EOF on XFS, fallocate(mode=PUNCH_HOLE|KEEP_SIZE) will
> > round the file size up to the nearest multiple of PAGE_SIZE:
> >
> > calvinow@vm-disks/generic-xfs-1 ~$ dd if=/dev/urandom of=test bs=2048 count=1
> > calvinow@vm-disks/generic-xfs-1 ~$ stat test
> > Size: 2048 Blocks: 8 IO Block: 4096 regular file
> > calvinow@vm-disks/generic-xfs-1 ~$ fallocate -n -l 2048 -o 2048 -p test
> > calvinow@vm-disks/generic-xfs-1 ~$ stat test
> > Size: 4096 Blocks: 8 IO Block: 4096 regular file
> >
> > Commit 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes") replaced
> > xfs_zero_remaining_bytes() with calls to iomap helpers. The new helpers
> > don't enforce that [pos,offset) lies strictly on [0,i_size) when being
> > called from xfs_free_file_space(), so by "leaking" these ranges into
> > xfs_zero_range() we get this buggy behavior.
> >
> > Fix this by reintroducing the checks xfs_zero_remaining_bytes() did
> > against i_size at the bottom of xfs_free_file_space().
> >
> > Reported-by: Aaron Gao <[email protected]>
> > Fixes: 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes")
> > Cc: Christoph Hellwig <[email protected]>
> > Cc: Brian Foster <[email protected]>
> > Cc: <[email protected]> # 4.8+
> > Signed-off-by: Calvin Owens <[email protected]>
> > ---
> > fs/xfs/xfs_bmap_util.c | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index 8b75dce..828532c 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -1311,8 +1311,16 @@ xfs_free_file_space(
> > /*
> > * Now that we've unmap all full blocks we'll have to zero out any
> > * partial block at the beginning and/or end. xfs_zero_range is
> > - * smart enough to skip any holes, including those we just created.
> > + * smart enough to skip any holes, including those we just created,
> > + * but we must take care not to zero beyond EOF and enlarge i_size.
> > */
> > +
> > + if (offset >= XFS_ISIZE(ip))
> > + return 0;
> > +
> > + if (offset + len > XFS_ISIZE(ip))
> > + len = XFS_ISIZE(ip) - offset;
> > +
>
> I'll pick this up for 4.12.
Bah, hit send too quickly.
I'll pick this up for 4.12, unless anyone wants me to push this for -rc6?
--D
> Reviewed-by: Darrick J. Wong <[email protected]>
>
> --D
>
> > return xfs_zero_range(ip, offset, len, NULL);
> > }
> >
> > --
> > 2.9.3
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 03, 2017 at 10:18:12AM -0700, Darrick J. Wong wrote:
> I'll pick this up for 4.12, unless anyone wants me to push this for -rc6?
Yes, please do. This is a regression that corrupts user visible data.