[PATCH 0/5] Fix a minor POSIX conformance problem

[PATCH 0/5] Fix a minor POSIX conformance problem

[Matthew Wilcox (Oracle)]

POSIX requires that on ftruncate() expansion, the new bytes must read
as zeroes.  If someone's mmap()ed the file and stored past EOF, for
most filesystems the bytes in that page will be not-zero.  It's a
pretty minor violation; someone could race you and write to the file
between the ftruncate() call and you reading from it, but it's a bit
of a QOI violation.  

I've tested xfs (passes before & after), ext4 and tmpfs (both fail
before, pass after).  Testing from other FS developers appreciated.
fstest to follow; not sure how to persuade git-send-email to work on
multiple repositories

Matthew Wilcox (Oracle) (5):
  truncate: Zero bytes after 'oldsize' if we're expanding the file
  ext4: Zero bytes after 'oldsize' if we're expanding the file
  tmpfs: Zero bytes after 'oldsize' if we're expanding the file
  afs: Zero bytes after 'oldsize' if we're expanding the file
  btrfs: Zero bytes after 'oldsize' if we're expanding the file

 fs/afs/inode.c   | 2 ++
 fs/btrfs/inode.c | 1 +
 fs/ext4/inode.c  | 1 +
 mm/shmem.c       | 2 ++
 mm/truncate.c    | 7 +++++--
 5 files changed, 11 insertions(+), 2 deletions(-)

-- 
2.35.1

[PATCH 1/5] truncate: Zero bytes after 'oldsize' if we're expanding the file

[Matthew Wilcox (Oracle)]

POSIX requires that "If the file size is increased, the extended area
shall appear as if it were zero-filled".  It is possible to use mmap to
write past EOF and that data will become visible instead of zeroes.
This fixes the problem for the filesystems which simply call
truncate_setsize().  More complex filesystems will need their own
patches.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/truncate.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/mm/truncate.c b/mm/truncate.c
index 7b4ea4c4a46b..cebfc5415e9a 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -763,9 +763,12 @@ void truncate_setsize(struct inode *inode, loff_t newsize)
 	loff_t oldsize = inode->i_size;
 
 	i_size_write(inode, newsize);
-	if (newsize > oldsize)
+	if (newsize > oldsize) {
 		pagecache_isize_extended(inode, oldsize, newsize);
-	truncate_pagecache(inode, newsize);
+		truncate_pagecache(inode, oldsize);
+	} else {
+		truncate_pagecache(inode, newsize);
+	}
 }
 EXPORT_SYMBOL(truncate_setsize);
 
-- 
2.35.1

Re: [PATCH 1/5] truncate: Zero bytes after 'oldsize' if we're expanding the file

[Brian Foster]

On Thu, Feb 02, 2023 at 08:44:23PM +0000, Matthew Wilcox (Oracle) wrote:
> POSIX requires that "If the file size is increased, the extended area
> shall appear as if it were zero-filled".  It is possible to use mmap to
> write past EOF and that data will become visible instead of zeroes.
> This fixes the problem for the filesystems which simply call
> truncate_setsize().  More complex filesystems will need their own
> patches.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  mm/truncate.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/truncate.c b/mm/truncate.c
> index 7b4ea4c4a46b..cebfc5415e9a 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -763,9 +763,12 @@ void truncate_setsize(struct inode *inode, loff_t newsize)
>  	loff_t oldsize = inode->i_size;
>  
>  	i_size_write(inode, newsize);
> -	if (newsize > oldsize)
> +	if (newsize > oldsize) {
>  		pagecache_isize_extended(inode, oldsize, newsize);
> -	truncate_pagecache(inode, newsize);
> +		truncate_pagecache(inode, oldsize);
> +	} else {
> +		truncate_pagecache(inode, newsize);
> +	}

I don't think this alone quite addresses the problem. Looking at ext4
for example, if the eof page is dirty and writeback occurs between the
i_size update (because writeback also zeroes the post-eof portion of the
page) and the truncate_setsize() call, we end up with pagecache
inconsistency because pagecache truncate doesn't dirty the page it
zeroes.

