[PATCH 1/2] misc: don't oom the box opening tmpfiles

[PATCH 1/2] misc: don't oom the box opening tmpfiles

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

For the t_open_tmpfiles tests, limit ourselves to half of file-max so
that we don't OOM the test machine.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 tests/generic/530 |    2 +-
 tests/generic/531 |    2 +-
 tests/xfs/501     |    2 +-
 tests/xfs/502     |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)


diff --git a/tests/generic/530 b/tests/generic/530
index a2968d25..2bc4a992 100755
--- a/tests/generic/530
+++ b/tests/generic/530
@@ -42,7 +42,7 @@ _scratch_mount
 # Set ULIMIT_NOFILE to min(file-max, 50000 files per LOAD_FACTOR)
 # so that this test doesn't take forever or OOM the box
 max_files=$((50000 * LOAD_FACTOR))
-max_allowable_files=$(( $(cat /proc/sys/fs/file-max) ))
+max_allowable_files=$(( $(cat /proc/sys/fs/file-max) / 2 ))
 test $max_allowable_files -gt 0 && test $max_files -gt $max_allowable_files && \
 	max_files=$max_allowable_files
 ulimit -n $max_files
diff --git a/tests/generic/531 b/tests/generic/531
index f3eb5cde..5d60e4b6 100755
--- a/tests/generic/531
+++ b/tests/generic/531
@@ -44,7 +44,7 @@ nr_cpus=$(( $(getconf _NPROCESSORS_ONLN) * 2 ))
 # Set ULIMIT_NOFILE to min(file-max, 50000 files per LOAD_FACTOR)
 # so that this test doesn't take forever or OOM the box
 max_files=$((50000 * LOAD_FACTOR))
-max_allowable_files=$(( $(cat /proc/sys/fs/file-max) ))
+max_allowable_files=$(( $(cat /proc/sys/fs/file-max) / 2 ))
 test $max_allowable_files -gt 0 && test $max_files -gt $max_allowable_files && \
 	max_files=$max_allowable_files
 ulimit -n $max_files
diff --git a/tests/xfs/501 b/tests/xfs/501
index 51cdb020..d689145f 100755
--- a/tests/xfs/501
+++ b/tests/xfs/501
@@ -47,7 +47,7 @@ _scratch_mount
 # Set ULIMIT_NOFILE to min(file-max, 30000 files per LOAD_FACTOR)
 # so that this test doesn't take forever or OOM the box
 max_files=$((30000 * LOAD_FACTOR))
-max_allowable_files=$(( $(cat /proc/sys/fs/file-max) ))
+max_allowable_files=$(( $(cat /proc/sys/fs/file-max) / 2 ))
 test $max_allowable_files -gt 0 && test $max_files -gt $max_allowable_files && \
 	max_files=$max_allowable_files
 ulimit -n $max_files
diff --git a/tests/xfs/502 b/tests/xfs/502
index bfb063f4..5ad10316 100755
--- a/tests/xfs/502
+++ b/tests/xfs/502
@@ -46,7 +46,7 @@ nr_cpus=$(( $(getconf _NPROCESSORS_ONLN) * 2 ))
 # Set ULIMIT_NOFILE to min(file-max, 30000 files per cpu per LOAD_FACTOR)
 # so that this test doesn't take forever or OOM the box
 max_files=$((30000 * LOAD_FACTOR))
-max_allowable_files=$(( $(cat /proc/sys/fs/file-max) ))
+max_allowable_files=$(( $(cat /proc/sys/fs/file-max) / 2 ))
 test $max_allowable_files -gt 0 && test $max_files -gt $max_allowable_files && \
 	max_files=$max_allowable_files
 ulimit -n $max_files

[PATCH 2/2] t_attr_corruption: fix this yet again

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Jeff Moyer pointed out that 'security.evm' actually has an expected
value format, which breaks the test if EVM is enabled.  It turns out
that the 'security.evm' setxattr call in the original syzkaller report
was a total red herring, as this bug can be reproduced without it.

