xfs: fix s_max_bytes to MAX_LFS_FILESIZE if needed

From: Jie Liu <[email protected]>

This patch fix the s_max_bytes to MAX_LFS_FILESIZE if the pre-calculated value is greater than it.

Reported-by Michael L. Semon <[email protected]>

Signed-off-by Jie Liu <[email protected]>

---
  fs/xfs/xfs_super.c |    6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

fs/xfs/xfs_super.c +1 -0

index ea341ce..0644d61 100644
@@ -585,6 +585,7 @@ xfs_max_file_offset(
  {
  	unsigned int		pagefactor = 1;
  	unsigned int		bitshift = BITS_PER_LONG - 1;
+	__uint64_t		offset;
 
  	/* Figure out maximum filesize, on Linux this can depend on
  	 * the filesystem blocksize (on 32 bit platforms).

@@ -610,7 +611,10 @@ xfs_max_file_offset(
  # endif

From: Jie Liu <[email protected]>

On 32-bit machine, the s_maxbytes is larger than the MAX_LFS_FILESIZE limits if CONFIG_LBDAF is not enabled. Hence it's possible to create a huge file via buffered-IO write with a given offset beyond this limitation. e.g.

# block_size=4096
# offset=$(((2**32 - 1) * $block_size))

# xfs_io -f -c "pwrite $offset $block_size" /storage/test_file

In this case, xfs_io will hang at the page writeback stage soon since the given offset would cause an overflow at xfs_vm_writepage():

end_index = offset >> PAGE_CACHE_SHIFT;
last_index = (offset - 1) >> PAGE_CACHE_SHIFT;

if (page->index >= end_index) {
                 unsigned offset_into_page = offset & (PAGE_CACHE_SIZE - 1);

                 /*
                  * Just skip the page if it is fully outside i_size, e.g. due
                  * to a truncate operation that is in progress.
                  */
                 if (page->index >= end_index + 1 || offset_into_page == 0) {
		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                         unlock_page(page);
                         return 0;
                 }

end_index is unsigned long so that the max value is '2^32-1 = 4294967295', and it would be evaluated to the max value with the given offset(when writing the page offset up to s_max_bytes) for above test case. As a result, (page->index >= end_index + 1) is ok as (end_index + 1) is overflowed to ZERO.

Actually, create a file as above on 32-bit machine should be failed with EFBIG error returned because there has strict check up at generic_write_checks() against the given offset with a *correct* s_max_bytes.

This patch fix the s_max_bytes to MAX_LFS_FILESIZE if the pre-calculated value is greater than it.

Reported-by Michael L. Semon <[email protected]>

Signed-off-by Jie Liu <[email protected]>

---
  fs/xfs/xfs_super.c |    6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

fs/xfs/xfs_super.c +1 -0

index ea341ce..0644d61 100644
@@ -585,6 +585,7 @@ xfs_max_file_offset(
  {
  	unsigned int		pagefactor = 1;
  	unsigned int		bitshift = BITS_PER_LONG - 1;
+	__uint64_t		offset;
 
  	/* Figure out maximum filesize, on Linux this can depend on
  	 * the filesystem blocksize (on 32 bit platforms).

@@ -610,7 +611,10 @@ xfs_max_file_offset(
  # endif

From: Jie Liu <[email protected]>

On 32-bit machine, the s_maxbytes is larger than the MAX_LFS_FILESIZE limits if CONFIG_LBDAF is not enabled. Hence it's possible to create a huge file via buffered-IO write with a given offset beyond this limitation. e.g.

# block_size=4096
# offset=$(((2**32 - 1) * $block_size))

# xfs_io -f -c "pwrite $offset $block_size" /storage/test_file

In this case, xfs_io will hang at the page writeback stage soon since the given offset would cause an overflow at xfs_vm_writepage():

end_index = offset >> PAGE_CACHE_SHIFT;
last_index = (offset - 1) >> PAGE_CACHE_SHIFT;

if (page->index >= end_index) {
                 unsigned offset_into_page = offset & (PAGE_CACHE_SIZE - 1);

                 /*
                  * Just skip the page if it is fully outside i_size, e.g. due
                  * to a truncate operation that is in progress.
                  */
                 if (page->index >= end_index + 1 || offset_into_page == 0) {
		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                         unlock_page(page);
                         return 0;
                 }

end_index is unsigned long so that the max value is '2^32-1 = 4294967295', and it would be evaluated to the max value with the given offset(when writing the page offset up to s_max_bytes) for above test case. As a result, (page->index >= end_index + 1) is ok as (end_index + 1) is overflowed to ZERO.

Actually, create a file as above on 32-bit machine should be failed with EFBIG error returned because there has strict check up at generic_write_checks() against the given offset with a *correct* s_max_bytes.

This patch fix the s_max_bytes to MAX_LFS_FILESIZE if the pre-calculated value is greater than it.

Reported-by Michael L. Semon <[email protected]>

Signed-off-by Jie Liu <[email protected]>

---
  fs/xfs/xfs_super.c |    6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

fs/xfs/xfs_super.c +1 -0

index ea341ce..0644d61 100644
@@ -585,6 +585,7 @@ xfs_max_file_offset(
  {
  	unsigned int		pagefactor = 1;
  	unsigned int		bitshift = BITS_PER_LONG - 1;
+	__uint64_t		offset;
 
  	/* Figure out maximum filesize, on Linux this can depend on
  	 * the filesystem blocksize (on 32 bit platforms).

@@ -610,7 +611,10 @@ xfs_max_file_offset(
  # endif

Update: My tests on my original hardware go exactly as they did in my Pentium 4 test. xfstests shared/[0-9][0-9][0-9] and xfs/003 through xfs/136 were run against it. No problems. Good job. I'm keeping the patch.

My final version of the bug summary goes like this:

On a 32-bit x86 PC, with a Linux kernel that has CONFIG_LBDAF=y...

xfstests generic/308, by writing to a file at an address just before 2**32, causes the following conditions on an XFS filesystem:

1) CPU usage becomes very high,

2) The xfs_io process cannot be killed,

3) The best way to shut down the PC is through use of the magic SysRq keys.

4) Afterwards, attempts to mount the filesystem result in a soft oops.

5) After an xfs_repair -L on the filesystem, all is OK, other than for what was lost by zeroing the log.

J. Liu wrote a patch that solves this problem, but he found the answers with CONFIG_LBDAF=n, which is a condition for which xfstests generic/308 passes on the two test PCs used.

Tests were conducted on a Pentium III (kernel 3.9-rc4 with numerous SGI patches) and on a Pentium 4 (kernel 3.9-rc6 with numerous SGI patches).

Could you verify these things by memory (no need to retest)?

a) With CONFIG_LBDAF=y, generic/308 caused filesystem corruption, and

b) With CONFIG_LBDAF=n, generic/308 passed the test.

c) Having CONFIG_LBDAF=n helped you to find the answers and write this fine patch.

Otherwise, the conclusion is "I don't know how you got there, but you got there. Good job! and thanks for finding the root cause of the problem."

Thanks again!

Michael

Quoted 69 lines · Jeff Liu

From: Jie Liu <[email protected]>

On 32-bit machine, the s_maxbytes is larger than the MAX_LFS_FILESIZE limits if CONFIG_LBDAF is not enabled. Hence it's possible to create a huge file via buffered-IO write with a given offset beyond this limitation. e.g.

# block_size=4096
# offset=$(((2**32 - 1) * $block_size))

# xfs_io -f -c "pwrite $offset $block_size" /storage/test_file

In this case, xfs_io will hang at the page writeback stage soon since the given offset would cause an overflow at xfs_vm_writepage():

end_index = offset >> PAGE_CACHE_SHIFT;
last_index = (offset - 1) >> PAGE_CACHE_SHIFT;

if (page->index >= end_index) {
                 unsigned offset_into_page = offset & (PAGE_CACHE_SIZE - 1);

                 /*
                  * Just skip the page if it is fully outside i_size, e.g. due
                  * to a truncate operation that is in progress.
                  */
                 if (page->index >= end_index + 1 || offset_into_page == 0) {
		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                         unlock_page(page);
                         return 0;
                 }

end_index is unsigned long so that the max value is '2^32-1 = 4294967295', and it would be evaluated to the max value with the given offset(when writing the page offset up to s_max_bytes) for above test case. As a result, (page->index >= end_index + 1) is ok as (end_index + 1) is overflowed to ZERO.

Actually, create a file as above on 32-bit machine should be failed with EFBIG error returned because there has strict check up at generic_write_checks() against the given offset with a *correct* s_max_bytes.

This patch fix the s_max_bytes to MAX_LFS_FILESIZE if the pre-calculated value is greater than it.

Reported-by Michael L. Semon <[email protected]>

Signed-off-by Jie Liu <[email protected]>

---
  fs/xfs/xfs_super.c |    6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

fs/xfs/xfs_super.c +5 -1

index ea341ce..0644d61 100644
@@ -585,6 +585,7 @@ xfs_max_file_offset(
  {
  	unsigned int		pagefactor = 1;
  	unsigned int		bitshift = BITS_PER_LONG - 1;
+	__uint64_t		offset;
 
  	/* Figure out maximum filesize, on Linux this can depend on
  	 * the filesystem blocksize (on 32 bit platforms).

@@ -610,7 +611,10 @@ xfs_max_file_offset(
  # endif
  #endif
 
-	return (((__uint64_t)pagefactor) << bitshift) - 1;
+	offset = (((__uint64_t)pagefactor) << bitshift) - 1;
+
+	/* Check against VM & VFS exposed limits */
+	return (offset > MAX_LFS_FILESIZE) ? MAX_LFS_FILESIZE : offset;
  }
 

fs/xfs/xfs_super.c xfs_max_file_offset( · line 620

Quoted 2 lines

fs/xfs/xfs_super.c +0 -0

  xfs_agnumber_t

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Hi Michael,

Thanks a lot for help verifying this fix and sorry for my too late response since I have traveled to US two days ago.

Quoted 18 lines · Michael L. Semon

Update: My tests on my original hardware go exactly as they did in my Pentium 4 test. xfstests shared/[0-9][0-9][0-9] and xfs/003 through xfs/136 were run against it. No problems. Good job. I'm keeping the patch.

My final version of the bug summary goes like this:

On a 32-bit x86 PC, with a Linux kernel that has CONFIG_LBDAF=y...

xfstests generic/308, by writing to a file at an address just before 2**32, causes the following conditions on an XFS filesystem:

1) CPU usage becomes very high,

2) The xfs_io process cannot be killed,

3) The best way to shut down the PC is through use of the magic SysRq keys.

Quoted 8 lines

4) Afterwards, attempts to mount the filesystem result in a soft oops.

5) After an xfs_repair -L on the filesystem, all is OK, other than for what was lost by zeroing the log.

J. Liu wrote a patch that solves this problem, but he found the answers with CONFIG_LBDAF=n, which is a condition for which xfstests generic/308 passes on the two test PCs used.

Ooops! it's wrong. My answer is misleading, you can think that I drink too much at that time.:( Actually, it quite the reverse, i.e. this issue can be reproduced against 32-bit kernel with CONFIG_LBADF=y, this is the default configuration of mine.
In this case, I observed that the s_maxbytes is larger than the MAX_LFS_FILESIZE. Hence, the current patch is written to make sure that the s_maxbytes should not beyond this limits.

For 32-bit kernel with CONFIG_LBADF=n, the s_maxbytes is just equal to MAX_LFS_FILESIZE, so the test is works to me. BTW, I only verified this fix upon the default mkfs options. i.e, 4k blocksize, that is, mkfs.xfs /dev/sdX.

Quoted 5 lines

Tests were conducted on a Pentium III (kernel 3.9-rc4 with numerous SGI patches) and on a Pentium 4 (kernel 3.9-rc6 with numerous SGI patches).

Could you verify these things by memory (no need to retest)?

As I mentioned above.

Quoted 1 line

a) With CONFIG_LBDAF=y, generic/308 caused filesystem corruption, and

In this case, the operation should be denied with -EFBIG error returned if trying to create a huge file.

Quoted 2 lines

b) With CONFIG_LBDAF=n, generic/308 passed the test.

Even don't applying this patch, the test run passed for the default mkfs setup.

Quoted 1 line

Quoted 2 lines

c) Having CONFIG_LBDAF=n helped you to find the answers and write this fine patch.