So for example, with this series plus a nefariously placed
filemap_flush() in ext4_setattr():

# xfs_io -fc "truncate 1" -c "mmap 0 1k" -c "mwrite 0 10" -c "truncate 5" -c "mread -v 0 5" /mnt/file
00000000:  58 00 00 00 00  X....
# umount /mnt/; mount <dev> /mnt/
# xfs_io -c "mmap 0 1k" -c "mread -v 0 5" /mnt/file 
00000000:  58 58 58 58 58  XXXXX

Brian

>  }
>  EXPORT_SYMBOL(truncate_setsize);
>  
> -- 
> 2.35.1
> 
> 

Re: [PATCH 1/5] truncate: Zero bytes after 'oldsize' if we're expanding the file

[Matthew Wilcox]

On Fri, Feb 03, 2023 at 08:00:16AM -0500, Brian Foster wrote:
> On Thu, Feb 02, 2023 at 08:44:23PM +0000, Matthew Wilcox (Oracle) wrote:
> > POSIX requires that "If the file size is increased, the extended area
> > shall appear as if it were zero-filled".  It is possible to use mmap to
> > write past EOF and that data will become visible instead of zeroes.
> > This fixes the problem for the filesystems which simply call
> > truncate_setsize().  More complex filesystems will need their own
> > patches.
> > 
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > ---
> >  mm/truncate.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/mm/truncate.c b/mm/truncate.c
> > index 7b4ea4c4a46b..cebfc5415e9a 100644
> > --- a/mm/truncate.c
> > +++ b/mm/truncate.c
> > @@ -763,9 +763,12 @@ void truncate_setsize(struct inode *inode, loff_t newsize)
> >  	loff_t oldsize = inode->i_size;
> >  
> >  	i_size_write(inode, newsize);
> > -	if (newsize > oldsize)
> > +	if (newsize > oldsize) {
> >  		pagecache_isize_extended(inode, oldsize, newsize);
> > -	truncate_pagecache(inode, newsize);
> > +		truncate_pagecache(inode, oldsize);
> > +	} else {
> > +		truncate_pagecache(inode, newsize);
> > +	}
> 
> I don't think this alone quite addresses the problem. Looking at ext4
> for example, if the eof page is dirty and writeback occurs between the
> i_size update (because writeback also zeroes the post-eof portion of the
> page) and the truncate_setsize() call, we end up with pagecache
> inconsistency because pagecache truncate doesn't dirty the page it
> zeroes.
> 
> So for example, with this series plus a nefariously placed
> filemap_flush() in ext4_setattr():
> 
> # xfs_io -fc "truncate 1" -c "mmap 0 1k" -c "mwrite 0 10" -c "truncate 5" -c "mread -v 0 5" /mnt/file
> 00000000:  58 00 00 00 00  X....
> # umount /mnt/; mount <dev> /mnt/
> # xfs_io -c "mmap 0 1k" -c "mread -v 0 5" /mnt/file 
> 00000000:  58 58 58 58 58  XXXXX

Hm, so switch the order of i_size_write() and truncate_pagecache()?
There could still be a store between old-EOF and new-EOF from another
thread, which would then be visible, but I don't think you could prove
that store should have been zeroed.  Not from the thread doing the
ftruncate() anyway -- I think the thread doing the store could prove
it, but that thread is relying on undefined behaviour anyway.

[PATCH 2/5] ext4: Zero bytes after 'oldsize' if we're expanding the file

[Matthew Wilcox (Oracle)]

POSIX requires that "If the file size is increased, the extended area
shall appear as if it were zero-filled".  It is possible to use mmap to
write past EOF and that data will become visible instead of zeroes.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/ext4/inode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 9d9f414f99fe..14e3fa99f733 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5604,6 +5604,7 @@ int ext4_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 			if (!shrink) {
 				pagecache_isize_extended(inode, oldsize,
 							 inode->i_size);
+				truncate_pagecache(inode, oldsize);
 			} else if (ext4_should_journal_data(inode)) {
 				ext4_wait_for_tail_page_commit(inode);
 			}