Fix the test case to do the minimum amount of work needed to reproduce
the corruption.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 src/t_attr_corruption.c |   28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)


diff --git a/src/t_attr_corruption.c b/src/t_attr_corruption.c
index f26611f9..9101024e 100644
--- a/src/t_attr_corruption.c
+++ b/src/t_attr_corruption.c
@@ -3,17 +3,21 @@
  * Copyright (C) 2019 Oracle.  All Rights Reserved.
  * Author: Darrick J. Wong <darrick.wong@oracle.com>
  *
- * Test program to tickle a use-after-free bug in xfs.
+ * XFS had a memory corruption bug in its handling of the POSIX ACL attribute
+ * names during a listxattr call.
  *
- * XFS had a use-after-free bug when xfs_xattr_put_listent runs out of
- * listxattr buffer space while trying to store the name
- * "system.posix_acl_access" and then corrupts memory by not checking the
- * seen_enough state and then trying to shove "trusted.SGI_ACL_FILE" into the
- * buffer as well.
+ * On IRIX, file ACLs were stored under the name "trusted.SGI_ACL_FILE",
+ * whereas on Linux the name is "system.posix_acl_access".  In order to
+ * maintain compatibility with old filesystems, XFS internally continues to
+ * use the old SGI_ACL_FILE name on disk and remap the new name whenever it
+ * sees it.
  *
- * In order to tickle the bug in a user visible way we must have already put a
- * name in the buffer, so we take advantage of the fact that "security.evm"
- * sorts before "system.posix_acl_access" to make sure this happens.
+ * In order to make this magic happen, XFS' listxattr implementation will emit
+ * first the Linux name and then the on-disk name.  Unfortunately, it doesn't
+ * correctly check the buffer length, so if the buffer is large enough to fit
+ * the on-disk name but not large enough to fit the Linux name, we screw up
+ * the buffer position accounting while trying to emit the Linux name and then
+ * corrupt memory when we try to emit the on-disk name.
  *
  * If we trigger the bug, the program will print the garbled string
  * "rusted.SGI_ACL_FILE".  If the bug is fixed, the flistxattr call returns
@@ -76,11 +80,7 @@ int main(int argc, char *argv[])
 	if (ret)
 		die("set posix acl");
 
-	ret = fsetxattr(fd, "security.evm", buf, 1, 1);
-	if (ret)
-		die("set evm");
-
-	sz = flistxattr(fd, buf, 30);
+	sz = flistxattr(fd, buf, 20);
 	if (sz < 0)
 		die("list attr");
 