CONFIG_LBADF=y instead.

Thanks again!
-Jeff

From: Jie Liu <[email protected]>

On 32-bit machine, the s_maxbytes is larger than the MAX_LFS_FILESIZE limits if CONFIG_LBDAF is not enabled. Hence it's possible to create a huge file via buffered-IO write with a given offset beyond this limitation. e.g.

# block_size=4096
# offset=$(((2**32 - 1) * $block_size))

# xfs_io -f -c "pwrite $offset $block_size" /storage/test_file

In this case, xfs_io will hang at the page writeback stage soon since the given offset would cause an overflow at xfs_vm_writepage():

end_index = offset >> PAGE_CACHE_SHIFT;
last_index = (offset - 1) >> PAGE_CACHE_SHIFT;

if (page->index >= end_index) {
                unsigned offset_into_page = offset & (PAGE_CACHE_SIZE - 1);

                /*
                 * Just skip the page if it is fully outside i_size, e.g. due
                 * to a truncate operation that is in progress.
                 */
                if (page->index >= end_index + 1 || offset_into_page == 0) {
		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                        unlock_page(page);
                        return 0;
                }

end_index is unsigned long so that the max value is '2^32-1 = 4294967295', and it would be evaluated to the max value with the given offset(when writing the page offset up to s_max_bytes) for above test case. As a result, (page->index >= end_index + 1) is ok as (end_index + 1) is overflowed to ZERO.

Actually, create a file as above on 32-bit machine should be failed with EFBIG error returned because there has strict check up at generic_write_checks() against the given offset with a *correct* s_max_bytes.

This patch fix the s_max_bytes to MAX_LFS_FILESIZE if the pre-calculated value is greater than it.

Reported-by Michael L. Semon <[email protected]>

Signed-off-by Jie Liu <[email protected]>

---
 fs/xfs/xfs_super.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

fs/xfs/xfs_super.c +1 -0

index ea341ce..0644d61 100644
@@ -585,6 +585,7 @@ xfs_max_file_offset(
 {
 	unsigned int		pagefactor = 1;
 	unsigned int		bitshift = BITS_PER_LONG - 1;
+	__uint64_t		offset;
 
 	/* Figure out maximum filesize, on Linux this can depend on
 	 * the filesystem blocksize (on 32 bit platforms).

@@ -610,7 +611,10 @@ xfs_max_file_offset(
 # endif

Could anyone help to review this patch?

Quoted 30 lines

On 32-bit machine, the s_maxbytes is larger than the MAX_LFS_FILESIZE limits if CONFIG_LBDAF is not enabled. Hence it's possible to create a huge file via buffered-IO write with a given offset beyond this limitation. e.g.

# block_size=4096
# offset=$(((2**32 - 1) * $block_size))

# xfs_io -f -c "pwrite $offset $block_size" /storage/test_file

In this case, xfs_io will hang at the page writeback stage soon since the given offset would cause an overflow at xfs_vm_writepage():

end_index = offset >> PAGE_CACHE_SHIFT;
last_index = (offset - 1) >> PAGE_CACHE_SHIFT;

if (page->index >= end_index) {
                unsigned offset_into_page = offset & (PAGE_CACHE_SIZE - 1);

                /*
                 * Just skip the page if it is fully outside i_size, e.g. due
                 * to a truncate operation that is in progress.
                 */
                if (page->index >= end_index + 1 || offset_into_page == 0) {
		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                        unlock_page(page);
                        return 0;
                }

end_index is unsigned long so that the max value is '2^32-1 = 4294967295', and it would be evaluated to the max value with the given offset(when writing the page offset up to s_max_bytes) for above test case. As a result, (page->index >= end_index + 1) is ok as (end_index + 1) is overflowed to ZERO.

Quoted 6 lines

Actually, create a file as above on 32-bit machine should be failed with EFBIG error returned because there has strict check up at generic_write_checks() against the given offset with a *correct* s_max_bytes.

This patch fix the s_max_bytes to MAX_LFS_FILESIZE if the pre-calculated value is greater than it.

Quoted 1 line · Jeff Liu

Could anyone help to review this patch?

Sorry, I missed it.

Quoted 34 lines

Quoted 34 lines

On 32-bit machine, the s_maxbytes is larger than the MAX_LFS_FILESIZE limits if CONFIG_LBDAF is not enabled. Hence it's possible to create a huge file via buffered-IO write with a given offset beyond this limitation. e.g.

# block_size=4096
# offset=$(((2**32 - 1) * $block_size))

# xfs_io -f -c "pwrite $offset $block_size" /storage/test_file

In this case, xfs_io will hang at the page writeback stage soon since the given offset would cause an overflow at xfs_vm_writepage():

end_index = offset >> PAGE_CACHE_SHIFT;
last_index = (offset - 1) >> PAGE_CACHE_SHIFT;

if (page->index >= end_index) {
                unsigned offset_into_page = offset & (PAGE_CACHE_SIZE - 1);

                /*
                 * Just skip the page if it is fully outside i_size, e.g. due
                 * to a truncate operation that is in progress.
                 */
                if (page->index >= end_index + 1 || offset_into_page == 0) {
		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                        unlock_page(page);
                        return 0;
                }

end_index is unsigned long so that the max value is '2^32-1 = 4294967295', and it would be evaluated to the max value with the given offset(when writing the page offset up to s_max_bytes) for above test case. As a result, (page->index >= end_index + 1) is ok as (end_index + 1) is overflowed to ZERO.

Actually, create a file as above on 32-bit machine should be failed with EFBIG error returned because there has strict check up at generic_write_checks() against the given offset with a *correct* s_max_bytes.

Quoted 2 lines

Quoted 2 lines

This patch fix the s_max_bytes to MAX_LFS_FILESIZE if the pre-calculated value is greater than it.

Isn't MAX_LFS_FILESIZE defined on 32 bit systems to 8TB and the problem here is that we are overflowing at 16TB? If so, that means addin gthis patch will potentially cause problems with existing working setups that have (sparse) files larger than 8TB on 32 bit systems.

So, can't we simply subtract PAGE_CACHE_SIZE from the offset being returned to avoid this overflow?