-- 
2.35.1

[PATCH 3/5] tmpfs: Zero bytes after 'oldsize' if we're expanding the file

[Matthew Wilcox (Oracle)]

POSIX requires that "If the file size is increased, the extended area
shall appear as if it were zero-filled".  It is possible to use mmap to
write past EOF and that data will become visible instead of zeroes.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/shmem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/shmem.c b/mm/shmem.c
index 0005ab2c29af..2c8e8b417b00 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1124,6 +1124,8 @@ static int shmem_setattr(struct user_namespace *mnt_userns,
 			if (oldsize > holebegin)
 				unmap_mapping_range(inode->i_mapping,
 							holebegin, 0, 1);
+		} else {
+			shmem_truncate_range(inode, oldsize, newsize);
 		}
 	}
 
-- 
2.35.1

[PATCH 4/5] afs: Zero bytes after 'oldsize' if we're expanding the file

[Matthew Wilcox (Oracle)]

POSIX requires that "If the file size is increased, the extended area
shall appear as if it were zero-filled".  It is possible to use mmap to
write past EOF and that data will become visible instead of zeroes.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/afs/inode.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/afs/inode.c b/fs/afs/inode.c
index 6d3a3dbe4928..92e2ba7625de 100644
--- a/fs/afs/inode.c
+++ b/fs/afs/inode.c
@@ -854,6 +854,8 @@ static void afs_setattr_edit_file(struct afs_operation *op)
 
 		if (size < i_size)
 			truncate_pagecache(inode, size);
+		else
+			truncate_pagecache(inode, i_size);
 		if (size != i_size)
 			fscache_resize_cookie(afs_vnode_cache(vp->vnode),
 					      vp->scb.status.size);
-- 
2.35.1

[PATCH 5/5] btrfs: Zero bytes after 'oldsize' if we're expanding the file

[Matthew Wilcox (Oracle)]

POSIX requires that "If the file size is increased, the extended area
shall appear as if it were zero-filled".  It is possible to use mmap to
write past EOF and that data will become visible instead of zeroes.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/btrfs/inode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 98a800b8bd43..b61ec4bb9cf0 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5234,6 +5234,7 @@ static int btrfs_setsize(struct inode *inode, struct iattr *attr)
 		i_size_write(inode, newsize);
 		btrfs_inode_safe_disk_i_size_write(BTRFS_I(inode), 0);
 		pagecache_isize_extended(inode, oldsize, newsize);
+		truncate_pagecache(inode, oldsize);
 		ret = btrfs_update_inode(trans, root, BTRFS_I(inode));
 		btrfs_drew_write_unlock(&root->snapshot_lock);
 		btrfs_end_transaction(trans);
-- 
2.35.1

[PATCH 6/5] generic: test ftruncate zeroes bytes after EOF

[Matthew Wilcox (Oracle)]

https://pubs.opengroup.org/onlinepubs/9699919799/functions/ftruncate.html
specifies that "If the file size is increased, the extended area shall
appear as if it were zero-filled."  Many filesystems do not currently
do this for the portion of the page after EOF.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 .gitignore            |  1 +
 src/Makefile          |  2 +-
 src/truncate-zero.c   | 50 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/707     | 31 +++++++++++++++++++++++++++
 tests/generic/707.out |  2 ++
 5 files changed, 85 insertions(+), 1 deletion(-)
 create mode 100644 src/truncate-zero.c
 create mode 100755 tests/generic/707
 create mode 100644 tests/generic/707.out

diff --git a/.gitignore b/.gitignore
index a6f433f1..6aa5bca9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -169,6 +169,7 @@ tags
 /src/test-nextquota
 /src/testx
 /src/trunc
+/src/truncate-zero
 /src/truncfile
 /src/unwritten_mmap
 /src/unwritten_sync
diff --git a/src/Makefile b/src/Makefile
index afdf6b30..83ca11ac 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -19,7 +19,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
 	t_ofd_locks t_mmap_collision mmap-write-concurrent \
 	t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \
 	t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \
-	t_mmap_cow_memory_failure fake-dump-rootino
+	t_mmap_cow_memory_failure fake-dump-rootino truncate-zero
 
 LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
 	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
diff --git a/src/truncate-zero.c b/src/truncate-zero.c
new file mode 100644
index 00000000..67f53912
--- /dev/null
+++ b/src/truncate-zero.c
@@ -0,0 +1,50 @@
+#include <fcntl.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <unistd.h>
+
+int main(int argc, char **argv)
+{
+	char *buf;
+	int i, fd;
+
+	if (argc != 2) {
+		fprintf(stderr, "Usage: %s <file>\n", argv[0]);
+		return 1;
+	}
+
+	fd = open(argv[1], O_RDWR | O_CREAT, 0644);
+	if (fd < 0) {
+		perror(argv[1]);
+		return 1;
+	}
+
+	if (ftruncate(fd, 1) < 0) {
+		perror("ftruncate");
+		return 1;
+	}
+
+	buf = mmap(NULL, 1024, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
+	if (buf == MAP_FAILED) {
+		perror("mmap");
+		return 1;
+	}
+
+	memset(buf, 'a', 10);
+
+	if (ftruncate(fd, 5) < 0) {
+		perror("ftruncate");
+		return 1;
+	}
+
+	if (memcmp(buf, "a\0\0\0\0", 5) == 0)
+		return 0;
+
+	fprintf(stderr, "Truncation did not zero new bytes:\n");
+	for (i = 0; i < 5; i++)
+		fprintf(stderr, "%#x ", buf[i]);
+	fputc('\n', stderr);
+
+	return 2;
+}
diff --git a/tests/generic/707 b/tests/generic/707
new file mode 100755
index 00000000..ddc82a9a
--- /dev/null
+++ b/tests/generic/707
@@ -0,0 +1,31 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2023 Matthew Wilcox for Oracle.  All Rights Reserved.
+#
+# FS QA Test 707
+#
+# Test whether we obey this part of POSIX-2017 ftruncate:
+# "If the file size is increased, the extended area shall appear as if
+# it were zero-filled"
+#
+. ./common/preamble
+_begin_fstest auto quick posix
+
+_supported_fs generic
+_require_test
+_require_test_program "truncate-zero"
+
+test_file=$TEST_DIR/test.$seq
+
+_cleanup()
+{
+	cd /
+	rm -f $test_file
+}
+
+$here/src/truncate-zero $test_file > $seqres.full 2>&1 ||
+	_fail "truncate zero failed!"
+
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/generic/707.out b/tests/generic/707.out
new file mode 100644
index 00000000..8e57a1d8
--- /dev/null
+++ b/tests/generic/707.out
@@ -0,0 +1,2 @@
+QA output created by 707
+Silence is golden
-- 
2.35.1

Re: [PATCH 6/5] generic: test ftruncate zeroes bytes after EOF

[Johannes Thumshirn]

On 02.02.23 21:45, Matthew Wilcox (Oracle) wrote:
> +	fprintf(stderr, "Truncation did not zero new bytes:\n");
> +	for (i = 0; i < 5; i++)
> +		fprintf(stderr, "%#x ", buf[i]);
> +	fputc('\n', stderr);
>

[...]

> +
> +$here/src/truncate-zero $test_file > $seqres.full 2>&1 ||
> +	_fail "truncate zero failed!"
> +
Is '_fail' really needed here? truncate-zero will spit out an error message
in case the truncation doesn't work.

Re: [PATCH 6/5] generic: test ftruncate zeroes bytes after EOF

[Matthew Wilcox]

On Fri, Feb 03, 2023 at 11:57:11AM +0000, Johannes Thumshirn wrote:
> On 02.02.23 21:45, Matthew Wilcox (Oracle) wrote:
> > +	fprintf(stderr, "Truncation did not zero new bytes:\n");
> > +	for (i = 0; i < 5; i++)
> > +		fprintf(stderr, "%#x ", buf[i]);
> > +	fputc('\n', stderr);
> >
> 
> [...]
> 
> > +
> > +$here/src/truncate-zero $test_file > $seqres.full 2>&1 ||
> > +	_fail "truncate zero failed!"
> > +
> Is '_fail' really needed here? truncate-zero will spit out an error message
> in case the truncation doesn't work.

I don't know what I'm doing.  I totally cargo-culted 706 to make 707.
If someone wants to completely rewrite it, go ahead!

Re: [PATCH 6/5] generic: test ftruncate zeroes bytes after EOF

[Darrick J. Wong]

On Thu, Feb 02, 2023 at 08:44:28PM +0000, Matthew Wilcox (Oracle) wrote:
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/ftruncate.html
> specifies that "If the file size is increased, the extended area shall
> appear as if it were zero-filled."  Many filesystems do not currently
> do this for the portion of the page after EOF.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  .gitignore            |  1 +
>  src/Makefile          |  2 +-
>  src/truncate-zero.c   | 50 +++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/707     | 31 +++++++++++++++++++++++++++
>  tests/generic/707.out |  2 ++
>  5 files changed, 85 insertions(+), 1 deletion(-)
>  create mode 100644 src/truncate-zero.c
>  create mode 100755 tests/generic/707
>  create mode 100644 tests/generic/707.out
> 
> diff --git a/.gitignore b/.gitignore
> index a6f433f1..6aa5bca9 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -169,6 +169,7 @@ tags
>  /src/test-nextquota
>  /src/testx
>  /src/trunc
> +/src/truncate-zero
>  /src/truncfile
>  /src/unwritten_mmap
>  /src/unwritten_sync
> diff --git a/src/Makefile b/src/Makefile
> index afdf6b30..83ca11ac 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -19,7 +19,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
>  	t_ofd_locks t_mmap_collision mmap-write-concurrent \
>  	t_get_file_time t_create_short_dirs t_create_long_dirs t_enospc \
>  	t_mmap_writev_overlap checkpoint_journal mmap-rw-fault allocstale \
> -	t_mmap_cow_memory_failure fake-dump-rootino
> +	t_mmap_cow_memory_failure fake-dump-rootino truncate-zero
>  
>  LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
>  	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
> diff --git a/src/truncate-zero.c b/src/truncate-zero.c
> new file mode 100644
> index 00000000..67f53912
> --- /dev/null
> +++ b/src/truncate-zero.c
> @@ -0,0 +1,50 @@

Needs to have a SPDX header and the customary Oracle copyright.

> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +#include <unistd.h>
> +
> +int main(int argc, char **argv)
> +{
> +	char *buf;
> +	int i, fd;
> +
> +	if (argc != 2) {
> +		fprintf(stderr, "Usage: %s <file>\n", argv[0]);
> +		return 1;
> +	}
> +
> +	fd = open(argv[1], O_RDWR | O_CREAT, 0644);
> +	if (fd < 0) {
> +		perror(argv[1]);
> +		return 1;
> +	}
> +
> +	if (ftruncate(fd, 1) < 0) {
> +		perror("ftruncate");
> +		return 1;
> +	}
> +
> +	buf = mmap(NULL, 1024, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
> +	if (buf == MAP_FAILED) {
> +		perror("mmap");
> +		return 1;
> +	}
> +
> +	memset(buf, 'a', 10);
> +
> +	if (ftruncate(fd, 5) < 0) {
> +		perror("ftruncate");
> +		return 1;
> +	}
> +
> +	if (memcmp(buf, "a\0\0\0\0", 5) == 0)
> +		return 0;
> +
> +	fprintf(stderr, "Truncation did not zero new bytes:\n");
> +	for (i = 0; i < 5; i++)
> +		fprintf(stderr, "%#x ", buf[i]);
> +	fputc('\n', stderr);
> +
> +	return 2;
> +}
> diff --git a/tests/generic/707 b/tests/generic/707
> new file mode 100755
> index 00000000..ddc82a9a
> --- /dev/null
> +++ b/tests/generic/707
> @@ -0,0 +1,31 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2023 Matthew Wilcox for Oracle.  All Rights Reserved.
> +#
> +# FS QA Test 707
> +#
> +# Test whether we obey this part of POSIX-2017 ftruncate:
> +# "If the file size is increased, the extended area shall appear as if
> +# it were zero-filled"
> +#
> +. ./common/preamble
> +_begin_fstest auto quick posix
> +
> +_supported_fs generic
> +_require_test
> +_require_test_program "truncate-zero"
> +
> +test_file=$TEST_DIR/test.$seq
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $test_file
> +}
> +
> +$here/src/truncate-zero $test_file > $seqres.full 2>&1 ||
> +	_fail "truncate zero failed!"

Omit the _fail here because any extra stdout/stderr output that is not
in the .out file suffices to record the test as failed.

_fail is only useful if you need to exit the shell script immediately.

> +
> +echo "Silence is golden"

It's customary (though not required) to put "silence is golden" before
"but my eyes still see"^W^W^W^W^Wthe test starts running programs.

Other than those minor things, this looks reasonable to me.

--D

> +status=0
> +exit
> diff --git a/tests/generic/707.out b/tests/generic/707.out
> new file mode 100644
> index 00000000..8e57a1d8
> --- /dev/null
> +++ b/tests/generic/707.out
> @@ -0,0 +1,2 @@
> +QA output created by 707
> +Silence is golden
> -- 
> 2.35.1
> 

Re: [PATCH 0/5] Fix a minor POSIX conformance problem

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1887 bytes --]