[PATCH 3/2] xfs/010: use correct type for finobt corrupting

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Use 'type finobt' for corrupting the finobt.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 tests/xfs/010 |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/xfs/010 b/tests/xfs/010
index ee1595c8..e220a651 100755
--- a/tests/xfs/010
+++ b/tests/xfs/010
@@ -63,14 +63,14 @@ _corrupt_finobt_records()
 			_filter_dbval`
 
 	# Corrupt a freecount value. This should never exceed 64.
-	$XFS_DB_PROG -x -c "fsb $free_root" -c "type inobt" \
+	$XFS_DB_PROG -x -c "fsb $free_root" -c "type finobt" \
 		-c "write recs[1].freecount 70" $dev
 
 	# Create a corrupted non-free record, which should never appear in the
 	# finobt.
-	$XFS_DB_PROG -x -c "fsb $free_root" -c "type inobt" \
+	$XFS_DB_PROG -x -c "fsb $free_root" -c "type finobt" \
 		 -c "write recs[2].freecount 0" $dev
-	$XFS_DB_PROG -x -c "fsb $free_root" -c "type inobt" \
+	$XFS_DB_PROG -x -c "fsb $free_root" -c "type finobt" \
 		-c "write recs[2].free 0" $dev
 }
 

Re: [PATCH 1/2] misc: don't oom the box opening tmpfiles

[Allison Henderson]

On 2/25/19 7:35 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> For the t_open_tmpfiles tests, limit ourselves to half of file-max so
> that we don't OOM the test machine.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   tests/generic/530 |    2 +-
>   tests/generic/531 |    2 +-
>   tests/xfs/501     |    2 +-
>   tests/xfs/502     |    2 +-
>   4 files changed, 4 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/tests/generic/530 b/tests/generic/530
> index a2968d25..2bc4a992 100755
> --- a/tests/generic/530
> +++ b/tests/generic/530
> @@ -42,7 +42,7 @@ _scratch_mount
>   # Set ULIMIT_NOFILE to min(file-max, 50000 files per LOAD_FACTOR)
>   # so that this test doesn't take forever or OOM the box
>   max_files=$((50000 * LOAD_FACTOR))
> -max_allowable_files=$(( $(cat /proc/sys/fs/file-max) ))
> +max_allowable_files=$(( $(cat /proc/sys/fs/file-max) / 2 ))
>   test $max_allowable_files -gt 0 && test $max_files -gt $max_allowable_files && \
>   	max_files=$max_allowable_files
>   ulimit -n $max_files
> diff --git a/tests/generic/531 b/tests/generic/531
> index f3eb5cde..5d60e4b6 100755
> --- a/tests/generic/531
> +++ b/tests/generic/531
> @@ -44,7 +44,7 @@ nr_cpus=$(( $(getconf _NPROCESSORS_ONLN) * 2 ))
>   # Set ULIMIT_NOFILE to min(file-max, 50000 files per LOAD_FACTOR)
>   # so that this test doesn't take forever or OOM the box
>   max_files=$((50000 * LOAD_FACTOR))
> -max_allowable_files=$(( $(cat /proc/sys/fs/file-max) ))
> +max_allowable_files=$(( $(cat /proc/sys/fs/file-max) / 2 ))

This looks like this would certainly help, but wouldn't we want it to be 
something more like file-max - file-nr ?  Or something similar?  I'm 
just thinking the threshold at which we pop the file limit would 
probably be more dependent on how many files are already allocated about 
the system.  The 2 probably solves it most of the time, but it's 
certainly possible that file-max / 2 may still be too much in some 
cases.  Thoughts?

Allison

>   test $max_allowable_files -gt 0 && test $max_files -gt $max_allowable_files && \
>   	max_files=$max_allowable_files
>   ulimit -n $max_files
> diff --git a/tests/xfs/501 b/tests/xfs/501
> index 51cdb020..d689145f 100755
> --- a/tests/xfs/501
> +++ b/tests/xfs/501
> @@ -47,7 +47,7 @@ _scratch_mount
>   # Set ULIMIT_NOFILE to min(file-max, 30000 files per LOAD_FACTOR)
>   # so that this test doesn't take forever or OOM the box
>   max_files=$((30000 * LOAD_FACTOR))
> -max_allowable_files=$(( $(cat /proc/sys/fs/file-max) ))
> +max_allowable_files=$(( $(cat /proc/sys/fs/file-max) / 2 ))
>   test $max_allowable_files -gt 0 && test $max_files -gt $max_allowable_files && \
>   	max_files=$max_allowable_files
>   ulimit -n $max_files
> diff --git a/tests/xfs/502 b/tests/xfs/502
> index bfb063f4..5ad10316 100755
> --- a/tests/xfs/502
> +++ b/tests/xfs/502
> @@ -46,7 +46,7 @@ nr_cpus=$(( $(getconf _NPROCESSORS_ONLN) * 2 ))
>   # Set ULIMIT_NOFILE to min(file-max, 30000 files per cpu per LOAD_FACTOR)
>   # so that this test doesn't take forever or OOM the box
>   max_files=$((30000 * LOAD_FACTOR))
> -max_allowable_files=$(( $(cat /proc/sys/fs/file-max) ))
> +max_allowable_files=$(( $(cat /proc/sys/fs/file-max) / 2 ))
>   test $max_allowable_files -gt 0 && test $max_files -gt $max_allowable_files && \
>   	max_files=$max_allowable_files
>   ulimit -n $max_files
> 

Re: [PATCH 2/2] t_attr_corruption: fix this yet again

[Allison Henderson]

On 2/25/19 7:35 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Jeff Moyer pointed out that 'security.evm' actually has an expected
> value format, which breaks the test if EVM is enabled.  It turns out
> that the 'security.evm' setxattr call in the original syzkaller report
> was a total red herring, as this bug can be reproduced without it.
> 
> Fix the test case to do the minimum amount of work needed to reproduce
> the corruption.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   src/t_attr_corruption.c |   28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)
> 
> 
> diff --git a/src/t_attr_corruption.c b/src/t_attr_corruption.c
> index f26611f9..9101024e 100644
> --- a/src/t_attr_corruption.c
> +++ b/src/t_attr_corruption.c
> @@ -3,17 +3,21 @@
>    * Copyright (C) 2019 Oracle.  All Rights Reserved.
>    * Author: Darrick J. Wong <darrick.wong@oracle.com>
>    *
> - * Test program to tickle a use-after-free bug in xfs.
> + * XFS had a memory corruption bug in its handling of the POSIX ACL attribute
> + * names during a listxattr call.
>    *
> - * XFS had a use-after-free bug when xfs_xattr_put_listent runs out of
> - * listxattr buffer space while trying to store the name
> - * "system.posix_acl_access" and then corrupts memory by not checking the
> - * seen_enough state and then trying to shove "trusted.SGI_ACL_FILE" into the
> - * buffer as well.
> + * On IRIX, file ACLs were stored under the name "trusted.SGI_ACL_FILE",
> + * whereas on Linux the name is "system.posix_acl_access".  In order to
> + * maintain compatibility with old filesystems, XFS internally continues to
> + * use the old SGI_ACL_FILE name on disk and remap the new name whenever it
> + * sees it.
>    *
> - * In order to tickle the bug in a user visible way we must have already put a
> - * name in the buffer, so we take advantage of the fact that "security.evm"
> - * sorts before "system.posix_acl_access" to make sure this happens.
> + * In order to make this magic happen, XFS' listxattr implementation will emit
> + * first the Linux name and then the on-disk name.  Unfortunately, it doesn't
> + * correctly check the buffer length, so if the buffer is large enough to fit
> + * the on-disk name but not large enough to fit the Linux name, we screw up
> + * the buffer position accounting while trying to emit the Linux name and then
> + * corrupt memory when we try to emit the on-disk name.

Ok, thanks for the explanation!  You can add my review:

Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

>    *
>    * If we trigger the bug, the program will print the garbled string
>    * "rusted.SGI_ACL_FILE".  If the bug is fixed, the flistxattr call returns
> @@ -76,11 +80,7 @@ int main(int argc, char *argv[])
>   	if (ret)
>   		die("set posix acl");
>   
> -	ret = fsetxattr(fd, "security.evm", buf, 1, 1);
> -	if (ret)
> -		die("set evm");
> -
> -	sz = flistxattr(fd, buf, 30);
> +	sz = flistxattr(fd, buf, 20);
>   	if (sz < 0)
>   		die("list attr");
>   
> 

Re: [PATCH 3/2] xfs/010: use correct type for finobt corrupting

[Allison Henderson]

On 2/25/19 8:46 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Use 'type finobt' for corrupting the finobt.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   tests/xfs/010 |    6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/xfs/010 b/tests/xfs/010
> index ee1595c8..e220a651 100755
> --- a/tests/xfs/010
> +++ b/tests/xfs/010
> @@ -63,14 +63,14 @@ _corrupt_finobt_records()
>   			_filter_dbval`
>   
>   	# Corrupt a freecount value. This should never exceed 64.
> -	$XFS_DB_PROG -x -c "fsb $free_root" -c "type inobt" \
> +	$XFS_DB_PROG -x -c "fsb $free_root" -c "type finobt" \
>   		-c "write recs[1].freecount 70" $dev
>   
>   	# Create a corrupted non-free record, which should never appear in the
>   	# finobt.
> -	$XFS_DB_PROG -x -c "fsb $free_root" -c "type inobt" \
> +	$XFS_DB_PROG -x -c "fsb $free_root" -c "type finobt" \
>   		 -c "write recs[2].freecount 0" $dev
> -	$XFS_DB_PROG -x -c "fsb $free_root" -c "type inobt" \
> +	$XFS_DB_PROG -x -c "fsb $free_root" -c "type finobt" \
>   		-c "write recs[2].free 0" $dev
>   }
>   
> 

Good catch, this looks like a pretty old test too.  Thanks for the fix!

Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

Re: [PATCH 1/2] misc: don't oom the box opening tmpfiles

[Darrick J. Wong]

On Tue, Feb 26, 2019 at 12:08:20PM -0700, Allison Henderson wrote:
> On 2/25/19 7:35 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > For the t_open_tmpfiles tests, limit ourselves to half of file-max so
> > that we don't OOM the test machine.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >   tests/generic/530 |    2 +-
> >   tests/generic/531 |    2 +-
> >   tests/xfs/501     |    2 +-
> >   tests/xfs/502     |    2 +-
> >   4 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > 
> > diff --git a/tests/generic/530 b/tests/generic/530
> > index a2968d25..2bc4a992 100755
> > --- a/tests/generic/530
> > +++ b/tests/generic/530
> > @@ -42,7 +42,7 @@ _scratch_mount
> >   # Set ULIMIT_NOFILE to min(file-max, 50000 files per LOAD_FACTOR)
> >   # so that this test doesn't take forever or OOM the box
> >   max_files=$((50000 * LOAD_FACTOR))
> > -max_allowable_files=$(( $(cat /proc/sys/fs/file-max) ))
> > +max_allowable_files=$(( $(cat /proc/sys/fs/file-max) / 2 ))
> >   test $max_allowable_files -gt 0 && test $max_files -gt $max_allowable_files && \
> >   	max_files=$max_allowable_files
> >   ulimit -n $max_files
> > diff --git a/tests/generic/531 b/tests/generic/531
> > index f3eb5cde..5d60e4b6 100755
> > --- a/tests/generic/531
> > +++ b/tests/generic/531
> > @@ -44,7 +44,7 @@ nr_cpus=$(( $(getconf _NPROCESSORS_ONLN) * 2 ))
> >   # Set ULIMIT_NOFILE to min(file-max, 50000 files per LOAD_FACTOR)
> >   # so that this test doesn't take forever or OOM the box
> >   max_files=$((50000 * LOAD_FACTOR))
> > -max_allowable_files=$(( $(cat /proc/sys/fs/file-max) ))
> > +max_allowable_files=$(( $(cat /proc/sys/fs/file-max) / 2 ))
> 
> This looks like this would certainly help, but wouldn't we want it to be
> something more like file-max - file-nr ?  Or something similar?  I'm just
> thinking the threshold at which we pop the file limit would probably be more
> dependent on how many files are already allocated about the system.  The 2
> probably solves it most of the time, but it's certainly possible that
> file-max / 2 may still be too much in some cases.  Thoughts?