On Feb 2, 2023, at 1:44 PM, Matthew Wilcox (Oracle) <willy@infradead.org> wrote:
> 
> POSIX requires that on ftruncate() expansion, the new bytes must read
> as zeroes.  If someone's mmap()ed the file and stored past EOF, for
> most filesystems the bytes in that page will be not-zero.  It's a
> pretty minor violation; someone could race you and write to the file
> between the ftruncate() call and you reading from it, but it's a bit
> of a QOI violation.

Is it possible to have mmap return SIGBUS for the writes beyond EOF?
On the one hand, that might indicate incorrect behavior of the application,
and on the other hand, it seems possible that the application doesn't
know it is writing beyond EOF and expects that data to be read back OK?

What happens if it is writing beyond EOF, but the block hasn't even been
allocated because PAGE_SIZE > blocksize?

IMHO, this seems better to stop the root of the problem (mmap() allowing
bad writes), rather than trying to fix it after the fact.

Cheers, Andreas

> I've tested xfs (passes before & after), ext4 and tmpfs (both fail
> before, pass after).  Testing from other FS developers appreciated.
> fstest to follow; not sure how to persuade git-send-email to work on
> multiple repositories
> 
> Matthew Wilcox (Oracle) (5):
>  truncate: Zero bytes after 'oldsize' if we're expanding the file
>  ext4: Zero bytes after 'oldsize' if we're expanding the file
>  tmpfs: Zero bytes after 'oldsize' if we're expanding the file
>  afs: Zero bytes after 'oldsize' if we're expanding the file
>  btrfs: Zero bytes after 'oldsize' if we're expanding the file
> 
> fs/afs/inode.c   | 2 ++
> fs/btrfs/inode.c | 1 +
> fs/ext4/inode.c  | 1 +
> mm/shmem.c       | 2 ++
> mm/truncate.c    | 7 +++++--
> 5 files changed, 11 insertions(+), 2 deletions(-)
> 
> --
> 2.35.1
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH 0/5] Fix a minor POSIX conformance problem