Admittedly, filemax/2 is a Weasel Number -- we want to avoid running the
system out of memory, but we also have no idea how much memory actually
gets consumed by an open tempfile.  On my debug system it's 1664 bytes
for the inode, 320 bytes for the dentry, and 16 bytes for the unlinked
backref, but that's not guaranteed.  Ideally we'd stop ourselves at
(free memory / 2k) tempfiles, but the "2k" part is hard to figure.

So we just cap ourselves at filemax/2 and hope that filemax is set
sanely.  I guess we I could go for (filemax-filenr)/2?  Though that
will cause minor fluctuations in the test behavior across runs on the
same machine.

(Though really, lots of things cause fluctuations :P)

--D

> 
> Allison
> 
> >   test $max_allowable_files -gt 0 && test $max_files -gt $max_allowable_files && \
> >   	max_files=$max_allowable_files
> >   ulimit -n $max_files
> > diff --git a/tests/xfs/501 b/tests/xfs/501
> > index 51cdb020..d689145f 100755
> > --- a/tests/xfs/501
> > +++ b/tests/xfs/501
> > @@ -47,7 +47,7 @@ _scratch_mount
> >   # Set ULIMIT_NOFILE to min(file-max, 30000 files per LOAD_FACTOR)
> >   # so that this test doesn't take forever or OOM the box
> >   max_files=$((30000 * LOAD_FACTOR))
> > -max_allowable_files=$(( $(cat /proc/sys/fs/file-max) ))
> > +max_allowable_files=$(( $(cat /proc/sys/fs/file-max) / 2 ))
> >   test $max_allowable_files -gt 0 && test $max_files -gt $max_allowable_files && \
> >   	max_files=$max_allowable_files
> >   ulimit -n $max_files
> > diff --git a/tests/xfs/502 b/tests/xfs/502
> > index bfb063f4..5ad10316 100755
> > --- a/tests/xfs/502
> > +++ b/tests/xfs/502
> > @@ -46,7 +46,7 @@ nr_cpus=$(( $(getconf _NPROCESSORS_ONLN) * 2 ))
> >   # Set ULIMIT_NOFILE to min(file-max, 30000 files per cpu per LOAD_FACTOR)
> >   # so that this test doesn't take forever or OOM the box
> >   max_files=$((30000 * LOAD_FACTOR))
> > -max_allowable_files=$(( $(cat /proc/sys/fs/file-max) ))
> > +max_allowable_files=$(( $(cat /proc/sys/fs/file-max) / 2 ))
> >   test $max_allowable_files -gt 0 && test $max_files -gt $max_allowable_files && \
> >   	max_files=$max_allowable_files
> >   ulimit -n $max_files
> > 