[Matthew Wilcox]

On Thu, Feb 02, 2023 at 04:08:49PM -0700, Andreas Dilger wrote:
> On Feb 2, 2023, at 1:44 PM, Matthew Wilcox (Oracle) <willy@infradead.org> wrote:
> > 
> > POSIX requires that on ftruncate() expansion, the new bytes must read
> > as zeroes.  If someone's mmap()ed the file and stored past EOF, for
> > most filesystems the bytes in that page will be not-zero.  It's a
> > pretty minor violation; someone could race you and write to the file
> > between the ftruncate() call and you reading from it, but it's a bit
> > of a QOI violation.
> 
> Is it possible to have mmap return SIGBUS for the writes beyond EOF?

Well, no.  The hardware only tells us about accesses on a per-page
basis.  We could SIGBUS on writes that _start_ after EOF, but this
test doesn't do that (it starts before EOF and extends past EOF).
And once the page is mapped writable, there's no page fault taken
for subsequent writes.

> On the one hand, that might indicate incorrect behavior of the application,
> and on the other hand, it seems possible that the application doesn't
> know it is writing beyond EOF and expects that data to be read back OK?

POSIX says:

"The system shall always zero-fill any partial page at the end of an
object. Further, the system shall never write out any modified portions
of the last page of an object which are beyond its end. References
within the address range starting at pa and continuing for len bytes to
whole pages following the end of an object shall result in delivery of
a SIGBUS signal."

https://pubs.opengroup.org/onlinepubs/9699919799/functions/mmap.html

So the application can't expect to read back anything it's written
(and if you look at page writeback, we currently zero beyond EOF at
writeback time).

> IMHO, this seems better to stop the root of the problem (mmap() allowing
> bad writes), rather than trying to fix it after the fact.

That would be nice, but we're rather stuck with the hardware that exists.
IIUC Cray-1 had byte-granularity range registers, but page-granularity
is what we have.

RE: [PATCH 0/5] Fix a minor POSIX conformance problem

[David Laight]

From: Matthew Wilcox
> Sent: 03 February 2023 13:21
> 
> On Thu, Feb 02, 2023 at 04:08:49PM -0700, Andreas Dilger wrote:
> > On Feb 2, 2023, at 1:44 PM, Matthew Wilcox (Oracle) <willy@infradead.org> wrote:
> > >
> > > POSIX requires that on ftruncate() expansion, the new bytes must read
> > > as zeroes.  If someone's mmap()ed the file and stored past EOF, for
> > > most filesystems the bytes in that page will be not-zero.  It's a
> > > pretty minor violation; someone could race you and write to the file
> > > between the ftruncate() call and you reading from it, but it's a bit
> > > of a QOI violation.
> >
> > Is it possible to have mmap return SIGBUS for the writes beyond EOF?
> 
> Well, no.  The hardware only tells us about accesses on a per-page
> basis.  We could SIGBUS on writes that _start_ after EOF, but this
> test doesn't do that (it starts before EOF and extends past EOF).
> And once the page is mapped writable, there's no page fault taken
> for subsequent writes.
> 
> > On the one hand, that might indicate incorrect behavior of the application,
> > and on the other hand, it seems possible that the application doesn't
> > know it is writing beyond EOF and expects that data to be read back OK?
> 
> POSIX says:
> 
> "The system shall always zero-fill any partial page at the end of an
> object. Further, the system shall never write out any modified portions
> of the last page of an object which are beyond its end. References
> within the address range starting at pa and continuing for len bytes to
> whole pages following the end of an object shall result in delivery of
> a SIGBUS signal."
> 
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/mmap.html

It also says (down at the bottom of the rational):

"The mmap() function can be used to map a region of memory that is larger
than the current size of the object. Memory access within the mapping but
beyond the current end of the underlying objects may result in SIGBUS
signals being sent to the process. The reason for this is that the size
of the object can be manipulated by other processes and can change at any
moment. The implementation should tell the application that a memory
reference is outside the object where this can be detected; otherwise,
written data may be lost and read data may not reflect actual data in the
object."

There are a lot of 'may' in that sentence.
Note that it only says that 'data written beyond the current eof may be
lost'.
I think that could be taken to take precedence over the zeroing clause
in ftruncate().
I'd bet a lot of beer that the original SYSV implementation (on with the
description is based) didn't zero the page buffer when ftruncate()
increased the file size.
Whether anything (important) actually relies on that is an interesting
question!

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 0/5] Fix a minor POSIX conformance problem