Re: [PATCH 1/2] misc: don't oom the box opening tmpfiles

[Allison Henderson]

On 2/26/19 2:05 PM, Darrick J. Wong wrote:
> On Tue, Feb 26, 2019 at 12:08:20PM -0700, Allison Henderson wrote:
>> On 2/25/19 7:35 PM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>>
>>> For the t_open_tmpfiles tests, limit ourselves to half of file-max so
>>> that we don't OOM the test machine.
>>>
>>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>>> ---
>>>    tests/generic/530 |    2 +-
>>>    tests/generic/531 |    2 +-
>>>    tests/xfs/501     |    2 +-
>>>    tests/xfs/502     |    2 +-
>>>    4 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>>
>>> diff --git a/tests/generic/530 b/tests/generic/530
>>> index a2968d25..2bc4a992 100755
>>> --- a/tests/generic/530
>>> +++ b/tests/generic/530
>>> @@ -42,7 +42,7 @@ _scratch_mount
>>>    # Set ULIMIT_NOFILE to min(file-max, 50000 files per LOAD_FACTOR)
>>>    # so that this test doesn't take forever or OOM the box
>>>    max_files=$((50000 * LOAD_FACTOR))
>>> -max_allowable_files=$(( $(cat /proc/sys/fs/file-max) ))
>>> +max_allowable_files=$(( $(cat /proc/sys/fs/file-max) / 2 ))
>>>    test $max_allowable_files -gt 0 && test $max_files -gt $max_allowable_files && \
>>>    	max_files=$max_allowable_files
>>>    ulimit -n $max_files
>>> diff --git a/tests/generic/531 b/tests/generic/531
>>> index f3eb5cde..5d60e4b6 100755
>>> --- a/tests/generic/531
>>> +++ b/tests/generic/531
>>> @@ -44,7 +44,7 @@ nr_cpus=$(( $(getconf _NPROCESSORS_ONLN) * 2 ))
>>>    # Set ULIMIT_NOFILE to min(file-max, 50000 files per LOAD_FACTOR)
>>>    # so that this test doesn't take forever or OOM the box
>>>    max_files=$((50000 * LOAD_FACTOR))
>>> -max_allowable_files=$(( $(cat /proc/sys/fs/file-max) ))
>>> +max_allowable_files=$(( $(cat /proc/sys/fs/file-max) / 2 ))
>>
>> This looks like this would certainly help, but wouldn't we want it to be
>> something more like file-max - file-nr ?  Or something similar?  I'm just
>> thinking the threshold at which we pop the file limit would probably be more
>> dependent on how many files are already allocated about the system.  The 2
>> probably solves it most of the time, but it's certainly possible that
>> file-max / 2 may still be too much in some cases.  Thoughts?
> 
> Admittedly, filemax/2 is a Weasel Number -- we want to avoid running the
> system out of memory, but we also have no idea how much memory actually
> gets consumed by an open tempfile.  On my debug system it's 1664 bytes
> for the inode, 320 bytes for the dentry, and 16 bytes for the unlinked
> backref, but that's not guaranteed.  Ideally we'd stop ourselves at
> (free memory / 2k) tempfiles, but the "2k" part is hard to figure.
> 
> So we just cap ourselves at filemax/2 and hope that filemax is set
> sanely.  I guess we I could go for (filemax-filenr)/2?  Though that
> will cause minor fluctuations in the test behavior across runs on the
> same machine.
> 
> (Though really, lots of things cause fluctuations :P)
> 
> --D

Alrighty then, that makes sense.  It occurred to me later that if the 
test takes any substantial amount of time, file-nr may change many times 
anyway, so trying to use it may get racy.  So it may be more stable as 
you have it.

Thanks for the explanation!  You can add my review:
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> 
>>
>> Allison
>>
>>>    test $max_allowable_files -gt 0 && test $max_files -gt $max_allowable_files && \
>>>    	max_files=$max_allowable_files
>>>    ulimit -n $max_files
>>> diff --git a/tests/xfs/501 b/tests/xfs/501
>>> index 51cdb020..d689145f 100755
>>> --- a/tests/xfs/501
>>> +++ b/tests/xfs/501
>>> @@ -47,7 +47,7 @@ _scratch_mount
>>>    # Set ULIMIT_NOFILE to min(file-max, 30000 files per LOAD_FACTOR)
>>>    # so that this test doesn't take forever or OOM the box
>>>    max_files=$((30000 * LOAD_FACTOR))
>>> -max_allowable_files=$(( $(cat /proc/sys/fs/file-max) ))
>>> +max_allowable_files=$(( $(cat /proc/sys/fs/file-max) / 2 ))
>>>    test $max_allowable_files -gt 0 && test $max_files -gt $max_allowable_files && \
>>>    	max_files=$max_allowable_files
>>>    ulimit -n $max_files
>>> diff --git a/tests/xfs/502 b/tests/xfs/502
>>> index bfb063f4..5ad10316 100755
>>> --- a/tests/xfs/502
>>> +++ b/tests/xfs/502
>>> @@ -46,7 +46,7 @@ nr_cpus=$(( $(getconf _NPROCESSORS_ONLN) * 2 ))
>>>    # Set ULIMIT_NOFILE to min(file-max, 30000 files per cpu per LOAD_FACTOR)
>>>    # so that this test doesn't take forever or OOM the box
>>>    max_files=$((30000 * LOAD_FACTOR))
>>> -max_allowable_files=$(( $(cat /proc/sys/fs/file-max) ))
>>> +max_allowable_files=$(( $(cat /proc/sys/fs/file-max) / 2 ))
>>>    test $max_allowable_files -gt 0 && test $max_files -gt $max_allowable_files && \
>>>    	max_files=$max_allowable_files
>>>    ulimit -n $max_files
>>>