[Matthew Wilcox]

On Fri, Feb 03, 2023 at 04:23:32PM +0000, David Laight wrote:
> From: Matthew Wilcox
> > "The system shall always zero-fill any partial page at the end of an
> > object. Further, the system shall never write out any modified portions
> > of the last page of an object which are beyond its end. References
> > within the address range starting at pa and continuing for len bytes to
> > whole pages following the end of an object shall result in delivery of
> > a SIGBUS signal."
> > 
> > https://pubs.opengroup.org/onlinepubs/9699919799/functions/mmap.html
> 
> It also says (down at the bottom of the rational):
> 
> "The mmap() function can be used to map a region of memory that is larger
> than the current size of the object. Memory access within the mapping but
> beyond the current end of the underlying objects may result in SIGBUS
> signals being sent to the process. The reason for this is that the size
> of the object can be manipulated by other processes and can change at any
> moment. The implementation should tell the application that a memory
> reference is outside the object where this can be detected; otherwise,
> written data may be lost and read data may not reflect actual data in the
> object."
> 
> There are a lot of 'may' in that sentence.
> Note that it only says that 'data written beyond the current eof may be
> lost'.
> I think that could be taken to take precedence over the zeroing clause
> in ftruncate().

How can the _rationale_ (explicitly labelled as informative) for one
function take precedence over the requirements for another function?
This is nonsense.

> I'd bet a lot of beer that the original SYSV implementation (on with the
> description is based) didn't zero the page buffer when ftruncate()
> increased the file size.
> Whether anything (important) actually relies on that is an interesting
> question!
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

Re: [PATCH 4/5] afs: Zero bytes after 'oldsize' if we're expanding the file

[David Howells]

Matthew Wilcox (Oracle) <willy@infradead.org> wrote:

> POSIX requires that "If the file size is increased, the extended area
> shall appear as if it were zero-filled".  It is possible to use mmap to
> write past EOF and that data will become visible instead of zeroes.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

That seems to work.  Do you want me to pass it on to Linus?  If not:

Acked-by: David Howells <dhowells@redhat.com>

Re: [PATCH 4/5] afs: Zero bytes after 'oldsize' if we're expanding the file

[Matthew Wilcox]

On Mon, Feb 27, 2023 at 01:49:27PM +0000, David Howells wrote:
> Matthew Wilcox (Oracle) <willy@infradead.org> wrote:
> 
> > POSIX requires that "If the file size is increased, the extended area
> > shall appear as if it were zero-filled".  It is possible to use mmap to
> > write past EOF and that data will become visible instead of zeroes.
> > 
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> That seems to work.  Do you want me to pass it on to Linus?  If not:
> 
> Acked-by: David Howells <dhowells@redhat.com>

I'll send a patch series with all of this; it doesn't seem terribly
urgent.  Do you think there's a similar problem for AFS that Brian
noted with the generic patch?

Re: [PATCH 4/5] afs: Zero bytes after 'oldsize' if we're expanding the file

[David Howells]

Matthew Wilcox <willy@infradead.org> wrote:

> I'll send a patch series with all of this; it doesn't seem terribly
> urgent.  Do you think there's a similar problem for AFS that Brian
> noted with the generic patch?

Do you have a link I can look at?

David

Re: [PATCH 4/5] afs: Zero bytes after 'oldsize' if we're expanding the file

[David Howells]

Matthew Wilcox <willy@infradead.org> wrote:

> I'll send a patch series with all of this; it doesn't seem terribly
> urgent.  Do you think there's a similar problem for AFS that Brian
> noted with the generic patch?

Probably not.  To avoid deadlocking itself, afs uses a mutex to prevent
writepages racing with truncate (vnode->validate_lock).

	commit ec0fa0b659144d9c68204d23f627b6a65fa53e50
	afs: Fix deadlock between writeback and truncate

the afs_setattr_edit_file() call that changes i_size and partially clears the
pagecache is applied to the local inode before the mutex is dropped.

David