Re: [PATCH 2/2] t_attr_corruption: fix this yet again

[Jeff Mahoney]

On 2/25/19 9:35 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Jeff Moyer pointed out that 'security.evm' actually has an expected

Mahoney :)

> value format, which breaks the test if EVM is enabled.  It turns out
> that the 'security.evm' setxattr call in the original syzkaller report
> was a total red herring, as this bug can be reproduced without it.
> 
> Fix the test case to do the minimum amount of work needed to reproduce
> the corruption.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Otherwise, works for me in the case that failed for me previously.

Thanks,

-Jeff

> ---
>  src/t_attr_corruption.c |   28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> 
> diff --git a/src/t_attr_corruption.c b/src/t_attr_corruption.c
> index f26611f9..9101024e 100644
> --- a/src/t_attr_corruption.c
> +++ b/src/t_attr_corruption.c
> @@ -3,17 +3,21 @@
>   * Copyright (C) 2019 Oracle.  All Rights Reserved.
>   * Author: Darrick J. Wong <darrick.wong@oracle.com>
>   *
> - * Test program to tickle a use-after-free bug in xfs.
> + * XFS had a memory corruption bug in its handling of the POSIX ACL attribute
> + * names during a listxattr call.
>   *
> - * XFS had a use-after-free bug when xfs_xattr_put_listent runs out of
> - * listxattr buffer space while trying to store the name
> - * "system.posix_acl_access" and then corrupts memory by not checking the
> - * seen_enough state and then trying to shove "trusted.SGI_ACL_FILE" into the
> - * buffer as well.
> + * On IRIX, file ACLs were stored under the name "trusted.SGI_ACL_FILE",
> + * whereas on Linux the name is "system.posix_acl_access".  In order to
> + * maintain compatibility with old filesystems, XFS internally continues to
> + * use the old SGI_ACL_FILE name on disk and remap the new name whenever it
> + * sees it.
>   *
> - * In order to tickle the bug in a user visible way we must have already put a
> - * name in the buffer, so we take advantage of the fact that "security.evm"
> - * sorts before "system.posix_acl_access" to make sure this happens.
> + * In order to make this magic happen, XFS' listxattr implementation will emit
> + * first the Linux name and then the on-disk name.  Unfortunately, it doesn't
> + * correctly check the buffer length, so if the buffer is large enough to fit
> + * the on-disk name but not large enough to fit the Linux name, we screw up
> + * the buffer position accounting while trying to emit the Linux name and then
> + * corrupt memory when we try to emit the on-disk name.
>   *
>   * If we trigger the bug, the program will print the garbled string
>   * "rusted.SGI_ACL_FILE".  If the bug is fixed, the flistxattr call returns
> @@ -76,11 +80,7 @@ int main(int argc, char *argv[])
>  	if (ret)
>  		die("set posix acl");
>  
> -	ret = fsetxattr(fd, "security.evm", buf, 1, 1);
> -	if (ret)
> -		die("set evm");
> -
> -	sz = flistxattr(fd, buf, 30);
> +	sz = flistxattr(fd, buf, 20);
>  	if (sz < 0)
>  		die("list attr");
>  
> 
> 


-- 
Jeff Mahoney
SUSE Labs

Re: [PATCH 2/2] t_attr_corruption: fix this yet again

[Eryu Guan]

On Thu, Feb 28, 2019 at 03:56:40PM -0500, Jeff Mahoney wrote:
> On 2/25/19 9:35 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Jeff Moyer pointed out that 'security.evm' actually has an expected
> 
> Mahoney :)

Fixed on commit :)

> 
> > value format, which breaks the test if EVM is enabled.  It turns out
> > that the 'security.evm' setxattr call in the original syzkaller report
> > was a total red herring, as this bug can be reproduced without it.
> > 
> > Fix the test case to do the minimum amount of work needed to reproduce
> > the corruption.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Otherwise, works for me in the case that failed for me previously.

Thanks for the fix and test!

Eryu